Merge branch '27765-403-no-job' into 'master'

Return 403 when job is deleted

See merge request gitlab-org/gitlab!59382

changelogs/unreleased/27765-403-no-job.yml

+---
+title: Return 403 status code to the Runner when CI Job is deleted
+merge_request: 59382
+author:
+type: fixed

lib/api/helpers/runner.rb

@@ -38,10 +38,17 @@ module API
         end
       end
 
+      # HTTP status codes to terminate the job on GitLab Runner:
+      # - 403
       def authenticate_job!(require_running: true)
         job = current_job
 
-        not_found! unless job
+        # 404 is not returned here because we want to terminate the job if it's
+        # running. A 404 can be returned from anywhere in the networking stack which is why
+        # we are explicit about a 403, we should improve this in
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/327703
+        forbidden! unless job
+
         forbidden! unless job_token_valid?(job)
 
         forbidden!('Project has been deleted!') if job.project.nil? || job.project.pending_delete?

spec/requests/api/ci/runner/jobs_artifacts_spec.rb

@@ -180,6 +180,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
                 it_behaves_like 'authorizes local file'
               end
             end
+
+            context 'when job does not exist anymore' do
+              before do
+                allow(job).to receive(:id).and_return(non_existing_record_id)
+              end
+
+              it 'returns 403 Forbidden' do
+                subject
+
+                expect(response).to have_gitlab_http_status(:forbidden)
+              end
+            end
           end
         end
 
@@ -321,6 +333,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             end
           end
 
+          context 'when job does not exist anymore' do
+            before do
+              allow(job).to receive(:id).and_return(non_existing_record_id)
+            end
+
+            it 'returns 403 Forbidden' do
+              upload_artifacts(file_upload, headers_with_token)
+
+              expect(response).to have_gitlab_http_status(:forbidden)
+            end
+          end
+
           context 'when job is running' do
             shared_examples 'successful artifacts upload' do
               it 'updates successfully' do
@@ -867,6 +891,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
         end
 
+        context 'when job does not exist anymore' do
+          before do
+            allow(job).to receive(:id).and_return(non_existing_record_id)
+          end
+
+          it 'responds with 403 Forbidden' do
+            get api("/jobs/#{job.id}/artifacts"), params: { token: token }, headers: headers
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
+
         def download_artifact(params = {}, request_headers = headers)
           params = params.merge(token: token)
           job.reload

spec/requests/api/ci/runner/jobs_put_spec.rb

@@ -278,14 +278,22 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         end
       end
 
-      def update_job(token = job.token, **params)
+      context 'when job does not exist anymore' do
+        it 'returns 403 Forbidden' do
+          update_job(non_existing_record_id, state: 'success', trace: 'BUILD TRACE UPDATED')
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+
+      def update_job(job_id = job.id, token = job.token, **params)
         new_params = params.merge(token: token)
-        put api("/jobs/#{job.id}"), params: new_params
+        put api("/jobs/#{job_id}"), params: new_params
       end
 
       def update_job_after_time(update_interval = 20.minutes, state = 'running')
         travel_to(job.updated_at + update_interval) do
-          update_job(job.token, state: state)
+          update_job(job.id, job.token, state: state)
         end
       end
     end

spec/requests/api/ci/runner/jobs_trace_spec.rb

@@ -219,6 +219,14 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         end
       end
 
+      context 'when job does not exist anymore' do
+        it 'returns 403 Forbidden' do
+          patch_the_trace(job_id: non_existing_record_id)
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+
       context 'when Runner makes a force-patch' do
         before do
           force_patch_the_trace
@@ -264,7 +272,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         it { expect(response).to have_gitlab_http_status(:forbidden) }
       end
 
-      def patch_the_trace(content = ' appended', request_headers = nil)
+      def patch_the_trace(content = ' appended', request_headers = nil, job_id: job.id)
         unless request_headers
           job.trace.read do |stream|
             offset = stream.size
@@ -274,7 +282,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         end
 
         Timecop.travel(job.updated_at + update_interval) do
-          patch api("/jobs/#{job.id}/trace"), params: content, headers: request_headers
+          patch api("/jobs/#{job_id}/trace"), params: content, headers: request_headers
           job.reload
         end
       end