Pull API job token behavior in line with other tokens, and expand test coverage

lib/api/api_guard.rb

@@ -45,7 +45,7 @@ module API
     # Helper Methods for Grape Endpoint
     module HelperMethods
       def find_current_user!
-        user = find_user_from_access_token || find_user_from_warden || find_user_by_job_token
+        user = find_user_from_access_token || find_user_from_job_token || find_user_from_warden
         return unless user
 
         forbidden!('User is blocked') unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api)
@@ -82,6 +82,20 @@ module API
         access_token.user || raise(UnauthorizedError)
       end
 
+      def find_user_from_job_token
+        return unless route_authentication_setting[:job_token_allowed]
+
+        token = (params[JOB_TOKEN_PARAM] || env[JOB_TOKEN_HEADER]).to_s
+        return unless token.present?
+
+        job = Ci::Build.find_by(token: token)
+        raise UnauthorizedError unless job
+
+        @job_token_authentication = true
+
+        job.user
+      end
+
       # Check the Rails session for valid authentication details
       def find_user_from_warden
         warden.try(:authenticate) if verified_request?
@@ -96,16 +110,6 @@ module API
         Gitlab::RequestForgeryProtection.verified?(env)
       end
 
-      def find_user_by_job_token
-        return @user_by_job_token if defined?(@user_by_job_token)
-
-        @user_by_job_token =
-          if route_authentication_setting[:job_token_allowed]
-            token_string = params[JOB_TOKEN_PARAM].presence || env[JOB_TOKEN_HEADER].presence
-            Ci::Build.find_by_token(token_string)&.user if token_string
-          end
-      end
-
       def route_authentication_setting
         return {} unless respond_to?(:route_setting)
 

lib/api/helpers.rb

@@ -408,7 +408,7 @@ module API
     end
 
     def job_token_authentication?
-      initial_current_user && initial_current_user == find_user_by_job_token
+      initial_current_user && @job_token_authentication
     end
 
     def warden

spec/requests/api/helpers_spec.rb

@@ -182,31 +182,47 @@ describe API::Helpers do
     end
 
     describe "when authenticating using a job token" do
-      let(:job) { create(:ci_build, user: current_user) }
-      let(:route_authentication_setting) { { job_token_allowed: true } }
+      let(:job) { create(:ci_build, user: user) }
 
-      before do
-        allow_any_instance_of(described_class).to receive(:doorkeeper_guard).and_return(nil)
-      end
+      context 'when route is allowed to be authenticated' do
+        let(:route_authentication_setting) { { job_token_allowed: true } }
 
-      it "returns nil for an invalid token" do
-        env[API::APIGuard::JOB_TOKEN_HEADER] = 'invalid token'
+        it "returns a 401 response for an invalid token" do
+          env[API::APIGuard::JOB_TOKEN_HEADER] = 'invalid token'
 
-        expect(current_user).to be_nil
-      end
+          expect { current_user }.to raise_error /401/
+        end
 
-      it "returns nil for a user without access" do
-        env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
-        allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
+        it "returns a 403 response for a user without access" do
+          env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
+          allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
+
+          expect { current_user }.to raise_error /403/
+        end
 
-        expect(current_user).to be_nil
+        it 'returns a 403 response for a user who is blocked' do
+          user.block!
+          env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
+
+          expect { current_user }.to raise_error /403/
+        end
+
+        it "sets current_user" do
+          env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
+
+          expect(current_user).to eq(user)
+        end
       end
 
-      it "returns nil for a user with access, but route not allowed to be authenticated" do
-        env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
-        allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(true)
+      context 'when route is not allowed to be authenticated' do
+        let(:route_authentication_setting) { { job_token_allowed: false } }
+
+        it "sets current_user to nil" do
+          env[API::APIGuard::JOB_TOKEN_HEADER] = job.token
+          allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(true)
 
-        expect(current_user).to be_nil
+          expect(current_user).to be_nil
+        end
       end
     end
   end