Merge branch 'fix_revocation_api' into 'master'

Resolve the race-condition of ScanSecurityReportSecretsWorker

See merge request gitlab-org/gitlab!50335

ee/app/workers/ee/build_finished_worker.rb

@@ -8,21 +8,9 @@ module EE
       # and `Namespace#namespace_statistics` will return stale data.
       ::Ci::Minutes::EmailNotificationService.new(build.project.reset).execute if ::Gitlab.com?
 
-      ScanSecurityReportSecretsWorker.perform_async(build.id) if revoke_secret_detection_token?(build)
       RequirementsManagement::ProcessRequirementsReportsWorker.perform_async(build.id)
 
       super
     end
-
-    private
-
-    def revoke_secret_detection_token?(build)
-      ::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/security/store_scans_worker.rb

@@ -11,7 +11,28 @@ 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/changelogs/unreleased/fix_revocation_api.yml

+---
+title: Resolve the race-condition of ScanSecurityReportSecretsWorker
+merge_request: 50335
+author:
+type: changed

ee/spec/workers/build_finished_worker_spec.rb

@@ -20,29 +20,6 @@ RSpec.describe BuildFinishedWorker do
     project.statistics || project.create_statistics(namespace: project.namespace)
   end
 
-  describe '#revoke_secret_detection_token?' do
-    using RSpec::Parameterized::TableSyntax
-
-    where(:token_revocation_enabled, :secret_detection_vulnerability_found, :expected_result) do
-      true  | true  | true
-      true  | false | false
-      false | true  | false
-      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 |build_finished_worker|
-          allow(build_finished_worker).to receive(:secret_detection_vulnerability_found?) { secret_detection_vulnerability_found }
-        end
-      end
-
-      specify { expect(described_class.new.send(:revoke_secret_detection_token?, build)).to eql(expected_result) }
-    end
-  end
-
   describe '#perform' do
     context 'when on .com' do
       before do
@@ -86,20 +63,6 @@ RSpec.describe BuildFinishedWorker do
       subject
     end
 
-    context 'when token revocation is enabled' do
-      before do
-        allow_next_instance_of(described_class) do |build_finished_worker|
-          allow(build_finished_worker).to receive(:revoke_secret_detection_token?) { true }
-        end
-      end
-
-      it 'scans security reports for token revocation' do
-        expect(ScanSecurityReportSecretsWorker).to receive(:perform_async)
-
-        subject
-      end
-    end
-
     context 'when token revocation is disabled' do
       before do
         allow_next_instance_of(described_class) do |build_finished_worker|

ee/spec/workers/security/store_scans_worker_spec.rb

@@ -3,16 +3,73 @@
 require 'spec_helper'
 
 RSpec.describe Security::StoreScansWorker do
-  let_it_be(:pipeline) { create(:ci_pipeline) }
+  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(pipeline.id) }
+    subject(:run_worker) { described_class.new.perform(secret_detection_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
@@ -33,6 +90,12 @@ 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