Commit 8b96bfdf authored by saikat sarkar's avatar saikat sarkar Committed by Stan Hu

Resolve the race-condition of ScanSecurityReportSecretsWorker

parent 61f1fd76
...@@ -8,21 +8,9 @@ module EE ...@@ -8,21 +8,9 @@ module EE
# and `Namespace#namespace_statistics` will return stale data. # and `Namespace#namespace_statistics` will return stale data.
::Ci::Minutes::EmailNotificationService.new(build.project.reset).execute if ::Gitlab.com? ::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) RequirementsManagement::ProcessRequirementsReportsWorker.perform_async(build.id)
super super
end 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
end end
...@@ -11,7 +11,28 @@ module Security ...@@ -11,7 +11,28 @@ module Security
break unless pipeline.can_store_security_reports? break unless pipeline.can_store_security_reports?
Security::StoreScansService.execute(pipeline) 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 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 end
end end
---
title: Resolve the race-condition of ScanSecurityReportSecretsWorker
merge_request: 50335
author:
type: changed
...@@ -20,29 +20,6 @@ RSpec.describe BuildFinishedWorker do ...@@ -20,29 +20,6 @@ RSpec.describe BuildFinishedWorker do
project.statistics || project.create_statistics(namespace: project.namespace) project.statistics || project.create_statistics(namespace: project.namespace)
end 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 describe '#perform' do
context 'when on .com' do context 'when on .com' do
before do before do
...@@ -86,20 +63,6 @@ RSpec.describe BuildFinishedWorker do ...@@ -86,20 +63,6 @@ RSpec.describe BuildFinishedWorker do
subject subject
end 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 context 'when token revocation is disabled' do
before do before do
allow_next_instance_of(described_class) do |build_finished_worker| allow_next_instance_of(described_class) do |build_finished_worker|
......
...@@ -3,16 +3,73 @@ ...@@ -3,16 +3,73 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Security::StoreScansWorker do 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 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 before do
allow(Security::StoreScansService).to receive(:execute) allow(Security::StoreScansService).to receive(:execute)
allow_next_found_instance_of(Ci::Pipeline) do |record| allow_next_found_instance_of(Ci::Pipeline) do |record|
allow(record).to receive(:can_store_security_reports?).and_return(can_store_security_reports) allow(record).to receive(:can_store_security_reports?).and_return(can_store_security_reports)
end 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 end
context 'when security reports can not be stored for the pipeline' do context 'when security reports can not be stored for the pipeline' do
...@@ -33,6 +90,12 @@ RSpec.describe Security::StoreScansWorker do ...@@ -33,6 +90,12 @@ RSpec.describe Security::StoreScansWorker do
expect(Security::StoreScansService).to have_received(:execute) expect(Security::StoreScansService).to have_received(:execute)
end end
it 'scans security reports for token revocation' do
expect(::ScanSecurityReportSecretsWorker).to receive(:perform_async)
run_worker
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment