From 91fa547664d74edb07edbbe0c91ede26ca98038f Mon Sep 17 00:00:00 2001 From: nrosandich Date: Wed, 12 Oct 2022 12:06:07 +1300 Subject: [PATCH 01/14] Event type for project path --- ee/lib/audit/project_changes_auditor.rb | 2 +- ee/spec/lib/audit/project_changes_auditor_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/lib/audit/project_changes_auditor.rb b/ee/lib/audit/project_changes_auditor.rb index 67eb0aa81f0662..68aa6c75242b8c 100644 --- a/ee/lib/audit/project_changes_auditor.rb +++ b/ee/lib/audit/project_changes_auditor.rb @@ -4,7 +4,7 @@ module Audit class ProjectChangesAuditor < BaseChangesAuditor def execute audit_changes(:visibility_level, as: 'visibility', model: model) - audit_changes(:path, as: 'path', model: model) + audit_changes(:path, as: 'path', model: model, event_type: 'project_path_updated') audit_changes(:name, as: 'name', model: model) audit_changes(:namespace_id, as: 'namespace', model: model) audit_changes(:repository_size_limit, as: 'repository_size_limit', model: model) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index dce4e865863661..2ccb93dba18dd0 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -58,6 +58,8 @@ expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'path' + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( + 'project_path_updated', anything, anything) end it 'creates an event when the namespace change' do -- GitLab From 7b91c47c6838435dbd82e056ae8db816f44f8a0b Mon Sep 17 00:00:00 2001 From: nrosandich Date: Wed, 12 Oct 2022 12:26:56 +1300 Subject: [PATCH 02/14] Update alignment error --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 2ccb93dba18dd0..b724e2e746cd11 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -58,8 +58,7 @@ expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'path' - expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( - 'project_path_updated', anything, anything) + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with('project_path_updated', anything, anything) end it 'creates an event when the namespace change' do -- GitLab From e909b807a189069f9b7600124da2db2682f2873b Mon Sep 17 00:00:00 2001 From: nrosandich Date: Wed, 12 Oct 2022 12:29:09 +1300 Subject: [PATCH 03/14] Fix long line --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 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 b724e2e746cd11..8505bd68b1cb85 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -58,7 +58,8 @@ expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'path' - expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with('project_path_updated', anything, anything) + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( + 'project_path_updated', anything, anything) end it 'creates an event when the namespace change' do -- GitLab From a952537044f9cdd4e649bf16e4730b46c60ac4d6 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 13 Oct 2022 16:11:02 +1300 Subject: [PATCH 04/14] Add group and destination factory to spec --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 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 8505bd68b1cb85..3dd1ec384ed390 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -7,9 +7,13 @@ describe '.audit_changes' do let_it_be(:user) { create(:user) } + let(:group) { create(:group) } + let(:destination) { create(:external_audit_event_destination, group: group) } + let(:project) do create( :project, + group: group, visibility_level: 0, name: 'interesting name', path: 'interesting-path', @@ -27,7 +31,7 @@ before do project.reload - stub_licensed_features(extended_audit_events: true) + stub_licensed_features(extended_audit_events: true, external_audit_events: true) end describe 'non audit changes' do -- GitLab From 4725bece9601c121800f2a5eb53c74ae6ab9fb44 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 13 Oct 2022 16:11:51 +1300 Subject: [PATCH 05/14] Remove trailing whitespace --- 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 3dd1ec384ed390..3dbf4f1fb332b7 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -7,7 +7,7 @@ describe '.audit_changes' do let_it_be(:user) { create(:user) } - let(:group) { create(:group) } + let(:group) { create(:group) } let(:destination) { create(:external_audit_event_destination, group: group) } let(:project) do -- GitLab From 4d7da3b1872360e8b25a459753db9688716c8961 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Thu, 13 Oct 2022 16:32:53 +1300 Subject: [PATCH 06/14] Execute instance --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 3dbf4f1fb332b7..8bd8fbf4f244dd 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -6,9 +6,8 @@ using RSpec::Parameterized::TableSyntax describe '.audit_changes' do let_it_be(:user) { create(:user) } - - let(:group) { create(:group) } - let(:destination) { create(:external_audit_event_destination, group: group) } + let_it_be(:group) { create(:group) } + let_it_be(:destination) { create(:external_audit_event_destination, group: group) } let(:project) do create( @@ -64,6 +63,8 @@ expect(AuditEvent.last.details[:change]).to eq 'path' expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( 'project_path_updated', anything, anything) + + foo_instance.execute end it 'creates an event when the namespace change' do -- GitLab From cbfd962257bec3d23ab648e7c6b98e86238d4b1d Mon Sep 17 00:00:00 2001 From: nrosandich Date: Fri, 14 Oct 2022 09:40:44 +1300 Subject: [PATCH 07/14] Add more descriptive instance name --- .../lib/audit/project_changes_auditor_spec.rb | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 8bd8fbf4f244dd..02a1c9d108f96e 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -26,7 +26,7 @@ ) end - subject(:foo_instance) { described_class.new(user, project) } + subject(:auditor_instance) { described_class.new(user, project) } before do project.reload @@ -37,7 +37,7 @@ it 'does not call the audit event service' do project.update!(description: 'new description') - expect { foo_instance.execute }.not_to change(AuditEvent, :count) + expect { auditor_instance.execute }.not_to change(AuditEvent, :count) end end @@ -45,26 +45,26 @@ it 'creates an event when the visibility change' do project.update!(visibility_level: 20) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'visibility' end it 'creates an event when the name change' do project.update!(name: 'new name') - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'name' end it 'creates an event when the path change' do project.update!(path: 'newpath') - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'path' expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( 'project_path_updated', anything, anything) - foo_instance.execute + auditor_instance.execute end it 'creates an event when the namespace change' do @@ -72,21 +72,21 @@ project.update!(namespace: new_namespace) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'namespace' end it 'creates an event when the repository size limit changes' do project.update!(repository_size_limit: 100) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details[:change]).to eq 'repository_size_limit' end it 'creates an event when the packages enabled setting changes' do project.update!(packages_enabled: false) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(2) + 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 @@ -94,7 +94,7 @@ it 'creates an event when the merge requests template changes' do project.update!(merge_requests_template: 'I am a merge request template') - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + 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', @@ -107,7 +107,7 @@ project.update!(merge_requests_author_approval: true) aggregate_failures do - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + 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, @@ -120,7 +120,7 @@ project.update!(merge_requests_disable_committers_approval: false) aggregate_failures do - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + 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, @@ -133,7 +133,7 @@ project.update!(reset_approvals_on_push: true) aggregate_failures do - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + 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, @@ -146,7 +146,7 @@ project.update!(require_password_to_approve: true) aggregate_failures do - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include( change: 'require user password for approvals', from: false, @@ -159,7 +159,7 @@ project.update!(disable_overriding_approvers_per_merge_request: true) aggregate_failures do - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + 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, @@ -186,7 +186,7 @@ it 'creates an audit event' do project.update_attribute(column, new_value) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include({ change: column, from: prev_value, @@ -202,7 +202,7 @@ new_value = "I'm a suggested commit message" project.update!(suggestion_commit_message: new_value) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include({ change: 'suggestion_commit_message', from: previous_value, @@ -213,7 +213,7 @@ it 'does not create an event when suggestion_commit_message change from nil to empty string' do project.update!(suggestion_commit_message: "") - expect { foo_instance.execute }.not_to change(AuditEvent, :count) + expect { auditor_instance.execute }.not_to change(AuditEvent, :count) end context 'when merge method is changed from Merge' do @@ -231,7 +231,7 @@ it 'creates an audit event' do project.update!(merge_requests_ff_only_enabled: ff, merge_requests_rebase_enabled: rebase) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include({ custom_message: "Changed merge method to #{method}" }) @@ -254,7 +254,7 @@ it 'creates an Merge method audit event' do project.update!(merge_requests_ff_only_enabled: false, merge_requests_rebase_enabled: false) - expect { foo_instance.execute }.to change(AuditEvent, :count).by(1) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) expect(AuditEvent.last.details).to include({ custom_message: "Changed merge method to Merge" }) -- GitLab From 9732f7e229d394a0081c58c4e0267435eba1e119 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Mon, 17 Oct 2022 09:22:32 +1300 Subject: [PATCH 08/14] Separate two path test --- .../lib/audit/project_changes_auditor_spec.rb | 21 ++++++++++++------- 1 file changed, 14 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 02a1c9d108f96e..b120dae62a705f 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -56,15 +56,22 @@ expect(AuditEvent.last.details[:change]).to eq 'name' end - it 'creates an event when the path change' do - project.update!(path: 'newpath') + context 'when project path is updated' do + it 'creates an audit event when the path change' do + project.update!(path: 'newpath') - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq 'path' - expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( - 'project_path_updated', anything, anything) + expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) + expect(AuditEvent.last.details[:change]).to eq('path') + end - auditor_instance.execute + it 'streams correct audit event type' do + project.update!(path: 'newpath') + + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( + 'project_path_updated', anything, anything) + + auditor_instance.execute + end end it 'creates an event when the namespace change' do -- GitLab From ae52b957d5dde1d8f0a4fc6578a35a3e203f55bc Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 18 Oct 2022 17:05:47 +1300 Subject: [PATCH 09/14] Update test to check audit event created --- .../lib/audit/project_changes_auditor_spec.rb | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index b120dae62a705f..c39739d9fe0da4 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -57,23 +57,23 @@ end context 'when project path is updated' do - it 'creates an audit event when the path change' do + it "logs project_path_updated event" do project.update!(path: 'newpath') - expect { auditor_instance.execute }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.details[:change]).to eq('path') - end - - it 'streams correct audit event type' do - project.update!(path: 'newpath') - - expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( - 'project_path_updated', anything, anything) + 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=>"#{project.full_path}"}, + target_details: "#{project.full_path}" + ).and_call_original auditor_instance.execute end end - + it 'creates an event when the namespace change' do new_namespace = create(:namespace) -- GitLab From 78d9a48b3b4d3fd7133c7f7d15e651f64afa5f6d Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 18 Oct 2022 17:08:36 +1300 Subject: [PATCH 10/14] Update hash to pass --- 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 c39739d9fe0da4..9ec65ed0b6fbbf 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -66,7 +66,7 @@ scope: project, target: project, message: "Changed path to #{project.full_path}", - additional_details: {:change=>"path", :from=>"",:target_details=>"#{project.full_path}",:to=>"#{project.full_path}"}, + additional_details: { change: "path", from: "", target_details: "#{project.full_path}", to: "#{project.full_path}" }, target_details: "#{project.full_path}" ).and_call_original -- GitLab From 5ccbf9f675d029b5a1840196895bb2a0f1b3add0 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 18 Oct 2022 17:14:25 +1300 Subject: [PATCH 11/14] Update to string over interpolation --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 9ec65ed0b6fbbf..b48d80645fdf6e 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -66,8 +66,8 @@ scope: project, target: project, message: "Changed path to #{project.full_path}", - additional_details: { change: "path", from: "", target_details: "#{project.full_path}", to: "#{project.full_path}" }, - target_details: "#{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 -- GitLab From 7f5a5bb50b816ec4e31331904459c2b05df41fc7 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 18 Oct 2022 17:15:58 +1300 Subject: [PATCH 12/14] fix rubocop offenses --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index b48d80645fdf6e..e0c955016b993c 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -66,14 +66,19 @@ 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 }, + 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 end end - + it 'creates an event when the namespace change' do new_namespace = create(:namespace) -- GitLab From 496a468ca60a8f2a406b60575687f9391cecf52f Mon Sep 17 00:00:00 2001 From: nrosandich Date: Tue, 18 Oct 2022 17:16:50 +1300 Subject: [PATCH 13/14] Trailing whitespace --- 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 e0c955016b993c..74a504e7118dd0 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -66,7 +66,7 @@ scope: project, target: project, message: "Changed path to #{project.full_path}", - additional_details: { + additional_details: { change: "path", from: "", target_details: project.full_path.to_s, -- GitLab From 1856823d52e838940bcf40032bb920c1011753c2 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Wed, 19 Oct 2022 09:41:48 +1300 Subject: [PATCH 14/14] Remove unneeded code from tests --- ee/spec/lib/audit/project_changes_auditor_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/spec/lib/audit/project_changes_auditor_spec.rb b/ee/spec/lib/audit/project_changes_auditor_spec.rb index 74a504e7118dd0..8a9763cb04d1d7 100644 --- a/ee/spec/lib/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/project_changes_auditor_spec.rb @@ -7,7 +7,6 @@ describe '.audit_changes' do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:destination) { create(:external_audit_event_destination, group: group) } let(:project) do create( @@ -30,7 +29,7 @@ before do project.reload - stub_licensed_features(extended_audit_events: true, external_audit_events: true) + stub_licensed_features(extended_audit_events: true) end describe 'non audit changes' do -- GitLab