From 4b3f6a0657b1979cd6534e0e87f168eab72d6c3f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20C=CC=8Cavoj?= <mcavoj@gitlab.com>
Date: Thu, 15 Feb 2024 12:05:14 +0100
Subject: [PATCH] Prevent policy bot message on non-applicable branches
Changelog: fixed
EE: true
---
config/sidekiq_queues.yml | 2 +
.../ee/merge_requests/update_service.rb | 2 +-
.../update_approvals_service.rb | 6 +-
.../sync_license_scanning_rules_service.rb | 5 +-
ee/app/workers/all_queues.yml | 9 ++
.../sync_policy_violation_comment_worker.rb | 35 ++++++
.../ee/merge_requests/update_service_spec.rb | 8 +-
.../update_approvals_service_spec.rb | 31 ++++-
...ync_license_scanning_rules_service_spec.rb | 21 +++-
...nc_policy_violation_comment_worker_spec.rb | 110 ++++++++++++++++++
10 files changed, 218 insertions(+), 11 deletions(-)
create mode 100644 ee/app/workers/security/sync_policy_violation_comment_worker.rb
create mode 100644 ee/spec/workers/security/sync_policy_violation_comment_worker_spec.rb
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 2e5fa7923b32e4..69e82ea6738b33 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -711,6 +711,8 @@
- 2
- - security_scans_purge_by_job_id
- 1
+- - security_sync_policy_violation_comment
+ - 1
- - security_sync_scan_policies
- 1
- - security_unenforceable_policy_rules_notification
diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb
index 523c49d83dd7e0..5cd14293c5659a 100644
--- a/ee/app/services/ee/merge_requests/update_service.rb
+++ b/ee/app/services/ee/merge_requests/update_service.rb
@@ -58,7 +58,7 @@ def sync_any_merge_request_approval_rules(merge_request)
end
def notify_for_policy_violations(merge_request)
- ::Security::UnenforceablePolicyRulesNotificationWorker.perform_async(merge_request.id)
+ ::Security::SyncPolicyViolationCommentWorker.perform_async(merge_request.id)
end
end
end
diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
index a38a5e23d910e5..30f22a8998a630 100644
--- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb
+++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
@@ -37,7 +37,11 @@ def execute
update_required_approvals(violated_rules, unviolated_rules)
violations.add(violated_rules.pluck(:scan_result_policy_id), unviolated_rules.pluck(:scan_result_policy_id)) # rubocop:disable CodeReuse/ActiveRecord
violations.execute
- generate_policy_bot_comment(merge_request, all_scan_finding_rules, :scan_finding)
+ generate_policy_bot_comment(
+ merge_request,
+ all_scan_finding_rules.applicable_to_branch(merge_request.target_branch),
+ :scan_finding
+ )
end
private
diff --git a/ee/app/services/security/sync_license_scanning_rules_service.rb b/ee/app/services/security/sync_license_scanning_rules_service.rb
index c025afa6dd4b27..c98da6a2636412 100644
--- a/ee/app/services/security/sync_license_scanning_rules_service.rb
+++ b/ee/app/services/security/sync_license_scanning_rules_service.rb
@@ -53,7 +53,10 @@ def remove_required_license_finding_approval(merge_request)
log_violated_rules(merge_request, violated_rules)
violations.add(violated_rules.pluck(:scan_result_policy_id), unviolated_rules.pluck(:scan_result_policy_id)) # rubocop:disable CodeReuse/ActiveRecord
violations.execute
- generate_policy_bot_comment(merge_request, license_approval_rules, :license_scanning)
+ generate_policy_bot_comment(
+ merge_request,
+ license_approval_rules.applicable_to_branch(merge_request.target_branch),
+ :license_scanning)
end
def update_required_approvals(merge_request, violated_rules, unviolated_rules)
diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml
index 750f9c80ffcc5a..e036f778be7230 100644
--- a/ee/app/workers/all_queues.yml
+++ b/ee/app/workers/all_queues.yml
@@ -2046,6 +2046,15 @@
:weight: 1
:idempotent: true
:tags: []
+- :name: security_sync_policy_violation_comment
+ :worker_name: Security::SyncPolicyViolationCommentWorker
+ :feature_category: :security_policy_management
+ :has_external_dependencies: false
+ :urgency: :low
+ :resource_boundary: :unknown
+ :weight: 1
+ :idempotent: true
+ :tags: []
- :name: security_sync_scan_policies
:worker_name: Security::SyncScanPoliciesWorker
:feature_category: :security_policy_management
diff --git a/ee/app/workers/security/sync_policy_violation_comment_worker.rb b/ee/app/workers/security/sync_policy_violation_comment_worker.rb
new file mode 100644
index 00000000000000..183cb9a49b8939
--- /dev/null
+++ b/ee/app/workers/security/sync_policy_violation_comment_worker.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module Security
+ class SyncPolicyViolationCommentWorker
+ include ApplicationWorker
+ include ::Security::ScanResultPolicies::PolicyViolationCommentGenerator
+
+ idempotent!
+ data_consistency :sticky
+ feature_category :security_policy_management
+
+ def perform(merge_request_id)
+ merge_request = MergeRequest.find_by_id(merge_request_id)
+
+ unless merge_request
+ logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id))
+ return
+ end
+
+ return unless merge_request.project.licensed_feature_available?(:security_orchestration_policies)
+
+ approval_rules = merge_request.approval_rules
+ generate_comment(merge_request, approval_rules.scan_finding, :scan_finding)
+ generate_comment(merge_request, approval_rules.license_scanning, :license_scanning)
+ end
+
+ private
+
+ def generate_comment(merge_request, rules, report_type)
+ return if rules.blank?
+
+ generate_policy_bot_comment(merge_request, rules.applicable_to_branch(merge_request.target_branch), report_type)
+ end
+ end
+end
diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb
index 8157011de20f33..74fbe02e4f269a 100644
--- a/ee/spec/services/ee/merge_requests/update_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb
@@ -462,8 +462,8 @@ def update_merge_request(opts)
subject(:execute) { update_merge_request(opts) }
- it 'enqueues Security::UnenforceablePolicyRulesNotificationWorker' do
- expect(Security::UnenforceablePolicyRulesNotificationWorker).to receive(:perform_async).with(merge_request.id)
+ it 'enqueues Security::SyncPolicyViolationCommentWorker' do
+ expect(Security::SyncPolicyViolationCommentWorker).to receive(:perform_async).with(merge_request.id)
execute
end
@@ -471,8 +471,8 @@ def update_merge_request(opts)
context 'when target_branch is not changing' do
let(:opts) { {} }
- it 'does not enqueue Security::UnenforceablePolicyRulesNotificationWorker' do
- expect(Security::UnenforceablePolicyRulesNotificationWorker).not_to receive(:perform_async)
+ it 'does not enqueue Security::SyncPolicyViolationCommentWorker' do
+ expect(Security::SyncPolicyViolationCommentWorker).not_to receive(:perform_async)
execute
end
diff --git a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb
index 19fe8976df4ef9..e93f96f6265f33 100644
--- a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb
+++ b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb
@@ -42,6 +42,8 @@
end
end
+ let_it_be(:scan_result_policy_read) { create(:scan_result_policy_read) }
+
let!(:report_approver_rule) do
create(:report_approver_rule, :scan_finding,
merge_request: merge_request,
@@ -50,7 +52,7 @@
vulnerabilities_allowed: vulnerabilities_allowed,
severity_levels: severity_levels,
vulnerability_states: vulnerability_states,
- scan_result_policy_id: create(:scan_result_policy_read).id
+ scan_result_policy_read: scan_result_policy_read
)
end
@@ -170,10 +172,12 @@
it_behaves_like 'triggers policy bot comment', :scan_finding, false
context 'when there are other scan_finding violations' do
+ let_it_be(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
let_it_be(:scan_result_policy_read_other_scan_finding) { create(:scan_result_policy_read, project: project) }
let_it_be(:approval_project_rule_other) do
create(:approval_project_rule, :scan_finding, project: project, approvals_required: 1,
- scan_result_policy_read: scan_result_policy_read_other_scan_finding)
+ scan_result_policy_read: scan_result_policy_read_other_scan_finding,
+ protected_branches: [protected_branch])
end
let_it_be(:approver_rule_other) do
@@ -198,6 +202,27 @@
it_behaves_like 'triggers policy bot comment', :scan_finding, true, requires_approval: false
end
+ context 'when targeting an unprotected branch' do
+ let_it_be(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
+ let!(:report_approver_project_rule) do
+ create(:approval_project_rule, :scan_finding, project: project,
+ approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read,
+ protected_branches: [protected_branch])
+ end
+
+ let!(:report_approver_rule) do
+ create(:report_approver_rule, :scan_finding, merge_request: merge_request,
+ approval_project_rule: report_approver_project_rule,
+ approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read)
+ end
+
+ before do
+ merge_request.update!(target_branch: 'non-protected')
+ end
+
+ it_behaves_like 'triggers policy bot comment', :scan_finding, false, requires_approval: false
+ end
+
context 'when target pipeline is nil' do
let_it_be(:merge_request) do
create(:merge_request, source_branch: 'feature', target_branch: 'target-branch')
@@ -431,7 +456,7 @@
vulnerabilities_allowed: vulnerabilities_allowed,
severity_levels: severity_levels,
vulnerability_states: vulnerability_states,
- scan_result_policy_id: create(:scan_result_policy_read).id
+ scan_result_policy_read: scan_result_policy_read
)
end
diff --git a/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb b/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb
index 887ce8b7e94f9d..35f2944b94fbb4 100644
--- a/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb
+++ b/ee/spec/services/security/sync_license_scanning_rules_service_spec.rb
@@ -10,7 +10,7 @@
create(:merge_request, :with_merge_request_pipeline, source_project: project)
end
- let_it_be(:pipeline) do
+ let_it_be_with_reload(:pipeline) do
create(
:ee_ci_pipeline,
:success,
@@ -64,14 +64,24 @@
let(:license_states) { ['newly_detected'] }
let(:match_on_inclusion_license) { true }
let(:approvals_required) { 1 }
+ let_it_be(:protected_branch) do
+ create(:protected_branch, name: merge_request.target_branch, project: project)
+ end
let(:scan_result_policy_read) do
create(:scan_result_policy_read, license_states: license_states,
match_on_inclusion_license: match_on_inclusion_license)
end
+ let!(:license_finding_project_rule) do
+ create(:approval_project_rule, :license_scanning, project: project,
+ approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read,
+ protected_branches: [protected_branch])
+ end
+
let!(:license_finding_rule) do
create(:report_approver_rule, :license_scanning, merge_request: merge_request,
+ approval_project_rule: license_finding_project_rule,
approvals_required: approvals_required, scan_result_policy_read: scan_result_policy_read)
end
@@ -118,6 +128,15 @@
it_behaves_like 'triggers policy bot comment', :license_scanning, true, requires_approval: false
end
+ context 'when targeting an unprotected branch' do
+ before do
+ merge_request.update!(target_branch: 'non-protected')
+ pipeline.update!(ref: 'non-protected')
+ end
+
+ it_behaves_like 'triggers policy bot comment', :license_scanning, false, requires_approval: false
+ end
+
context 'when most recent base pipeline lacks SBOM report' do
let(:pipeline_without_sbom) do
create(
diff --git a/ee/spec/workers/security/sync_policy_violation_comment_worker_spec.rb b/ee/spec/workers/security/sync_policy_violation_comment_worker_spec.rb
new file mode 100644
index 00000000000000..9124206eb8165c
--- /dev/null
+++ b/ee/spec/workers/security/sync_policy_violation_comment_worker_spec.rb
@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SyncPolicyViolationCommentWorker, feature_category: :security_policy_management do
+ describe '#perform' do
+ let_it_be(:project) { create(:project) }
+ let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+ let(:merge_request_id) { merge_request.id }
+ let(:licensed_feature) { true }
+ let(:approvals_required) { 1 }
+ let_it_be(:protected_branch) { create(:protected_branch, project: project, name: merge_request.target_branch) }
+ let_it_be(:scan_result_policy_read) { create(:scan_result_policy_read) }
+ let_it_be(:scan_finding_project_rule) do
+ create(:approval_project_rule, :scan_finding, project: project, protected_branches: [protected_branch],
+ scan_result_policy_read: scan_result_policy_read)
+ end
+
+ let!(:scan_finding_rule) do
+ create(:report_approver_rule, :scan_finding, merge_request: merge_request,
+ approval_project_rule: scan_finding_project_rule, approvals_required: approvals_required,
+ scan_result_policy_read: scan_result_policy_read)
+ end
+
+ let_it_be(:license_scanning_project_rule) do
+ create(:approval_project_rule, :license_scanning, project: project, protected_branches: [protected_branch],
+ scan_result_policy_read: scan_result_policy_read)
+ end
+
+ let!(:license_scanning_rule) do
+ create(:report_approver_rule, :license_scanning, merge_request: merge_request,
+ approval_project_rule: scan_finding_project_rule, approvals_required: approvals_required,
+ scan_result_policy_read: scan_result_policy_read)
+ end
+
+ before do
+ stub_licensed_features(security_orchestration_policies: licensed_feature)
+ end
+
+ subject(:perform) { described_class.new.perform(merge_request_id) }
+
+ it_behaves_like 'an idempotent worker' do
+ let(:job_args) { [merge_request_id] }
+ end
+
+ it 'enqueues Security::GeneratePolicyViolationCommentWorker' do
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'scan_finding', 'violated_policy' => false, 'requires_approval' => true })
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'license_scanning', 'violated_policy' => false, 'requires_approval' => true })
+
+ perform
+ end
+
+ context 'when there are violations' do
+ before do
+ create(:scan_result_policy_violation, scan_result_policy_read: scan_result_policy_read,
+ merge_request: merge_request, project: project)
+ end
+
+ it 'enqueues Security::GeneratePolicyViolationCommentWorker with correct params' do
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'scan_finding', 'violated_policy' => true, 'requires_approval' => true })
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'license_scanning', 'violated_policy' => true, 'requires_approval' => true })
+
+ perform
+ end
+
+ context 'when approvals are optional' do
+ let(:approvals_required) { 0 }
+
+ it 'enqueues Security::GeneratePolicyViolationCommentWorker with correct params' do
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'scan_finding', 'violated_policy' => true, 'requires_approval' => false })
+ expect(Security::GeneratePolicyViolationCommentWorker)
+ .to receive(:perform_async).with(merge_request.id,
+ { 'report_type' => 'license_scanning', 'violated_policy' => true, 'requires_approval' => false })
+
+ perform
+ end
+ end
+ end
+
+ context 'with a non-existing merge request' do
+ let(:merge_request_id) { non_existing_record_id }
+
+ it 'does not enqueue the worker' do
+ expect(Security::GeneratePolicyViolationCommentWorker).not_to receive(:perform_async)
+
+ perform
+ end
+ end
+
+ context 'when feature is not licensed' do
+ let(:licensed_feature) { false }
+
+ it 'does not enqueue the worker' do
+ expect(Security::GeneratePolicyViolationCommentWorker).not_to receive(:perform_async)
+
+ perform
+ end
+ end
+ end
+end
--
GitLab