Merge branch 'ali/reorder-build-processing-for-deployment-approvals' into 'master'

Reorder CI build processing to prevent bypass of Deployment Approvals

See merge request gitlab-org/gitlab!77892

app/services/ci/process_build_service.rb

@@ -4,14 +4,7 @@ module Ci
   class ProcessBuildService < BaseService
     def execute(build, current_status)
       if valid_statuses_for_build(build).include?(current_status)
-        if build.schedulable?
-          build.schedule
-        elsif build.action?
-          build.actionize
-        else
-          enqueue(build)
-        end
-
+        process(build)
         true
       else
         build.skip
@@ -21,6 +14,16 @@ module Ci
 
     private
 
+    def process(build)
+      if build.schedulable?
+        build.schedule
+      elsif build.action?
+        build.actionize
+      else
+        enqueue(build)
+      end
+    end
+
     def enqueue(build)
       build.enqueue
     end

ee/app/services/ee/ci/process_build_service.rb

@@ -4,14 +4,20 @@ module EE
     module ProcessBuildService
       extend ::Gitlab::Utils::Override
 
+      override :process
+      def process(build)
+        if build.persisted_environment.try(:needs_approval?)
+          build.run_after_commit { |build| build.deployment&.block! }
+          return build.actionize!
+        end
+
+        super
+      end
+
       override :enqueue
       def enqueue(build)
-        # TODO: Refactor to check these conditions before actionizing or scheduling a build. See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75710#note_756445822
         if build.persisted_environment.try(:protected_from?, build.user)
           return build.drop!(:protected_environment_failure)
-        elsif build.persisted_environment.try(:needs_approval?)
-          build.actionize!
-          return build.deployment&.block!
         end
 
         super

ee/spec/services/ci/process_build_service_spec.rb

@@ -40,6 +40,14 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do
           expect(ci_build.failed?).to be_truthy
           expect(ci_build.failure_reason).to eq('protected_environment_failure')
         end
+
+        context 'and the build is manual' do
+          let(:ci_build) { create(:ci_build, :created, :actionable, environment: environment.name, user: user, project: project) }
+
+          it 'actionizes the build' do
+            expect { subject }.to change { ci_build.status }.from('created').to('manual')
+          end
+        end
       end
 
       context 'when user has access to the environment' do
@@ -63,8 +71,7 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do
           end
 
           context 'and the build has a deployment' do
-            let(:deployment) { create(:deployment, deployable: ci_build, environment: environment, user: user, project: project) }
-
+            shared_examples_for 'blocked deployment' do
               it 'blocks the deployment' do
                 expect { subject }.to change { deployment.reload.status }.from('created').to('blocked')
               end
@@ -73,6 +80,23 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do
                 expect { subject }.to change { ci_build.status }.from('created').to('manual')
               end
             end
+
+            let!(:deployment) { create(:deployment, deployable: ci_build, environment: environment, user: user, project: project) }
+
+            include_examples 'blocked deployment'
+
+            context 'and the build is schedulable' do
+              let(:ci_build) { create(:ci_build, :created, :schedulable, environment: environment.name, user: user, project: project) }
+
+              include_examples 'blocked deployment'
+            end
+
+            context 'and the build is actionable' do
+              let(:ci_build) { create(:ci_build, :created, :actionable, environment: environment.name, user: user, project: project) }
+
+              include_examples 'blocked deployment'
+            end
+          end
         end
       end
     end

ee/spec/services/ee/ci/pipeline_processing/atomic_processing_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do
+  describe 'Pipeline Processing Service' do
+    let(:project) { create(:project, :repository) }
+    let(:user)    { project.owner }
+
+    let(:pipeline) do
+      create(:ci_empty_pipeline, ref: 'master', project: project)
+    end
+
+    context 'when protected environments are defined', :sidekiq_inline do
+      let(:staging_job) { create_build('staging:deploy', environment: 'staging', user: user) }
+      let(:production_job) { create_build('production:deploy', environment: 'production', user: user) }
+
+      before do
+        stub_licensed_features(protected_environments: true)
+
+        # Protection for the staging environment
+        staging = create(:environment, name: 'staging', project: project)
+        create(:protected_environment, name: 'staging', project: project, authorize_user_to_deploy: user)
+        create(:deployment, environment: staging, deployable: staging_job, project: project)
+
+        # Protection for the production environment (with Deployment Approvals)
+        production = create(:environment, name: 'production', project: project)
+        create(:protected_environment, name: 'production', project: project, authorize_user_to_deploy: user, required_approval_count: 1)
+        create(:deployment, environment: production, deployable: production_job, project: project)
+      end
+
+      it 'blocks pipeline on stage with first manual action' do
+        process_pipeline
+
+        expect(builds_names).to match_array %w[staging:deploy production:deploy]
+        expect(staging_job.reload).to be_pending
+        expect(staging_job.deployment).to be_created
+        expect(production_job.reload).to be_manual
+        expect(production_job.deployment).to be_blocked
+        expect(pipeline.reload).to be_running
+      end
+    end
+
+    private
+
+    def all_builds
+      pipeline.processables.order(:stage_idx, :id)
+    end
+
+    def builds
+      all_builds.where.not(status: [:created, :skipped])
+    end
+
+    def builds_names
+      builds.pluck(:name)
+    end
+
+    def create_build(name, **opts)
+      create(:ci_build, :created, pipeline: pipeline, name: name, **with_stage_opts(opts))
+    end
+
+    def with_stage_opts(opts)
+      { stage: "stage-#{opts[:stage_idx].to_i}" }.merge(opts)
+    end
+  end
+
+  private
+
+  def process_pipeline
+    described_class.new(pipeline).execute
+  end
+end