Skip to content

Prevent username overriding while creating a Deploy Token via the API.

machine424 requested to merge machine424/gitlab:fiw-username-override into master

What does this MR do?

Fixes: #211963 (closed)

Grape declared returns a Hashie::Mash but after merging it into result_hash = {} here https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/api/deploy_tokens.rb#L69, the symbol key :username becomes "username" and ::Projects::DeployTokens::CreateService sets deploy_token.username to nil here https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/services/concerns/deploy_token_methods.rb#L8

irb(main):181:0> f = Hashie::Mash.new({e: "e"})
=> #<Hashie::Mash e="e">
irb(main):182:0> f[:e].presence
=> "e"
irb(main):183:0> d = {"r" => "e"}
=> {"r"=>"e"}
irb(main):184:0> d.merge(f)
=> {"r"=>"e", "e"=>"e"}
irb(main):185:0> d[:e].presence
=> nil

And

irb(main):164:0> p
=> #<Project id:2 root/fromfile>>
irb(main):165:0> u
=> #<User id:1 @root>
irb(main):166:0>  g = ::Projects::DeployTokens::CreateService.new(p, u, {"name" => "that", "read_registry" => "1", "username"  => "myuser"}).execute
D, [2020-03-27T03:50:17.087440 #34950] DEBUG -- :    (0.7ms)  BEGIN
D, [2020-03-27T03:50:17.090453 #34950] DEBUG -- :   ProjectDeployToken Exists? (2.6ms)  SELECT 1 AS one FROM "project_deploy_tokens" WHERE "project_deploy_tokens"."deploy_token_id" IS NULL AND "project_deploy_tokens"."project_id" = 2 LIMIT 1
D, [2020-03-27T03:50:17.093966 #34950] DEBUG -- :   DeployToken Load (1.7ms)  SELECT "deploy_tokens".* FROM "deploy_tokens" WHERE "deploy_tokens"."token_encrypted" = 'xFAP6yY/zrZddVvF7cxqX84YUnRmRH1ih/UNvcDuo4W//h/f' LIMIT 1
D, [2020-03-27T03:50:17.095817 #34950] DEBUG -- :   DeployToken Load (0.8ms)  SELECT "deploy_tokens".* FROM "deploy_tokens" WHERE "deploy_tokens"."token" = 'USwnRCZ-hZHdk3AACZR4' LIMIT 1
D, [2020-03-27T03:50:17.099323 #34950] DEBUG -- :   DeployToken Create (2.1ms)  INSERT INTO "deploy_tokens" ("read_registry", "created_at", "name", "token_encrypted", "expires_at") VALUES (TRUE, '2020-03-27 03:50:17.096594', 'that', 'xFAP6yY/zrZddVvF7cxqX84YUnRmRH1ih/UNvcDuo4W//h/f', '3000-01-01 00:00:00') RETURNING "id"
D, [2020-03-27T03:50:17.106081 #34950] DEBUG -- :   ProjectDeployToken Create (5.5ms)  INSERT INTO "project_deploy_tokens" ("project_id", "deploy_token_id", "created_at") VALUES (2, 49, '2020-03-27 03:50:17.100114') RETURNING "id"
D, [2020-03-27T03:50:17.110892 #34950] DEBUG -- :    (3.9ms)  COMMIT
=> #<DeployToken id: 49, revoked: false, read_repository: false, read_registry: true, expires_at: "3000-01-01 00:00:00", created_at: "2020-03-27 03:50:17", name: "that", token: nil, username: nil, token_encrypted: "xFAP6yY/zrZddVvF7cxqX84YUnRmRH1ih/UNvcDuo4W//h/f", deploy_token_type: "project_type">

shows that the username disappears at ::Projects::DeployTokens::CreateService.

And it works if we pass :username as the key.

irb(main):189:0>  g = ::Projects::DeployTokens::CreateService.new(p, u, {"name" => "that", "read_registry" => "1", :username  => "ayo"}).execute
D, [2020-03-27T04:34:30.242172 #34950] DEBUG -- :    (0.6ms)  BEGIN
D, [2020-03-27T04:34:30.245917 #34950] DEBUG -- :   ProjectDeployToken Exists? (3.4ms)  SELECT 1 AS one FROM "project_deploy_tokens" WHERE "project_deploy_tokens"."deploy_token_id" IS NULL AND "project_deploy_tokens"."project_id" = 2 LIMIT 1
D, [2020-03-27T04:34:30.249143 #34950] DEBUG -- :   DeployToken Load (1.3ms)  SELECT "deploy_tokens".* FROM "deploy_tokens" WHERE "deploy_tokens"."token_encrypted" = '92ccxwcU/NwDW16YyrhSat47cRdvPRJVAZVk+edQYbOES+2V' LIMIT 1
D, [2020-03-27T04:34:30.251295 #34950] DEBUG -- :   DeployToken Load (1.1ms)  SELECT "deploy_tokens".* FROM "deploy_tokens" WHERE "deploy_tokens"."token" = 'fddBshhG6tM9LGytSyqW' LIMIT 1
D, [2020-03-27T04:34:30.254958 #34950] DEBUG -- :   DeployToken Create (2.2ms)  INSERT INTO "deploy_tokens" ("read_registry", "created_at", "name", "username", "token_encrypted", "expires_at") VALUES (TRUE, '2020-03-27 04:34:30.252045', 'that', 'ayo', '92ccxwcU/NwDW16YyrhSat47cRdvPRJVAZVk+edQYbOES+2V', '3000-01-01 00:00:00') RETURNING "id"
D, [2020-03-27T04:34:30.258595 #34950] DEBUG -- :   ProjectDeployToken Create (2.2ms)  INSERT INTO "project_deploy_tokens" ("project_id", "deploy_token_id", "created_at") VALUES (2, 50, '2020-03-27 04:34:30.255901') RETURNING "id"
D, [2020-03-27T04:34:30.264218 #34950] DEBUG -- :    (4.7ms)  COMMIT
=> #<DeployToken id: 50, revoked: false, read_repository: false, read_registry: true, expires_at: "3000-01-01 00:00:00", created_at: "2020-03-27 04:34:30", name: "that", token: nil, username: "ayo", token_encrypted: "92ccxwcU/NwDW16YyrhSat47cRdvPRJVAZVk+edQYbOES+2V", deploy_token_type: "project_type">

Another solution is to replace deploy_token.username = params[:username].presence with deploy_token.username = params[:username].presence || params["username"].presence in ::Projects::DeployTokens::CreateService. I don't get why this line is needed, couldn't create deal with it ?

I will try (Ruby newbie) to add tests to check that username will not be ignored.

@sabrams

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Peter Leitzen

Merge request reports