From 22fc8eb394e11a47ad75bf9932f40a1e9c381fff Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Fri, 7 Oct 2022 13:58:53 +0200 Subject: [PATCH] Move the auditing of approval rules to post merge MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/100496 Issue: gitlab.com/gitlab-org/gitlab/-/issues/375060 --- ee/app/models/approval_merge_request_rule.rb | 17 --- .../approval_wrapped_code_owner_rule.rb | 4 +- .../ee/merge_requests/post_merge_service.rb | 36 +++++++ .../approval_merge_request_rule_spec.rb | 97 +---------------- .../merge_requests/post_merge_service_spec.rb | 101 ++++++++++++++++++ 5 files changed, 142 insertions(+), 113 deletions(-) diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index c715f10e6a944f..354569120e921b 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -42,7 +42,6 @@ class ApprovalMergeRequestRule < ApplicationRecord before_update :compare_with_project_rule after_save :track_approvers_for_report_approver, if: :report_approver? - after_save :audit_log_for_invalid_report_approver, unless: :any_approver? validate :validate_approval_project_rule @@ -169,20 +168,4 @@ def track_approvers_for_report_approver Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter.track_invalid_approvers(merge_request: merge_request) end - - def audit_log_for_invalid_report_approver - return unless Feature.enabled?(:audit_invalid_approver_rules, project) - return if approvers.present? - - audit_context = { - name: 'merge_request_invalid_approver_rules', - author: merge_request.author, - scope: project, - target: merge_request, - message: 'Invalid merge request approver rules', - target_details: { title: merge_request.title, iid: merge_request.iid, id: merge_request.id } - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - end end diff --git a/ee/app/models/approval_wrapped_code_owner_rule.rb b/ee/app/models/approval_wrapped_code_owner_rule.rb index 268fa730057b81..bfe671a5fd2155 100644 --- a/ee/app/models/approval_wrapped_code_owner_rule.rb +++ b/ee/app/models/approval_wrapped_code_owner_rule.rb @@ -12,8 +12,6 @@ def approvals_required end end - private - def branch_requires_code_owner_approval? return false unless project.code_owner_approval_required_available? return false if section_optional? @@ -21,6 +19,8 @@ def branch_requires_code_owner_approval? ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch) end + private + def section_optional? Gitlab::CodeOwners.optional_section?(project, merge_request.target_branch, section) end diff --git a/ee/app/services/ee/merge_requests/post_merge_service.rb b/ee/app/services/ee/merge_requests/post_merge_service.rb index 7d5e2fc162d9cd..79a25727e01c20 100644 --- a/ee/app/services/ee/merge_requests/post_merge_service.rb +++ b/ee/app/services/ee/merge_requests/post_merge_service.rb @@ -15,6 +15,8 @@ def execute(merge_request) ComplianceManagement::MergeRequests::ComplianceViolationsWorker.perform_async(merge_request.id) end + audit_approval_rules(merge_request) + return unless target_project.licensed_feature_available?(:security_orchestration_policies) Security::OrchestrationPolicyConfiguration @@ -29,6 +31,40 @@ def execute(merge_request) def compliance_violations_enabled?(group) group.licensed_feature_available?(:group_level_compliance_dashboard) end + + def audit_approval_rules(merge_request) + return unless ::Feature.enabled?(:audit_invalid_approver_rules, project) + + merge_request.wrapped_approval_rules.each do |rule| + next if rule.any_approver? + next unless rule.approvers.empty? + + if rule.code_owner? + audit_invalid_rule(merge_request, rule) if rule.branch_requires_code_owner_approval? + elsif rule.approvals_required > 0 + audit_invalid_rule(merge_request, rule) + end + end + end + + def audit_invalid_rule(merge_request, rule) + audit_context = { + name: 'merge_request_invalid_approver_rules', + author: merge_request.author, + scope: project, + target: merge_request, + message: 'Invalid merge request approver rules', + target_details: { + title: merge_request.title, + iid: merge_request.iid, + id: merge_request.id, + rule_type: rule.rule_type, + rule_name: rule.name + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index 065203d988d29c..86840de746fb21 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -264,9 +264,11 @@ end context 'when project merge_requests_author_approval is false' do - it 'does not contain author' do + before do merge_request.project.update!(merge_requests_author_approval: false) + end + it 'does not contain author' do expect(described_class.find(subject.id).approvers).to be_empty end @@ -439,97 +441,4 @@ end end end - - describe '#audit_log_for_invalid_report_approver' do - let(:expected_params) do - { - name: 'merge_request_invalid_approver_rules', - author: merge_request.author, - scope: merge_request.project, - target: merge_request, - target_details: { title: merge_request.title, iid: merge_request.iid, id: merge_request.id }, - message: 'Invalid merge request approver rules' - } - end - - context 'when audit_invalid_approver_rules is disabled' do - before do - stub_feature_flags(audit_invalid_approver_rules: false) - end - - it 'does not call track_invalid_approvers' do - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - - create(:code_owner_rule, merge_request: merge_request) - end - end - - context 'when audit_invalid_approver_rules is enabled' do - before do - stub_feature_flags(audit_invalid_approver_rules: true) - end - - context 'with any_approver as rule_type' do - it 'does not call track_invalid_approvers' do - create(:any_approver_rule, merge_request: merge_request) - - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - end - end - - context 'with code_owner_rule as rule_type' do - it 'calls track_invalid_approvers' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_params) - - create(:code_owner_rule, merge_request: merge_request) - end - - context 'with approvers' do - it 'does not call track_invalid_approvers' do - rule = create(:code_owner_rule, merge_request: merge_request) - - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - - rule.users << project.first_owner - end - end - end - - context 'with approval_merge_request_rule as rule_type' do - it 'calls track_invalid_approvers' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_params) - - create(:approval_merge_request_rule, merge_request: merge_request) - end - - context 'with approvers' do - it 'does not call track_invalid_approvers' do - rule = create(:approval_merge_request_rule, merge_request: merge_request) - - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - - rule.users << project.first_owner - end - end - end - - context 'with report_approver as rule_type' do - it 'calls track_invalid_approvers' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_params) - - create(:report_approver_rule, merge_request: merge_request) - end - - context 'with approvers' do - it 'does not call track_invalid_approvers' do - rule = create(:report_approver_rule, merge_request: merge_request) - - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) - - rule.users << project.first_owner - end - end - end - end - end end diff --git a/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb index 8d861c2d7f4720..39a09b4d7842cf 100644 --- a/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb @@ -54,6 +54,107 @@ end end + context 'auditing invalid logs' do + shared_examples 'auditing invalid logs' do + let(:expected_params) do + { + name: 'merge_request_invalid_approver_rules', + author: merge_request.author, + scope: merge_request.project, + target: merge_request, + target_details: { + title: merge_request.title, + iid: merge_request.iid, + id: merge_request.id, + rule_type: rule.rule_type, + rule_name: rule.name + }, + message: 'Invalid merge request approver rules' + } + end + + context 'when the rule is valid' do + let!(:rule) { valid_rule } + + it 'does not audit the event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + subject + end + end + + context 'when invalid' do + let!(:rule) { invalid_rule } + + it 'audit logs the event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_params) + + subject + end + + context 'when audit_invalid_approver_rules is disabled' do + before do + stub_feature_flags(audit_invalid_approver_rules: false) + end + + it 'does not track the event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + subject + end + end + end + end + + context 'when the rule is code owner' do + let(:valid_rule) { create(:code_owner_rule, merge_request: merge_request, users: create_list(:user, 1)) } + let(:invalid_rule) { create(:code_owner_rule, merge_request: merge_request) } + + before do + stub_licensed_features(code_owner_approval_required: true) + create(:protected_branch, project: project, name: merge_request.target_branch, code_owner_approval_required: true) + end + + include_examples 'auditing invalid logs' + end + + context 'when the rule is any_approver' do + context 'when the rule is valid' do + let!(:rule) { create(:any_approver_rule, merge_request: merge_request, users: create_list(:user, 1)) } + + it 'does not audit the event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + subject + end + end + + context 'when invalid' do + let!(:rule) { create(:any_approver_rule, merge_request: merge_request, approvals_required: 1) } + + it 'does not audit the event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + subject + end + end + end + + context 'when the rule is approval_merge_request_rule' do + let(:valid_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: create_list(:user, 1)) } + let(:invalid_rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1) } + + include_examples 'auditing invalid logs' + end + + context 'when the rule is report_approver' do + let(:valid_rule) { create(:report_approver_rule, merge_request: merge_request, users: create_list(:user, 1)) } + let(:invalid_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 1) } + + include_examples 'auditing invalid logs' + end + end + context 'security orchestration policy configuration' do let(:security_orchestration_enabled) { true } let(:policy_configuration) { create(:security_orchestration_policy_configuration, project: main_project, security_policy_management_project: project) } -- GitLab