Add sudo API scope

11 files changed, 74 insertions, 98 deletions

diff --git a/app/controllers/admin/impersonation_tokens_controller.rb b/app/controllers/admin/impersonation_tokens_controller.rb
index 07c8bf714fc..7a2c7234a1e 100644
--- a/app/controllers/admin/impersonation_tokens_controller.rb
+++ b/app/controllers/admin/impersonation_tokens_controller.rb
@@ -44,7 +44,7 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
   end
 
   def set_index_vars
-    @scopes = Gitlab::Auth::API_SCOPES
+    @scopes = Gitlab::Auth.available_scopes(current_user)
 
     @impersonation_token ||= finder.build
     @inactive_impersonation_tokens = finder(state: 'inactive').execute
diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb
index 4146deefa89..6d9873e38df 100644
--- a/app/controllers/profiles/personal_access_tokens_controller.rb
+++ b/app/controllers/profiles/personal_access_tokens_controller.rb
@@ -39,7 +39,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   end
 
   def set_index_vars
-    @scopes = Gitlab::Auth.available_scopes
+    @scopes = Gitlab::Auth.available_scopes(current_user)
 
     @inactive_personal_access_tokens = finder(state: 'inactive').execute
     @active_personal_access_tokens = finder(state: 'active').execute.order(:expires_at)
diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 9c00ea789ec..46e19230328 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -39,11 +39,8 @@ class AccessTokenValidationService
       token_scopes = token.scopes.map(&:to_sym)
 
       required_scopes.any? do |scope|
-        if scope.respond_to?(:sufficient?)
-          scope.sufficient?(token_scopes, request)
-        else
-          API::Scope.new(scope).sufficient?(token_scopes, request)
-        end
+        scope = API::Scope.new(scope) unless scope.is_a?(API::Scope)
+        scope.sufficient?(token_scopes, request)
       end
     end
   end
diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml
index 14d49885fb3..0da6b14c29e 100644
--- a/config/locales/doorkeeper.en.yml
+++ b/config/locales/doorkeeper.en.yml
@@ -58,9 +58,10 @@ en:
           expired: "The access token expired"
           unknown: "The access token is invalid"
     scopes:
-      api: Access your API
-      read_user: Read user information
+      api: Access the authenticated user's API
+      read_user: Read the authenticated user's personal information
       openid: Authenticate using OpenID Connect
+      sudo: Perform API actions as any user in the system (if the authenticated user is an admin)
 
     flash:
       applications:
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 0ff376bbab6..b9c7d443f6c 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -42,62 +42,42 @@ module API
 
     # Helper Methods for Grape Endpoint
     module HelperMethods
-      def find_current_user
-        user =
-          find_user_from_personal_access_token ||
-          find_user_from_oauth_token ||
-          find_user_from_warden
+      def find_current_user!
+        user = find_user_from_access_token || find_user_from_warden
+        return unless user
 
-        return nil unless user
-
-        raise UnauthorizedError unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api)
+        forbidden!('User is blocked') unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api)
 
         user
       end
 
-      def private_token
-        params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
-      end
-
-      private
-
-      def find_user_from_personal_access_token
-        token_string = private_token.to_s
-        return nil unless token_string.present?
+      def access_token
+        return @access_token if defined?(@access_token)
 
-        access_token = PersonalAccessToken.find_by_token(token_string)
-        raise UnauthorizedError unless access_token
-
-        user = find_user_by_access_token(access_token)
+        @access_token = find_oauth_access_token || find_personal_access_token
+      end
 
-        raise UnauthorizedError unless user
+      def validate_access_token!(scopes: [])
+        return unless access_token
 
-        user
+        case AccessTokenValidationService.new(access_token, request: request).validate(scopes: scopes)
+        when AccessTokenValidationService::INSUFFICIENT_SCOPE
+          raise InsufficientScopeError.new(scopes)
+        when AccessTokenValidationService::EXPIRED
+          raise ExpiredError
+        when AccessTokenValidationService::REVOKED
+          raise RevokedError
+        end
       end
 
