Merge branch 'move_scan_security_report_secrets_worker' into 'master'

Move ScanSecurityReportSecretsWorker to avoid race-condition

See merge request gitlab-org/gitlab!50815

ee/app/workers/scan_security_report_secrets_worker.rb

@@ -15,11 +15,11 @@ class ScanSecurityReportSecretsWorker # rubocop:disable Scalability/IdempotentWo
 
   ScanSecurityReportSecretsWorkerError = Class.new(StandardError)
 
-  def perform(build_id)
-    build = Ci::Build.find_by_id(build_id)
-    return unless build
+  def perform(pipeline_id)
+    pipeline = Ci::Pipeline.find_by_id(pipeline_id)
+    return unless pipeline
 
-    keys = revocable_keys(build)
+    keys = revocable_keys(pipeline)
 
     if keys.present?
       executed_result = Security::TokenRevocationService.new(revocable_keys: keys).execute
@@ -30,8 +30,8 @@ class ScanSecurityReportSecretsWorker # rubocop:disable Scalability/IdempotentWo
 
   private
 
-  def revocable_keys(build)
-    vulnerability_findings = build.pipeline.vulnerability_findings.report_type(:secret_detection)
+  def revocable_keys(pipeline)
+    vulnerability_findings = pipeline.vulnerability_findings.report_type(:secret_detection)
 
     vulnerability_findings.map do |vulnerability_finding|
       {

ee/app/workers/security/store_scans_worker.rb

@@ -11,28 +11,7 @@ module Security
         break unless pipeline.can_store_security_reports?
 
         Security::StoreScansService.execute(pipeline)
-        security_build = secret_detection_build(pipeline)
-
-        if revoke_secret_detection_token?(security_build)
-          ::ScanSecurityReportSecretsWorker.perform_async(security_build.id)
-        end
       end
     end
-
-    private
-
-    def secret_detection_build(pipeline)
-      pipeline.security_scans.secret_detection.last&.build
-    end
-
-    def revoke_secret_detection_token?(build)
-      build.present? &&
-        ::Gitlab::CurrentSettings.secret_detection_token_revocation_enabled? &&
-        secret_detection_vulnerability_found?(build)
-    end
-
-    def secret_detection_vulnerability_found?(build)
-      build.pipeline.vulnerability_findings.secret_detection.any?
-    end
   end
 end

ee/app/workers/store_security_reports_worker.rb

@@ -13,6 +13,25 @@ class StoreSecurityReportsWorker # rubocop:disable Scalability/IdempotentWorker
       break unless pipeline.project.can_store_security_reports?
 
       ::Security::StoreReportsService.new(pipeline).execute
+
+      if revoke_secret_detection_token?(pipeline)
+        logger.info "StoreSecurityReportsWorker: token revocation started for pipeline: #{pipeline.id}"
+        ::ScanSecurityReportSecretsWorker.perform_async(pipeline.id)
+      else
+        logger.info "StoreSecurityReportsWorker: did not revoke token for pipeline: #{pipeline.id}"
+      end
     end
   end
+
+  private
+
+  def revoke_secret_detection_token?(pipeline)
+    pipeline.present? &&
+      ::Gitlab::CurrentSettings.secret_detection_token_revocation_enabled? &&
+      secret_detection_vulnerability_found?(pipeline)
+  end
+
+  def secret_detection_vulnerability_found?(pipeline)
+    pipeline.vulnerability_findings.secret_detection.any?
+  end
 end

ee/changelogs/unreleased/move_scan_security_report_secrets_worker.yml

+---
+title: Move ScanSecurityReportSecretsWorker to avoid race-condition
+merge_request: 50815
+author:
+type: fixed

ee/spec/workers/scan_security_report_secrets_worker_spec.rb

@@ -6,7 +6,6 @@ RSpec.describe  ScanSecurityReportSecretsWorker do
   let(:project) { create(:project, :repository) }
   let(:pipeline) { create(:ci_pipeline, :success, project: project) }
 
-  let(:secret_detection_build) { create(:ci_build, :secret_detection, pipeline: pipeline) }
   let(:file) { 'aws-key1.py' }
   let(:api_key) { 'AKIAIOSFODNN7EXAMPLE' }
   let(:identifier_type) { 'gitleaks_rule_id' }
@@ -29,7 +28,7 @@ RSpec.describe  ScanSecurityReportSecretsWorker do
 
   describe '#perform' do
     include_examples 'an idempotent worker' do
-      let(:job_args) { [secret_detection_build.id] }
+      let(:job_args) { [pipeline.id] }
 
       before do
         allow_next_instance_of(Security::TokenRevocationService) do |revocation_service|
@@ -42,7 +41,7 @@ RSpec.describe  ScanSecurityReportSecretsWorker do
           expect(revocation_service).to receive(:execute).and_return({ message: '', status: :success })
         end
 
-        worker.perform(secret_detection_build.id)
+        worker.perform(pipeline.id)
       end
     end
 
@@ -54,14 +53,14 @@ RSpec.describe  ScanSecurityReportSecretsWorker do
       end
 
       it 'does not execute the service' do
-        expect { worker.perform(secret_detection_build.id) }.to raise_error('This is an error')
+        expect { worker.perform(pipeline.id) }.to raise_error('This is an error')
       end
     end
   end
 
   describe '#revocable_keys' do
     it 'returns a list of revocable_keys' do
-      key = worker.send(:revocable_keys, secret_detection_build).first
+      key = worker.send(:revocable_keys, pipeline).first
 
       expect(key[:type]).to eql(revocation_key_type)
       expect(key[:token]).to eql(api_key)

ee/spec/workers/security/store_scans_worker_spec.rb

@@ -3,73 +3,18 @@
 require 'spec_helper'
 
 RSpec.describe Security::StoreScansWorker do
-  let_it_be(:secret_detection_scan_1) { create(:security_scan, scan_type: :secret_detection) }
-  let_it_be(:secret_detection_scan_2) { create(:security_scan, scan_type: :secret_detection) }
-  let_it_be(:secret_detection_pipeline) { secret_detection_scan_2.pipeline }
-  let_it_be(:secret_detection_build) { secret_detection_pipeline.security_scans.secret_detection.last&.build }
   let_it_be(:sast_scan) { create(:security_scan, scan_type: :sast) }
   let_it_be(:sast_pipeline) { sast_scan.pipeline }
   let_it_be(:sast_build) { sast_pipeline.security_scans.sast.last&.build }
 
-  describe '#secret_detection_build' do
-    before do
-      secret_detection_scan_1
-      secret_detection_scan_2
-    end
-
-    specify { expect(described_class.new.send(:secret_detection_build, secret_detection_pipeline)).to eql(secret_detection_scan_2.build) }
-    specify { expect(described_class.new.send(:secret_detection_build, sast_pipeline)).to be(nil) }
-  end
-
-  describe '#secret_detection_vulnerability_found?' do
-    before do
-      create(:vulnerabilities_finding, :with_secret_detection, pipelines: [secret_detection_pipeline], project: secret_detection_pipeline.project)
-    end
-
-    specify { expect(described_class.new.send(:secret_detection_vulnerability_found?, secret_detection_build)).to be(true) }
-    specify { expect(described_class.new.send(:secret_detection_vulnerability_found?, sast_build)).to be(false) }
-  end
-
-  describe '#revoke_secret_detection_token?' do
-    using RSpec::Parameterized::TableSyntax
-
-    where(:secret_detection_build, :token_revocation_enabled, :secret_detection_vulnerability_found, :expected_result) do
-      Object.new  | true  | true  | true
-      Object.new  | true  | false | false
-      Object.new  | false | true  | false
-      Object.new  | false | false | false
-      nil         | true  | true  | false
-      nil         | true  | false | false
-      nil         | false | true  | false
-      nil         | false | false | false
-    end
-
-    with_them do
-      before do
-        stub_application_setting(secret_detection_token_revocation_enabled: token_revocation_enabled)
-
-        allow_next_instance_of(described_class) do |store_scans_worker|
-          allow(store_scans_worker).to receive(:secret_detection_vulnerability_found?) { secret_detection_vulnerability_found }
-        end
-      end
-
-      specify { expect(described_class.new.send(:revoke_secret_detection_token?, secret_detection_build)).to eql(expected_result) }
-    end
-  end
-
   describe '#perform' do
-    subject(:run_worker) { described_class.new.perform(secret_detection_pipeline.id) }
+    subject(:run_worker) { described_class.new.perform(sast_pipeline.id) }
 
     before do
       allow(Security::StoreScansService).to receive(:execute)
       allow_next_found_instance_of(Ci::Pipeline) do |record|
         allow(record).to receive(:can_store_security_reports?).and_return(can_store_security_reports)
       end
-
-      allow(::ScanSecurityReportSecretsWorker).to receive(:perform_async).and_return(nil)
-      allow_next_instance_of(described_class) do |store_scans_worker|
-        allow(store_scans_worker).to receive(:revoke_secret_detection_token?) { true }
-      end
     end
 
     context 'when security reports can not be stored for the pipeline' do
@@ -90,12 +35,6 @@ RSpec.describe Security::StoreScansWorker do
 
         expect(Security::StoreScansService).to have_received(:execute)
       end
-
-      it 'scans security reports for token revocation' do
-        expect(::ScanSecurityReportSecretsWorker).to receive(:perform_async)
-
-        run_worker
-      end
     end
   end
 end

ee/spec/workers/store_security_reports_worker_spec.rb

@@ -3,6 +3,45 @@
 require 'spec_helper'
 
 RSpec.describe StoreSecurityReportsWorker do
+  let_it_be(:secret_detection_scan_1) { create(:security_scan, scan_type: :secret_detection) }
+  let_it_be(:secret_detection_scan_2) { create(:security_scan, scan_type: :secret_detection) }
+  let_it_be(:secret_detection_pipeline) { secret_detection_scan_2.pipeline }
+
+  describe '#secret_detection_vulnerability_found?' do
+    before do
+      create(:vulnerabilities_finding, :with_secret_detection, pipelines: [secret_detection_pipeline], project: secret_detection_pipeline.project)
+    end
+
+    specify { expect(described_class.new.send(:secret_detection_vulnerability_found?, secret_detection_pipeline)).to be(true) }
+  end
+
+  describe '#revoke_secret_detection_token?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:pipeline, :token_revocation_enabled, :secret_detection_vulnerability_found, :expected_result) do
+      Object.new  | true  | true  | true
+      Object.new  | true  | false | false
+      Object.new  | false | true  | false
+      Object.new  | false | false | false
+      nil         | true  | true  | false
+      nil         | true  | false | false
+      nil         | false | true  | false
+      nil         | false | false | false
+    end
+
+    with_them do
+      before do
+        stub_application_setting(secret_detection_token_revocation_enabled: token_revocation_enabled)
+
+        allow_next_instance_of(described_class) do |store_scans_worker|
+          allow(store_scans_worker).to receive(:secret_detection_vulnerability_found?) { secret_detection_vulnerability_found }
+        end
+      end
+
+      specify { expect(described_class.new.send(:revoke_secret_detection_token?, pipeline)).to eql(expected_result) }
+    end
+  end
+
   describe '#perform' do
     let(:group)   { create(:group) }
     let(:project) { create(:project, namespace: group) }
@@ -10,6 +49,11 @@ RSpec.describe StoreSecurityReportsWorker do
 
     before do
       allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline }
+
+      allow(::ScanSecurityReportSecretsWorker).to receive(:perform_async).and_return(nil)
+      allow_next_instance_of(described_class) do |store_scans_worker|
+        allow(store_scans_worker).to receive(:revoke_secret_detection_token?) { true }
+      end
     end
 
     context 'when at least one security report feature is enabled' do
@@ -26,6 +70,12 @@ RSpec.describe StoreSecurityReportsWorker do
 
           described_class.new.perform(pipeline.id)
         end
+
+        it 'scans security reports for token revocation' do
+          expect(::ScanSecurityReportSecretsWorker).to receive(:perform_async)
+
+          described_class.new.perform(pipeline.id)
+        end
       end
     end