From 79b2d35b4117fd312e964ef45401b694a3d92015 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 26 Apr 2023 15:06:25 +0530 Subject: [PATCH 1/4] Add merged merge request delete audit event This commit adds audit event for merged merge request deletion with message including approvers, committers and merged by user details EE: true Changelog: added --- .../services/ee/issuable/destroy_service.rb | 30 +++++---- ee/lib/audit/merge_request_destroy_auditor.rb | 51 ++++++++++++++++ .../merge_request_destroy_auditor_spec.rb | 61 +++++++++++++++++++ .../ee/issuable/destroy_service_spec.rb | 8 ++- 4 files changed, 136 insertions(+), 14 deletions(-) create mode 100644 ee/lib/audit/merge_request_destroy_auditor.rb create mode 100644 ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb diff --git a/ee/app/services/ee/issuable/destroy_service.rb b/ee/app/services/ee/issuable/destroy_service.rb index eba865e545ce6d..6cfbb8287931d2 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/lib/audit/merge_request_destroy_auditor.rb b/ee/lib/audit/merge_request_destroy_auditor.rb new file mode 100644 index 00000000000000..e09ff4234e468c --- /dev/null +++ b/ee/lib/audit/merge_request_destroy_auditor.rb @@ -0,0 +1,51 @@ +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(:title).to_sentence + approvers = merge_request.approved_by_users.pluck(:id).to_sentence + merged_by_id = merge_request.metrics.merged_by_id + committer_ids = merge_request.committers.pluck(:id).to_sentence + + "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} "\ + "and ID: #{merge_request.id}), labels: #{labels}, approver_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 00000000000000..94d2f3919be645 --- /dev/null +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::MergeRequestDestroyAuditor do + let(:current_user) { create(:user) } + let(:project) { create(:project) } + let(: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).and_return(['label1', 'label2']) + allow(merge_request).to receive_message_chain(:approved_by_users, :pluck).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(:committers, :pluck).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, approver_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 d8086138106c97..24520844331840 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 -- GitLab From f48821c6e568a2e1126f146c11a1164b54031e8c Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 26 Apr 2023 19:58:17 +0530 Subject: [PATCH 2/4] Add audit event yml definition --- app/models/label.rb | 4 ++++ .../types/merged_merge_request_deleted.yml | 9 +++++++++ ee/lib/audit/merge_request_destroy_auditor.rb | 15 +++++++++------ .../merge_request_destroy_auditor_spec.rb | 19 +++++++++++-------- spec/models/label_spec.rb | 11 +++++++++++ 5 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 ee/config/audit_events/types/merged_merge_request_deleted.yml diff --git a/app/models/label.rb b/app/models/label.rb index 3d9ea39d86077c..32b399ac4613f5 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/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 00000000000000..cb209ce9664ab0 --- /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/-/issues/408288 +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 index e09ff4234e468c..ff270c7f0ac8e3 100644 --- a/ee/lib/audit/merge_request_destroy_auditor.rb +++ b/ee/lib/audit/merge_request_destroy_auditor.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Audit class MergeRequestDestroyAuditor attr_reader :merge_request, :current_user @@ -32,20 +34,21 @@ def execute private + # rubocop: disable CodeReuse/ActiveRecord def audit_event_message if merge_request.merged? - labels = merge_request.labels.pluck(:title).to_sentence - approvers = merge_request.approved_by_users.pluck(:id).to_sentence + 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.committers.pluck(:id).to_sentence - "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} "\ - "and ID: #{merge_request.id}), labels: #{labels}, approver_user_ids: #{approvers}, "\ - "merged_by_user_id: #{merged_by_id}, committer_user_ids: #{committer_ids}" + "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 - + # rubocop: enable CodeReuse/ActiveRecord 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 index 94d2f3919be645..a558e0ef44b3a9 100644 --- a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Audit::MergeRequestDestroyAuditor do +RSpec.describe Audit::MergeRequestDestroyAuditor, feature_category: :audit_events do let(:current_user) { create(:user) } let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } @@ -28,7 +28,8 @@ 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})") + expect(args[:message]).to eq("Removed MergeRequest(#{merge_request.title} with IID: " \ + "#{merge_request.iid} and ID: #{merge_request.id})") end end end @@ -36,23 +37,25 @@ 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).and_return(['label1', 'label2']) - allow(merge_request).to receive_message_chain(:approved_by_users, :pluck).and_return([1, 2]) + 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(:committers, :pluck).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')) + 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, approver_user_ids: 1 and 2, "\ - "merged_by_user_id: 3, committer_user_ids: 4 and 5" + 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 diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index ff7ac0ebd2ac4a..26a1edcbcffbf1 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 -- GitLab From 997729d13b2d84ef28af571d8df9185099d5a472 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 26 Apr 2023 21:15:03 +0530 Subject: [PATCH 3/4] Update merge request url --- .../audit_events/types/merged_merge_request_deleted.yml | 2 +- ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/config/audit_events/types/merged_merge_request_deleted.yml b/ee/config/audit_events/types/merged_merge_request_deleted.yml index cb209ce9664ab0..c5bfbc4f7b2e9a 100644 --- a/ee/config/audit_events/types/merged_merge_request_deleted.yml +++ b/ee/config/audit_events/types/merged_merge_request_deleted.yml @@ -2,7 +2,7 @@ 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/-/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 diff --git a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb index a558e0ef44b3a9..837b0eee6ce0a1 100644 --- a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -4,8 +4,9 @@ RSpec.describe Audit::MergeRequestDestroyAuditor, feature_category: :audit_events do let(:current_user) { create(:user) } - let(:project) { create(:project) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + 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) } -- GitLab From 04f0e827e7761fa640be3148557ff3565ca48292 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 4 May 2023 18:43:20 +0530 Subject: [PATCH 4/4] Backend reviewer suggestion --- app/models/commit_collection.rb | 4 ++++ ee/lib/audit/merge_request_destroy_auditor.rb | 4 +--- .../merge_request_destroy_auditor_spec.rb | 2 +- spec/models/commit_collection_spec.rb | 18 ++++++++++++++++++ 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb index eb7db0fc9b4607..edc60a757d2d0f 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/ee/lib/audit/merge_request_destroy_auditor.rb b/ee/lib/audit/merge_request_destroy_auditor.rb index ff270c7f0ac8e3..d015f1202697cd 100644 --- a/ee/lib/audit/merge_request_destroy_auditor.rb +++ b/ee/lib/audit/merge_request_destroy_auditor.rb @@ -34,13 +34,12 @@ def execute private - # rubocop: disable CodeReuse/ActiveRecord 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.committers.pluck(:id).to_sentence + 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}, " \ @@ -49,6 +48,5 @@ def audit_event_message "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} and ID: #{merge_request.id})" end end - # rubocop: enable CodeReuse/ActiveRecord 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 index 837b0eee6ce0a1..01bec181c08cf3 100644 --- a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -41,7 +41,7 @@ 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(:committers, :pluck).and_return([4, 5]) + 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 diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb index 706f18a5337192..1d2d89573bb8fc 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") -- GitLab