-      # Invokes the doorkeeper guard.
-      #
-      # If token is presented and valid, then it sets @current_user.
-      #
-      # If the token does not have sufficient scopes to cover the requred scopes,
-      # then it raises InsufficientScopeError.
-      #
-      # If the token is expired, then it raises ExpiredError.
-      #
-      # If the token is revoked, then it raises RevokedError.
-      #
-      # If the token is not found (nil), then it returns nil
-      #
-      # Arguments:
-      #
-      #   scopes: (optional) scopes required for this guard.
-      #           Defaults to empty array.
-      #
-      def find_user_from_oauth_token
-        access_token = find_oauth_access_token
+      private
+
+      def find_user_from_access_token
         return unless access_token
 
-        find_user_by_access_token(access_token)
+        validate_access_token!
+
+        access_token.user || raise(UnauthorizedError)
       end
 
       # Check the Rails session for valid authentication details
@@ -115,34 +95,26 @@ module API
       end
 
       def find_oauth_access_token
-        return @oauth_access_token if defined?(@oauth_access_token)
-
         token = Doorkeeper::OAuth::Token.from_request(doorkeeper_request, *Doorkeeper.configuration.access_token_methods)
-        return @oauth_access_token = nil unless token
+        return unless token
 
-        @oauth_access_token = OauthAccessToken.by_token(token)
-        raise UnauthorizedError unless @oauth_access_token
+        # Expiration, revocation and scopes are verified in `find_user_by_access_token`
+        access_token = OauthAccessToken.by_token(token)
+        raise UnauthorizedError unless access_token
 
-        @oauth_access_token.revoke_previous_refresh_token!
-        @oauth_access_token
+        access_token.revoke_previous_refresh_token!
+        access_token
       end
 
-      def find_user_by_access_token(access_token)
-        scopes = scopes_registered_for_endpoint
+      def find_personal_access_token
+        token = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
+        return unless token.present?
 
-        case AccessTokenValidationService.new(access_token, request: request).validate(scopes: scopes)
-        when AccessTokenValidationService::INSUFFICIENT_SCOPE
-          raise InsufficientScopeError.new(scopes)
-
-        when AccessTokenValidationService::EXPIRED
-          raise ExpiredError
-
-        when AccessTokenValidationService::REVOKED
-          raise RevokedError
+        # Expiration, revocation and scopes are verified in `find_user_by_access_token`
+        access_token = PersonalAccessToken.find_by(token: token)
+        raise UnauthorizedError unless access_token
 
-        when AccessTokenValidationService::VALID
-          access_token.user
-        end
+        access_token
       end
 
       def doorkeeper_request
@@ -226,7 +198,7 @@ module API
     class InsufficientScopeError < StandardError
       attr_reader :scopes
       def initialize(scopes)
-        @scopes = scopes
+        @scopes = scopes.map { |s| s.try(:name) || s }
       end
     end
   end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 7a2ec865860..b1b855fdd9c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -41,6 +41,8 @@ module API
 
       sudo!
 
+      validate_access_token!(scopes: scopes_registered_for_endpoint) unless sudo?
+
       @current_user
     end
 
@@ -385,7 +387,7 @@ module API
       return @initial_current_user if defined?(@initial_current_user)
 
       begin
-        @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user }
+        @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user! }
       rescue APIGuard::UnauthorizedError
         unauthorized!
       end
@@ -393,24 +395,26 @@ module API
 
     def sudo!
       return unless sudo_identifier
-      return unless initial_current_user
+
+      raise UnauthorizedError unless initial_current_user
 
       unless initial_current_user.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')
+      unless access_token
+        forbidden!('Must be authenticated using an OAuth or Personal Access Token to use sudo')
       end
 
+      validate_access_token!(scopes: [:sudo])
+
       sudoed_user = find_user(sudo_identifier)
 
