diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb index eb7db0fc9b460790789fd22b04aebb0f3be1f173..edc60a757d2d0f9a9f336e19cb9598f0112ebc8c 100644 --- a/app/models/commit_collection.rb +++ b/app/models/commit_collection.rb @@ -30,6 +30,10 @@ def committers User.by_any_email(emails) end + def committer_user_ids + committers.pluck(:id) + end + def without_merge_commits strong_memoize(:without_merge_commits) do # `#enrich!` the collection to ensure all commits contain diff --git a/app/models/label.rb b/app/models/label.rb index 3d9ea39d86077cde7a4ee4d9ff631d8048263d0f..32b399ac4613f59c0e4a3bf3917a573ae406f428 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -67,6 +67,10 @@ class Label < ApplicationRecord .with_preloaded_container end + def self.pluck_titles + pluck(:title) + end + def self.prioritized(project) joins(:priorities) .where(label_priorities: { project_id: project }) diff --git a/ee/app/services/ee/issuable/destroy_service.rb b/ee/app/services/ee/issuable/destroy_service.rb index eba865e545ce6db6b77f515b1925e479e83673f5..6cfbb8287931d2ad63a976ee519ea6269dbe6c6f 100644 --- a/ee/app/services/ee/issuable/destroy_service.rb +++ b/ee/app/services/ee/issuable/destroy_service.rb @@ -32,19 +32,23 @@ def track_usage_ping_epic_destroyed(epic) def log_audit_event(issuable) return unless current_user - issuable_name = issuable.is_a?(Issue) ? issuable.work_item_type.name : issuable.class.name - - audit_context = { - name: "delete_#{issuable.to_ability_name}", - stream_only: true, - author: current_user, - target: issuable, - scope: issuable.resource_parent, - message: "Removed #{issuable_name}(#{issuable.title} with IID: #{issuable.iid} and ID: #{issuable.id})", - target_details: { title: issuable.title, iid: issuable.iid, id: issuable.id, type: issuable_name } - } - - ::Gitlab::Audit::Auditor.audit(audit_context) + if issuable.is_a?(MergeRequest) + ::Audit::MergeRequestDestroyAuditor.new(issuable, current_user).execute + else + issuable_name = issuable.is_a?(Issue) ? issuable.work_item_type.name : issuable.class.name + + audit_context = { + name: "delete_#{issuable.to_ability_name}", + stream_only: true, + author: current_user, + target: issuable, + scope: issuable.resource_parent, + message: "Removed #{issuable_name}(#{issuable.title} with IID: #{issuable.iid} and ID: #{issuable.id})", + target_details: { title: issuable.title, iid: issuable.iid, id: issuable.id, type: issuable_name } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/config/audit_events/types/merged_merge_request_deleted.yml b/ee/config/audit_events/types/merged_merge_request_deleted.yml new file mode 100644 index 0000000000000000000000000000000000000000..c5bfbc4f7b2e9a185892aabea48d81b0adef94e4 --- /dev/null +++ b/ee/config/audit_events/types/merged_merge_request_deleted.yml @@ -0,0 +1,9 @@ +--- +name: merged_merge_request_deleted +description: Audit event triggered when a merged merge request is deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/408288 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118793 +milestone: '16.0' +feature_category: source_code_management +saved_to_database: false +streamed: true diff --git a/ee/lib/audit/merge_request_destroy_auditor.rb b/ee/lib/audit/merge_request_destroy_auditor.rb new file mode 100644 index 0000000000000000000000000000000000000000..d015f1202697cd25bd8372102c261461d4cb560d --- /dev/null +++ b/ee/lib/audit/merge_request_destroy_auditor.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Audit + class MergeRequestDestroyAuditor + attr_reader :merge_request, :current_user + + def initialize(merge_request, current_user) + @merge_request = merge_request + @current_user = current_user + end + + def execute + return unless current_user + + event_name = merge_request.merged? ? 'merged_merge_request_deleted' : "delete_merge_request" + + audit_context = { + name: event_name, + stream_only: true, + author: current_user, + target: merge_request, + scope: merge_request.resource_parent, + message: audit_event_message, + target_details: { + title: merge_request.title, + iid: merge_request.iid, + id: merge_request.id, + type: merge_request.class.name + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + private + + def audit_event_message + if merge_request.merged? + labels = merge_request.labels.pluck_titles.to_sentence + approvers = merge_request.approved_by_user_ids.to_sentence + merged_by_id = merge_request.metrics.merged_by_id + committer_ids = merge_request.commits.committer_user_ids.to_sentence + + "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ + "and ID: #{merge_request.id}), labels: #{labels}, approved_by_user_ids: #{approvers}, " \ + "merged_by_user_id: #{merged_by_id}, committer_user_ids: #{committer_ids}" + else + "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} and ID: #{merge_request.id})" + end + end + end +end diff --git a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..01bec181c08cf3d79fdcf97aab885cc00a3abea4 --- /dev/null +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::MergeRequestDestroyAuditor, feature_category: :audit_events do + let(:current_user) { create(:user) } + + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + subject { described_class.new(merge_request, current_user) } + + before do + allow(Gitlab::Audit::Auditor).to receive(:audit) + end + + describe '#execute' do + context 'when current_user is nil' do + let(:current_user) { nil } + + it 'does not audit the event' do + subject.execute + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) + end + end + + context 'when merge request is not merged' do + it 'audits a delete_merge_request event' do + subject.execute + expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| + expect(args[:name]).to eq('delete_merge_request') + expect(args[:message]).to eq("Removed MergeRequest(#{merge_request.title} with IID: " \ + "#{merge_request.iid} and ID: #{merge_request.id})") + end + end + end + + context 'when merge request is merged' do + before do + allow(merge_request).to receive(:merged?).and_return(true) + allow(merge_request).to receive_message_chain(:labels, :pluck_titles).and_return(%w[label1 label2]) + allow(merge_request).to receive(:approved_by_user_ids).and_return([1, 2]) + allow(merge_request).to receive_message_chain(:metrics, :merged_by_id).and_return(3) + allow(merge_request).to receive_message_chain(:commits, :committer_user_ids).and_return([4, 5]) + end + + it 'audits a merged_merge_request_deleted event' do + subject.execute + expect(Gitlab::Audit::Auditor).to have_received(:audit).with( + hash_including(name: 'merged_merge_request_deleted')) + end + + it 'includes additional details in the message' do + subject.execute + expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| + expected_message = "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ + "and ID: #{merge_request.id}), labels: label1 and label2, " \ + "approved_by_user_ids: 1 and 2, " \ + "merged_by_user_id: 3, committer_user_ids: 4 and 5" + expect(args[:message]).to eq(expected_message) + end + end + end + end +end diff --git a/ee/spec/services/ee/issuable/destroy_service_spec.rb b/ee/spec/services/ee/issuable/destroy_service_spec.rb index d8086138106c97d02e11fd64d0fc53cc26ff327c..24520844331840fb4368c205eb3408c7b8132ef5 100644 --- a/ee/spec/services/ee/issuable/destroy_service_spec.rb +++ b/ee/spec/services/ee/issuable/destroy_service_spec.rb @@ -79,7 +79,13 @@ let(:issuable_name) { 'MergeRequest' } let(:scope) { issuable.project } - it_behaves_like 'logs delete issuable audit event' + it 'calls MergeRequestDestroyAuditor with correct arguments' do + expect_next_instance_of(Audit::MergeRequestDestroyAuditor, issuable, user) do |instance| + expect(instance).to receive(:execute) + end + + service.execute(issuable) + end end end end diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb index 706f18a5337192f3dfc0a51cf841ae1c731e3431..1d2d89573bb8fc47a5b9bc43c8f99d8996e5f8a3 100644 --- a/spec/models/commit_collection_spec.rb +++ b/spec/models/commit_collection_spec.rb @@ -45,6 +45,24 @@ end end + describe '#committer_user_ids' do + subject(:collection) { described_class.new(project, [commit]) } + + it 'returns an array of committer user IDs' do + user = create(:user, email: commit.committer_email) + + expect(collection.committer_user_ids).to contain_exactly(user.id) + end + + context 'when there are no committers' do + subject(:collection) { described_class.new(project, []) } + + it 'returns an empty array' do + expect(collection.committer_user_ids).to be_empty + end + end + end + describe '#without_merge_commits' do it 'returns all commits except merge commits' do merge_commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98") diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index ff7ac0ebd2ac4a185674eac9a4d280dbeba3ec27..26a1edcbcffbf105e9c52b1fb98041d926beac77 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -257,4 +257,15 @@ end end end + + describe '.pluck_titles' do + subject(:pluck_titles) { described_class.pluck_titles } + + it 'returns the audit event type of the event type filter' do + label1 = create(:label, title: "TITLE1") + label2 = create(:label, title: "TITLE2") + + expect(pluck_titles).to contain_exactly(label1.title, label2.title) + end + end end