From 909ff5d66559259064c5ac6e9d8873ca55d22a4d Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 13 Dec 2022 12:13:07 +1300 Subject: [PATCH 1/6] Add meaningful event names for project updates --- ee/lib/audit/project_changes_auditor.rb | 108 ++++-- .../lib/audit/project_changes_auditor_spec.rb | 347 ++++++++++++------ 2 files changed, 313 insertions(+), 142 deletions(-) diff --git a/ee/lib/audit/project_changes_auditor.rb b/ee/lib/audit/project_changes_auditor.rb index 6b5c3e39f187ed..076910dca2eb00 100644 --- a/ee/lib/audit/project_changes_auditor.rb +++ b/ee/lib/audit/project_changes_auditor.rb @@ -3,37 +3,101 @@ module Audit class ProjectChangesAuditor < BaseChangesAuditor def execute - audit_changes(:visibility_level, as: 'visibility', model: model) + audit_changes(:visibility_level, as: 'visibility', model: model, event_type: 'project_visibility_updated') audit_changes(:path, as: 'path', model: model, event_type: 'project_path_updated') audit_changes(:name, as: 'name', model: model, event_type: 'project_name_updated') - audit_changes(:namespace_id, as: 'namespace', model: model) - audit_changes(:repository_size_limit, as: 'repository_size_limit', model: model) - audit_changes(:packages_enabled, as: 'packages_enabled', model: model) + audit_changes(:namespace_id, as: 'namespace', model: model, event_type: 'project_namespace_updated') + audit_changes( + :repository_size_limit, + as: 'repository_size_limit', + model: model, + event_type: 'project_repository_size_limit_updated' + ) + audit_changes( + :packages_enabled, + as: 'packages_enabled', + model: model, + event_type: 'project_packages_enabled_updated' + ) - audit_changes(:merge_requests_author_approval, as: 'prevent merge request approval from authors', model: model) - audit_changes(:merge_requests_disable_committers_approval, - as: 'prevent merge request approval from committers', model: model) - audit_changes(:reset_approvals_on_push, - as: 'require new approvals when new commits are added to an MR', model: model) - audit_changes(:disable_overriding_approvers_per_merge_request, - as: 'prevent users from modifying MR approval rules in merge requests', model: model) - audit_changes(:require_password_to_approve, as: 'require user password for approvals', model: model) + audit_changes( + :merge_requests_author_approval, + as: 'prevent merge request approval from authors', + model: model, + event_type: 'project_merge_requests_author_approval_updated' + ) + audit_changes( + :merge_requests_disable_committers_approval, + as: 'prevent merge request approval from committers', + model: model, + event_type: 'project_merge_requests_disable_committers_approval_updated' + ) + audit_changes( + :reset_approvals_on_push, + as: 'require new approvals when new commits are added to an MR', + model: model, + event_type: 'project_reset_approvals_on_push_updated' + ) + audit_changes( + :disable_overriding_approvers_per_merge_request, + as: 'prevent users from modifying MR approval rules in merge requests', + model: model, + event_type: 'project_disable_overriding_approvers_per_merge_request_updated' + ) + audit_changes( + :require_password_to_approve, + as: 'require user password for approvals', + model: model, + event_type: 'project_require_password_to_approve_updated' + ) if should_audit? :merge_requests_template - audit_changes(:merge_requests_template, as: 'merge_requests_template', model: model) + audit_changes( + :merge_requests_template, + as: 'merge_requests_template', + model: model, + event_type: 'project_merge_requests_template_updated' + ) end - audit_changes(:resolve_outdated_diff_discussions, as: 'resolve_outdated_diff_discussions', model: model) - audit_changes(:printing_merge_request_link_enabled, - as: 'printing_merge_request_link_enabled', model: model) - audit_changes(:remove_source_branch_after_merge, as: 'remove_source_branch_after_merge', model: model) - audit_changes(:only_allow_merge_if_pipeline_succeeds, - as: 'only_allow_merge_if_pipeline_succeeds', model: model) - audit_changes(:only_allow_merge_if_all_discussions_are_resolved, - as: 'only_allow_merge_if_all_discussions_are_resolved', model: model) + audit_changes( + :resolve_outdated_diff_discussions, + as: 'resolve_outdated_diff_discussions', + model: model, + event_type: 'project_resolve_outdated_diff_discussions_updated' + ) + audit_changes( + :printing_merge_request_link_enabled, + as: 'printing_merge_request_link_enabled', + model: model, + event_type: 'project_printing_merge_request_link_enabled_updated' + ) + audit_changes( + :remove_source_branch_after_merge, + as: 'remove_source_branch_after_merge', + model: model, + event_type: 'project_remove_source_branch_after_merge_updated' + ) + audit_changes( + :only_allow_merge_if_pipeline_succeeds, + as: 'only_allow_merge_if_pipeline_succeeds', + model: model, + event_type: 'project_only_allow_merge_if_pipeline_succeeds_updated' + ) + audit_changes( + :only_allow_merge_if_all_discussions_are_resolved, + as: 'only_allow_merge_if_all_discussions_are_resolved', + model: model, + event_type: 'project_only_allow_merge_if_all_discussions_are_resolved_updated' + ) if should_audit?(:suggestion_commit_message) - audit_changes(:suggestion_commit_message, as: 'suggestion_commit_message', model: model) + audit_changes( + :suggestion_commit_message, + as: 'suggestion_commit_message', + model: model, + event_type: 'project_suggestion_commit_message_updated' + ) end audit_merge_method diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 7a9dbdc9a112b0..6b611d3b475b52 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -32,6 +32,48 @@ stub_licensed_features(extended_audit_events: true) end + def expect_project_audit_events_from(project, event, change, change_from, change_to) + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: event.to_s, + author: user, + scope: project, + target: project, + message: "Changed #{change} from #{change_from} to #{change_to}", + additional_details: { + change: change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end + + def expect_project_audit_events(project, event, change, change_from, change_to) + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: event.to_s, + author: user, + scope: project, + target: project, + message: "Changed #{change} to #{change_to}", + additional_details: { + change: change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end + describe 'non audit changes' do it 'does not call the audit event service' do project.update!(description: 'new description') @@ -41,163 +83,163 @@ end describe 'audit changes' do - it 'creates an event when the visibility change' do - project.update!(visibility_level: 20) + context 'when project visibility is updated' do + it "logs project_visibility_updated event" do + change = "visibility" + event = "project_visibility_updated" + from = "Private" + to = "Public" - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq 'visibility' + project.update!(visibility_level: 20) + + expect_project_audit_events_from(project, event, change, from, to) + end end context 'when project name is updated' do it "logs project_name_updated event" do - old_project_name = project.full_name.to_s + change = "name" + event = "project_name_updated" + from = project.full_name project.update!(name: 'newname') - expect(Gitlab::Audit::Auditor).to receive(:audit).with( - { - name: 'project_name_updated', - author: user, - scope: project, - target: project, - message: "Changed name from #{old_project_name} to #{project.full_name}", - additional_details: { - change: "name", - from: old_project_name, - target_details: project.full_path.to_s, - to: project.full_name.to_s - }, - target_details: project.full_path.to_s - } - ).and_call_original - - auditor_instance.execute + to = project.full_name + + expect_project_audit_events_from(project, event, change, from, to) end end context 'when project path is updated' do it "logs project_path_updated event" do + change = "path" + event = "project_path_updated" + from = "" + project.update!(path: 'newpath') - expect(Gitlab::Audit::Auditor).to receive(:audit).with( - { - name: 'project_path_updated', - author: user, - scope: project, - target: project, - message: "Changed path to #{project.full_path}", - additional_details: { - change: "path", - from: "", - target_details: project.full_path.to_s, - to: project.full_path.to_s - }, - target_details: project.full_path.to_s - } - ).and_call_original - - auditor_instance.execute + to = project.full_path + + expect_project_audit_events(project, event, change, from, to) end end - it 'creates an event when the namespace change' do - new_namespace = create(:namespace) + context 'when project namespace is updated' do + it "logs project_namespace_updated event" do + change = "namespace" + event = "project_namespace_updated" + from = project.old_path_with_namespace + new_namespace = create(:namespace) + + project.update!(namespace: new_namespace) - project.update!(namespace: new_namespace) + to = project.full_path - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq 'namespace' + expect_project_audit_events(project, event, change, from, to) + end end - it 'creates an event when the repository size limit changes' do - project.update!(repository_size_limit: 100) + context 'when project repository size limit is updated' do + it "logs project_repository_size_limit_updated event" do + change = "repository_size_limit" + event = "project_repository_size_limit_updated" + from = project.repository_size_limit + to = 100 + + project.update!(repository_size_limit: to) - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq 'repository_size_limit' + expect_project_audit_events_from(project, event, change, from, to) + end end - it 'creates an event when the packages enabled setting changes' do - project.update!(packages_enabled: false) + context 'when project packages enabled setting changes is updated' do + it "logs project_packages_enabled_updated event" do + change = "packages_enabled" + event = "project_packages_enabled_updated" + from = true + to = false - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(2) - expect(AuditEvent.last(2).map { |e| e.details[:change] }) - .to eq %w[packages_enabled package_registry_access_level] + project.update!(packages_enabled: to) + + expect_project_audit_events_from(project, event, change, from, to) + end end - it 'creates an event when the merge requests template changes' do - project.update!(merge_requests_template: 'I am a merge request template') + context 'when project merge_requests_template is updated' do + it "logs project_merge_requests_template_updated event" do + change = "merge_requests_template" + event = "project_merge_requests_template_updated" + from = project.merge_requests_template + to = 'I am a merge request template' + + project.update!(merge_requests_template: to) - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq 'merge_requests_template' - expect(AuditEvent.last.details).to include({ - change: 'merge_requests_template', - from: nil, - to: 'I am a merge request template' - }) + expect_project_audit_events(project, event, change, from, to) + end end - it 'creates an event when the merge requests author approval changes' do - project.update!(merge_requests_author_approval: true) + context 'when project merge_requests_author_approval is updated' do + it "logs project_merge_requests_author_approval_updated event" do + change = "prevent merge request approval from authors" + event = "project_merge_requests_author_approval_updated" + from = true + to = false + + project.update!(merge_requests_author_approval: true) - aggregate_failures do - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include( - change: 'prevent merge request approval from authors', - from: true, - to: false - ) + expect_project_audit_events_from(project, event, change, from, to) end end - it 'creates an event when the merge requests committers approval changes' do - project.update!(merge_requests_disable_committers_approval: false) + context 'when project merge_requests_disable_committers_approval is updated' do + it "logs project_merge_requests_disable_committers_approval_updated event" do + change = "prevent merge request approval from committers" + event = "project_merge_requests_disable_committers_approval_updated" + from = true + to = false + + project.update!(merge_requests_disable_committers_approval: to) - aggregate_failures do - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include( - change: 'prevent merge request approval from committers', - from: true, - to: false - ) + expect_project_audit_events_from(project, event, change, from, to) end end - it 'creates an event when the reset approvals on push changes' do - project.update!(reset_approvals_on_push: true) + context 'when project reset_approvals_on_push is updated' do + it "logs project_reset_approvals_on_push_updated event" do + change = "require new approvals when new commits are added to an MR" + event = "project_reset_approvals_on_push_updated" + from = false + to = true - aggregate_failures do - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include( - change: 'require new approvals when new commits are added to an MR', - from: false, - to: true - ) + project.update!(reset_approvals_on_push: to) + + expect_project_audit_events_from(project, event, change, from, to) end end - it 'creates an event when the require password to approve changes' do - project.update!(require_password_to_approve: true) + context 'when project require_password_to_approve is updated' do + it "logs project_require_password_to_approve_updated event" do + change = "require user password for approvals" + event = "project_require_password_to_approve_updated" + from = false + to = true + + project.update!(require_password_to_approve: to) - aggregate_failures do - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include( - change: 'require user password for approvals', - from: false, - to: true - ) + expect_project_audit_events_from(project, event, change, from, to) end end - it 'creates an event when the disable overriding approvers per merge request changes' do - project.update!(disable_overriding_approvers_per_merge_request: true) + context 'when project disable_overriding_approvers_per_merge_request is updated' do + it "logs project_disable_overriding_approvers_per_merge_request_updated event" do + change = "prevent users from modifying MR approval rules in merge requests" + event = "project_disable_overriding_approvers_per_merge_request_updated" + from = false + to = true + + project.update!(disable_overriding_approvers_per_merge_request: to) - aggregate_failures do - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include( - change: 'prevent users from modifying MR approval rules in merge requests', - from: false, - to: true - ) + expect_project_audit_events_from(project, event, change, from, to) end end @@ -230,17 +272,17 @@ end end - it 'creates an event when suggestion_commit_message change' do - previous_value = project.suggestion_commit_message - new_value = "I'm a suggested commit message" - project.update!(suggestion_commit_message: new_value) - - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details).to include({ - change: 'suggestion_commit_message', - from: previous_value, - to: new_value - }) + context 'when project suggestion_commit_message is updated' do + it "logs project_suggestion_commit_message_updated event" do + change = "suggestion_commit_message" + event = "project_suggestion_commit_message_updated" + from = project.suggestion_commit_message + to = "I'm a suggested commit message" + + project.update!(suggestion_commit_message: to) + + expect_project_audit_events(project, event, change, from, to) + end end it 'does not create an event when suggestion_commit_message change from nil to empty string' do @@ -294,6 +336,71 @@ end end end + + context 'when project resolve_outdated_diff_discussions is updated' do + it "logs project_resolve_outdated_diff_discussions_updated event" do + change = "resolve_outdated_diff_discussions" + event = "project_resolve_outdated_diff_discussions_updated" + from = project.resolve_outdated_diff_discussions + to = true + + project.update!(resolve_outdated_diff_discussions: to) + + expect_project_audit_events_from(project, event, change, from, to) + end + end + + context 'when project printing_merge_request_link_enabled is updated' do + it "logs project_printing_merge_request_link_enabled_updated event" do + change = "printing_merge_request_link_enabled" + event = "project_printing_merge_request_link_enabled_updated" + from = project.printing_merge_request_link_enabled + to = false + + project.update!(printing_merge_request_link_enabled: to) + + expect_project_audit_events_from(project, event, change, from, to) + end + end + + context 'when project remove_source_branch_after_merge is updated' do + it "logs project_remove_source_branch_after_merge_updated event" do + change = "remove_source_branch_after_merge" + event = "project_remove_source_branch_after_merge_updated" + from = project.remove_source_branch_after_merge + to = false + + project.update!(remove_source_branch_after_merge: to) + + expect_project_audit_events_from(project, event, change, from, to) + end + end + + context 'when project only_allow_merge_if_pipeline_succeeds is updated' do + it "logs project_only_allow_merge_if_pipeline_succeeds_updated event" do + change = "only_allow_merge_if_pipeline_succeeds" + event = "project_only_allow_merge_if_pipeline_succeeds_updated" + from = project.only_allow_merge_if_pipeline_succeeds + to = true + + project.update!(only_allow_merge_if_pipeline_succeeds: to) + + expect_project_audit_events_from(project, event, change, from, to) + end + end + + context 'when project only_allow_merge_if_all_discussions_are_resolved is updated' do + it "logs project_only_allow_merge_if_all_discussions_are_resolved_updated event" do + change = "only_allow_merge_if_all_discussions_are_resolved" + event = "project_only_allow_merge_if_all_discussions_are_resolved_updated" + from = project.only_allow_merge_if_all_discussions_are_resolved + to = true + + project.update!(only_allow_merge_if_all_discussions_are_resolved: to) + + expect_project_audit_events_from(project, event, change, from, to) + end + end end end end -- GitLab From 00f39ea1f4f10ad6ad43fc8f4a81337b0ecb4f51 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 13 Dec 2022 16:00:27 +1300 Subject: [PATCH 2/6] Get packages enabled spec working --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 6b611d3b475b52..0eab377d89d770 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -154,14 +154,11 @@ def expect_project_audit_events(project, event, change, change_from, change_to) context 'when project packages enabled setting changes is updated' do it "logs project_packages_enabled_updated event" do - change = "packages_enabled" - event = "project_packages_enabled_updated" - from = true - to = false + project.update!(packages_enabled: false) - project.update!(packages_enabled: to) - - expect_project_audit_events_from(project, event, change, from, to) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(2) + expect(AuditEvent.last(2).map { |e| e.details[:change] }) + .to eq %w[packages_enabled package_registry_access_level] end end -- GitLab From 0c902d459b48bca5d0535301a68cd37323a1fefe Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 15 Dec 2022 11:26:54 +1300 Subject: [PATCH 3/6] Add test for column change --- ee/lib/audit/project_changes_auditor.rb | 7 ++++- .../lib/audit/project_changes_auditor_spec.rb | 27 ++++++++++++------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/ee/lib/audit/project_changes_auditor.rb b/ee/lib/audit/project_changes_auditor.rb index 076910dca2eb00..8f3e633f77ffbc 100644 --- a/ee/lib/audit/project_changes_auditor.rb +++ b/ee/lib/audit/project_changes_auditor.rb @@ -3,7 +3,12 @@ module Audit class ProjectChangesAuditor < BaseChangesAuditor def execute - audit_changes(:visibility_level, as: 'visibility', model: model, event_type: 'project_visibility_updated') + audit_changes( + :visibility_level, + as: 'visibility_level', + model: model, + event_type: 'project_visibility_level_updated' + ) audit_changes(:path, as: 'path', model: model, event_type: 'project_path_updated') audit_changes(:name, as: 'name', model: model, event_type: 'project_name_updated') audit_changes(:namespace_id, as: 'namespace', model: model, event_type: 'project_namespace_updated') diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 0eab377d89d770..ad58694a9e5012 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -29,7 +29,8 @@ before do project.reload - stub_licensed_features(extended_audit_events: true) + stub_licensed_features(extended_audit_events: true, external_audit_events: true) + group.external_audit_event_destinations.create!(destination_url: 'http://example.com') end def expect_project_audit_events_from(project, event, change, change_from, change_to) @@ -83,10 +84,10 @@ def expect_project_audit_events(project, event, change, change_from, change_to) end describe 'audit changes' do - context 'when project visibility is updated' do - it "logs project_visibility_updated event" do - change = "visibility" - event = "project_visibility_updated" + context 'when project visibility_level is updated' do + it "logs project_visibility_level_updated event" do + change = "visibility_level" + event = "project_visibility_level_updated" from = "Private" to = "Public" @@ -250,14 +251,13 @@ def expect_project_audit_events(project, event, change, change_from, change_to) false | true end - before do - project.update_attribute(column, prev_value) - end - with_them do - it 'creates an audit event' do + before do + project.update_attribute(column, prev_value) project.update_attribute(column, new_value) + end + it 'creates an audit event' do expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include({ change: column, @@ -265,6 +265,13 @@ def expect_project_audit_events(project, event, change, change_from, change_to) to: new_value }) end + + it 'streams correct audit event', :aggregate_failures do + event_name = "project_#{column}_updated" + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async) + .with(event_name, anything, anything) + subject.execute + end end end end -- GitLab From 0c159285ea073eff504bd55008eac1cfe117717d Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 15 Dec 2022 11:40:11 +1300 Subject: [PATCH 4/6] Add feature category --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index ad58694a9e5012..2a09f3fd23c151 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Audit::ProjectChangesAuditor do +RSpec.describe Audit::ProjectChangesAuditor, feature_category: :audit_events do using RSpec::Parameterized::TableSyntax describe '.audit_changes' do let_it_be(:user) { create(:user) } -- GitLab From 618cbd6b28d1970dfd55954a623d4f9c042f980d Mon Sep 17 00:00:00 2001 From: nrosandich Date: Fri, 16 Dec 2022 16:23:06 +1300 Subject: [PATCH 5/6] Remove unneeded specs --- .../lib/audit/project_changes_auditor_spec.rb | 65 ------------------- .../services/projects/update_service_spec.rb | 4 +- 2 files changed, 2 insertions(+), 67 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 2a09f3fd23c151..b3d4b3fe69a2b5 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -340,71 +340,6 @@ def expect_project_audit_events(project, event, change, change_from, change_to) end end end - - context 'when project resolve_outdated_diff_discussions is updated' do - it "logs project_resolve_outdated_diff_discussions_updated event" do - change = "resolve_outdated_diff_discussions" - event = "project_resolve_outdated_diff_discussions_updated" - from = project.resolve_outdated_diff_discussions - to = true - - project.update!(resolve_outdated_diff_discussions: to) - - expect_project_audit_events_from(project, event, change, from, to) - end - end - - context 'when project printing_merge_request_link_enabled is updated' do - it "logs project_printing_merge_request_link_enabled_updated event" do - change = "printing_merge_request_link_enabled" - event = "project_printing_merge_request_link_enabled_updated" - from = project.printing_merge_request_link_enabled - to = false - - project.update!(printing_merge_request_link_enabled: to) - - expect_project_audit_events_from(project, event, change, from, to) - end - end - - context 'when project remove_source_branch_after_merge is updated' do - it "logs project_remove_source_branch_after_merge_updated event" do - change = "remove_source_branch_after_merge" - event = "project_remove_source_branch_after_merge_updated" - from = project.remove_source_branch_after_merge - to = false - - project.update!(remove_source_branch_after_merge: to) - - expect_project_audit_events_from(project, event, change, from, to) - end - end - - context 'when project only_allow_merge_if_pipeline_succeeds is updated' do - it "logs project_only_allow_merge_if_pipeline_succeeds_updated event" do - change = "only_allow_merge_if_pipeline_succeeds" - event = "project_only_allow_merge_if_pipeline_succeeds_updated" - from = project.only_allow_merge_if_pipeline_succeeds - to = true - - project.update!(only_allow_merge_if_pipeline_succeeds: to) - - expect_project_audit_events_from(project, event, change, from, to) - end - end - - context 'when project only_allow_merge_if_all_discussions_are_resolved is updated' do - it "logs project_only_allow_merge_if_all_discussions_are_resolved_updated event" do - change = "only_allow_merge_if_all_discussions_are_resolved" - event = "project_only_allow_merge_if_all_discussions_are_resolved_updated" - from = project.only_allow_merge_if_all_discussions_are_resolved - to = true - - project.update!(only_allow_merge_if_all_discussions_are_resolved: to) - - expect_project_audit_events_from(project, event, change, from, to) - end - end end end end diff --git a/ee/spec/services/projects/update_service_spec.rb b/ee/spec/services/projects/update_service_spec.rb index 61bf47ba63a03a..9f0f9e07bcf9a3 100644 --- a/ee/spec/services/projects/update_service_spec.rb +++ b/ee/spec/services/projects/update_service_spec.rb @@ -265,10 +265,10 @@ def operation let(:attributes) do audit_event_params.tap do |param| param[:details].merge!( - change: 'visibility', + change: 'visibility_level', from: 'Private', to: 'Internal', - custom_message: "Changed visibility from Private to Internal" + custom_message: "Changed visibility_level from Private to Internal" ) end end -- GitLab From e61a4ca62c5f144af5baf39b124b07d9b412ce7b Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 20 Dec 2022 11:11:31 +1300 Subject: [PATCH 6/6] Update to use shared_examples --- .../lib/audit/project_changes_auditor_spec.rb | 267 +++++++++--------- 1 file changed, 134 insertions(+), 133 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index b3d4b3fe69a2b5..b5f4c3929a1c6c 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -33,46 +33,50 @@ group.external_audit_event_destinations.create!(destination_url: 'http://example.com') end - def expect_project_audit_events_from(project, event, change, change_from, change_to) - expect(Gitlab::Audit::Auditor).to receive(:audit).with( - { - name: event.to_s, - author: user, - scope: project, - target: project, - message: "Changed #{change} from #{change_from} to #{change_to}", - additional_details: { - change: change.to_s, - from: change_from, - target_details: project.full_path.to_s, - to: change_to - }, - target_details: project.full_path.to_s - } - ).and_call_original - - auditor_instance.execute + shared_examples 'project_audit_events_from_to' do + it 'calls auditor' do + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: event.to_s, + author: user, + scope: project, + target: project, + message: "Changed #{change} from #{change_from} to #{change_to}", + additional_details: { + change: change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end end - def expect_project_audit_events(project, event, change, change_from, change_to) - expect(Gitlab::Audit::Auditor).to receive(:audit).with( - { - name: event.to_s, - author: user, - scope: project, - target: project, - message: "Changed #{change} to #{change_to}", - additional_details: { - change: change.to_s, - from: change_from, - target_details: project.full_path.to_s, - to: change_to - }, - target_details: project.full_path.to_s - } - ).and_call_original - - auditor_instance.execute + shared_examples 'project_audit_events_to' do + it 'calls auditor' do + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + { + name: event.to_s, + author: user, + scope: project, + target: project, + message: "Changed #{change} to #{change_to}", + additional_details: { + change: change.to_s, + from: change_from, + target_details: project.full_path.to_s, + to: change_to + }, + target_details: project.full_path.to_s + } + ).and_call_original + + auditor_instance.execute + end end describe 'non audit changes' do @@ -85,72 +89,69 @@ def expect_project_audit_events(project, event, change, change_from, change_to) describe 'audit changes' do context 'when project visibility_level is updated' do - it "logs project_visibility_level_updated event" do - change = "visibility_level" - event = "project_visibility_level_updated" - from = "Private" - to = "Public" + let(:change) { "visibility_level" } + let(:event) { "project_visibility_level_updated" } + let(:change_from) { "Private" } + let(:change_to) { "Public" } + before do project.update!(visibility_level: 20) - - expect_project_audit_events_from(project, event, change, from, to) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project name is updated' do - it "logs project_name_updated event" do - change = "name" - event = "project_name_updated" - from = project.full_name + let(:change) { "name" } + let(:event) { "project_name_updated" } + let(:change_from) { "group1 / interesting name" } + let(:change_to) { project.full_name } + before do project.update!(name: 'newname') - - to = project.full_name - - expect_project_audit_events_from(project, event, change, from, to) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project path is updated' do - it "logs project_path_updated event" do - change = "path" - event = "project_path_updated" - from = "" + let(:change) { "path" } + let(:event) { "project_path_updated" } + let(:change_from) { "" } + let(:change_to) { project.full_path } + before do project.update!(path: 'newpath') - - to = project.full_path - - expect_project_audit_events(project, event, change, from, to) end + + it_behaves_like 'project_audit_events_to' end context 'when project namespace is updated' do - it "logs project_namespace_updated event" do - change = "namespace" - event = "project_namespace_updated" - from = project.old_path_with_namespace - new_namespace = create(:namespace) + let(:change) { "namespace" } + let(:event) { "project_namespace_updated" } + let(:change_from) { project.old_path_with_namespace } + let(:change_to) { project.full_path } + before do + new_namespace = create(:namespace) project.update!(namespace: new_namespace) - - to = project.full_path - - expect_project_audit_events(project, event, change, from, to) end + + it_behaves_like 'project_audit_events_to' end context 'when project repository size limit is updated' do - it "logs project_repository_size_limit_updated event" do - change = "repository_size_limit" - event = "project_repository_size_limit_updated" - from = project.repository_size_limit - to = 100 - - project.update!(repository_size_limit: to) + let(:change) { "repository_size_limit" } + let(:event) { "project_repository_size_limit_updated" } + let(:change_from) { 10 } + let(:change_to) { 100 } - expect_project_audit_events_from(project, event, change, from, to) + before do + project.update!(repository_size_limit: 100) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project packages enabled setting changes is updated' do @@ -164,81 +165,81 @@ def expect_project_audit_events(project, event, change, change_from, change_to) end context 'when project merge_requests_template is updated' do - it "logs project_merge_requests_template_updated event" do - change = "merge_requests_template" - event = "project_merge_requests_template_updated" - from = project.merge_requests_template - to = 'I am a merge request template' + let(:change) { "merge_requests_template" } + let(:event) { "project_merge_requests_template_updated" } + let(:change_from) { nil } + let(:change_to) { 'I am a merge request template' } - project.update!(merge_requests_template: to) - - expect_project_audit_events(project, event, change, from, to) + before do + project.update!(merge_requests_template: 'I am a merge request template') end + + it_behaves_like 'project_audit_events_to' end context 'when project merge_requests_author_approval is updated' do - it "logs project_merge_requests_author_approval_updated event" do - change = "prevent merge request approval from authors" - event = "project_merge_requests_author_approval_updated" - from = true - to = false + let(:change) { "prevent merge request approval from authors" } + let(:event) { "project_merge_requests_author_approval_updated" } + let(:change_from) { true } + let(:change_to) { false } + before do project.update!(merge_requests_author_approval: true) - - expect_project_audit_events_from(project, event, change, from, to) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project merge_requests_disable_committers_approval is updated' do - it "logs project_merge_requests_disable_committers_approval_updated event" do - change = "prevent merge request approval from committers" - event = "project_merge_requests_disable_committers_approval_updated" - from = true - to = false + let(:change) { "prevent merge request approval from committers" } + let(:event) { "project_merge_requests_disable_committers_approval_updated" } + let(:change_from) { true } + let(:change_to) { false } - project.update!(merge_requests_disable_committers_approval: to) - - expect_project_audit_events_from(project, event, change, from, to) + before do + project.update!(merge_requests_disable_committers_approval: false) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project reset_approvals_on_push is updated' do - it "logs project_reset_approvals_on_push_updated event" do - change = "require new approvals when new commits are added to an MR" - event = "project_reset_approvals_on_push_updated" - from = false - to = true + let(:change) { "require new approvals when new commits are added to an MR" } + let(:event) { "project_reset_approvals_on_push_updated" } + let(:change_from) { false } + let(:change_to) { true } - project.update!(reset_approvals_on_push: to) - - expect_project_audit_events_from(project, event, change, from, to) + before do + project.update!(reset_approvals_on_push: true) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project require_password_to_approve is updated' do - it "logs project_require_password_to_approve_updated event" do - change = "require user password for approvals" - event = "project_require_password_to_approve_updated" - from = false - to = true - - project.update!(require_password_to_approve: to) + let(:change) { "require user password for approvals" } + let(:event) { "project_require_password_to_approve_updated" } + let(:change_from) { false } + let(:change_to) { true } - expect_project_audit_events_from(project, event, change, from, to) + before do + project.update!(require_password_to_approve: true) end + + it_behaves_like 'project_audit_events_from_to' end context 'when project disable_overriding_approvers_per_merge_request is updated' do - it "logs project_disable_overriding_approvers_per_merge_request_updated event" do - change = "prevent users from modifying MR approval rules in merge requests" - event = "project_disable_overriding_approvers_per_merge_request_updated" - from = false - to = true - - project.update!(disable_overriding_approvers_per_merge_request: to) + let(:change) { "prevent users from modifying MR approval rules in merge requests" } + let(:event) { "project_disable_overriding_approvers_per_merge_request_updated" } + let(:change_from) { false } + let(:change_to) { true } - expect_project_audit_events_from(project, event, change, from, to) + before do + project.update!(disable_overriding_approvers_per_merge_request: true) end + + it_behaves_like 'project_audit_events_from_to' end context 'when auditable boolean column is changed' do @@ -277,16 +278,16 @@ def expect_project_audit_events(project, event, change, change_from, change_to) end context 'when project suggestion_commit_message is updated' do - it "logs project_suggestion_commit_message_updated event" do - change = "suggestion_commit_message" - event = "project_suggestion_commit_message_updated" - from = project.suggestion_commit_message - to = "I'm a suggested commit message" - - project.update!(suggestion_commit_message: to) + let(:change) { "suggestion_commit_message" } + let(:event) { "project_suggestion_commit_message_updated" } + let(:change_from) { nil } + let(:change_to) { "I'm a suggested commit message" } - expect_project_audit_events(project, event, change, from, to) + before do + project.update!(suggestion_commit_message: "I'm a suggested commit message") end + + it_behaves_like 'project_audit_events_to' end it 'does not create an event when suggestion_commit_message change from nil to empty string' do -- GitLab