Merge branch '25482-fix-api-sudo' into 'master'

API: Memoize the current_user so that the sudo can work properly

Closes #25482

See merge request !8017

app/models/user.rb

@@ -304,10 +304,6 @@ class User < ActiveRecord::Base
       personal_access_token.user if personal_access_token
     end
 
-    def by_username_or_id(name_or_id)
-      find_by('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i)
-    end
-
     # Returns a user for the given SSH key.
     def find_by_ssh_key_id(key_id)
       find_by(id: Key.unscoped.select(:user_id).where(id: key_id))

changelogs/unreleased/25482-fix-api-sudo.yml

+---
+title: 'API: Memoize the current_user so that sudo can work properly'
+merge_request: 8017
+author:

lib/api/helpers.rb

@@ -7,67 +7,23 @@ module API
     SUDO_HEADER = "HTTP_SUDO"
     SUDO_PARAM = :sudo
 
-    def private_token
-      params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
-    end
-
-    def warden
-      env['warden']
-    end
-
-    # Check the Rails session for valid authentication details
-    #
-    # Until CSRF protection is added to the API, disallow this method for
-    # state-changing endpoints
-    def find_user_from_warden
-      warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
-    end
-
     def declared_params(options = {})
       options = { include_parent_namespaces: false }.merge(options)
       declared(params, options).to_h.symbolize_keys
     end
 
-    def find_user_by_private_token
-      token = private_token
-      return nil unless token.present?
-
-      User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
-    end
-
     def current_user
-      @current_user ||= find_user_by_private_token
-      @current_user ||= doorkeeper_guard
-      @current_user ||= find_user_from_warden
-
-      unless @current_user && Gitlab::UserAccess.new(@current_user).allowed?
-        return nil
-      end
+      return @current_user if defined?(@current_user)
 
-      identifier = sudo_identifier
+      @current_user = initial_current_user
 
-      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
+      sudo!
 
       @current_user
     end
 
-    def sudo_identifier
-      identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
-
-      # Regex for integers
-      if !!(identifier =~ /\A[0-9]+\z/)
-        identifier.to_i
-      else
-        identifier
-      end
+    def sudo?
+      initial_current_user != current_user
     end
 
     def user_project
@@ -78,6 +34,14 @@ module API
       @available_labels ||= LabelsFinder.new(current_user, project_id: user_project.id).execute
     end
 
+    def find_user(id)
+      if id =~ /^\d+$/
+        User.find_by(id: id)
+      else
+        User.find_by(username: id)
+      end
+    end
+
     def find_project(id)
       if id =~ /^\d+$/
         Project.find_by(id: id)
@@ -343,6 +307,69 @@ module API
 
     private
 
+    def private_token
+      params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
+    end
+
+    def warden
+      env['warden']
+    end
+
+    # Check the Rails session for valid authentication details
+    #
+    # Until CSRF protection is added to the API, disallow this method for
+    # state-changing endpoints
+    def find_user_from_warden
+      warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
+    end
+
+    def find_user_by_private_token
+      token = private_token
+      return nil unless token.present?
+
+      User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
+    end
+
+    def initial_current_user
+      return @initial_current_user if defined?(@initial_current_user)
+
+      @initial_current_user ||= find_user_by_private_token
+      @initial_current_user ||= doorkeeper_guard
+      @initial_current_user ||= find_user_from_warden
+
+      unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
+        @initial_current_user = nil
+      end
+
+      @initial_current_user
+    end
+
+    def sudo!
+      return unless sudo_identifier
+      return unless initial_current_user
+
+      unless initial_current_user.is_admin?
+        forbidden!('Must be admin to use sudo')
+      end
+
+      # Only private tokens should be used for the SUDO feature
+      unless private_token == initial_current_user.private_token
+        forbidden!('Private token must be specified in order to use sudo')
+      end
+
+      sudoed_user = find_user(sudo_identifier)
+
+      if sudoed_user
+        @current_user = sudoed_user
+      else
+        not_found!("No user id or username for: #{sudo_identifier}")
+      end
+    end
+
+    def sudo_identifier
+      @sudo_identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
+    end
+
     def add_pagination_headers(paginated_data)
       header 'X-Total',       paginated_data.total_count.to_s
       header 'X-Total-Pages', paginated_data.total_pages.to_s
@@ -375,10 +402,6 @@ 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/users.rb

@@ -353,7 +353,7 @@ module API
         success Entities::UserPublic
       end
       get do
-        present current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic
+        present current_user, with: sudo? ? Entities::UserWithPrivateToken : Entities::UserPublic
       end
 
       desc "Get the currently authenticated user's SSH keys" do

spec/models/user_spec.rb

@@ -727,17 +727,6 @@ describe User, models: true do
     end
   end
 
-  describe 'by_username_or_id' do
-    let(:user1) { create(:user, username: 'foo') }
-
-    it "gets the correct user" do
-      expect(User.by_username_or_id(user1.id)).to eq(user1)
-      expect(User.by_username_or_id('foo')).to eq(user1)
-      expect(User.by_username_or_id(-1)).to be_nil
-      expect(User.by_username_or_id('bar')).to be_nil
-    end
-  end
-
   describe '.find_by_ssh_key_id' do
     context 'using an existing SSH key ID' do
       let(:user) { create(:user) }

spec/requests/api/api_helpers_spec.rb → spec/requests/api/helpers_spec.rb

@@ -2,7 +2,6 @@ require 'spec_helper'
 
 describe API::Helpers, api: true do
   include API::Helpers
-  include ApiHelpers
   include SentryHelper
 
   let(:user) { create(:user) }
@@ -13,18 +12,18 @@ describe API::Helpers, api: true do
   let(:env) { { 'REQUEST_METHOD' => 'GET' } }
   let(:request) { Rack::Request.new(env) }
 
-  def set_env(token_usr, identifier)
+  def set_env(user_or_token, identifier)
     clear_env
     clear_param
-    env[API::Helpers::PRIVATE_TOKEN_HEADER] = token_usr.private_token
-    env[API::Helpers::SUDO_HEADER] = identifier
+    env[API::Helpers::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    env[API::Helpers::SUDO_HEADER] = identifier.to_s
   end
 
-  def set_param(token_usr, identifier)
+  def set_param(user_or_token, identifier)
     clear_env
     clear_param
-    params[API::Helpers::PRIVATE_TOKEN_PARAM] = token_usr.private_token
-    params[API::Helpers::SUDO_PARAM] = identifier
+    params[API::Helpers::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    params[API::Helpers::SUDO_PARAM] = identifier.to_s
   end
 
   def clear_env
@@ -163,6 +162,13 @@ describe API::Helpers, api: true do
               expect(current_user).to eq(user)
             end
 
+            it 'memoize the current_user: sudo permissions are not run against the sudoed user' do
+              set_env(admin, user.id)
+
+              expect(current_user).to eq(user)
+              expect(current_user).to eq(user)
+            end
+
             it 'handles sudo to oneself' do
               set_env(admin, admin.id)
 
@@ -294,33 +300,48 @@ describe API::Helpers, api: true do
     end
   end
 
-  describe '.sudo_identifier' do
-    it "returns integers when input is an int" do
-      set_env(admin, '123')
-      expect(sudo_identifier).to eq(123)
-      set_env(admin, '0001234567890')
-      expect(sudo_identifier).to eq(1234567890)
+  describe '.sudo?' do
+    context 'when no sudo env or param is passed' do
+      before do
+        doorkeeper_guard_returns(nil)
+      end
 
-      set_param(admin, '123')
-      expect(sudo_identifier).to eq(123)
-      set_param(admin, '0001234567890')
-      expect(sudo_identifier).to eq(1234567890)
+      it 'returns false' do
+        expect(sudo?).to be_falsy
+      end
     end
 
-    it "returns string when input is an is not an int" do
-      set_env(admin, '12.30')
-      expect(sudo_identifier).to eq("12.30")
-      set_env(admin, 'hello')
-      expect(sudo_identifier).to eq('hello')
-      set_env(admin, ' 123')
-      expect(sudo_identifier).to eq(' 123')
+    context 'when sudo env or param is passed', 'user is not an admin' do
+      before do
+        set_env(user, '123')
+      end
 
-      set_param(admin, '12.30')
-      expect(sudo_identifier).to eq("12.30")
-      set_param(admin, 'hello')
-      expect(sudo_identifier).to eq('hello')
-      set_param(admin, ' 123')
-      expect(sudo_identifier).to eq(' 123')
+      it 'returns an 403 Forbidden' do
+        expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden  - Must be admin to use sudo"}'
+      end
+    end
+
+    context 'when sudo env or param is passed', 'user is admin' do
+      context 'personal access token is used' do
+        before do
+          personal_access_token = create(:personal_access_token, user: admin)
+          set_env(personal_access_token.token, user.id)
+        end
+
+        it 'returns an 403 Forbidden' do
+          expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden  - Private token must be specified in order to use sudo"}'
+        end
+      end
+
+      context 'private access token is used' do
+        before do
+          set_env(admin.private_token, user.id)
+        end
+
+        it 'returns true' do
+          expect(sudo?).to be_truthy
+        end
+      end
     end
   end
 

spec/requests/api/users_spec.rb

@@ -651,13 +651,12 @@ describe API::Users, api: true  do
   end
 
   describe "GET /user" do
-    let(:personal_access_token) { create(:personal_access_token, user: user) }
-    let(:private_token) { user.private_token }
+    let(:personal_access_token) { create(:personal_access_token, user: user).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}")
+          get api("/user?private_token=#{personal_access_token}&sudo=123")
 
           expect(response).to have_http_status(403)
         end
@@ -665,7 +664,7 @@ describe API::Users, api: true  do
 
       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}")
+          get api("/user?private_token=#{user.private_token}&sudo=123")
 
           expect(response).to have_http_status(403)
         end
@@ -676,40 +675,44 @@ describe API::Users, api: true  do
 
         expect(response).to have_http_status(200)
         expect(response).to match_response_schema('user/public')
+        expect(json_response['id']).to eq(user.id)
       end
     end
 
     context 'with admin' do
-      let(:user) { create(:admin) }
+      let(:admin_personal_access_token) { create(:personal_access_token, user: admin).token }
 
       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}")
+          get api("/user?private_token=#{admin_personal_access_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}")
+        it 'returns initial current user without private token when sudo not defined' do
+          get api("/user?private_token=#{admin_personal_access_token}")
 
           expect(response).to have_http_status(200)
           expect(response).to match_response_schema('user/public')
+          expect(json_response['id']).to eq(admin.id)
         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}")
+        it 'returns sudoed user with private token when sudo defined' do
+          get api("/user?private_token=#{admin.private_token}&sudo=#{user.id}")
 
           expect(response).to have_http_status(200)
           expect(response).to match_response_schema('user/login')
+          expect(json_response['id']).to eq(user.id)
         end
 
-        it 'returns current user without private token when sudo not defined' do
-          get api("/user?private_token=#{private_token}")
+        it 'returns initial current user without private token when sudo not defined' do
+          get api("/user?private_token=#{admin.private_token}")
 
           expect(response).to have_http_status(200)
           expect(response).to match_response_schema('user/public')
+          expect(json_response['id']).to eq(admin.id)
         end
       end
     end