From de051e539c6f4cf1c26b9035fd2ccf17aec9b370 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Mon, 2 Sep 2024 23:10:47 +0200 Subject: [PATCH] Audit merged MRs - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/442279 Changelog: added EE: true --- config/sidekiq_queues.yml | 2 + doc/user/compliance/audit_event_types.md | 1 + ee/app/events/merge_requests/merged_event.rb | 17 +++ .../ee/merge_requests/post_merge_service.rb | 7 + .../merge_audit_event_service.rb | 86 +++++++++++++ ee/app/workers/all_queues.yml | 9 ++ .../process_merge_audit_event_worker.rb | 37 ++++++ .../types/merge_request_merged.yml | 10 ++ .../ff_compliance_audit_mr_merge.yml | 9 ++ ee/lib/ee/gitlab/event_store.rb | 14 +- ee/spec/lib/ee/gitlab/event_store_spec.rb | 1 + .../merge_requests/post_merge_service_spec.rb | 33 ++++- .../merge_audit_event_service_spec.rb | 121 ++++++++++++++++++ .../process_merge_audit_event_worker_spec.rb | 42 ++++++ lib/gitlab/audit/deleted_author.rb | 3 + spec/lib/gitlab/audit/deleted_author_spec.rb | 13 ++ 16 files changed, 400 insertions(+), 5 deletions(-) create mode 100644 ee/app/events/merge_requests/merged_event.rb create mode 100644 ee/app/services/merge_requests/merge_audit_event_service.rb create mode 100644 ee/app/workers/merge_requests/process_merge_audit_event_worker.rb create mode 100644 ee/config/audit_events/types/merge_request_merged.yml create mode 100644 ee/config/feature_flags/gitlab_com_derisk/ff_compliance_audit_mr_merge.yml create mode 100644 ee/spec/services/merge_requests/merge_audit_event_service_spec.rb create mode 100644 ee/spec/workers/merge_requests/process_merge_audit_event_worker_spec.rb create mode 100644 spec/lib/gitlab/audit/deleted_author_spec.rb diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 9d4744e8e60400..39526eb8b78260 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -537,6 +537,8 @@ - 1 - - merge_requests_process_auto_merge_from_event - 1 +- - merge_requests_process_merge_audit_event + - 1 - - merge_requests_remove_user_approval_rules - 1 - - merge_requests_resolve_todos diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 8a7645dd9bd131..6cf7a4df30b15e 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -156,6 +156,7 @@ Audit event types belong to the following product categories. | [`member_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109711) | Triggered when a membership is destroyed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374112) | Group, Project | | [`member_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109711) | Triggered when a membership is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374112) | Group, Project | | [`merge_request_create`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/90911) | Triggered when a merge request is created | **{dotted-circle}** No | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/367239) | Project | +| [`merge_request_merged`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164846) | When a merge request is merged | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/442279) | Project | | [`omniauth_login_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Triggered when an OmniAuth login fails | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | | [`password_reset_requested`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114548) | Triggered when a user requests a password reset using a registered email address | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | | [`personal_access_token_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108952) | Triggered when a user creates a personal access token | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374113) | User | diff --git a/ee/app/events/merge_requests/merged_event.rb b/ee/app/events/merge_requests/merged_event.rb new file mode 100644 index 00000000000000..d7a524290ce946 --- /dev/null +++ b/ee/app/events/merge_requests/merged_event.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module MergeRequests + class MergedEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + merge_request_id + ], + 'properties' => { + 'merge_request_id' => { 'type' => 'integer' } + } + } + end + end +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 00910d2116010d..c80d5669b3ea6a 100644 --- a/ee/app/services/ee/merge_requests/post_merge_service.rb +++ b/ee/app/services/ee/merge_requests/post_merge_service.rb @@ -27,10 +27,17 @@ def execute(merge_request, source = nil) sync_security_scan_orchestration_policies(target_project) trigger_blocked_merge_requests_merge_status_updated(merge_request) track_policies_fallback_behavior(merge_request) + publish_event_store_event(merge_request) end private + def publish_event_store_event(merge_request) + return unless ::Feature.enabled? :ff_compliance_audit_mr_merge, merge_request.project + + ::Gitlab::EventStore.publish ::MergeRequests::MergedEvent.new(data: { merge_request_id: merge_request.id }) + end + def compliance_violations_enabled?(group) group.licensed_feature_available?(:group_level_compliance_dashboard) end diff --git a/ee/app/services/merge_requests/merge_audit_event_service.rb b/ee/app/services/merge_requests/merge_audit_event_service.rb new file mode 100644 index 00000000000000..cdcdf62d2c5fe0 --- /dev/null +++ b/ee/app/services/merge_requests/merge_audit_event_service.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +module MergeRequests + class MergeAuditEventService + attr_reader :merge_request + + def initialize(merge_request:) + @merge_request = merge_request + end + + def execute + return unless merge_request.merged? + + audit_context = { + name: 'merge_request_merged', + author: merged_by, + scope: merge_request.project, + target: merge_request, + message: 'Merge request merged', + additional_details: audit_details + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + private + + def audit_details + { + title: merge_request.title, + description: merge_request.description, + required_approvals: required_approvals, + approval_count: approval_count, + approvers: approvers, + approving_committers: approving_committers, + approving_author: approving_author, + merged_at: merged_at, + commit_shas: commit_shas, + target_branch: merge_request.target_branch, + target_project_id: merge_request.target_project.id + } + end + + def approval_count + merge_request.approvals.count + end + + def approvers + merge_request.approved_by_users.map(&:username).sort + end + + def approving_committers + approvers.select { |approver| approver.in? committers } + end + + def committers + @committers ||= merge_request + .committers(with_merge_commits: true, lazy: true, include_author_when_signed: true) + .map(&:username) + end + + def author + merge_request.author.username + end + + def approving_author + author.in? approvers + end + + def merged_by + merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -3, name: 'Unknown User') + end + + def merged_at + merge_request.merged_at + end + + def commit_shas + merge_request.merge_request_diff&.commit_shas + end + + def required_approvals + merge_request.approvals_required + end + end +end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 43eaec3671a0dc..ae38198eccca95 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -1821,6 +1821,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: merge_requests_process_merge_audit_event + :worker_name: MergeRequests::ProcessMergeAuditEventWorker + :feature_category: :compliance_management + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_remove_user_approval_rules :worker_name: MergeRequests::RemoveUserApprovalRulesWorker :feature_category: :code_review_workflow diff --git a/ee/app/workers/merge_requests/process_merge_audit_event_worker.rb b/ee/app/workers/merge_requests/process_merge_audit_event_worker.rb new file mode 100644 index 00000000000000..3a9f9291c48678 --- /dev/null +++ b/ee/app/workers/merge_requests/process_merge_audit_event_worker.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module MergeRequests + # rubocop: disable Scalability/IdempotentWorker -- EventStore::Subscriber inlcudes idempotent + class ProcessMergeAuditEventWorker + include Gitlab::EventStore::Subscriber # adds idempotent! + + data_consistency :always + feature_category :compliance_management + urgency :low + + # Audit may stream to external destination with HTTP request if configured for the group + worker_has_external_dependencies! + + def handle_event(event) + @event = event + + unless merge_request + logger.info structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id) + return + end + + ::MergeRequests::MergeAuditEventService.new(merge_request: merge_request).execute + end + + private + + def merge_request_id + @event.data[:merge_request_id] + end + + def merge_request + @merge_request ||= MergeRequest.find_by_id merge_request_id + end + end + # rubocop: enable Scalability/IdempotentWorker +end diff --git a/ee/config/audit_events/types/merge_request_merged.yml b/ee/config/audit_events/types/merge_request_merged.yml new file mode 100644 index 00000000000000..6e9eaa69f7e144 --- /dev/null +++ b/ee/config/audit_events/types/merge_request_merged.yml @@ -0,0 +1,10 @@ +--- +name: merge_request_merged +description: When a merge request is merged +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/442279 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164846 +feature_category: compliance_management +milestone: '17.5' +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/config/feature_flags/gitlab_com_derisk/ff_compliance_audit_mr_merge.yml b/ee/config/feature_flags/gitlab_com_derisk/ff_compliance_audit_mr_merge.yml new file mode 100644 index 00000000000000..8e158354e3e608 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/ff_compliance_audit_mr_merge.yml @@ -0,0 +1,9 @@ +--- +name: ff_compliance_audit_mr_merge +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/442279 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164846 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/482720 +milestone: '17.5' +group: group::compliance +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb index 789655083accae..ab21bd3251acd8 100644 --- a/ee/lib/ee/gitlab/event_store.rb +++ b/ee/lib/ee/gitlab/event_store.rb @@ -17,7 +17,7 @@ module EventStore # # Only EE subscriptions should be declared in this module. override :configure! - def configure!(store) + def configure!(store) # rubocop:disable Metrics/AbcSize -- This is basically a configuration file super(store) ### @@ -81,8 +81,8 @@ def configure!(store) subscribe_to_work_item_events(store) subscribe_to_milestone_events(store) subscribe_to_zoekt_events(store) - subscribe_to_users_activity_events(store) + subscribe_to_merge_events(store) end def register_security_policy_subscribers(store) @@ -204,6 +204,16 @@ def subscribe_to_users_activity_events(store) ) } end + + def subscribe_to_merge_events(store) + store.subscribe ::MergeRequests::ProcessMergeAuditEventWorker, + to: ::MergeRequests::MergedEvent, + if: ->(event) { + mr = ::MergeRequest.find_by_id(event.data[:merge_request_id]) + actor = ::Project.actor_from_id(mr&.project) + ::Feature.enabled? :ff_compliance_audit_mr_merge, actor + } + end end end end diff --git a/ee/spec/lib/ee/gitlab/event_store_spec.rb b/ee/spec/lib/ee/gitlab/event_store_spec.rb index e0ea70c278afef..e19aeb9b8b5d18 100644 --- a/ee/spec/lib/ee/gitlab/event_store_spec.rb +++ b/ee/spec/lib/ee/gitlab/event_store_spec.rb @@ -12,6 +12,7 @@ ::Ci::PipelineCreatedEvent, ::Repositories::KeepAroundRefsCreatedEvent, ::MergeRequests::ApprovedEvent, + ::MergeRequests::MergedEvent, ::MergeRequests::JiraTitleDescriptionUpdateEvent, ::MergeRequests::ApprovalsResetEvent, ::MergeRequests::DraftStateChangeEvent, 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 fd990d43b16e5e..d6bc506501b98c 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 @@ -14,6 +14,28 @@ subject { service.execute(merge_request) } describe '#execute' do + context 'merged event' do + context 'when ff_compliance_audit_mr_merge is enabled' do + it 'publishes to the MergedEvent' do + expect(::Gitlab::EventStore).to receive(:publish) + expect(MergeRequests::MergedEvent).to receive(:new) + + subject + end + end + + context 'when ff_compliance_audit_mr_merge is disabled' do + it 'does not publish to the MergedEvent' do + stub_feature_flags ff_compliance_audit_mr_merge: false + + expect(::Gitlab::EventStore).not_to receive(:publish) + expect(MergeRequests::MergedEvent).not_to receive(:new) + + subject + end + end + end + context 'finalize approvals' do let(:finalize_service) { double(:finalize_service) } @@ -234,8 +256,8 @@ }) end - it 'does sends any additional unblocked events' do - expect(::Gitlab::EventStore).to receive(:publish).twice + it 'does send any additional unblocked events' do + expect(::Gitlab::EventStore).to receive(:publish).exactly(3).times subject end @@ -243,12 +265,17 @@ end context 'when merge_when_checks_pass is disabled' do + let(:merge_request) { create :merge_request } + before do stub_feature_flags(merge_when_checks_pass: false) end - it 'does sends an unblocked events' do + it 'does not send any unblocked events' do expect(::Gitlab::EventStore).not_to receive(:publish) + .with(::MergeRequests::UnblockedStateEvent.new(data: { + current_user_id: current_user.id, merge_request_id: merge_request.id + })) subject end diff --git a/ee/spec/services/merge_requests/merge_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merge_audit_event_service_spec.rb new file mode 100644 index 00000000000000..0117d65e8229c7 --- /dev/null +++ b/ee/spec/services/merge_requests/merge_audit_event_service_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MergeAuditEventService, feature_category: :compliance_management do + let(:merger) { create :user } + let(:approver) { create :user, username: 'approver one' } + let(:mr_author) { create :user, username: 'author one' } + let(:project) { create :project, :repository } + let!(:merge_time) { Time.now.utc } + let(:merge_request) do + create :merge_request, + :with_productivity_metrics, + :with_merged_metrics, + :merged, + title: 'MR One', + description: 'This was a triumph', + author: mr_author, + source_project: project + end + + let!(:approval) do + create :approval, + merge_request: merge_request, + user: approver, + patch_id_sha: merge_request.current_patch_id_sha + end + + subject(:audit_service) { described_class.new merge_request: merge_request } + + before do + merge_request.metrics.update_columns merged_by_id: merger.id, merged_at: merge_time + end + + describe '#execute' do + it 'audits the event' do + audit_context = { + name: 'merge_request_merged', + author: merger, + scope: merge_request.project, + target: merge_request, + message: 'Merge request merged', + additional_details: hash_including({ + title: 'MR One', + approvers: ['approver one'], + approving_committers: [], + approving_author: false, + merged_at: merge_time, + commit_shas: merge_request.commits.commits.map(&:sha), + required_approvals: 0, + approval_count: 1, + description: 'This was a triumph', + target_project_id: project.id + }) + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context).and_call_original + + audit_service.execute + end + + context 'when author approves' do + let!(:approval) do + create :approval, + merge_request: merge_request, + user: mr_author, + patch_id_sha: merge_request.current_patch_id_sha + end + + it 'audits with approving author' do + audit_context = hash_including({ + name: 'merge_request_merged', + additional_details: hash_including({ + approvers: ['author one'], + approving_committers: [], + approving_author: true + }) + }) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context).and_call_original + + audit_service.execute + end + end + + context 'with approving committers' do + it 'audits with approving committer' do + allow(merge_request).to receive(:committers).and_return(User.where(id: approver.id)) + + audit_context = hash_including({ + name: 'merge_request_merged', + additional_details: hash_including({ + approvers: ['approver one'], + approving_committers: ['approver one'] + }) + }) + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context).and_call_original + + audit_service.execute + end + end + + context 'with required approvals' do + it 'audits with required approvals' do + allow(merge_request).to receive(:approvals_required).and_return(2) + + audit_context = hash_including({ + name: 'merge_request_merged', + additional_details: hash_including({ + approvers: ['approver one'], + required_approvals: 2, + approval_count: 1 + }) + }) + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context).and_call_original + + audit_service.execute + end + end + end +end diff --git a/ee/spec/workers/merge_requests/process_merge_audit_event_worker_spec.rb b/ee/spec/workers/merge_requests/process_merge_audit_event_worker_spec.rb new file mode 100644 index 00000000000000..e60aeda2976023 --- /dev/null +++ b/ee/spec/workers/merge_requests/process_merge_audit_event_worker_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::MergeRequests::ProcessMergeAuditEventWorker, feature_category: :compliance_management do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { create(:merge_request, :with_productivity_metrics, :merged, source_project: project) } + + let(:data) { { merge_request_id: merge_request.id } } + let(:merged_event) { MergeRequests::MergedEvent.new(data: data) } + + before do + merge_request.metrics.update_columns merged_by_id: user.id + end + + it_behaves_like 'subscribes to event' do + let(:event) { merged_event } + end + + it 'calls MergeRequests::MergeAuditEventSerivce' do + expect_next_instance_of( + MergeRequests::MergeAuditEventService, + merge_request: merge_request + ) do |service| + expect(service).to receive(:execute) + end + + described_class.new.perform(merged_event.class.name, merged_event.data) + end + + context 'when the merge request does not exist' do + it 'logs and does not call MergeRequests::MergeAuditEventService' do + merge_request.destroy! + + expect(Sidekiq.logger).to receive(:info) + expect(MergeRequests::MergeAuditEventService).not_to receive(:new) + + expect { described_class.new.perform(merged_event.class.name, merged_event.data) }.not_to raise_exception + end + end +end diff --git a/lib/gitlab/audit/deleted_author.rb b/lib/gitlab/audit/deleted_author.rb index e3b8ad5ad21c97..678c894fc09ef1 100644 --- a/lib/gitlab/audit/deleted_author.rb +++ b/lib/gitlab/audit/deleted_author.rb @@ -3,6 +3,9 @@ module Gitlab module Audit class DeletedAuthor < Gitlab::Audit::NullAuthor + def impersonated? + false + end end end end diff --git a/spec/lib/gitlab/audit/deleted_author_spec.rb b/spec/lib/gitlab/audit/deleted_author_spec.rb new file mode 100644 index 00000000000000..deaf86b95f0683 --- /dev/null +++ b/spec/lib/gitlab/audit/deleted_author_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::Audit::DeletedAuthor, feature_category: :compliance_management do + subject(:deleted_author) { described_class.new id: 0, name: 'delete this' } + + describe '#impersonated?' do + it 'returns false' do + expect(deleted_author.impersonated?).to be false + end + end +end -- GitLab