Make sure API responds with 401 when invalid authentication info is provided

changelogs/unreleased/dm-api-unauthorized.yml

+---
+title: Make sure API responds with 401 when invalid authentication info is provided
+merge_request:
+author:
+type: fixed

lib/api/api_guard.rb

@@ -75,7 +75,7 @@ module API
           raise RevokedError
 
         when AccessTokenValidationService::VALID
-          @current_user = User.find(access_token.resource_owner_id)
+          User.find(access_token.resource_owner_id)
         end
       end
 
@@ -84,11 +84,13 @@ module API
 
         return nil unless token_string.present?
 
-        find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes)
-      end
+        user =
+          find_user_by_authentication_token(token_string) ||
+          find_user_by_personal_access_token(token_string, scopes)
+
+        raise UnauthorizedError unless user
 
-      def current_user
-        @current_user
+        user
       end
 
       private
@@ -107,7 +109,16 @@ module API
       end
 
       def find_access_token
-        @access_token ||= Doorkeeper.authenticate(doorkeeper_request, Doorkeeper.configuration.access_token_methods)
+        return @access_token if defined?(@access_token)
+
+        token = Doorkeeper::OAuth::Token.from_request(doorkeeper_request, *Doorkeeper.configuration.access_token_methods)
+        return @access_token = nil unless token
+
+        @access_token = Doorkeeper::AccessToken.by_token(token)
+        raise UnauthorizedError unless @access_token
+
+        @access_token.revoke_previous_refresh_token!
+        @access_token
       end
 
       def doorkeeper_request
@@ -169,6 +180,7 @@ module API
     TokenNotFoundError = Class.new(StandardError)
     ExpiredError = Class.new(StandardError)
     RevokedError = Class.new(StandardError)
+    UnauthorizedError = Class.new(StandardError)
 
     class InsufficientScopeError < StandardError
       attr_reader :scopes

lib/api/helpers.rb

@@ -3,6 +3,8 @@ module API
     include Gitlab::Utils
     include Helpers::Pagination
 
+    UnauthorizedError = Class.new(StandardError)
+
     SUDO_HEADER = "HTTP_SUDO".freeze
     SUDO_PARAM = :sudo
 
@@ -139,7 +141,7 @@ module API
     end
 
     def authenticate!
-      unauthorized! unless current_user && can?(initial_current_user, :access_api)
+      unauthorized! unless current_user
     end
 
     def authenticate_non_get!
@@ -397,19 +399,27 @@ module API
 
     def initial_current_user
       return @initial_current_user if defined?(@initial_current_user)
-      Gitlab::Auth::UniqueIpsLimiter.limit_user! do
-        @initial_current_user ||= find_user_by_private_token(scopes: scopes_registered_for_endpoint)
-        @initial_current_user ||= doorkeeper_guard(scopes: scopes_registered_for_endpoint)
-        @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
+      begin
+        @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user }
+      rescue APIGuard::UnauthorizedError, UnauthorizedError
+        unauthorized!
       end
     end
 
+    def find_current_user
+      user =
+        find_user_by_private_token(scopes: scopes_registered_for_endpoint) ||
+        doorkeeper_guard(scopes: scopes_registered_for_endpoint) ||
+        find_user_from_warden
+
+      return nil unless user
+
+      raise UnauthorizedError unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api)
+
+      user
+    end
+
     def sudo!
       return unless sudo_identifier
       return unless initial_current_user

spec/requests/api/helpers_spec.rb

@@ -159,18 +159,25 @@ describe API::Helpers do
     end
 
     describe "when authenticating using a user's private token" do
-      it "returns nil for an invalid token" do
+      it "returns a 401 response for an invalid token" do
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
         allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
       end
 
-      it "returns nil for a user without access" do
+      it "returns a 401 response for a user without access" do
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
         allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
+      end
+
+      it 'returns a 401 response for a user who is blocked' do
+        user.block!
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
+
+        expect { current_user }.to raise_error /401/
       end
 
       it "leaves user as is when sudo not specified" do
@@ -193,24 +200,31 @@ describe API::Helpers do
         allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
       end
 
-      it "returns nil for an invalid token" do
+      it "returns a 401 response for an invalid token" do
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
       end
 
-      it "returns nil for a user without access" do
+      it "returns a 401 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 be_nil
+        expect { current_user }.to raise_error /401/
+      end
+
+      it 'returns a 401 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/
       end
 
-      it "returns nil for a token without the appropriate scope" do
+      it "returns a 401 response for a token without the appropriate scope" do
         personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user'])
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
       end
 
       it "leaves user as is when sudo not specified" do
@@ -226,14 +240,14 @@ describe API::Helpers do
         personal_access_token.revoke!
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
       end
 
       it 'does not allow expired tokens' do
         personal_access_token.update_attributes!(expires_at: 1.day.ago)
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
 
-        expect(current_user).to be_nil
+        expect { current_user }.to raise_error /401/
       end
     end
 
@@ -351,6 +365,18 @@ describe API::Helpers do
             end
           end
         end
+
+        context 'when user is blocked' do
+          before do
+            user.block!
+          end
+
+          it 'changes current_user to sudo' do
+            set_env(admin, user.id)
+
+            expect(current_user).to eq(user)
+          end
+        end
       end
 
       context 'with regular user' do
@@ -490,11 +516,10 @@ describe API::Helpers do
     context 'current_user is nil' do
       before do
         expect_any_instance_of(self.class).to receive(:current_user).and_return(nil)
-        allow_any_instance_of(self.class).to receive(:initial_current_user).and_return(nil)
       end
 
       it 'returns a 401 response' do
-        expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}'
+        expect { authenticate! }.to raise_error /401/
       end
     end
 
@@ -502,35 +527,12 @@ describe API::Helpers do
       let(:user) { build(:user) }
 
       before do
-        expect_any_instance_of(self.class).to receive(:current_user).at_least(:once).and_return(user)
-        expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(user)
+        expect_any_instance_of(self.class).to receive(:current_user).and_return(user)
       end
 
       it 'does not raise an error' do
         expect { authenticate! }.not_to raise_error
       end
     end
-
-    context 'current_user is blocked' do
-      let(:user) { build(:user, :blocked) }
-
-      before do
-        expect_any_instance_of(self.class).to receive(:current_user).at_least(:once).and_return(user)
-      end
-
-      it 'raises an error' do
-        expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(user)
-
-        expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}'
-      end
-
-      it "doesn't raise an error if an admin user is impersonating a blocked user (via sudo)" do
-        admin_user = build(:user, :admin)
-
-        expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(admin_user)
-
-        expect { authenticate! }.not_to raise_error
-      end
-    end
   end
 end