diff --git a/app/validators/json_schemas/scan_result_policy_violation_data.json b/app/validators/json_schemas/scan_result_policy_violation_data.json
index 555f9bebcae81cff793a6f16ba16f075db9daf03..2e6f8c1f19a83012a08fe236837ffee43d0f6850 100644
--- a/app/validators/json_schemas/scan_result_policy_violation_data.json
+++ b/app/validators/json_schemas/scan_result_policy_violation_data.json
@@ -7,7 +7,28 @@
"type": "object"
},
"violations": {
- "type": "object"
+ "type": "object",
+ "properties": {
+ "any_merge_request": {
+ "type": "object",
+ "properties": {
+ "commits": {
+ "oneOf": [
+ {
+ "type": "boolean"
+ },
+ {
+ "type": "array",
+ "minItems": 1,
+ "items": {
+ "type": "string"
+ }
+ }
+ ]
+ }
+ }
+ }
+ }
},
"errors": {
"type": "array",
diff --git a/ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb b/ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb
index 8b6762c4319963edcdfcafd1061eb0888e3c53cc..b28270bb40e8471f8a41a02c0c75275aa829b6e3 100644
--- a/ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb
+++ b/ee/app/services/security/scan_result_policies/sync_any_merge_request_rules_service.rb
@@ -10,6 +10,7 @@ def initialize(merge_request)
@merge_request = merge_request
@violations = Security::SecurityOrchestrationPolicies::UpdateViolationsService
.new(merge_request, :any_merge_request)
+ @violations_by_policy = {}
end
def execute
@@ -20,7 +21,7 @@ def execute
private
- attr_reader :merge_request, :violations
+ attr_reader :merge_request, :violations, :violations_by_policy
delegate :project, to: :merge_request, private: true
@@ -36,22 +37,30 @@ def sync_required_approvals
log_violated_rules(violated_rules)
# rubocop:disable CodeReuse/ActiveRecord
+ violated_policies_ids -= unviolated_rules.pluck(:scan_result_policy_id)
violations.add(
- violated_policies_ids - unviolated_rules.pluck(:scan_result_policy_id),
+ violated_policies_ids,
unviolated_policies_ids + unviolated_rules.pluck(:scan_result_policy_id)
)
# rubocop:enable CodeReuse/ActiveRecord
+ save_violation_data(violated_policies_ids)
violations.execute
generate_policy_bot_comment(merge_request, violated_rules, :any_merge_request)
end
def evaluate_policy_violations(scan_result_policy_reads)
- has_unsigned_commits = !merge_request.commits(load_from_gitaly: true).all?(&:has_signature?)
+ unsigned_commits = merge_request.commits(load_from_gitaly: true)
+ .select { |commit| !commit.has_signature? }.map(&:short_id)
violated, unviolated = scan_result_policy_reads.partition do |scan_result_policy_read|
- next false unless scan_result_policy_read.commits_any? ||
- (scan_result_policy_read.commits_unsigned? && has_unsigned_commits)
+ targets_any_commits = scan_result_policy_read.commits_any?
+ next false unless targets_any_commits || (scan_result_policy_read.commits_unsigned? && unsigned_commits.any?)
- policy_affected_by_target_branch?(scan_result_policy_read)
+ policy_affected_by_target_branch?(scan_result_policy_read).tap do |violated|
+ next unless violated
+
+ violations_by_policy[scan_result_policy_read.id] =
+ targets_any_commits ? true : Security::ScanResultPolicyViolation.trim_violations(unsigned_commits)
+ end
end
[violated.pluck(:id), unviolated.pluck(:id)] # rubocop:disable CodeReuse/ActiveRecord
end
@@ -136,6 +145,12 @@ def log_violated_rule(**attributes)
}
Gitlab::AppJsonLogger.info(message: 'Updating MR approval rule', **default_attributes.merge(attributes))
end
+
+ def save_violation_data(violated_policy_ids)
+ violated_policy_ids.each do |policy_id|
+ violations.add_violation(policy_id, { commits: violations_by_policy[policy_id] })
+ end
+ end
end
end
end
diff --git a/ee/spec/models/security/scan_result_policy_violation_spec.rb b/ee/spec/models/security/scan_result_policy_violation_spec.rb
index 08e00b47df88d2b4bf53edb821154f7c5e987e3b..a467005a7b8a3917e656d1f8bf7c4eaef5985ac2 100644
--- a/ee/spec/models/security/scan_result_policy_violation_spec.rb
+++ b/ee/spec/models/security/scan_result_policy_violation_spec.rb
@@ -20,20 +20,52 @@
it { is_expected.not_to allow_value('string').for(:violation_data) }
it { is_expected.to allow_value({}).for(:violation_data) }
- it do
+ it 'allows combination of all possible values' do
is_expected.to allow_value(
- { violations: { uuids: { newly_detected: ['123'], previously_existing: ['456'] }, licenses: ['MIT'] },
- context: { pipeline_ids: [123], target_pipeline_ids: [456] } }
+ {
+ violations: {
+ scan_finding: { uuids: { newly_detected: ['123'], previously_existing: ['456'] } },
+ license_scanning: { licenses: ['MIT'] },
+ any_merge_request: { commits: ['abcd1234'] }
+ },
+ context: { pipeline_ids: [123], target_pipeline_ids: [456] },
+ errors: [{ error: 'SCAN_REMOVED', missing_scans: ['sast'] }]
+ }
).for(:violation_data)
end
- it do
- is_expected.to allow_value(
- { errors: [{ error: 'SCAN_REMOVED', missing_scans: ['sast'] }] }
- ).for(:violation_data)
+ describe 'errors' do
+ it do
+ is_expected.to allow_value(
+ { errors: [{ error: 'SCAN_REMOVED', missing_scans: ['sast'] }] }
+ ).for(:violation_data)
+ end
end
it { is_expected.not_to allow_value({ errors: [{}] }).for(:violation_data) }
+
+ describe 'violations' do
+ using RSpec::Parameterized::TableSyntax
+
+ describe 'commits' do
+ where(:report_type, :data, :valid) do
+ :any_merge_request | { commits: ['abcd1234'] } | true
+ :any_merge_request | { commits: true } | true
+ :any_merge_request | { commits: 'abcd1234' } | false
+ :any_merge_request | { commits: [] } | false
+ end
+
+ with_them do
+ it do
+ if valid
+ expect(violation).to allow_value(violations: { report_type => data }).for(:violation_data)
+ else
+ expect(violation).not_to allow_value(violations: { report_type => data }).for(:violation_data)
+ end
+ end
+ end
+ end
+ end
end
end
diff --git a/ee/spec/services/security/scan_result_policies/sync_any_merge_request_rules_service_spec.rb b/ee/spec/services/security/scan_result_policies/sync_any_merge_request_rules_service_spec.rb
index 300bd79245232579621f88e3dc1df09982958575..8f44ec60e22f7155998dcec0cc31bc5e938f782e 100644
--- a/ee/spec/services/security/scan_result_policies/sync_any_merge_request_rules_service_spec.rb
+++ b/ee/spec/services/security/scan_result_policies/sync_any_merge_request_rules_service_spec.rb
@@ -18,8 +18,8 @@
subject(:execute) { service.execute }
let(:approvals_required) { 1 }
- let(:signed_commit) { instance_double(Commit, has_signature?: true) }
- let(:unsigned_commit) { instance_double(Commit, has_signature?: false) }
+ let(:signed_commit) { instance_double(Commit, has_signature?: true, short_id: 'abcd1234') }
+ let(:unsigned_commit) { instance_double(Commit, has_signature?: false, short_id: 'dcba5678') }
let_it_be(:protected_branch) do
create(:protected_branch, name: merge_request.target_branch, project: project)
end
@@ -59,6 +59,10 @@
it_behaves_like 'does not update approval rules'
it_behaves_like 'does not trigger policy bot comment'
+
+ it 'creates no violation records' do
+ expect { execute }.not_to change { merge_request.scan_result_policy_violations.count }
+ end
end
describe 'approval rules' do
@@ -108,7 +112,7 @@
it_behaves_like 'triggers policy bot comment', :any_merge_request, true, requires_approval: false
end
- context 'when approval are required but approval_merge_request_rules have been made optional' do
+ context 'when approvals are required but approval_merge_request_rules have been made optional' do
let!(:approval_project_rule) do
create(:approval_project_rule, :any_merge_request, project: project, approvals_required: 1,
scan_result_policy_read: scan_result_policy_read)
@@ -127,11 +131,11 @@
it_behaves_like 'triggers policy bot comment', :any_merge_request, true
end
- where(:policy_commits, :merge_request_commits) do
- :unsigned | [ref(:unsigned_commit)]
- :unsigned | [ref(:signed_commit), ref(:unsigned_commit)]
- :any | [ref(:signed_commit)]
- :any | [ref(:unsigned_commit)]
+ where(:policy_commits, :merge_request_commits, :expected_violation) do
+ :unsigned | [ref(:unsigned_commit)] | ['dcba5678']
+ :unsigned | [ref(:signed_commit), ref(:unsigned_commit)] | ['dcba5678']
+ :any | [ref(:signed_commit)] | true
+ :any | [ref(:unsigned_commit)] | true
end
with_them do
@@ -145,6 +149,25 @@
execute
end
+ it 'persists violation details' do
+ expect { execute }
+ .to change { merge_request.scan_result_policy_violations.last&.violation_data }
+ .from(nil)
+ .to('violations' => { 'any_merge_request' => { 'commits' => expected_violation } })
+ end
+
+ context 'when feature flag "save_policy_violation_data" is disabled' do
+ before do
+ stub_feature_flags(save_policy_violation_data: false)
+ end
+
+ it 'does not persist violation details' do
+ execute
+
+ expect(merge_request.scan_result_policy_violations.last.violation_data).to be_nil
+ end
+ end
+
it_behaves_like 'when no policy is applicable due to the policy scope' do
it_behaves_like 'does not update approval rules'
end
@@ -155,15 +178,20 @@
let!(:approver_rule) { nil }
context 'when policies target commits' do
+ let(:violation) { merge_request.scan_result_policy_violations.first }
let_it_be(:scan_result_policy_read_with_commits, reload: true) do
create(:scan_result_policy_read, project: project, commits: :unsigned, rule_idx: 0)
end
+ before do
+ allow(merge_request).to receive(:commits).and_return([unsigned_commit])
+ end
+
it 'creates violations for policies that have no approval rules' do
expect { execute }.to change { merge_request.scan_result_policy_violations.count }.by(1)
- expect(merge_request.scan_result_policy_violations.first.scan_result_policy_read).to(
- eq scan_result_policy_read_with_commits
- )
+ expect(violation.scan_result_policy_read).to(eq scan_result_policy_read_with_commits)
+ expect(violation.violation_data)
+ .to match('violations' => { 'any_merge_request' => { 'commits' => ['dcba5678'] } })
end
context 'with previous violation for policy that is now unviolated' do