Merge branch 'fix-forbidden-for-build-api-for-deleted-project' into 'master'

Give forbidden if project for the build was deleted

I guess we don't need a change log entry because this is just for an internal corner case fix.

Closes #25309

See merge request !8091

3 files changed, 37 insertions, 17 deletions

diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb
index ed87a2603e8..142bce82286 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -41,7 +41,7 @@ module Ci
         put ":id" do
           authenticate_runner!
           build = Ci::Build.where(runner_id: current_runner.id).running.find(params[:id])
-          forbidden!('Build has been erased!') if build.erased?
+          validate_build!(build)
 
           update_runner_info
 
@@ -71,9 +71,7 @@ module Ci
         #   PATCH /builds/:id/trace.txt
         patch ":id/trace.txt" do
           build = Ci::Build.find_by_id(params[:id])
-          not_found! unless build
-          authenticate_build_token!(build)
-          forbidden!('Build has been erased!') if build.erased?
+          authenticate_build!(build)
 
           error!('400 Missing header Content-Range', 400) unless request.headers.has_key?('Content-Range')
           content_range = request.headers['Content-Range']
@@ -104,8 +102,7 @@ module Ci
           Gitlab::Workhorse.verify_api_request!(headers)
           not_allowed! unless Gitlab.config.artifacts.enabled
           build = Ci::Build.find_by_id(params[:id])
-          not_found! unless build
-          authenticate_build_token!(build)
+          authenticate_build!(build)
           forbidden!('build is not running') unless build.running?
 
           if params[:filesize]
@@ -142,10 +139,8 @@ module Ci
           require_gitlab_workhorse!
           not_allowed! unless Gitlab.config.artifacts.enabled
           build = Ci::Build.find_by_id(params[:id])
-          not_found! unless build
-          authenticate_build_token!(build)
+          authenticate_build!(build)
           forbidden!('Build is not running!') unless build.running?
-          forbidden!('Build has been erased!') if build.erased?
 
           artifacts_upload_path = ArtifactUploader.artifacts_upload_path
           artifacts = uploaded_file(:file, artifacts_upload_path)
@@ -176,8 +171,7 @@ module Ci
         #   GET /builds/:id/artifacts
         get ":id/artifacts" do
           build = Ci::Build.find_by_id(params[:id])
-          not_found! unless build
-          authenticate_build_token!(build)
+          authenticate_build!(build)
           artifacts_file = build.artifacts_file
 
           unless artifacts_file.file_storage?
@@ -202,8 +196,7 @@ module Ci
         #   DELETE /builds/:id/artifacts
         delete ":id/artifacts" do
           build = Ci::Build.find_by_id(params[:id])
-          not_found! unless build
-          authenticate_build_token!(build)
+          authenticate_build!(build)
 
           build.erase_artifacts!
         end
diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb
index e608f5f6cad..31fbd1da108 100644
--- a/lib/ci/api/helpers.rb
+++ b/lib/ci/api/helpers.rb
@@ -13,8 +13,19 @@ module Ci
         forbidden! unless current_runner
       end
 
-      def authenticate_build_token!(build)
-        forbidden! unless build_token_valid?(build)
+      def authenticate_build!(build)
+        validate_build!(build) do
+          forbidden! unless build_token_valid?(build)
+        end
+      end
+
+      def validate_build!(build)
+        not_found! unless build
+
+        yield if block_given?
+
+        forbidden!('Project has been deleted!') unless build.project
+        forbidden!('Build has been erased!') if build.erased?
       end
 
       def runner_registration_token_valid?
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 80652129928..79f12ace999 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -249,7 +249,11 @@ describe Ci::API::Builds do
     end
 
     describe 'PATCH /builds/:id/trace.txt' do
-      let(:build) { create(:ci_build, :pending, :trace, runner_id: runner.id) }
+      let(:build) do
+        attributes = { runner_id: runner.id, pipeline: pipeline }
+        create(:ci_build, :running, :trace, attributes)
+      end
+
       let(:headers) { { Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token, 'Content-Type' => 'text/plain' } }
       let(:headers_with_range) { headers.merge({ 'Content-Range' => '11-20' }) }
       let(:update_interval) { 10.seconds.to_i }
@@ -276,7 +280,6 @@ describe Ci::API::Builds do
       end
 
       before do
-        build.run!
         initial_patch_the_trace
       end
 
@@ -329,6 +332,19 @@ describe Ci::API::Builds do
             end
           end
         end
+
+        context 'when project for the build has been deleted' do
+          let(:build) do
+            attributes = { runner_id: runner.id, pipeline: pipeline }
+            create(:ci_build, :running, :trace, attributes) do |build|
+              build.project.update(pending_delete: true)
+            end
+          end
+
+          it 'responds with forbidden' do
+            expect(response.status).to eq(403)
+          end
+        end
       end
 
       context 'when Runner makes a force-patch' do