-      if sudoed_user
-        @current_user = sudoed_user
-      else
+      unless sudoed_user
         not_found!("No user id or username for: #{sudo_identifier}")
       end
+
+      @current_user = sudoed_user
     end
 
     def sudo_identifier
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 2d8f4654b4b..0ad9285c0ea 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -5,7 +5,7 @@ module Gitlab
     REGISTRY_SCOPES = [:read_registry].freeze
 
     # Scopes used for GitLab API access
-    API_SCOPES = [:api, :read_user].freeze
+    API_SCOPES = [:api, :read_user, :sudo].freeze
 
     # Scopes used for OpenID Connect
     OPENID_SCOPES = [:openid].freeze
@@ -226,8 +226,10 @@ module Gitlab
         []
       end
 
-      def available_scopes
-        API_SCOPES + registry_scopes
+      def available_scopes(current_user = nil)
+        scopes = API_SCOPES + registry_scopes
+        scopes.delete(:sudo) if current_user && !current_user.admin?
+        scopes
       end
 
       # Other available scopes
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 72c40bf8f90..54a853c9ce3 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::Auth do
 
   describe 'constants' do
     it 'API_SCOPES contains all scopes for API access' do
-      expect(subject::API_SCOPES).to eq [:api, :read_user]
+      expect(subject::API_SCOPES).to eq %i[api read_user sudo]
     end
 
     it 'OPENID_SCOPES contains all scopes for OpenID Connect' do
@@ -19,7 +19,7 @@ describe Gitlab::Auth do
     it 'optional_scopes contains all non-default scopes' do
       stub_container_registry_config(enabled: true)
 
-      expect(subject.optional_scopes).to eq %i[read_user read_registry openid]
+      expect(subject.optional_scopes).to eq %i[read_user sudo read_registry openid]
     end
 
     context 'registry_scopes' do
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index 174593593f8..308134eba72 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/spec/requests/api/doorkeeper_access_spec.rb
@@ -39,20 +39,20 @@ describe 'doorkeeper access' do
   end
 
   describe "when user is blocked" do
-    it "returns authentication error" do
+    it "returns authorization error" do
       user.block
       get api("/user"), access_token: token.token
 
-      expect(response).to have_gitlab_http_status(401)
+      expect(response).to have_gitlab_http_status(403)
     end
   end
 
   describe "when user is ldap_blocked" do
-    it "returns authentication error" do
+    it "returns authorization error" do
       user.ldap_block
       get api("/user"), access_token: token.token
 
-      expect(response).to have_gitlab_http_status(401)
+      expect(response).to have_gitlab_http_status(403)
     end
   end
 end
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 0ab9f94376c..9631324607f 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -173,18 +173,18 @@ describe API::Helpers do
         expect { current_user }.to raise_error /401/
       end
 
-      it "returns a 401 response for a user without access" do
+      it "returns a 403 response for a user without access" do
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
         allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
 
-        expect { current_user }.to raise_error /401/
+        expect { current_user }.to raise_error /403/
       end
 
-      it 'returns a 401 response for a user who is blocked' do
+      it 'returns a 403 response for a user who is blocked' do
         user.block!
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
 
-        expect { current_user }.to raise_error /401/
+        expect { current_user }.to raise_error /403/
       end
 
       it "leaves user as is when sudo not specified" do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index fc1d055afe2..634c8dae0ba 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -127,8 +127,8 @@ describe API::Users do
     context "when admin" do
       context 'when sudo is defined' do
         it 'does not return 500' do
-          admin_personal_access_token = create(:personal_access_token, user: admin).token
-          get api("/users?private_token=#{admin_personal_access_token}&sudo=#{user.id}", admin)
+          admin_personal_access_token = create(:personal_access_token, user: admin, scopes: [:sudo])
+          get api("/users?sudo=#{user.id}", admin, personal_access_token: admin_personal_access_token)
 
           expect(response).to have_gitlab_http_status(:success)
         end