Merge branch '24537-reenable-private-token-with-sudo' into 'master'

Reenables the API /users to return `private-token` when sudo is either a parameter or passed as a header and the user is admin.

Closes #24537

See merge request !7615
Signed-off-by: Rémy Coutable <remy@rymai.me>

changelogs/unreleased/24537-reenable-private-token-with-sudo.yml

+---
+title: Reenables /user API request to return private-token if user is admin and request
+  is made with sudo
+merge_request: 7615
+author: 

doc/api/users.md

@@ -277,7 +277,9 @@ Parameters:
 
 - `id` (required) - The ID of the user
 
-## Current user
+## User
+
+### For normal users
 
 Gets currently authenticated user.
 
@@ -321,6 +323,53 @@ GET /user
 }
 ```
 
+### For admins
+
+Parameters:
+
+- `sudo` (required) - the ID of a user
+
+```
+GET /user
+```
+
+```json
+{
+  "id": 1,
+  "username": "john_smith",
+  "email": "john@example.com",
+  "name": "John Smith",
+  "state": "active",
+  "avatar_url": "http://localhost:3000/uploads/user/avatar/1/index.jpg",
+  "web_url": "http://localhost:3000/john_smith",
+  "created_at": "2012-05-23T08:00:58Z",
+  "is_admin": false,
+  "bio": null,
+  "location": null,
+  "skype": "",
+  "linkedin": "",
+  "twitter": "",
+  "website_url": "",
+  "organization": "",
+  "last_sign_in_at": "2012-06-01T11:41:01Z",
+  "confirmed_at": "2012-05-23T09:05:22Z",
+  "theme_id": 1,
+  "color_scheme_id": 2,
+  "projects_limit": 100,
+  "current_sign_in_at": "2012-06-02T06:36:55Z",
+  "identities": [
+    {"provider": "github", "extern_uid": "2435223452345"},
+    {"provider": "bitbucket", "extern_uid": "john_smith"},
+    {"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"}
+  ],
+  "can_create_group": true,
+  "can_create_project": true,
+  "two_factor_enabled": true,
+  "external": false,
+  "private_token": "dd34asd13as"
+}
+```
+
 ## List SSH keys
 
 Get a list of currently authenticated user's SSH keys.

lib/api/entities.rb

@@ -22,7 +22,7 @@ module API
       expose :provider, :extern_uid
     end
 
-    class UserFull < User
+    class UserPublic < User
       expose :last_sign_in_at
       expose :confirmed_at
       expose :email
@@ -34,7 +34,7 @@ module API
       expose :external
     end
 
-    class UserLogin < UserFull
+    class UserWithPrivateToken < UserPublic
       expose :private_token
     end
 
@@ -283,7 +283,7 @@ module API
     end
 
     class SSHKeyWithUser < SSHKey
-      expose :user, using: Entities::UserFull
+      expose :user, using: Entities::UserPublic
     end
 
     class Note < Grape::Entity

lib/api/helpers.rb

@@ -39,11 +39,14 @@ module API
         return nil
       end
 
-      identifier = sudo_identifier()
+      identifier = sudo_identifier
 
-      # If the sudo is the current user do nothing
-      if identifier && !(@current_user.id == identifier || @current_user.username == identifier)
+      if identifier
+        # We check for private_token because we cannot allow PAT to be used
         forbidden!('Must be admin to use sudo') unless @current_user.is_admin?
+        forbidden!('Private token must be specified in order to use sudo') unless private_token_used?
+
+        @impersonator = @current_user
         @current_user = User.by_username_or_id(identifier)
         not_found!("No user id or username for: #{identifier}") if @current_user.nil?
       end
@@ -410,6 +413,10 @@ module API
       links.join(', ')
     end
 
+    def private_token_used?
+      private_token == @current_user.private_token
+    end
+
     def secret_token
       Gitlab::Shell.secret_token
     end

lib/api/session.rb

@@ -15,7 +15,7 @@ module API
 
       return unauthorized! unless user
       return render_api_error!('401 Unauthorized. You have 2FA enabled. Please use a personal access token to access the API', 401) if user.two_factor_enabled?
-      present user, with: Entities::UserLogin
+      present user, with: Entities::UserWithPrivateToken
     end
   end
 end

lib/api/users.rb

@@ -25,7 +25,7 @@ module API
         end
 
         if current_user.is_admin?
-          present @users, with: Entities::UserFull
+          present @users, with: Entities::UserPublic
         else
           present @users, with: Entities::UserBasic
         end
@@ -41,7 +41,7 @@ module API
         @user = User.find(params[:id])
 
         if current_user && current_user.is_admin?
-          present @user, with: Entities::UserFull
+          present @user, with: Entities::UserPublic
         elsif can?(current_user, :read_user, @user)
           present @user, with: Entities::User
         else
@@ -88,7 +88,7 @@ module API
         end
 
         if user.save
-          present user, with: Entities::UserFull
+          present user, with: Entities::UserPublic
         else
           conflict!('Email has already been taken') if User.
               where(email: user.email).
@@ -151,7 +151,7 @@ module API
         end
 
         if user.update_attributes(attrs)
-          present user, with: Entities::UserFull
+          present user, with: Entities::UserPublic
         else
           render_validation_error!(user)
         end
@@ -349,7 +349,7 @@ module API
       # Example Request:
       #   GET /user
       get do
-        present @current_user, with: Entities::UserFull
+        present @current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic
       end
 
       # Get currently authenticated user's keys

spec/fixtures/api/schemas/user/login.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "username",
+    "email",
+    "name",
+    "state",
+    "avatar_url",
+    "web_url",
+    "created_at",
+    "is_admin",
+    "bio",
+    "location",
+    "skype",
+    "linkedin",
+    "twitter",
+    "website_url",
+    "organization",
+    "last_sign_in_at",
+    "confirmed_at",
+    "theme_id",
+    "color_scheme_id",
+    "projects_limit",
+    "current_sign_in_at",
+    "identities",
+    "can_create_group",
+    "can_create_project",
+    "two_factor_enabled",
+    "external",
+    "private_token"
+  ],
+  "properties": {
+    "$ref": "full.json",
+    "private_token": { "type": "string" }
+  }
+}

spec/fixtures/api/schemas/user/public.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "username",
+    "email",
+    "name",
+    "state",
+    "avatar_url",
+    "web_url",
+    "created_at",
+    "is_admin",
+    "bio",
+    "location",
+    "skype",
+    "linkedin",
+    "twitter",
+    "website_url",
+    "organization",
+    "last_sign_in_at",
+    "confirmed_at",
+    "theme_id",
+    "color_scheme_id",
+    "projects_limit",
+    "current_sign_in_at",
+    "identities",
+    "can_create_group",
+    "can_create_project",
+    "two_factor_enabled",
+    "external"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "username": { "type": "string" },
+    "email": { 
+      "type": "string",
+      "pattern": "^[^@]+@[^@]+$"
+    },
+    "name": { "type": "string" },
+    "state": { 
+      "type": "string",
+      "enum": ["active", "blocked"] 
+    },
+    "avatar_url": { "type": "string" },
+    "web_url": { "type": "string" },
+    "created_at": { "type": "date" },
+    "is_admin": { "type": "boolean" },
+    "bio": { "type": ["string", "null"] },
+    "location": { "type": ["string", "null"] },
+    "skype": { "type": "string" },
+    "linkedin": { "type": "string" },
+    "twitter": { "type": "string "},
+    "website_url": { "type": "string" },
+    "organization": { "type": ["string", "null"] },
+    "last_sign_in_at": { "type": "date" },
+    "confirmed_at": { "type": ["date", "null"] },
+    "theme_id": { "type": "integer" },
+    "color_scheme_id": { "type": "integer" },
+    "projects_limit": { "type": "integer" },
+    "current_sign_in_at": { "type": "date" },
+    "identities": { 
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {
+          "provider": { 
+            "type": "string",
+            "enum": ["github", "bitbucket", "google_oauth2"] 
+          },
+          "extern_uid": { "type": ["number", "string"] }
+        }
+      }
+    },
+    "can_create_group": { "type": "boolean" },
+    "can_create_project": { "type": "boolean" },
+    "two_factor_enabled": { "type": "boolean" },
+    "external": { "type": "boolean" } 
+  }
+}

spec/requests/api/api_helpers_spec.rb

@@ -153,85 +153,144 @@ describe API::Helpers, api: true do
       end
     end
 
-    it "changes current user to sudo when admin" do
-      set_env(admin, user.id)
-      expect(current_user).to eq(user)
-      set_param(admin, user.id)
-      expect(current_user).to eq(user)
-      set_env(admin, user.username)
-      expect(current_user).to eq(user)
-      set_param(admin, user.username)
-      expect(current_user).to eq(user)
-    end
+    context 'sudo usage' do
+      context 'with admin' do
+        context 'with header' do
+          context 'with id' do
+            it 'changes current_user to sudo' do
+              set_env(admin, user.id)
 
-    it "throws an error when the current user is not an admin and attempting to sudo" do
-      set_env(user, admin.id)
-      expect { current_user }.to raise_error(Exception)
-      set_param(user, admin.id)
-      expect { current_user }.to raise_error(Exception)
-      set_env(user, admin.username)
-      expect { current_user }.to raise_error(Exception)
-      set_param(user, admin.username)
-      expect { current_user }.to raise_error(Exception)
-    end
+              expect(current_user).to eq(user)
+            end
 
-    it "throws an error when the user cannot be found for a given id" do
-      id = user.id + admin.id
-      expect(user.id).not_to eq(id)
-      expect(admin.id).not_to eq(id)
-      set_env(admin, id)
-      expect { current_user }.to raise_error(Exception)
+            it 'handles sudo to oneself' do
+              set_env(admin, admin.id)
 
-      set_param(admin, id)
-      expect { current_user }.to raise_error(Exception)
-    end
+              expect(current_user).to eq(admin)
+            end
 
-    it "throws an error when the user cannot be found for a given username" do
-      username = "#{user.username}#{admin.username}"
-      expect(user.username).not_to eq(username)
-      expect(admin.username).not_to eq(username)
-      set_env(admin, username)
-      expect { current_user }.to raise_error(Exception)
+            it 'throws an error when user cannot be found' do
+              id = user.id + admin.id
+              expect(user.id).not_to eq(id)
+              expect(admin.id).not_to eq(id)
 
-      set_param(admin, username)
-      expect { current_user }.to raise_error(Exception)
-    end
+              set_env(admin, id)
 
-    it "handles sudo's to oneself" do
-      set_env(admin, admin.id)
-      expect(current_user).to eq(admin)
-      set_param(admin, admin.id)
-      expect(current_user).to eq(admin)
-      set_env(admin, admin.username)
-      expect(current_user).to eq(admin)
-      set_param(admin, admin.username)
-      expect(current_user).to eq(admin)
-    end
+              expect { current_user }.to raise_error(Exception)
+            end
+          end
 
-    it "handles multiple sudo's to oneself" do
-      set_env(admin, user.id)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
-      set_env(admin, user.username)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
-
-      set_param(admin, user.id)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
-      set_param(admin, user.username)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
-    end
+          context 'with username' do
+            it 'changes current_user to sudo' do
+              set_env(admin, user.username)
+
+              expect(current_user).to eq(user)
+            end
+
+            it 'handles sudo to oneself' do
+              set_env(admin, admin.username)
+
+              expect(current_user).to eq(admin)
+            end
+
+            it "throws an error when the user cannot be found for a given username" do
+              username = "#{user.username}#{admin.username}"
+              expect(user.username).not_to eq(username)
+              expect(admin.username).not_to eq(username)
+
+              set_env(admin, username)
+
+              expect { current_user }.to raise_error(Exception)
+            end
+          end
+        end
+
+        context 'with param' do
+          context 'with id' do
+            it 'changes current_user to sudo' do
+              set_param(admin, user.id)
+
+              expect(current_user).to eq(user)
+            end
+
+            it 'handles sudo to oneself' do
+              set_param(admin, admin.id)
+
+              expect(current_user).to eq(admin)
+            end
+
+            it 'handles sudo to oneself using string' do
+              set_env(admin, user.id.to_s)
+
+              expect(current_user).to eq(user)
+            end
+
+            it 'throws an error when user cannot be found' do
+              id = user.id + admin.id
+              expect(user.id).not_to eq(id)
+              expect(admin.id).not_to eq(id)
 
-    it "handles multiple sudo's to oneself using string ids" do
-      set_env(admin, user.id.to_s)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
+              set_param(admin, id)
 
-      set_param(admin, user.id.to_s)
-      expect(current_user).to eq(user)
-      expect(current_user).to eq(user)
+              expect { current_user }.to raise_error(Exception)
+            end
+          end
+
+          context 'with username' do
+            it 'changes current_user to sudo' do
+              set_param(admin, user.username)
+
+              expect(current_user).to eq(user)
+            end
+
+            it 'handles sudo to oneself' do
+              set_param(admin, admin.username)
+
+              expect(current_user).to eq(admin)
+            end
+
+            it "throws an error when the user cannot be found for a given username" do
+              username = "#{user.username}#{admin.username}"
+              expect(user.username).not_to eq(username)
+              expect(admin.username).not_to eq(username)
+
+              set_param(admin, username)
+
+              expect { current_user }.to raise_error(Exception)
+            end
+          end
+        end
+      end
+
+      context 'with regular user' do
+        context 'with env' do
+          it 'changes current_user to sudo when admin and user id' do
+            set_env(user, admin.id)
+
+            expect { current_user }.to raise_error(Exception)
+          end
+
+          it 'changes current_user to sudo when admin and user username' do
+            set_env(user, admin.username)
+
+            expect { current_user }.to raise_error(Exception)
+          end
+        end
+
+        context 'with params' do
+          it 'changes current_user to sudo when admin and user id' do
+            set_param(user, admin.id)
+
+            expect { current_user }.to raise_error(Exception)
+          end
+
+          it 'changes current_user to sudo when admin and user username' do
+            set_param(user, admin.username)
+
+            expect { current_user }.to raise_error(Exception)
+          end
+        end
+      end
     end
   end
 

spec/requests/api/users_spec.rb

@@ -615,20 +615,75 @@ describe API::API, api: true  do
   end
 
   describe "GET /user" do
-    it "returns current user" do
-      get api("/user", user)
-      expect(response).to have_http_status(200)
-      expect(json_response['email']).to eq(user.email)
-      expect(json_response['is_admin']).to eq(user.is_admin?)
-      expect(json_response['can_create_project']).to eq(user.can_create_project?)
-      expect(json_response['can_create_group']).to eq(user.can_create_group?)
-      expect(json_response['projects_limit']).to eq(user.projects_limit)
-      expect(json_response['private_token']).to be_blank
+    let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let(:private_token) { user.private_token }
+
+    context 'with regular user' do
+      context 'with personal access token' do
+        it 'returns 403 without private token when sudo is defined' do
+          get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
+
+          expect(response).to have_http_status(403)
+        end
+      end
+
+      context 'with private token' do
+        it 'returns 403 without private token when sudo defined' do
+          get api("/user?private_token=#{private_token}&sudo=#{user.id}")
+
+          expect(response).to have_http_status(403)
+        end
+      end
+
+      it 'returns current user without private token when sudo not defined' do
+        get api("/user", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to match_response_schema('user/public')
+      end
     end
 
-    it "returns 401 error if user is unauthenticated" do
-      get api("/user")
-      expect(response).to have_http_status(401)
+    context 'with admin' do
+      let(:user) { create(:admin) }
+
+      context 'with personal access token' do
+        it 'returns 403 without private token when sudo defined' do
+          get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
+
+          expect(response).to have_http_status(403)
+        end
+
+        it 'returns current user without private token when sudo not defined' do
+          get api("/user?private_token=#{personal_access_token.token}")
+
+          expect(response).to have_http_status(200)
+          expect(response).to match_response_schema('user/public')
+        end
+      end
+
+      context 'with private token' do
+        it 'returns current user with private token when sudo defined' do
+          get api("/user?private_token=#{private_token}&sudo=#{user.id}")
+
+          expect(response).to have_http_status(200)
+          expect(response).to match_response_schema('user/login')
+        end
+
+        it 'returns current user without private token when sudo not defined' do
+          get api("/user?private_token=#{private_token}")
+
+          expect(response).to have_http_status(200)
+          expect(response).to match_response_schema('user/public')
+        end
+      end
+    end
+
+    context 'with unauthenticated user' do
+      it "returns 401 error if user is unauthenticated" do
+        get api("/user")
+
+        expect(response).to have_http_status(401)
+      end
     end
   end