From c3d869027cfc9660d40e452dd46a35c096b8422d Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Thu, 26 Jun 2025 18:31:25 +0200 Subject: [PATCH 1/9] Audit event for merge requests merged with violations Adds audit event for merge requests merged with security policy violations. --- ..._request_merged_with_policy_violations.yml | 10 + doc/user/compliance/audit_event_types.md | 1 + ee/app/models/approval_wrapped_rule.rb | 10 +- ...h_policy_violations_audit_event_service.rb | 97 +++++++++ ...cleanup_merge_request_violations_worker.rb | 14 +- ...ed_with_policy_violations_audit_events.yml | 10 + ee/spec/models/approval_wrapped_rule_spec.rb | 52 +++++ ...icy_violations_audit_event_service_spec.rb | 191 ++++++++++++++++++ ...up_merge_request_violations_worker_spec.rb | 21 ++ 9 files changed, 400 insertions(+), 6 deletions(-) create mode 100644 config/audit_events/types/merge_request_merged_with_policy_violations.yml create mode 100644 ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb create mode 100644 ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml create mode 100644 ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb diff --git a/config/audit_events/types/merge_request_merged_with_policy_violations.yml b/config/audit_events/types/merge_request_merged_with_policy_violations.yml new file mode 100644 index 00000000000000..be73d13cd42c5f --- /dev/null +++ b/config/audit_events/types/merge_request_merged_with_policy_violations.yml @@ -0,0 +1,10 @@ +--- +name: merge_request_merged_with_policy_violations +description: A merge request merged with security policy violations +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/work_items/549813 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775 +feature_category: security_policy_management +milestone: '18.2' +saved_to_database: true +streamed: true +scope: [Project] diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 3c9fbdd6aef471..796f9798bbf288 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -539,6 +539,7 @@ Audit event types belong to the following product categories. | [`security_policy_delete`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is deleted | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`security_policy_update`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is updated | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`merge_request_branch_bypassed_by_security_policy`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195942) | The merge request's approval is bypassed by the branches configured in the security policy | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/issues/549646) | Project | +| [`merge_request_merged_with_policy_violations`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775) | A merge request merged with security policy violations | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/549813) | Project | | [`policy_violations_detected`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193482) | Security policy violation is detected in the merge request | {{< icon name="dotted-circle" >}} No | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/549811) | Project | | [`policy_violations_resolved`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193482) | Security policy violations are resolved in the merge request | {{< icon name="dotted-circle" >}} No | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/issues/549812) | Project | diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index dcfc7c4ac9164a..14040684dae3cf 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -24,7 +24,7 @@ class ApprovalWrappedRule def_delegators( :@approval_rule, :regular?, :any_approver?, :code_owner?, :report_approver?, - :overridden?, :id, :users, :groups, :code_owner, + :overridden?, :id, :users, :groups, :code_owner, :from_scan_result_policy?, :source_rule, :rule_type, :report_type, :approvals_required, :section, :to_global_id ) @@ -99,7 +99,7 @@ def invalid_rule? def allow_merge_when_invalid? return true if fail_open? - !approval_rule.from_scan_result_policy? || + !from_scan_result_policy? || Security::OrchestrationPolicyConfiguration.policy_management_project?(project) end @@ -143,7 +143,7 @@ def unactioned_approvers end def name - return approval_rule.name unless approval_rule.from_scan_result_policy? + return approval_rule.name unless from_scan_result_policy? if policy_has_multiple_actions? "#{approval_rule.policy_name} - Action #{approval_rule.approval_policy_action_idx + 1}" @@ -152,12 +152,12 @@ def name end end - private - def fail_open? approval_rule.scan_result_policy_read&.fail_open? || false end + private + def filter_approvers(approvers) filtered_approvers = ApprovalState.filter_author(approvers, merge_request) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb new file mode 100644 index 00000000000000..bd8bb5865bd33b --- /dev/null +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +module MergeRequests + class MergedWithPolicyViolationsAuditEventService + include Gitlab::Utils::StrongMemoize + + attr_reader :merge_request, :approval_state + + def initialize(merge_request) + @merge_request = merge_request + @approval_state = ApprovalState.new(merge_request) + end + + def execute + return unless merge_request.merged? + + audit_context = { + name: 'merge_request_merged_with_policy_violations', + author: merged_by, + scope: policy_configuration&.security_policy_management_project, + target: merge_request, + message: "Merge request merged with #{details.violations_count} security policy violations", + additional_details: audit_details + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + private + + def policy_configuration + merge_request.project.security_orchestration_policy_configuration + end + strong_memoize_attr :policy_configuration + + def merged_by + merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -3, name: 'Unknown User') + end + + def audit_details + { + merge_request_title: merge_request.title, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + merged_at: merge_request.merged_at, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + project_id: merge_request.project.id, + project_name: merge_request.project.name, + project_full_path: merge_request.project.full_path, + security_policy_approval_rules: policy_approval_rules, + violation_details: violation_details + } + end + + # There can be violated policies which were skipped because of invalid rules + # We collect both types here for visibility + def wrapped_approval_rules + approval_state.wrapped_approval_rules + approval_state.invalid_approvers_rules + end + + def policy_approval_rules + wrapped_approval_rules.filter_map do |wrapped_rule| + next unless wrapped_rule.from_scan_result_policy? + + { + name: wrapped_rule.name, + report_type: wrapped_rule.report_type, + approvals_required: wrapped_rule.approvals_required, + approved: wrapped_rule.approved?, + approved_approvers: wrapped_rule.approved_approvers.map(&:username).sort, + invalid_rule: wrapped_rule.invalid_rule?, + fail_open: wrapped_rule.fail_open? + } + end + end + + def violation_details + { + fail_open_policies: details.fail_open_policies.as_json, + fail_closed_policies: details.fail_closed_policies.as_json, + warn_mode_policies: details.warn_mode_policies.as_json, + new_scan_finding_violations: details.new_scan_finding_violations.as_json, + previous_scan_finding_violations: details.previous_scan_finding_violations.as_json, + license_scanning_violations: details.license_scanning_violations.as_json, + any_merge_request_violations: details.any_merge_request_violations.as_json, + errors: details.errors.as_json, + comparison_pipelines: details.comparison_pipelines.as_json + } + end + + def details + ::Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) + end + strong_memoize_attr :details + end +end diff --git a/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb b/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb index cdcbb94fd8a729..7924aa813cf280 100644 --- a/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb +++ b/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb @@ -15,7 +15,12 @@ def handle_event(event) project = merge_request.project return unless project.licensed_feature_available?(:security_orchestration_policies) - log_running_violations_after_merge(merge_request) if event.is_a?(::MergeRequests::MergedEvent) + return if merge_request.scan_result_policy_violations.none? + + if event.is_a?(::MergeRequests::MergedEvent) + log_running_violations_after_merge(merge_request) + audit_merged_with_policy_violations(merge_request) + end merge_request.scan_result_policy_violations.delete_all(:delete_all) end @@ -36,6 +41,13 @@ def log_running_violations_after_merge(merge_request) approval_policy_rule_ids: running_violations.filter_map(&:approval_policy_rule_id) ) end + + def audit_merged_with_policy_violations(merge_request) + return unless Feature.enabled?(:collect_merge_request_merged_with_policy_violations_audit_events, + merge_request.project) + + MergeRequests::MergedWithPolicyViolationsAuditEventService.new(merge_request).execute + end end end end diff --git a/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml b/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml new file mode 100644 index 00000000000000..c78d2eb5f3831f --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml @@ -0,0 +1,10 @@ +--- +name: collect_merge_request_merged_with_policy_violations_audit_events +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/549813 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/551784 +milestone: '18.2' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb index 480177a25af519..98008ae8f3a38a 100644 --- a/ee/spec/models/approval_wrapped_rule_spec.rb +++ b/ee/spec/models/approval_wrapped_rule_spec.rb @@ -507,4 +507,56 @@ def approved_approvers_for_rule_id(rule_id) end end end + + describe '#fail_open?' do + subject { approval_wrapped_rule.fail_open? } + + context 'when rule has a linked scan_result_policy_read' do + before do + rule.update!(scan_result_policy_read: scan_result_policy_read) + end + + context 'when rule is fail open' do + let(:scan_result_policy_read) { create(:scan_result_policy_read, :fail_open) } + + it { is_expected.to eq(true) } + end + + context 'when rule is not fail closed' do + let(:scan_result_policy_read) { create(:scan_result_policy_read, :fail_closed) } + + it { is_expected.to eq(false) } + end + end + + context 'when rule has no linked scan_result_policy_read' do + it { is_expected.to eq(false) } + end + end + + describe "#from_scan_result_policy" do + subject { approval_wrapped_rule.from_scan_result_policy? } + + it 'delegates from_scan_result_policy to approval_rule' do + expect(rule).to receive(:from_scan_result_policy?) + + subject + end + + context 'when the approval rule is from a scan result policy' do + before do + rule.update!(report_type: :scan_finding) + end + + it { is_expected.to eq(true) } + end + + context 'when the approval rule is not from a scan result policy' do + before do + rule.update!(report_type: :code_coverage) + end + + it { is_expected.to eq(false) } + end + end end diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb new file mode 100644 index 00000000000000..4e27af1c687696 --- /dev/null +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -0,0 +1,191 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MergedWithPolicyViolationsAuditEventService, feature_category: :security_policy_management do + let_it_be(:merger) { create :user } + let_it_be(:approver) { create :user, username: 'approver one' } + let_it_be(:mr_author) { create :user, username: 'author one' } + let_it_be(:project) { create :project, :repository } + let_it_be(:merge_time) { Time.now.utc } + let_it_be_with_reload(:merge_request) do + create :merge_request, + title: 'MR One', + description: 'This was a triumph', + author: mr_author, + source_project: project, + target_project: project + end + + let_it_be(:policy_project) { create(:project, :repository) } + let_it_be_with_reload(:policy_configuration) do + create(:security_orchestration_policy_configuration, project: project, + security_policy_management_project: policy_project) + end + + let_it_be(:security_policy_name) { 'Test Policy' } + let_it_be_with_reload(:security_policy) do + create(:security_policy, :approval_policy, + name: security_policy_name, + security_orchestration_policy_configuration: policy_configuration) + end + + let(:service) { described_class.new(merge_request) } + + def build_violation_details(policy, data, status = :failed) + create(:scan_result_policy_violation, status, project: project, merge_request: merge_request, + scan_result_policy_read: policy, approval_policy_rule: approval_policy_rule, violation_data: data) + end + + def build_violation_with_error(policy, error, status = :failed, **extra_data) + build_violation_details(policy, { 'errors' => [{ 'error' => error, **extra_data }] }, status) + end + + describe '#execute' do + let(:execute_service) { service.execute } + + let(:audit_context) do + { + name: 'merge_request_merged_with_policy_violations', + author: merger, + scope: policy_project, + target: merge_request, + message: "Merge request merged with 1 security policy violations", + additional_details: { + merge_request_title: merge_request.title, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + merged_at: merge_request.merged_at, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + project_id: project.id, + project_name: project.name, + project_full_path: project.full_path, + security_policy_approval_rules: policy_approval_rules, + violation_details: violation_details + } + } + end + + shared_examples 'recording the audit event' do + it 'records a merge_request_merged_with_policy_violations audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + execute_service + end + end + + shared_examples 'not recording the audit event' do + it 'does not record a merge_request_merged_with_policy_violations audit event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(anything) + + execute_service + end + end + + context 'when merge request is merged' do + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, :license_finding, security_policy: security_policy) + end + + let_it_be(:approval_rule) do + create( + :report_approver_rule, + :license_scanning, + merge_request: merge_request, + approval_policy_rule: approval_policy_rule, + approvals_required: 1, + user_ids: [approver.id] + ) + end + + let_it_be(:approval_merge_request_rules_approved_approver) do + create(:approval_merge_request_rules_approved_approver, + approval_merge_request_rule: approval_rule, user: approver) + end + + let(:policy_approval_rules) do + [ + { + name: approval_rule.name, + report_type: approval_rule.report_type, + approved: true, + approvals_required: 1, + approved_approvers: ['approver one'], + invalid_rule: false, + fail_open: false + } + ] + end + + let(:license_scanning_violations_json) do + { + "dependencies" => %w[A B], + "license" => "MIT License", + "url" => "https://spdx.org/licenses/MIT.html" + } + end + + let(:violations_count) { 1 } + let(:violation_details) do + { + fail_open_policies: [], + fail_closed_policies: [security_policy_name], + warn_mode_policies: [], + new_scan_finding_violations: [], + previous_scan_finding_violations: [], + license_scanning_violations: [license_scanning_violations_json], + any_merge_request_violations: [], + errors: [], + comparison_pipelines: [] + } + end + + before do + policy = create(:scan_result_policy_read, project: project, + security_orchestration_policy_configuration: policy_configuration) + + create(:report_approver_rule, :license_scanning, merge_request: merge_request, + scan_result_policy_read: policy, name: security_policy_name) + + build_violation_details(policy, + violations: { license_scanning: { 'MIT License' => %w[A B] } } + ) + merge_request.update!(state_id: MergeRequest.available_states[:merged]) + merge_request.metrics.update!(merged_at: merge_time, merged_by: merger) + end + + it_behaves_like 'recording the audit event' + + context 'with invalid rules' do + before do + approval_rule.update_columns(approvals_required: 2) + end + + let(:policy_approval_rules) do + [ + { + name: approval_rule.name, + report_type: approval_rule.report_type, + approved: false, + approvals_required: 2, + approved_approvers: ['approver one'], + invalid_rule: true, + fail_open: false + } + ] + end + + it_behaves_like 'recording the audit event' + end + end + + context 'when merge request is not merged' do + before do + allow(merge_request).to receive(:merged?).and_return(false) + end + + it_behaves_like 'not recording the audit event' + end + end +end diff --git a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb index 153aff2a3f5646..80230b4ea72b90 100644 --- a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb +++ b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb @@ -62,6 +62,7 @@ context 'with existing merge request' do context 'when event is MergedEvent' do let(:event) { merge_request_merged_event } + let(:audit_event_service) { MergeRequests::MergedWithPolicyViolationsAuditEventService.new(merge_request) } it_behaves_like 'an idempotent worker' it_behaves_like 'deletes approval policy violations' @@ -77,6 +78,26 @@ perform end + it 'calls MergedWithPolicyViolationsAuditEventService' do + allow(MergeRequests::MergedWithPolicyViolationsAuditEventService).to receive(:new).with( + merge_request).and_return(audit_event_service) + expect(audit_event_service).to receive(:execute) + + perform + end + + context 'when thee collect_merge_request_merged_with_policy_violations_audit_events feature is disabled' do + before do + stub_feature_flags(collect_merge_request_merged_with_policy_violations_audit_events: false) + end + + it 'does not call MergedWithPolicyViolationsAuditEventService' do + expect(MergeRequests::MergedWithPolicyViolationsAuditEventService).not_to receive(:new) + + perform + end + end + context 'when there are no running violations' do before do merge_request_violation.update!(status: :failed) -- GitLab From 827144611c900d718b87d6a799aaa7685111df1b Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Mon, 30 Jun 2025 11:08:29 +0200 Subject: [PATCH 2/9] Update rspec method name --- ee/spec/models/approval_wrapped_rule_spec.rb | 6 +-- ...up_merge_request_violations_worker_spec.rb | 52 ++++++++++++------- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb index 98008ae8f3a38a..909b8a19b9c9fd 100644 --- a/ee/spec/models/approval_wrapped_rule_spec.rb +++ b/ee/spec/models/approval_wrapped_rule_spec.rb @@ -517,13 +517,13 @@ def approved_approvers_for_rule_id(rule_id) end context 'when rule is fail open' do - let(:scan_result_policy_read) { create(:scan_result_policy_read, :fail_open) } + let(:scan_result_policy_read) { build(:scan_result_policy_read, :fail_open) } it { is_expected.to eq(true) } end context 'when rule is not fail closed' do - let(:scan_result_policy_read) { create(:scan_result_policy_read, :fail_closed) } + let(:scan_result_policy_read) { build(:scan_result_policy_read, :fail_closed) } it { is_expected.to eq(false) } end @@ -534,7 +534,7 @@ def approved_approvers_for_rule_id(rule_id) end end - describe "#from_scan_result_policy" do + describe "#from_scan_result_policy?" do subject { approval_wrapped_rule.from_scan_result_policy? } it 'delegates from_scan_result_policy to approval_rule' do diff --git a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb index 80230b4ea72b90..5c8089262e3f55 100644 --- a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb +++ b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb @@ -59,10 +59,25 @@ end end + shared_examples_for 'not logging running violations' do + it 'does not log running violations' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + + perform + end + end + + shared_examples_for 'not recording audit event for violations' do + it 'does not record merged with policy violations audit event' do + expect(MergeRequests::MergedWithPolicyViolationsAuditEventService).not_to receive(:new) + + perform + end + end + context 'with existing merge request' do context 'when event is MergedEvent' do let(:event) { merge_request_merged_event } - let(:audit_event_service) { MergeRequests::MergedWithPolicyViolationsAuditEventService.new(merge_request) } it_behaves_like 'an idempotent worker' it_behaves_like 'deletes approval policy violations' @@ -78,24 +93,29 @@ perform end - it 'calls MergedWithPolicyViolationsAuditEventService' do - allow(MergeRequests::MergedWithPolicyViolationsAuditEventService).to receive(:new).with( - merge_request).and_return(audit_event_service) - expect(audit_event_service).to receive(:execute) + it 'records merged with policy violations audit event' do + allow_next_instance_of(::MergeRequests::MergedWithPolicyViolationsAuditEventService) do |audit_event_service| + expect(audit_event_service).to receive(:execute) + end perform end - context 'when thee collect_merge_request_merged_with_policy_violations_audit_events feature is disabled' do + context 'when there is no merge_request scan result policy violations' do before do - stub_feature_flags(collect_merge_request_merged_with_policy_violations_audit_events: false) + merge_request_violation.update!(merge_request_id: unrelated_merge_request_violation.merge_request_id) end - it 'does not call MergedWithPolicyViolationsAuditEventService' do - expect(MergeRequests::MergedWithPolicyViolationsAuditEventService).not_to receive(:new) + it_behaves_like 'not logging running violations' + it_behaves_like 'not recording audit event for violations' + end - perform + context 'when the collect merged with policy violations audit event feature is disabled' do + before do + stub_feature_flags(collect_merge_request_merged_with_policy_violations_audit_events: false) end + + it_behaves_like 'not recording audit event for violations' end context 'when there are no running violations' do @@ -103,11 +123,7 @@ merge_request_violation.update!(status: :failed) end - it 'does not log running violations' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - perform - end + it_behaves_like 'not logging running violations' end end @@ -117,11 +133,7 @@ it_behaves_like 'an idempotent worker' it_behaves_like 'deletes approval policy violations' - it 'does not log running violations' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - perform - end + it_behaves_like 'not logging running violations' end context 'when feature is not licensed' do -- GitLab From 7751928065d8f9a7a584244fcd75f38bbbf257b2 Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Mon, 30 Jun 2025 13:06:53 +0200 Subject: [PATCH 3/9] Update audit event scope --- ...ith_policy_violations_audit_event_service.rb | 17 +++++++---------- ...olicy_violations_audit_event_service_spec.rb | 10 +++++----- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index bd8bb5865bd33b..d0e7f06928865b 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -4,11 +4,10 @@ module MergeRequests class MergedWithPolicyViolationsAuditEventService include Gitlab::Utils::StrongMemoize - attr_reader :merge_request, :approval_state + attr_reader :merge_request def initialize(merge_request) @merge_request = merge_request - @approval_state = ApprovalState.new(merge_request) end def execute @@ -17,7 +16,7 @@ def execute audit_context = { name: 'merge_request_merged_with_policy_violations', author: merged_by, - scope: policy_configuration&.security_policy_management_project, + scope: merge_request.project, target: merge_request, message: "Merge request merged with #{details.violations_count} security policy violations", additional_details: audit_details @@ -28,11 +27,6 @@ def execute private - def policy_configuration - merge_request.project.security_orchestration_policy_configuration - end - strong_memoize_attr :policy_configuration - def merged_by merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -3, name: 'Unknown User') end @@ -89,9 +83,12 @@ def violation_details } end + def approval_state + @_approval_state ||= ApprovalState.new(merge_request) + end + def details - ::Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) + @_details ||= Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) end - strong_memoize_attr :details end end diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb index 4e27af1c687696..53784fcf98c5d7 100644 --- a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -18,13 +18,13 @@ end let_it_be(:policy_project) { create(:project, :repository) } - let_it_be_with_reload(:policy_configuration) do + let_it_be(:policy_configuration) do create(:security_orchestration_policy_configuration, project: project, security_policy_management_project: policy_project) end let_it_be(:security_policy_name) { 'Test Policy' } - let_it_be_with_reload(:security_policy) do + let_it_be(:security_policy) do create(:security_policy, :approval_policy, name: security_policy_name, security_orchestration_policy_configuration: policy_configuration) @@ -48,7 +48,7 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) { name: 'merge_request_merged_with_policy_violations', author: merger, - scope: policy_project, + scope: project, target: merge_request, message: "Merge request merged with 1 security policy violations", additional_details: { @@ -108,7 +108,7 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) [ { name: approval_rule.name, - report_type: approval_rule.report_type, + report_type: 'license_scanning', approved: true, approvals_required: 1, approved_approvers: ['approver one'], @@ -166,7 +166,7 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) [ { name: approval_rule.name, - report_type: approval_rule.report_type, + report_type: 'license_scanning', approved: false, approvals_required: 2, approved_approvers: ['approver one'], -- GitLab From ea281da0d0ca7340912fac8eea375724e95c32ed Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Mon, 30 Jun 2025 13:43:47 +0200 Subject: [PATCH 4/9] Address review comments --- ...h_policy_violations_audit_event_service.rb | 32 +++- ...icy_violations_audit_event_service_spec.rb | 176 ++++++++++-------- 2 files changed, 123 insertions(+), 85 deletions(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index d0e7f06928865b..f11699b6e63288 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -12,11 +12,12 @@ def initialize(merge_request) def execute return unless merge_request.merged? + return if violations.empty? audit_context = { name: 'merge_request_merged_with_policy_violations', author: merged_by, - scope: merge_request.project, + scope: target_project, target: merge_request, message: "Merge request merged with #{details.violations_count} security policy violations", additional_details: audit_details @@ -39,9 +40,10 @@ def audit_details merged_at: merge_request.merged_at, source_branch: merge_request.source_branch, target_branch: merge_request.target_branch, - project_id: merge_request.project.id, - project_name: merge_request.project.name, - project_full_path: merge_request.project.full_path, + project_id: target_project.id, + project_name: target_project.name, + project_full_path: target_project.full_path, + violated_policies: violated_policies, security_policy_approval_rules: policy_approval_rules, violation_details: violation_details } @@ -83,6 +85,28 @@ def violation_details } end + def violated_policies + violations.filter_map do |violation| + security_policy = violation.security_policy + next unless security_policy + + { + policy_name: security_policy.name, + policy_type: security_policy.type, + security_orchestration_policy_configuration_id: security_policy.security_orchestration_policy_configuration_id + } + end + end + + def violations + merge_request.scan_result_policy_violations + end + strong_memoize_attr :violations + + def target_project + @_target_project ||= merge_request.project + end + def approval_state @_approval_state ||= ApprovalState.new(merge_request) end diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb index 53784fcf98c5d7..e49a5997b069b0 100644 --- a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -10,6 +10,7 @@ let_it_be(:merge_time) { Time.now.utc } let_it_be_with_reload(:merge_request) do create :merge_request, + :opened, title: 'MR One', description: 'This was a triumph', author: mr_author, @@ -37,10 +38,6 @@ def build_violation_details(policy, data, status = :failed) scan_result_policy_read: policy, approval_policy_rule: approval_policy_rule, violation_data: data) end - def build_violation_with_error(policy, error, status = :failed, **extra_data) - build_violation_details(policy, { 'errors' => [{ 'error' => error, **extra_data }] }, status) - end - describe '#execute' do let(:execute_service) { service.execute } @@ -61,6 +58,13 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) project_id: project.id, project_name: project.name, project_full_path: project.full_path, + violated_policies: [ + { + policy_name: security_policy_name, + policy_type: 'approval_policy', + security_orchestration_policy_configuration_id: policy_configuration.id + } + ], security_policy_approval_rules: policy_approval_rules, violation_details: violation_details } @@ -84,82 +88,25 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) end context 'when merge request is merged' do - let_it_be(:approval_policy_rule) do - create(:approval_policy_rule, :license_finding, security_policy: security_policy) - end - - let_it_be(:approval_rule) do - create( - :report_approver_rule, - :license_scanning, - merge_request: merge_request, - approval_policy_rule: approval_policy_rule, - approvals_required: 1, - user_ids: [approver.id] - ) - end - - let_it_be(:approval_merge_request_rules_approved_approver) do - create(:approval_merge_request_rules_approved_approver, - approval_merge_request_rule: approval_rule, user: approver) - end + context 'with scan result policy violations' do + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, :license_finding, security_policy: security_policy) + end - let(:policy_approval_rules) do - [ - { - name: approval_rule.name, - report_type: 'license_scanning', - approved: true, + let_it_be_with_reload(:approval_rule) do + create( + :report_approver_rule, + :license_scanning, + merge_request: merge_request.reload, + approval_policy_rule: approval_policy_rule, approvals_required: 1, - approved_approvers: ['approver one'], - invalid_rule: false, - fail_open: false - } - ] - end - - let(:license_scanning_violations_json) do - { - "dependencies" => %w[A B], - "license" => "MIT License", - "url" => "https://spdx.org/licenses/MIT.html" - } - end - - let(:violations_count) { 1 } - let(:violation_details) do - { - fail_open_policies: [], - fail_closed_policies: [security_policy_name], - warn_mode_policies: [], - new_scan_finding_violations: [], - previous_scan_finding_violations: [], - license_scanning_violations: [license_scanning_violations_json], - any_merge_request_violations: [], - errors: [], - comparison_pipelines: [] - } - end - - before do - policy = create(:scan_result_policy_read, project: project, - security_orchestration_policy_configuration: policy_configuration) - - create(:report_approver_rule, :license_scanning, merge_request: merge_request, - scan_result_policy_read: policy, name: security_policy_name) - - build_violation_details(policy, - violations: { license_scanning: { 'MIT License' => %w[A B] } } - ) - merge_request.update!(state_id: MergeRequest.available_states[:merged]) - merge_request.metrics.update!(merged_at: merge_time, merged_by: merger) - end - - it_behaves_like 'recording the audit event' + user_ids: [approver.id] + ) + end - context 'with invalid rules' do - before do - approval_rule.update_columns(approvals_required: 2) + let_it_be(:approval_merge_request_rules_approved_approver) do + create(:approval_merge_request_rules_approved_approver, + approval_merge_request_rule: approval_rule, user: approver) end let(:policy_approval_rules) do @@ -167,22 +114,89 @@ def build_violation_with_error(policy, error, status = :failed, **extra_data) { name: approval_rule.name, report_type: 'license_scanning', - approved: false, - approvals_required: 2, + approved: true, + approvals_required: 1, approved_approvers: ['approver one'], - invalid_rule: true, + invalid_rule: false, fail_open: false } ] end + let(:license_scanning_violations_json) do + { + "dependencies" => %w[A B], + "license" => "MIT License", + "url" => "https://spdx.org/licenses/MIT.html" + } + end + + let(:violations_count) { 1 } + let(:violation_details) do + { + fail_open_policies: [], + fail_closed_policies: [security_policy_name], + warn_mode_policies: [], + new_scan_finding_violations: [], + previous_scan_finding_violations: [], + license_scanning_violations: [license_scanning_violations_json], + any_merge_request_violations: [], + errors: [], + comparison_pipelines: [] + } + end + + before do + policy = create(:scan_result_policy_read, project: project, + security_orchestration_policy_configuration: policy_configuration) + + create(:report_approver_rule, :license_scanning, merge_request: merge_request, + scan_result_policy_read: policy, name: security_policy_name) + + build_violation_details(policy, + violations: { license_scanning: { 'MIT License' => %w[A B] } } + ) + merge_request.update!(state_id: MergeRequest.available_states[:merged]) + merge_request.metrics.update!(merged_at: merge_time, merged_by: merger) + end + it_behaves_like 'recording the audit event' + + context 'with invalid rules' do + before do + approval_rule.update_columns(approvals_required: 2) + end + + let(:policy_approval_rules) do + [ + { + name: approval_rule.name, + report_type: 'license_scanning', + approved: false, + approvals_required: 2, + approved_approvers: ['approver one'], + invalid_rule: true, + fail_open: false + } + ] + end + + it_behaves_like 'recording the audit event' + end + end + + context 'without scan result policy violations' do + before do + merge_request.update!(state_id: MergeRequest.available_states[:merged]) + end + + it_behaves_like 'not recording the audit event' end end context 'when merge request is not merged' do before do - allow(merge_request).to receive(:merged?).and_return(false) + merge_request.update!(state_id: MergeRequest.available_states[:closed]) end it_behaves_like 'not recording the audit event' -- GitLab From b1249aec5f77205fb174109583bd43c11c53a0db Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Mon, 30 Jun 2025 14:53:03 +0200 Subject: [PATCH 5/9] Update the audit message --- .../merged_with_policy_violations_audit_event_service.rb | 2 +- .../merged_with_policy_violations_audit_event_service_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index f11699b6e63288..062725ab0ec246 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -19,7 +19,7 @@ def execute author: merged_by, scope: target_project, target: merge_request, - message: "Merge request merged with #{details.violations_count} security policy violations", + message: "Merge request merged with #{details.violations_count} security policy violation(s)", additional_details: audit_details } diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb index e49a5997b069b0..46d2f23bb505bf 100644 --- a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -47,7 +47,7 @@ def build_violation_details(policy, data, status = :failed) author: merger, scope: project, target: merge_request, - message: "Merge request merged with 1 security policy violations", + message: "Merge request merged with 1 security policy violation(s)", additional_details: { merge_request_title: merge_request.title, merge_request_id: merge_request.id, -- GitLab From 559eff417146068ebced126c41c10b4026ca13fd Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Mon, 30 Jun 2025 17:29:44 +0200 Subject: [PATCH 6/9] Fix: Audit event message and details for policy violations Updates the audit event message to include the merge request title. Also includes additional details about the violated policies. --- ...erged_with_policy_violations_audit_event_service.rb | 10 ++++++++-- ..._with_policy_violations_audit_event_service_spec.rb | 4 +++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index 062725ab0ec246..287244259d371c 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -19,7 +19,8 @@ def execute author: merged_by, scope: target_project, target: merge_request, - message: "Merge request merged with #{details.violations_count} security policy violation(s)", + message: "Merge request with title '#{merge_request.title}' was merged with " \ + "#{details.violations_count} security policy violation(s)", additional_details: audit_details } @@ -90,10 +91,15 @@ def violated_policies security_policy = violation.security_policy next unless security_policy + policy_configuration = security_policy.security_orchestration_policy_configuration + next unless policy_configuration + { + policy_id: security_policy.id, policy_name: security_policy.name, policy_type: security_policy.type, - security_orchestration_policy_configuration_id: security_policy.security_orchestration_policy_configuration_id + security_orchestration_policy_configuration_id: policy_configuration.id, + security_policy_management_project_id: policy_configuration.security_policy_management_project_id } end end diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb index 46d2f23bb505bf..4995b0222b8418 100644 --- a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -47,7 +47,7 @@ def build_violation_details(policy, data, status = :failed) author: merger, scope: project, target: merge_request, - message: "Merge request merged with 1 security policy violation(s)", + message: "Merge request with title 'MR One' was merged with 1 security policy violation(s)", additional_details: { merge_request_title: merge_request.title, merge_request_id: merge_request.id, @@ -60,8 +60,10 @@ def build_violation_details(policy, data, status = :failed) project_full_path: project.full_path, violated_policies: [ { + policy_id: security_policy.id, policy_name: security_policy_name, policy_type: 'approval_policy', + security_policy_management_project_id: policy_project.id, security_orchestration_policy_configuration_id: policy_configuration.id } ], -- GitLab From 5e63e4fa255c40d0edd721cb72636acb16b3702c Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Tue, 1 Jul 2025 11:24:30 +0200 Subject: [PATCH 7/9] Remove the unnecessary nil check --- .../merged_with_policy_violations_audit_event_service.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index 287244259d371c..4e0dd4ed65ef89 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -92,7 +92,6 @@ def violated_policies next unless security_policy policy_configuration = security_policy.security_orchestration_policy_configuration - next unless policy_configuration { policy_id: security_policy.id, -- GitLab From d60be465165b90b9cfb77dcc29679c7d142eb5a4 Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Wed, 2 Jul 2025 14:17:47 +0200 Subject: [PATCH 8/9] Update id of deleted author --- ...with_policy_violations_audit_event_service.rb | 2 +- ...policy_violations_audit_event_service_spec.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index 4e0dd4ed65ef89..a99bfc806b9f3d 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -30,7 +30,7 @@ def execute private def merged_by - merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -3, name: 'Unknown User') + merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -4, name: 'Unknown User') end def audit_details diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb index 4995b0222b8418..9762f7f711b926 100644 --- a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -185,6 +185,22 @@ def build_violation_details(policy, data, status = :failed) it_behaves_like 'recording the audit event' end + + context 'when merged by author is not available' do + before do + merge_request.metrics.update!(merged_by: nil) + end + + it 'audits with a deleted author' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + a_hash_including( + author: an_instance_of(Gitlab::Audit::DeletedAuthor) + ) + ) + + execute_service + end + end end context 'without scan result policy violations' do -- GitLab From d04e760791684a4d9a7709d2873542edc9313e8e Mon Sep 17 00:00:00 2001 From: Imam Hossain Date: Wed, 2 Jul 2025 14:41:24 +0200 Subject: [PATCH 9/9] Apply strong memoize attr --- ...rged_with_policy_violations_audit_event_service.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb index a99bfc806b9f3d..75d2769680b5b1 100644 --- a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -20,7 +20,7 @@ def execute scope: target_project, target: merge_request, message: "Merge request with title '#{merge_request.title}' was merged with " \ - "#{details.violations_count} security policy violation(s)", + "#{violations.count} security policy violation(s)", additional_details: audit_details } @@ -109,15 +109,18 @@ def violations strong_memoize_attr :violations def target_project - @_target_project ||= merge_request.project + merge_request.project end + strong_memoize_attr :target_project def approval_state - @_approval_state ||= ApprovalState.new(merge_request) + ApprovalState.new(merge_request) end + strong_memoize_attr :approval_state def details - @_details ||= Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) + Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) end + strong_memoize_attr :details end end -- GitLab