From 53cb69a7c16ba461b953f8aa0a96bf2d940a94f8 Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Wed, 17 May 2023 02:01:12 +0530 Subject: [PATCH 1/4] Add audit events for activities performed by a project access token Add audit events when the following actions are performed by a project access token * When an MR is created, closed, reopened or merged * When a comment is added to an issue, MR etc. Changelog: added EE: true --- app/services/merge_requests/close_service.rb | 2 +- app/services/merge_requests/reopen_service.rb | 2 ++ .../ee/merge_requests/after_create_service.rb | 18 ++++++++++ .../ee/merge_requests/close_service.rb | 12 +++++++ .../ee/merge_requests/post_merge_service.rb | 19 ++++++++++ .../ee/merge_requests/reopen_service.rb | 35 +++++++++++++++++++ .../services/ee/notes/post_process_service.rb | 22 ++++++++++++ .../types/comment_by_project_bot.yml | 10 ++++++ .../merge_request_closed_by_project_bot.yml | 9 +++++ .../merge_request_created_by_project_bot.yml | 9 +++++ .../merge_request_merged_by_project_bot.yml | 9 +++++ .../merge_request_reopened_by_project_bot.yml | 9 +++++ .../after_create_service_spec.rb | 21 +++++++++++ .../ee/merge_requests/close_service_spec.rb | 31 ++++++++++++++++ .../merge_requests/post_merge_service_spec.rb | 32 +++++++++++++++++ .../ee/merge_requests/reopen_service_spec.rb | 35 +++++++++++++++++++ .../ee/notes/post_process_service_spec.rb | 26 ++++++++++++++ 17 files changed, 300 insertions(+), 1 deletion(-) create mode 100644 ee/app/services/ee/merge_requests/reopen_service.rb create mode 100644 ee/config/audit_events/types/comment_by_project_bot.yml create mode 100644 ee/config/audit_events/types/merge_request_closed_by_project_bot.yml create mode 100644 ee/config/audit_events/types/merge_request_created_by_project_bot.yml create mode 100644 ee/config/audit_events/types/merge_request_merged_by_project_bot.yml create mode 100644 ee/config/audit_events/types/merge_request_reopened_by_project_bot.yml create mode 100644 ee/spec/services/ee/merge_requests/reopen_service_spec.rb diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 867e8221e5e01f..62928e05a8995b 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -51,4 +51,4 @@ def trigger_merge_request_merge_status_updated(merge_request) end end -MergeRequests::CloseService.prepend_mod_with('MergeRequests::CloseService') +MergeRequests::CloseService.prepend_mod diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb index d2247a6d4c110b..b2e15cc7c7e442 100644 --- a/app/services/merge_requests/reopen_service.rb +++ b/app/services/merge_requests/reopen_service.rb @@ -37,3 +37,5 @@ def create_event(merge_request) end end end + +MergeRequests::ReopenService.prepend_mod diff --git a/ee/app/services/ee/merge_requests/after_create_service.rb b/ee/app/services/ee/merge_requests/after_create_service.rb index feaf0b76b2f48d..30d65d341cba44 100644 --- a/ee/app/services/ee/merge_requests/after_create_service.rb +++ b/ee/app/services/ee/merge_requests/after_create_service.rb @@ -11,6 +11,7 @@ def prepare_merge_request(merge_request) schedule_sync_for(merge_request) schedule_fetch_suggested_reviewers(merge_request) + log_audit_event(merge_request) if current_user.project_bot? end private @@ -34,6 +35,23 @@ def schedule_fetch_suggested_reviewers(merge_request) ::MergeRequests::FetchSuggestedReviewersWorker.perform_async(merge_request.id) end + + def log_audit_event(merge_request) + audit_context = { + name: 'merge_request_created_by_project_bot', + author: current_user, + scope: merge_request.target_project, + target: merge_request, + message: "Created merge request #{merge_request.title}", + target_details: + { iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/app/services/ee/merge_requests/close_service.rb b/ee/app/services/ee/merge_requests/close_service.rb index 5721df914e5ea0..92c681f351e156 100644 --- a/ee/app/services/ee/merge_requests/close_service.rb +++ b/ee/app/services/ee/merge_requests/close_service.rb @@ -3,6 +3,18 @@ module EE module MergeRequests module CloseService + extend ::Gitlab::Utils::Override + + override :execute + def execute(merge_request, commit = nil) + super.tap do + if current_user.project_bot? + log_audit_event(merge_request, 'merge_request_closed_by_project_bot', + "Closed merge request #{merge_request.title}") + end + end + end + def expire_unapproved_key(merge_request) merge_request.approval_state.expire_unapproved_key! 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 6fac0815059336..3242cabcb7c3e4 100644 --- a/ee/app/services/ee/merge_requests/post_merge_service.rb +++ b/ee/app/services/ee/merge_requests/post_merge_service.rb @@ -18,6 +18,7 @@ def execute(merge_request) merge_request.approval_state.expire_unapproved_key! audit_approval_rules(merge_request) + audit_merge_event(merge_request) if current_user.project_bot? sync_security_scan_orchestration_policies(target_project) trigger_blocked_merge_requests_merge_status_updated(merge_request) end @@ -77,6 +78,24 @@ def trigger_blocked_merge_requests_merge_status_updated(merge_request) ::GraphqlTriggers.merge_request_merge_status_updated(blocked_mr) end end + + def audit_merge_event(merge_request) + audit_context = { + name: 'merge_request_merged_by_project_bot', + author: current_user, + scope: merge_request.target_project, + target: merge_request, + message: "Merged merge request #{merge_request.title}", + target_details: + { iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + merge_commit_sha: merge_request.merge_commit_sha } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/app/services/ee/merge_requests/reopen_service.rb b/ee/app/services/ee/merge_requests/reopen_service.rb new file mode 100644 index 00000000000000..dfee4e14a40ef0 --- /dev/null +++ b/ee/app/services/ee/merge_requests/reopen_service.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module ReopenService + extend ::Gitlab::Utils::Override + + override :execute + def execute(merge_request) + super.tap do + log_audit_event(merge_request) if current_user.project_bot? + end + end + + private + + def log_audit_event(merge_request) + audit_context = { + name: 'merge_request_reopened_by_project_bot', + author: current_user, + scope: merge_request.target_project, + target: merge_request, + message: "Reopened merge request #{merge_request.title}", + target_details: + { iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/app/services/ee/notes/post_process_service.rb b/ee/app/services/ee/notes/post_process_service.rb index 7fe31806ccdf5d..3c2a19cf897024 100644 --- a/ee/app/services/ee/notes/post_process_service.rb +++ b/ee/app/services/ee/notes/post_process_service.rb @@ -11,6 +11,28 @@ def execute super ::Analytics::RefreshCommentsData.for_note(note)&.execute + + log_audit_event if note.author.project_bot? + end + + private + + def log_audit_event + audit_context = { + name: 'comment_by_project_bot', + author: note.author, + scope: note.project, + target: note, + message: "Added comment: #{::Gitlab::UrlBuilder.note_url(note)}", + target_details: { + id: note.id, + noteable_type: note.noteable_type, + noteable_id: note.noteable_id + }, + stream_only: true + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end end end diff --git a/ee/config/audit_events/types/comment_by_project_bot.yml b/ee/config/audit_events/types/comment_by_project_bot.yml new file mode 100644 index 00000000000000..b0ecb02e34240e --- /dev/null +++ b/ee/config/audit_events/types/comment_by_project_bot.yml @@ -0,0 +1,10 @@ +--- +name: comment_by_project_bot +description: Triggered when a comment is added to an issue or an MR using the project + access token +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 +feature_category: team_planning +milestone: '16.1' +saved_to_database: false +streamed: true diff --git a/ee/config/audit_events/types/merge_request_closed_by_project_bot.yml b/ee/config/audit_events/types/merge_request_closed_by_project_bot.yml new file mode 100644 index 00000000000000..95c3f7dd322ff0 --- /dev/null +++ b/ee/config/audit_events/types/merge_request_closed_by_project_bot.yml @@ -0,0 +1,9 @@ +--- +name: merge_request_closed_by_project_bot +description: Triggered when a merge request is closed using a project access token +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 +feature_category: code_review_workflow +milestone: '16.1' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/merge_request_created_by_project_bot.yml b/ee/config/audit_events/types/merge_request_created_by_project_bot.yml new file mode 100644 index 00000000000000..5c88538e9787a2 --- /dev/null +++ b/ee/config/audit_events/types/merge_request_created_by_project_bot.yml @@ -0,0 +1,9 @@ +--- +name: merge_request_created_by_project_bot +description: Triggered when a merge request is created using a project access token +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 +feature_category: code_review_workflow +milestone: '16.1' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/merge_request_merged_by_project_bot.yml b/ee/config/audit_events/types/merge_request_merged_by_project_bot.yml new file mode 100644 index 00000000000000..2b2b0f9820edaa --- /dev/null +++ b/ee/config/audit_events/types/merge_request_merged_by_project_bot.yml @@ -0,0 +1,9 @@ +--- +name: merge_request_merged_by_project_bot +description: Triggered when a merge request is merged using a project access token +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 +feature_category: code_review_workflow +milestone: '16.1' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/merge_request_reopened_by_project_bot.yml b/ee/config/audit_events/types/merge_request_reopened_by_project_bot.yml new file mode 100644 index 00000000000000..bedbcc41dcdf22 --- /dev/null +++ b/ee/config/audit_events/types/merge_request_reopened_by_project_bot.yml @@ -0,0 +1,9 @@ +--- +name: merge_request_reopened_by_project_bot +description: Triggered when a merge request is reopened using a project access token +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 +feature_category: code_review_workflow +milestone: '16.1' +saved_to_database: true +streamed: true diff --git a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb index 85f5f7b54efb49..ea279f0c03e001 100644 --- a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb @@ -112,5 +112,26 @@ end end end + + context 'for audit events' do + context 'when merge request author is a project bot' do + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + + it 'creates an audit event' do + expect { execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + + expect(event.details[:custom_message]).to eq("Created merge request #{merge_request.title}") + end + end + + context 'when merge request author is not a project bot' do + it 'does not create an audit event' do + expect { execute }.to change { AuditEvent.count }.by(0) + end + end + end end end diff --git a/ee/spec/services/ee/merge_requests/close_service_spec.rb b/ee/spec/services/ee/merge_requests/close_service_spec.rb index 485e43040cec9b..44423fe01528d9 100644 --- a/ee/spec/services/ee/merge_requests/close_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/close_service_spec.rb @@ -48,5 +48,36 @@ end end end + + context 'for audit events' do + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + + include_examples 'audit event logging' do + let(:operation) { service.execute(merge_request) } + let(:event_type) { 'merge_request_closed_by_project_bot' } + let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch + }.to_s, + author_class: project_bot.class.name, + custom_message: "Closed merge request #{merge_request.title}" + } + } + 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 cd09ac1526af56..290e721e687846 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 @@ -218,5 +218,37 @@ expect(merge_request.approval_state.temporarily_unapproved?).to be_falsey end end + + context 'for audit events' do + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + + include_examples 'audit event logging' do + let(:operation) { subject } + let(:event_type) { 'merge_request_merged_by_project_bot' } + let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + merge_commit_sha: merge_request.merge_commit_sha + }.to_s, + author_class: project_bot.class.name, + custom_message: "Merged merge request #{merge_request.title}" + } + } + end + end + end end end diff --git a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb new file mode 100644 index 00000000000000..41c4a072ad946c --- /dev/null +++ b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ReopenService, feature_category: :code_review_workflow do + describe '#execute' do + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:project) { merge_request.target_project } + + let(:service_object) { described_class.new(project: project, current_user: merge_request.author) } + + subject(:merge_request_close_service) { service_object.execute(merge_request) } + + context 'for audit events' do + context 'when merge request author is a project bot' do + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + + it 'creates an audit event' do + expect { merge_request_close_service }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + + expect(event.details[:custom_message]).to eq("Reopened merge request #{merge_request.title}") + end + end + + context 'when merge request author is not a project bot' do + it 'does not create an audit event' do + expect { merge_request_close_service }.to change { AuditEvent.count }.by(0) + end + end + end + end +end diff --git a/ee/spec/services/ee/notes/post_process_service_spec.rb b/ee/spec/services/ee/notes/post_process_service_spec.rb index e775ac3f7fa358..8fbc2f934470ce 100644 --- a/ee/spec/services/ee/notes/post_process_service_spec.rb +++ b/ee/spec/services/ee/notes/post_process_service_spec.rb @@ -18,5 +18,31 @@ subject.execute end end + + context 'for audit events' do + subject(:notes_post_process_service) { described_class.new(note) } + + context 'when note author is a project bot' do + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + + let(:note) { create(:note, author: project_bot) } + + it 'audits with correct name' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(name: "comment_by_project_bot", stream_only: true) + ).and_call_original + + subject.execute + end + end + + context 'when note author is not a project bot' do + let(:note) { create(:note) } + + it 'does not create an audit event' do + expect { notes_post_process_service.execute }.to change { AuditEvent.count }.by(0) + end + end + end end end -- GitLab From 3022783293d8ea1ca3d45f79cec14b6e9f878736 Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 18 May 2023 16:52:22 +0530 Subject: [PATCH 2/4] Move log_audit_event method to MergeRequests::BaserService --- .../ee/merge_requests/after_create_service.rb | 23 ++++------------- .../ee/merge_requests/base_service.rb | 21 ++++++++++++++++ .../ee/merge_requests/post_merge_service.rb | 25 +++++-------------- .../ee/merge_requests/reopen_service.rb | 24 +++--------------- 4 files changed, 36 insertions(+), 57 deletions(-) diff --git a/ee/app/services/ee/merge_requests/after_create_service.rb b/ee/app/services/ee/merge_requests/after_create_service.rb index 30d65d341cba44..96257416e99d7e 100644 --- a/ee/app/services/ee/merge_requests/after_create_service.rb +++ b/ee/app/services/ee/merge_requests/after_create_service.rb @@ -11,7 +11,11 @@ def prepare_merge_request(merge_request) schedule_sync_for(merge_request) schedule_fetch_suggested_reviewers(merge_request) - log_audit_event(merge_request) if current_user.project_bot? + + if current_user.project_bot? # rubocop:disable Style/GuardClause + log_audit_event(merge_request, 'merge_request_created_by_project_bot', + "Created merge request #{merge_request.title}") + end end private @@ -35,23 +39,6 @@ def schedule_fetch_suggested_reviewers(merge_request) ::MergeRequests::FetchSuggestedReviewersWorker.perform_async(merge_request.id) end - - def log_audit_event(merge_request) - audit_context = { - name: 'merge_request_created_by_project_bot', - author: current_user, - scope: merge_request.target_project, - target: merge_request, - message: "Created merge request #{merge_request.title}", - target_details: - { iid: merge_request.iid, - id: merge_request.id, - source_branch: merge_request.source_branch, - target_branch: merge_request.target_branch } - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - end end end end diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb index 40cc5231f28fd3..e14892443e2bd7 100644 --- a/ee/app/services/ee/merge_requests/base_service.rb +++ b/ee/app/services/ee/merge_requests/base_service.rb @@ -108,6 +108,27 @@ def capture_suggested_reviewers_accepted(merge_request) ::MergeRequests::CaptureSuggestedReviewersAcceptedWorker .perform_async(merge_request.id, suggested_reviewer_ids) end + + def log_audit_event(merge_request, event_name, message) + audit_context = { + name: event_name, + author: current_user, + scope: merge_request.target_project, + target: merge_request, + message: message, + target_details: + { iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch } + } + + if event_name == 'merge_request_merged_by_project_bot' + audit_context[:target_details][:merge_commit_sha] = merge_request.merge_commit_sha + end + + ::Gitlab::Audit::Auditor.audit(audit_context) + end 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 3242cabcb7c3e4..593827d15e2590 100644 --- a/ee/app/services/ee/merge_requests/post_merge_service.rb +++ b/ee/app/services/ee/merge_requests/post_merge_service.rb @@ -18,7 +18,12 @@ def execute(merge_request) merge_request.approval_state.expire_unapproved_key! audit_approval_rules(merge_request) - audit_merge_event(merge_request) if current_user.project_bot? + + if current_user.project_bot? + log_audit_event(merge_request, 'merge_request_merged_by_project_bot', + "Merged merge request #{merge_request.title}") + end + sync_security_scan_orchestration_policies(target_project) trigger_blocked_merge_requests_merge_status_updated(merge_request) end @@ -78,24 +83,6 @@ def trigger_blocked_merge_requests_merge_status_updated(merge_request) ::GraphqlTriggers.merge_request_merge_status_updated(blocked_mr) end end - - def audit_merge_event(merge_request) - audit_context = { - name: 'merge_request_merged_by_project_bot', - author: current_user, - scope: merge_request.target_project, - target: merge_request, - message: "Merged merge request #{merge_request.title}", - target_details: - { iid: merge_request.iid, - id: merge_request.id, - source_branch: merge_request.source_branch, - target_branch: merge_request.target_branch, - merge_commit_sha: merge_request.merge_commit_sha } - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - end end end end diff --git a/ee/app/services/ee/merge_requests/reopen_service.rb b/ee/app/services/ee/merge_requests/reopen_service.rb index dfee4e14a40ef0..58e62471429241 100644 --- a/ee/app/services/ee/merge_requests/reopen_service.rb +++ b/ee/app/services/ee/merge_requests/reopen_service.rb @@ -8,28 +8,12 @@ module ReopenService override :execute def execute(merge_request) super.tap do - log_audit_event(merge_request) if current_user.project_bot? + if current_user.project_bot? + log_audit_event(merge_request, 'merge_request_reopened_by_project_bot', + "Reopened merge request #{merge_request.title}") + end end end - - private - - def log_audit_event(merge_request) - audit_context = { - name: 'merge_request_reopened_by_project_bot', - author: current_user, - scope: merge_request.target_project, - target: merge_request, - message: "Reopened merge request #{merge_request.title}", - target_details: - { iid: merge_request.iid, - id: merge_request.id, - source_branch: merge_request.source_branch, - target_branch: merge_request.target_branch } - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - end end end end -- GitLab From 074c1743a774b6c8d117cf272d90aabcaf9d5568 Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 18 May 2023 18:24:16 +0530 Subject: [PATCH 3/4] Use audit event logging shared example Make fail_condition! as optional in audit_event_logging_shared_examples as some of the audit events might not have a fail condition --- .../types/comment_by_project_bot.yml | 3 +- .../after_create_service_spec.rb | 31 +++++++++++++---- .../ee/merge_requests/close_service_spec.rb | 2 +- .../ee/merge_requests/reopen_service_spec.rb | 33 ++++++++++++++----- .../ee/notes/post_process_service_spec.rb | 16 +++++++-- .../audit_event_logging_shared_examples.rb | 6 ++-- 6 files changed, 68 insertions(+), 23 deletions(-) diff --git a/ee/config/audit_events/types/comment_by_project_bot.yml b/ee/config/audit_events/types/comment_by_project_bot.yml index b0ecb02e34240e..6e915a00b33a72 100644 --- a/ee/config/audit_events/types/comment_by_project_bot.yml +++ b/ee/config/audit_events/types/comment_by_project_bot.yml @@ -1,7 +1,6 @@ --- name: comment_by_project_bot -description: Triggered when a comment is added to an issue or an MR using the project - access token +description: Triggered when a comment is added to an issue or an MR using the project access token introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/323299 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120927 feature_category: team_planning diff --git a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb index ea279f0c03e001..2b26c2f6e2b6f8 100644 --- a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb @@ -118,18 +118,35 @@ let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } let_it_be(:merge_request) { create(:merge_request, author: project_bot) } - it 'creates an audit event' do - expect { execute }.to change { AuditEvent.count }.by(1) - - event = AuditEvent.last - - expect(event.details[:custom_message]).to eq("Created merge request #{merge_request.title}") + include_examples 'audit event logging' do + let(:operation) { execute } + let(:event_type) { 'merge_request_created_by_project_bot' } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch + }.to_s, + author_class: project_bot.class.name, + custom_message: "Added comment: #{::Gitlab::UrlBuilder.note_url(note)}" + } + } + end end end context 'when merge request author is not a project bot' do it 'does not create an audit event' do - expect { execute }.to change { AuditEvent.count }.by(0) + expect { execute }.not_to change { AuditEvent.count } end end end diff --git a/ee/spec/services/ee/merge_requests/close_service_spec.rb b/ee/spec/services/ee/merge_requests/close_service_spec.rb index 44423fe01528d9..58dc6b4a49c9f8 100644 --- a/ee/spec/services/ee/merge_requests/close_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/close_service_spec.rb @@ -54,7 +54,7 @@ let_it_be(:merge_request) { create(:merge_request, author: project_bot) } include_examples 'audit event logging' do - let(:operation) { service.execute(merge_request) } + let(:operation) { merge_request_close_service } let(:event_type) { 'merge_request_closed_by_project_bot' } let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } let(:attributes) do diff --git a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb index 41c4a072ad946c..130587850b0755 100644 --- a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb @@ -9,25 +9,42 @@ let(:service_object) { described_class.new(project: project, current_user: merge_request.author) } - subject(:merge_request_close_service) { service_object.execute(merge_request) } + subject(:merge_request_reopen_service) { service_object.execute(merge_request) } context 'for audit events' do context 'when merge request author is a project bot' do let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } let_it_be(:merge_request) { create(:merge_request, author: project_bot) } - it 'creates an audit event' do - expect { merge_request_close_service }.to change { AuditEvent.count }.by(1) - - event = AuditEvent.last - - expect(event.details[:custom_message]).to eq("Reopened merge request #{merge_request.title}") + include_examples 'audit event logging' do + let(:operation) { merge_request_reopen_service } + let(:event_type) { 'merge_request_reopened_by_project_bot' } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch + }.to_s, + author_class: project_bot.class.name, + custom_message: "Reopened merge request #{merge_request.title}" + } + } + end end end context 'when merge request author is not a project bot' do it 'does not create an audit event' do - expect { merge_request_close_service }.to change { AuditEvent.count }.by(0) + expect { merge_request_reopen_service }.not_to change { AuditEvent.count } end end end diff --git a/ee/spec/services/ee/notes/post_process_service_spec.rb b/ee/spec/services/ee/notes/post_process_service_spec.rb index 8fbc2f934470ce..c791f69c1ff45d 100644 --- a/ee/spec/services/ee/notes/post_process_service_spec.rb +++ b/ee/spec/services/ee/notes/post_process_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Notes::PostProcessService do +RSpec.describe Notes::PostProcessService, feature_category: :team_planning do describe '#execute' do context 'analytics' do subject { described_class.new(note) } @@ -32,15 +32,25 @@ hash_including(name: "comment_by_project_bot", stream_only: true) ).and_call_original - subject.execute + notes_post_process_service.execute + end + + it 'does not persist the audit event to database' do + expect { notes_post_process_service.execute }.not_to change { AuditEvent.count } end end context 'when note author is not a project bot' do let(:note) { create(:note) } + it 'does not invoke Gitlab::Audit::Auditor' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + notes_post_process_service.execute + end + it 'does not create an audit event' do - expect { notes_post_process_service.execute }.to change { AuditEvent.count }.by(0) + expect { notes_post_process_service.execute }.not_to change { AuditEvent.count } end end end diff --git a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb index a44124b65a3f98..04b01f1538cc1c 100644 --- a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb @@ -35,9 +35,11 @@ end it 'does not log audit event if operation fails' do - fail_condition! + if defined?(fail_condition!) + fail_condition! - expect { operation }.not_to change(AuditEvent, :count) + expect { operation }.not_to change(AuditEvent, :count) + end end it 'does not log audit event if operation results in no change' do -- GitLab From a094820dc5c1324b00b7825d7faf5f0d0f71100a Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Fri, 19 May 2023 14:05:44 +0530 Subject: [PATCH 4/4] Use base_service log_audit_event method in close_service & update tests Use fail_condition! from audit_event_logging_shared_examples in all the RSpecs --- .../ee/merge_requests/after_create_service.rb | 8 +-- .../after_create_service_spec.rb | 57 ++++++++----------- .../ee/merge_requests/close_service_spec.rb | 2 +- .../ee/merge_requests/reopen_service_spec.rb | 55 ++++++++---------- .../audit_event_logging_shared_examples.rb | 6 +- 5 files changed, 56 insertions(+), 72 deletions(-) diff --git a/ee/app/services/ee/merge_requests/after_create_service.rb b/ee/app/services/ee/merge_requests/after_create_service.rb index 96257416e99d7e..1ff370798aa082 100644 --- a/ee/app/services/ee/merge_requests/after_create_service.rb +++ b/ee/app/services/ee/merge_requests/after_create_service.rb @@ -9,13 +9,13 @@ module AfterCreateService def prepare_merge_request(merge_request) super - schedule_sync_for(merge_request) - schedule_fetch_suggested_reviewers(merge_request) - - if current_user.project_bot? # rubocop:disable Style/GuardClause + if current_user.project_bot? log_audit_event(merge_request, 'merge_request_created_by_project_bot', "Created merge request #{merge_request.title}") end + + schedule_sync_for(merge_request) + schedule_fetch_suggested_reviewers(merge_request) end private diff --git a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb index 2b26c2f6e2b6f8..3760829c07eb5b 100644 --- a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb @@ -114,39 +114,32 @@ end context 'for audit events' do - context 'when merge request author is a project bot' do - let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } - let_it_be(:merge_request) { create(:merge_request, author: project_bot) } - - include_examples 'audit event logging' do - let(:operation) { execute } - let(:event_type) { 'merge_request_created_by_project_bot' } - let(:attributes) do - { - author_id: project_bot.id, - entity_id: merge_request.target_project.id, - entity_type: 'Project', - details: { - author_name: project_bot.name, - target_id: merge_request.id, - target_type: 'MergeRequest', - target_details: { - iid: merge_request.iid, - id: merge_request.id, - source_branch: merge_request.source_branch, - target_branch: merge_request.target_branch - }.to_s, - author_class: project_bot.class.name, - custom_message: "Added comment: #{::Gitlab::UrlBuilder.note_url(note)}" - } + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + + include_examples 'audit event logging' do + let(:operation) { execute } + let(:event_type) { 'merge_request_created_by_project_bot' } + let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch + }.to_s, + author_class: project_bot.class.name, + custom_message: "Created merge request #{merge_request.title}" } - end - end - end - - context 'when merge request author is not a project bot' do - it 'does not create an audit event' do - expect { execute }.not_to change { AuditEvent.count } + } end end end diff --git a/ee/spec/services/ee/merge_requests/close_service_spec.rb b/ee/spec/services/ee/merge_requests/close_service_spec.rb index 58dc6b4a49c9f8..44423fe01528d9 100644 --- a/ee/spec/services/ee/merge_requests/close_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/close_service_spec.rb @@ -54,7 +54,7 @@ let_it_be(:merge_request) { create(:merge_request, author: project_bot) } include_examples 'audit event logging' do - let(:operation) { merge_request_close_service } + let(:operation) { service.execute(merge_request) } let(:event_type) { 'merge_request_closed_by_project_bot' } let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } let(:attributes) do diff --git a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb index 130587850b0755..4e25ee5a296931 100644 --- a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb @@ -12,39 +12,32 @@ subject(:merge_request_reopen_service) { service_object.execute(merge_request) } context 'for audit events' do - context 'when merge request author is a project bot' do - let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } - let_it_be(:merge_request) { create(:merge_request, author: project_bot) } + let_it_be(:project_bot) { create(:user, :project_bot, email: "bot@example.com") } + let_it_be(:merge_request) { create(:merge_request, author: project_bot) } - include_examples 'audit event logging' do - let(:operation) { merge_request_reopen_service } - let(:event_type) { 'merge_request_reopened_by_project_bot' } - let(:attributes) do - { - author_id: project_bot.id, - entity_id: merge_request.target_project.id, - entity_type: 'Project', - details: { - author_name: project_bot.name, - target_id: merge_request.id, - target_type: 'MergeRequest', - target_details: { - iid: merge_request.iid, - id: merge_request.id, - source_branch: merge_request.source_branch, - target_branch: merge_request.target_branch - }.to_s, - author_class: project_bot.class.name, - custom_message: "Reopened merge request #{merge_request.title}" - } + include_examples 'audit event logging' do + let(:operation) { merge_request_reopen_service } + let(:event_type) { 'merge_request_reopened_by_project_bot' } + let(:fail_condition!) { expect(project_bot).to receive(:project_bot?).and_return(false) } + let(:attributes) do + { + author_id: project_bot.id, + entity_id: merge_request.target_project.id, + entity_type: 'Project', + details: { + author_name: project_bot.name, + target_id: merge_request.id, + target_type: 'MergeRequest', + target_details: { + iid: merge_request.iid, + id: merge_request.id, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch + }.to_s, + author_class: project_bot.class.name, + custom_message: "Reopened merge request #{merge_request.title}" } - end - end - end - - context 'when merge request author is not a project bot' do - it 'does not create an audit event' do - expect { merge_request_reopen_service }.not_to change { AuditEvent.count } + } end end end diff --git a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb index 04b01f1538cc1c..a44124b65a3f98 100644 --- a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb @@ -35,11 +35,9 @@ end it 'does not log audit event if operation fails' do - if defined?(fail_condition!) - fail_condition! + fail_condition! - expect { operation }.not_to change(AuditEvent, :count) - end + expect { operation }.not_to change(AuditEvent, :count) end it 'does not log audit event if operation results in no change' do -- GitLab