From 4137a99f896c7b8df1c800b004327d846919f29f Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 18 Apr 2023 03:12:14 -0500 Subject: [PATCH 1/7] Refactor audit events for Email create service --- .../ee/applications/create_service.rb | 17 +++++++----- ee/app/services/ee/emails/base_service.rb | 26 +++++++++++++++++-- ee/app/services/ee/emails/create_service.rb | 2 +- ee/app/services/ee/emails/destroy_service.rb | 2 +- ee/app/services/ee/keys/create_service.rb | 16 +++++------- .../types/application_created.yml | 9 +++++++ .../audit_events/types/email_created.yml | 9 +++++++ .../audit_events/types/email_destroyed.yml | 9 +++++++ ee/config/audit_events/types/key_created.yml | 9 +++++++ .../applications/create_service_spec.rb | 14 +++++++++- .../services/emails/create_service_spec.rb | 16 +++++++++++- .../services/emails/destroy_service_spec.rb | 16 +++++++++++- ee/spec/services/keys/create_service_spec.rb | 13 +++++++--- 13 files changed, 132 insertions(+), 26 deletions(-) create mode 100644 ee/config/audit_events/types/application_created.yml create mode 100644 ee/config/audit_events/types/email_created.yml create mode 100644 ee/config/audit_events/types/email_destroyed.yml create mode 100644 ee/config/audit_events/types/key_created.yml diff --git a/ee/app/services/ee/applications/create_service.rb b/ee/app/services/ee/applications/create_service.rb index f683d58d0a2490..87cf9a95b5d49a 100644 --- a/ee/app/services/ee/applications/create_service.rb +++ b/ee/app/services/ee/applications/create_service.rb @@ -9,16 +9,19 @@ module CreateService def execute(request) super.tap do |application| entity = application.owner || current_user - audit_event_service(entity, request.remote_ip).for_user(full_path: application.name, entity_id: application.id).security_event + audit_event(entity, request.remote_ip, application) end end - def audit_event_service(entity, ip_address) - ::AuditEventService.new(current_user, - entity, - action: :custom, - custom_message: 'OAuth application added', - ip_address: ip_address) + def audit_event(entity, ip_address, application) + ::Gitlab::Audit::Auditor.audit({ + name: "application_created", + author: current_user, + scope: entity, + target: application, + message: "OAuth application added", + ip_address: ip_address + }) end end end diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb index 031eee37a3b221..d978054e87f273 100644 --- a/ee/app/services/ee/emails/base_service.rb +++ b/ee/app/services/ee/emails/base_service.rb @@ -6,8 +6,30 @@ module BaseService private def log_audit_event(options = {}) - ::AuditEventService.new(@current_user, @user, options) - .for_email(@email).security_event + audit_context = case options[:action] + when :create + { + name: "email_created", + message: "Email created", + additional_details: { add: "email" } + } + when :destroy + { + name: "email_destroyed", + message: "Email destroyed", + additional_details: { remove: "email" } + } + end + + ::Gitlab::Audit::Auditor.audit(audit_context.deep_merge({ + author: @current_user, + scope: @user, + target: options[:target], + additional_details: { + author_name: @current_user.name, + target_type: "Email" + } + })) end end end diff --git a/ee/app/services/ee/emails/create_service.rb b/ee/app/services/ee/emails/create_service.rb index 6c46cc24c62b9d..6278131a962db5 100644 --- a/ee/app/services/ee/emails/create_service.rb +++ b/ee/app/services/ee/emails/create_service.rb @@ -8,7 +8,7 @@ module CreateService override :execute def execute(extra_params = {}) super.tap do |email| - log_audit_event(action: :create) if email.persisted? + log_audit_event(action: :create, target: email) if email.persisted? end end end diff --git a/ee/app/services/ee/emails/destroy_service.rb b/ee/app/services/ee/emails/destroy_service.rb index 2eb5363cee96ed..cd298cc0dce040 100644 --- a/ee/app/services/ee/emails/destroy_service.rb +++ b/ee/app/services/ee/emails/destroy_service.rb @@ -8,7 +8,7 @@ module DestroyService override :execute def execute(email) super.tap do - log_audit_event(action: :destroy) + log_audit_event(action: :destroy, target: email) end end end diff --git a/ee/app/services/ee/keys/create_service.rb b/ee/app/services/ee/keys/create_service.rb index 6616784b473478..da07729d6318c0 100644 --- a/ee/app/services/ee/keys/create_service.rb +++ b/ee/app/services/ee/keys/create_service.rb @@ -10,15 +10,13 @@ def execute end def log_audit_event(key) - audit_event_service.for_user(full_path: key.title, entity_id: key.id).security_event - end - - def audit_event_service - ::AuditEventService.new(current_user, - user, - action: :custom, - custom_message: 'Added SSH key', - ip_address: @ip_address) # rubocop:disable Gitlab/ModuleWithInstanceVariables + ::Gitlab::Audit::Auditor.audit({ + name: "key_created", + author: current_user, + scope: user, + target: key, + message: "Added SSH key" + }) end end end diff --git a/ee/config/audit_events/types/application_created.yml b/ee/config/audit_events/types/application_created.yml new file mode 100644 index 00000000000000..b254a53e75de36 --- /dev/null +++ b/ee/config/audit_events/types/application_created.yml @@ -0,0 +1,9 @@ +--- +name: application_created +description: Event triggered when an OAuth application is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/email_created.yml b/ee/config/audit_events/types/email_created.yml new file mode 100644 index 00000000000000..9079c432e8c91a --- /dev/null +++ b/ee/config/audit_events/types/email_created.yml @@ -0,0 +1,9 @@ +--- +name: email_created +description: Event triggered when an email is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/config/audit_events/types/email_destroyed.yml b/ee/config/audit_events/types/email_destroyed.yml new file mode 100644 index 00000000000000..eec34951d1ac51 --- /dev/null +++ b/ee/config/audit_events/types/email_destroyed.yml @@ -0,0 +1,9 @@ +--- +name: email_destroyed +description: Event triggered when an email is destroyed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/config/audit_events/types/key_created.yml b/ee/config/audit_events/types/key_created.yml new file mode 100644 index 00000000000000..c0556fab206178 --- /dev/null +++ b/ee/config/audit_events/types/key_created.yml @@ -0,0 +1,9 @@ +--- +name: key_created +description: Event triggered when an SSH key is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 +feature_category: compliance_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/services/applications/create_service_spec.rb b/ee/spec/services/applications/create_service_spec.rb index e64407298b7332..8f0d44c6f0958f 100644 --- a/ee/spec/services/applications/create_service_spec.rb +++ b/ee/spec/services/applications/create_service_spec.rb @@ -28,8 +28,20 @@ end it 'creates AuditEvent with correct entity type' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "application_created" + })).and_call_original + expect { subject.execute(test_request) }.to change(AuditEvent, :count).by(1) - expect(AuditEvent.last.entity_type).to eq(entity_type) + + audit_event = AuditEvent.last + + expect(audit_event.entity_type).to eq(entity_type) + expect(audit_event.author).to eq(user) + expect(audit_event.details).to include({ + custom_message: "OAuth application added", + author_name: user.name + }) end end end diff --git a/ee/spec/services/emails/create_service_spec.rb b/ee/spec/services/emails/create_service_spec.rb index 983837e232ccfe..6850e6f839427e 100644 --- a/ee/spec/services/emails/create_service_spec.rb +++ b/ee/spec/services/emails/create_service_spec.rb @@ -10,10 +10,24 @@ subject(:service) { described_class.new(user, opts) } describe '#execute' do - it 'registers a security event' do + it 'creates an audit event' do stub_licensed_features(extended_audit_events: true) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: 'email_created' + })).and_call_original + expect { service.execute }.to change { AuditEvent.count }.by(1) + + audit_event = AuditEvent.last + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(user) + expect(audit_event.target_type).to eq("Email") + expect(audit_event.details).to include({ + add: "email", + author_name: user.name, + target_type: "Email" + }) end end end diff --git a/ee/spec/services/emails/destroy_service_spec.rb b/ee/spec/services/emails/destroy_service_spec.rb index b2e8f0b462c139..d44840a1517ef7 100644 --- a/ee/spec/services/emails/destroy_service_spec.rb +++ b/ee/spec/services/emails/destroy_service_spec.rb @@ -9,10 +9,24 @@ subject(:service) { described_class.new(user, user: user) } describe '#execute' do - it 'registers a security event' do + it 'creates audit event' do stub_licensed_features(extended_audit_events: true) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: 'email_destroyed' + })).and_call_original + expect { service.execute(email) }.to change { AuditEvent.count }.by(1) + + audit_event = AuditEvent.last + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(user) + expect(audit_event.target_type).to eq("Email") + expect(audit_event.details).to include({ + remove: "email", + author_name: user.name, + target_type: "Email" + }) end end end diff --git a/ee/spec/services/keys/create_service_spec.rb b/ee/spec/services/keys/create_service_spec.rb index 34aa4db0bdc72c..96a6fdc38aa3d5 100644 --- a/ee/spec/services/keys/create_service_spec.rb +++ b/ee/spec/services/keys/create_service_spec.rb @@ -10,14 +10,21 @@ subject { described_class.new(admin, params) } - it 'creates' do + it 'creates audit event' do stub_licensed_features(extended_audit_events: true) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "key_created" + })).and_call_original + expect { subject.execute }.to change { AuditEvent.count }.by(1) event = AuditEvent.last - expect(event.author_name).to eq(admin.name) - expect(event.entity_id).to eq(user.id) + expect(event.author).to eq(admin) + expect(event.entity).to eq(user) + expect(event.details).to include({ + custom_message: "Added SSH key" + }) end end -- GitLab From 699ad9f6d11ce43d1c8f725ce54d84a523ff7e36 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Thu, 27 Apr 2023 14:47:31 -0500 Subject: [PATCH 2/7] Refactor user impersonation audit events --- .../impersonation_audit_event_service.rb | 13 ------ ...impersonation_group_audit_event_service.rb | 10 ++++- .../ee/applications/create_service.rb | 17 ++++---- ee/app/services/ee/keys/create_service.rb | 16 +++---- .../types/application_created.yml | 9 ---- ee/config/audit_events/types/key_created.yml | 9 ---- .../applications/create_service_spec.rb | 14 +------ .../impersonation_audit_event_service_spec.rb | 42 ------------------- ...sonation_group_audit_event_service_spec.rb | 4 ++ ee/spec/services/keys/create_service_spec.rb | 13 ++---- 10 files changed, 32 insertions(+), 115 deletions(-) delete mode 100644 ee/app/services/audit_events/impersonation_audit_event_service.rb delete mode 100644 ee/config/audit_events/types/application_created.yml delete mode 100644 ee/config/audit_events/types/key_created.yml delete mode 100644 ee/spec/services/audit_events/impersonation_audit_event_service_spec.rb diff --git a/ee/app/services/audit_events/impersonation_audit_event_service.rb b/ee/app/services/audit_events/impersonation_audit_event_service.rb deleted file mode 100644 index 75577adfbe479d..00000000000000 --- a/ee/app/services/audit_events/impersonation_audit_event_service.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module AuditEvents - class ImpersonationAuditEventService < ::AuditEventService - def initialize(author, ip_address, message, created_at) - super(author, author, { - action: :custom, - custom_message: message, - ip_address: ip_address - }, :database_and_stream, created_at) - end - end -end diff --git a/ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb b/ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb index a2d7f4615715fa..66346f62145856 100644 --- a/ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb +++ b/ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb @@ -18,8 +18,14 @@ def execute end def log_instance_audit_event - AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip, "#{@action.capitalize} Impersonation", @created_at) - .for_user(full_path: @user.username, entity_id: @user.id).security_event + ::Gitlab::Audit::Auditor.audit({ + name: "user_impersonation", + author: @impersonator, + scope: @impersonator, + target: @user, + message: "#{@action.capitalize} Impersonation", + created_at: @created_at + }) end def log_groups_audit_events diff --git a/ee/app/services/ee/applications/create_service.rb b/ee/app/services/ee/applications/create_service.rb index 87cf9a95b5d49a..f683d58d0a2490 100644 --- a/ee/app/services/ee/applications/create_service.rb +++ b/ee/app/services/ee/applications/create_service.rb @@ -9,19 +9,16 @@ module CreateService def execute(request) super.tap do |application| entity = application.owner || current_user - audit_event(entity, request.remote_ip, application) + audit_event_service(entity, request.remote_ip).for_user(full_path: application.name, entity_id: application.id).security_event end end - def audit_event(entity, ip_address, application) - ::Gitlab::Audit::Auditor.audit({ - name: "application_created", - author: current_user, - scope: entity, - target: application, - message: "OAuth application added", - ip_address: ip_address - }) + def audit_event_service(entity, ip_address) + ::AuditEventService.new(current_user, + entity, + action: :custom, + custom_message: 'OAuth application added', + ip_address: ip_address) end end end diff --git a/ee/app/services/ee/keys/create_service.rb b/ee/app/services/ee/keys/create_service.rb index da07729d6318c0..6616784b473478 100644 --- a/ee/app/services/ee/keys/create_service.rb +++ b/ee/app/services/ee/keys/create_service.rb @@ -10,13 +10,15 @@ def execute end def log_audit_event(key) - ::Gitlab::Audit::Auditor.audit({ - name: "key_created", - author: current_user, - scope: user, - target: key, - message: "Added SSH key" - }) + audit_event_service.for_user(full_path: key.title, entity_id: key.id).security_event + end + + def audit_event_service + ::AuditEventService.new(current_user, + user, + action: :custom, + custom_message: 'Added SSH key', + ip_address: @ip_address) # rubocop:disable Gitlab/ModuleWithInstanceVariables end end end diff --git a/ee/config/audit_events/types/application_created.yml b/ee/config/audit_events/types/application_created.yml deleted file mode 100644 index b254a53e75de36..00000000000000 --- a/ee/config/audit_events/types/application_created.yml +++ /dev/null @@ -1,9 +0,0 @@ ---- -name: application_created -description: Event triggered when an OAuth application is created -introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 -feature_category: compliance_management -milestone: '15.11' -saved_to_database: true -streamed: true diff --git a/ee/config/audit_events/types/key_created.yml b/ee/config/audit_events/types/key_created.yml deleted file mode 100644 index c0556fab206178..00000000000000 --- a/ee/config/audit_events/types/key_created.yml +++ /dev/null @@ -1,9 +0,0 @@ ---- -name: key_created -description: Event triggered when an SSH key is created -introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114546 -feature_category: compliance_management -milestone: '15.11' -saved_to_database: true -streamed: false diff --git a/ee/spec/services/applications/create_service_spec.rb b/ee/spec/services/applications/create_service_spec.rb index 8f0d44c6f0958f..e64407298b7332 100644 --- a/ee/spec/services/applications/create_service_spec.rb +++ b/ee/spec/services/applications/create_service_spec.rb @@ -28,20 +28,8 @@ end it 'creates AuditEvent with correct entity type' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "application_created" - })).and_call_original - expect { subject.execute(test_request) }.to change(AuditEvent, :count).by(1) - - audit_event = AuditEvent.last - - expect(audit_event.entity_type).to eq(entity_type) - expect(audit_event.author).to eq(user) - expect(audit_event.details).to include({ - custom_message: "OAuth application added", - author_name: user.name - }) + expect(AuditEvent.last.entity_type).to eq(entity_type) end end end diff --git a/ee/spec/services/audit_events/impersonation_audit_event_service_spec.rb b/ee/spec/services/audit_events/impersonation_audit_event_service_spec.rb deleted file mode 100644 index 3784f0f2c990fe..00000000000000 --- a/ee/spec/services/audit_events/impersonation_audit_event_service_spec.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AuditEvents::ImpersonationAuditEventService do - let(:impersonator) { create(:user) } - let(:ip_address) { '127.0.0.1' } - let(:message) { 'Impersonation Started' } - let(:logger) { instance_double(Gitlab::AuditJsonLogger) } - let(:service) { described_class.new(impersonator, ip_address, message, 3.weeks.ago) } - - describe '#security_event' do - before do - stub_licensed_features(extended_audit_events: true) - end - - it 'creates an event and logs to a file with the provided details' do - expect(service).to receive(:file_logger).and_return(logger) - expect(logger).to receive(:info).with({ author_id: impersonator.id, - author_name: impersonator.name, - entity_id: impersonator.id, - entity_type: "User", - action: :custom, - ip_address: ip_address, - custom_message: message, - created_at: anything }) - - expect { service.security_event }.to change(AuditEvent, :count).by(1) - security_event = AuditEvent.last - - expect(security_event.details).to eq({ - author_name: impersonator.name, - custom_message: message, - ip_address: ip_address, - action: :custom - }) - expect(security_event.author_id).to eq(impersonator.id) - expect(security_event.entity_id).to eq(impersonator.id) - expect(security_event.entity_type).to eq('User') - end - end -end diff --git a/ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb b/ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb index 715fafd955a345..5d806a29fc6f0a 100644 --- a/ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb +++ b/ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb @@ -22,6 +22,10 @@ it 'creates audit events for both the instance and group level' do freeze_time do + expect(::Gitlab::Audit::Auditor).to receive(:audit).twice.with(hash_including({ + name: "user_impersonation" + })).and_call_original + expect { service.execute }.to change { AuditEvent.count }.by(2) event = AuditEvent.first diff --git a/ee/spec/services/keys/create_service_spec.rb b/ee/spec/services/keys/create_service_spec.rb index 96a6fdc38aa3d5..34aa4db0bdc72c 100644 --- a/ee/spec/services/keys/create_service_spec.rb +++ b/ee/spec/services/keys/create_service_spec.rb @@ -10,21 +10,14 @@ subject { described_class.new(admin, params) } - it 'creates audit event' do + it 'creates' do stub_licensed_features(extended_audit_events: true) - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "key_created" - })).and_call_original - expect { subject.execute }.to change { AuditEvent.count }.by(1) event = AuditEvent.last - expect(event.author).to eq(admin) - expect(event.entity).to eq(user) - expect(event.details).to include({ - custom_message: "Added SSH key" - }) + expect(event.author_name).to eq(admin.name) + expect(event.entity_id).to eq(user.id) end end -- GitLab From a65aa78df5ad737407bca813429b204a508546e7 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 15 May 2023 01:29:38 -0500 Subject: [PATCH 3/7] Refactor audit events for SmartcardController --- ee/app/controllers/smartcard_controller.rb | 9 +++++++- .../smartcard_authentication_created.yml | 9 ++++++++ ee/spec/requests/smartcard_controller_spec.rb | 23 ++++++++++--------- 3 files changed, 29 insertions(+), 12 deletions(-) create mode 100644 ee/config/audit_events/types/smartcard_authentication_created.yml diff --git a/ee/app/controllers/smartcard_controller.rb b/ee/app/controllers/smartcard_controller.rb index a1ff07cef5b726..071e5f69ca3337 100644 --- a/ee/app/controllers/smartcard_controller.rb +++ b/ee/app/controllers/smartcard_controller.rb @@ -114,7 +114,14 @@ def store_active_session end def log_audit_event(user, options = {}) - AuditEventService.new(user, user, options).for_authentication.security_event + ::Gitlab::Audit::Auditor.audit({ + name: "smartcard_authentication_created", + author: user, + scope: user, + target: user, + message: "User authenticated with smartcard", + additional_details: options + }) end def after_sign_in_path_for(resource) diff --git a/ee/config/audit_events/types/smartcard_authentication_created.yml b/ee/config/audit_events/types/smartcard_authentication_created.yml new file mode 100644 index 00000000000000..8da628941e1c9e --- /dev/null +++ b/ee/config/audit_events/types/smartcard_authentication_created.yml @@ -0,0 +1,9 @@ +--- +name: smartcard_authentication_created +description: Event triggered when a user authenticates with smartcard +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/726 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/8120 +feature_category: compliance_management +milestone: '16.0' +saved_to_database: true +streamed: false diff --git a/ee/spec/requests/smartcard_controller_spec.rb b/ee/spec/requests/smartcard_controller_spec.rb index 79b7576a8b5e43..65b8552d40b29e 100644 --- a/ee/spec/requests/smartcard_controller_spec.rb +++ b/ee/spec/requests/smartcard_controller_spec.rb @@ -164,18 +164,19 @@ end it 'logs audit event' do - audit_event_service = instance_double(::AuditEventService) - # Creating a confirmed user also creates an email corresponding to the user primary email - expect(::AuditEventService).to( - receive(:new) - .with(instance_of(User), instance_of(User), { action: :create }).and_call_original) - - expect(::AuditEventService).to( - receive(:new) - .with(instance_of(User), instance_of(User), { with: auth_method, ip_address: '127.0.0.1' }) - .and_return(audit_event_service)) - expect(audit_event_service).to receive_message_chain(:for_authentication, :security_event) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "email_created" + })).and_call_original + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "smartcard_authentication_created", + author: instance_of(User), + scope: instance_of(User), + target: instance_of(User), + message: "User authenticated with smartcard", + additional_details: { with: auth_method, ip_address: '127.0.0.1' } + })).and_call_original subject end -- GitLab From b3d63602c27896ea979d467e5f1fc13d6d3e01e0 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 30 May 2023 08:30:14 -0500 Subject: [PATCH 4/7] Update tests to ignore email audit events --- .../controllers/projects_controller_spec.rb | 1 + .../merge_request_destroy_auditor_spec.rb | 35 +++++++++++++------ .../approval_rules/create_service_spec.rb | 4 ++- .../services/projects/restore_service_spec.rb | 2 ++ .../create_service_spec.rb | 4 ++- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index 8b60acf548ba63..ff3a25f69ef6a1 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -17,6 +17,7 @@ shared_examples 'audit events with event type' do it 'logs the audit event' do + allow(::Gitlab::Audit::Auditor).to receive(:audit) expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(name: audit_name) ).and_call_original diff --git a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb index 4bfe6be069d3e8..9005f07d34ef67 100644 --- a/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb +++ b/ee/spec/lib/audit/merge_request_destroy_auditor_spec.rb @@ -26,13 +26,13 @@ context 'when merge request is not merged' do it 'audits a delete_merge_request event' do - subject.execute + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + name: 'delete_merge_request', + message: "Removed MergeRequest(#{merge_request.title} with IID: " \ + "#{merge_request.iid} and ID: #{merge_request.id})" + )) - expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| - expect(args[:name]).to eq('delete_merge_request') - expect(args[:message]).to eq("Removed MergeRequest(#{merge_request.title} with IID: " \ - "#{merge_request.iid} and ID: #{merge_request.id})") - end + subject.execute end end @@ -42,12 +42,27 @@ end it 'audits a merged_merge_request_deleted event' do + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + name: 'merged_merge_request_deleted', + message: "Removed MergeRequest(#{merge_request.title} with IID: " \ + "#{merge_request.iid} and ID: #{merge_request.id})" + )) + subject.execute + end + + context 'when metrics is not present' do + before do + allow(merge_request).to receive(:metrics).and_return(nil) + end + + it 'does not include merged_by_user_id in the message' do + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + message: "Removed MergeRequest(#{merge_request.title} with IID: " \ + "#{merge_request.iid} and ID: #{merge_request.id})" + )) - expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| - expect(args[:name]).to eq('merged_merge_request_deleted') - expect(args[:message]).to eq("Removed MergeRequest(#{merge_request.title} with IID: " \ - "#{merge_request.iid} and ID: #{merge_request.id})") + subject.execute end end end diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index c3eed30423ae39..ac2e0128bd4df5 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -160,7 +160,9 @@ end it 'creates approval without audit' do - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with( + hash_including(name: 'approval_rule_created') + ) expect(result[:status]).to eq(:success) end diff --git a/ee/spec/services/projects/restore_service_spec.rb b/ee/spec/services/projects/restore_service_spec.rb index 856f0130e7b39d..323892cddc0609 100644 --- a/ee/spec/services/projects/restore_service_spec.rb +++ b/ee/spec/services/projects/restore_service_spec.rb @@ -102,6 +102,8 @@ context 'audit events' do it 'saves audit event' do + allow(::Gitlab::Audit::Auditor).to receive(:audit) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(name: 'project_path_updated') ).and_call_original diff --git a/ee/spec/services/protected_environments/create_service_spec.rb b/ee/spec/services/protected_environments/create_service_spec.rb index 34e3291b111be1..0f03c7ea2bc66f 100644 --- a/ee/spec/services/protected_environments/create_service_spec.rb +++ b/ee/spec/services/protected_environments/create_service_spec.rb @@ -49,7 +49,9 @@ end it 'does not store or log the audit event' do - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including( + name: 'environment_protected' + )) subject end -- GitLab From 4a69e9183a61c83a3b50d5b19f28941defa48c36 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 30 May 2023 10:14:56 -0500 Subject: [PATCH 5/7] Update audit event tests for Notes::PostProcessService --- ee/spec/services/ee/notes/post_process_service_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 c791f69c1ff45d..7c130a974a0618 100644 --- a/ee/spec/services/ee/notes/post_process_service_spec.rb +++ b/ee/spec/services/ee/notes/post_process_service_spec.rb @@ -28,6 +28,7 @@ let(:note) { create(:note, author: project_bot) } it 'audits with correct name' do + allow(::Gitlab::Audit::Auditor).to receive(:audit) expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(name: "comment_by_project_bot", stream_only: true) ).and_call_original @@ -44,7 +45,9 @@ let(:note) { create(:note) } it 'does not invoke Gitlab::Audit::Auditor' do - expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including( + name: 'comment_by_project_bot' + )) notes_post_process_service.execute end -- GitLab From 2b3ed672a751133eb403b0614ece09826a21116a Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 5 Jun 2023 09:46:20 -0500 Subject: [PATCH 6/7] Refactor MergeRequestBeforeDestroyAuditor spec for email audits --- ...rge_request_before_destroy_auditor_spec.rb | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/ee/spec/lib/audit/merge_request_before_destroy_auditor_spec.rb b/ee/spec/lib/audit/merge_request_before_destroy_auditor_spec.rb index b7471bf60bbabb..567ede1a55f7ae 100644 --- a/ee/spec/lib/audit/merge_request_before_destroy_auditor_spec.rb +++ b/ee/spec/lib/audit/merge_request_before_destroy_auditor_spec.rb @@ -33,7 +33,9 @@ it 'does not audit the event' do subject.execute - expect(Gitlab::Audit::Auditor).not_to have_received(:audit) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit).with(hash_including( + name: 'merged_merge_request_deletion_started' + )) end end @@ -58,16 +60,14 @@ end it 'includes additional details in the message' do - subject.execute - - expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| - expected_message = "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ - "and ID: #{merge_request.id}), labels: label1 and label2, " \ - "approved_by_user_ids: 1 and 2, " \ - "committer_user_ids: 4 and 5, merged_by_user_id: 3" + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + message: "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ + "and ID: #{merge_request.id}), labels: label1 and label2, " \ + "approved_by_user_ids: 1 and 2, " \ + "committer_user_ids: 4 and 5, merged_by_user_id: 3" + )) - expect(args[:message]).to eq(expected_message) - end + subject.execute end context 'when metrics is not present' do @@ -76,15 +76,14 @@ end it 'does not include merged_by_user_id in the message' do - subject.execute + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + message: "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ + "and ID: #{merge_request.id}), labels: label1 and label2, " \ + "approved_by_user_ids: 1 and 2, " \ + "committer_user_ids: 4 and 5" + )) - expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| - expected_message = "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ - "and ID: #{merge_request.id}), labels: label1 and label2, " \ - "approved_by_user_ids: 1 and 2, " \ - "committer_user_ids: 4 and 5" - expect(args[:message]).to eq(expected_message) - end + subject.execute end end end @@ -96,14 +95,12 @@ end it 'omits labels and approvers from the message' do - subject.execute - - expect(Gitlab::Audit::Auditor).to have_received(:audit) do |args| - expected_message = "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ - "and ID: #{merge_request.id}), committer_user_ids: 4 and 5, merged_by_user_id: 3" \ + expect(Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + message: "Removed MergeRequest(#{merge_request.title} with IID: #{merge_request.iid} " \ + "and ID: #{merge_request.id}), committer_user_ids: 4 and 5, merged_by_user_id: 3" + )) - expect(args[:message]).to eq(expected_message) - end + subject.execute end end end -- GitLab From cf8293865c83ddd4ead0d002d8a4a5d9d0ca1444 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 27 Jun 2023 11:15:57 -0500 Subject: [PATCH 7/7] Add comment to audit spec stubs --- .../ee/registrations_controller_spec.rb | 3 +++ .../controllers/projects_controller_spec.rb | 2 ++ .../requests/ee/verifies_with_email_spec.rb | 3 +++ .../ee/notes/post_process_service_spec.rb | 2 ++ .../services/emails/create_service_spec.rb | 19 ++++++++++--------- .../services/projects/restore_service_spec.rb | 1 + 6 files changed, 21 insertions(+), 9 deletions(-) diff --git a/ee/spec/controllers/ee/registrations_controller_spec.rb b/ee/spec/controllers/ee/registrations_controller_spec.rb index 80a12e4383a2d2..15dff803e3a001 100644 --- a/ee/spec/controllers/ee/registrations_controller_spec.rb +++ b/ee/spec/controllers/ee/registrations_controller_spec.rb @@ -132,6 +132,9 @@ end it 'logs the audit event info', :aggregate_failures do + # Stub .audit here so that only relevant audit events are received below + allow(::Gitlab::Audit::Auditor).to receive(:audit) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ name: "registration_created" })).and_call_original diff --git a/ee/spec/controllers/projects_controller_spec.rb b/ee/spec/controllers/projects_controller_spec.rb index ff3a25f69ef6a1..c9b2fbf67a29cf 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -17,7 +17,9 @@ shared_examples 'audit events with event type' do it 'logs the audit event' do + # Stub .audit here so that only relevant audit events are received below allow(::Gitlab::Audit::Auditor).to receive(:audit) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(name: audit_name) ).and_call_original diff --git a/ee/spec/requests/ee/verifies_with_email_spec.rb b/ee/spec/requests/ee/verifies_with_email_spec.rb index f33cec0706a437..3972d987bc007d 100644 --- a/ee/spec/requests/ee/verifies_with_email_spec.rb +++ b/ee/spec/requests/ee/verifies_with_email_spec.rb @@ -22,6 +22,9 @@ end it 'logs a user_access_locked audit event with correct message', feature_category: :audit_events do + # Stub .audit here so that only relevant audit events are received below + allow(::Gitlab::Audit::Auditor).to receive(:audit) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including( name: 'user_access_locked', 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 7c130a974a0618..48d2daef7497aa 100644 --- a/ee/spec/services/ee/notes/post_process_service_spec.rb +++ b/ee/spec/services/ee/notes/post_process_service_spec.rb @@ -28,7 +28,9 @@ let(:note) { create(:note, author: project_bot) } it 'audits with correct name' do + # Stub .audit here so that only relevant audit events are received below allow(::Gitlab::Audit::Auditor).to receive(:audit) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(name: "comment_by_project_bot", stream_only: true) ).and_call_original diff --git a/ee/spec/services/emails/create_service_spec.rb b/ee/spec/services/emails/create_service_spec.rb index 6850e6f839427e..ec9297c741444b 100644 --- a/ee/spec/services/emails/create_service_spec.rb +++ b/ee/spec/services/emails/create_service_spec.rb @@ -19,15 +19,16 @@ expect { service.execute }.to change { AuditEvent.count }.by(1) - audit_event = AuditEvent.last - expect(audit_event.author).to eq(user) - expect(audit_event.entity).to eq(user) - expect(audit_event.target_type).to eq("Email") - expect(audit_event.details).to include({ - add: "email", - author_name: user.name, - target_type: "Email" - }) + expect(AuditEvent.last).to have_attributes( + author: user, + entity: user, + target_type: "Email", + details: hash_including({ + add: "email", + author_name: user.name, + target_type: "Email" + }) + ) end end end diff --git a/ee/spec/services/projects/restore_service_spec.rb b/ee/spec/services/projects/restore_service_spec.rb index 323892cddc0609..668fc50ba1e3f6 100644 --- a/ee/spec/services/projects/restore_service_spec.rb +++ b/ee/spec/services/projects/restore_service_spec.rb @@ -102,6 +102,7 @@ context 'audit events' do it 'saves audit event' do + # Stub .audit here so that only relevant audit events are received below allow(::Gitlab::Audit::Auditor).to receive(:audit) expect(::Gitlab::Audit::Auditor).to receive(:audit).with( -- GitLab