diff --git a/ee/app/controllers/smartcard_controller.rb b/ee/app/controllers/smartcard_controller.rb index a1ff07cef5b7261529d40a775585bfaace2d0667..071e5f69ca333747f9a1410ce357ece3e79928bd 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/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 75577adfbe479de6bd1ad40817e801fb6d69aa43..0000000000000000000000000000000000000000 --- 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 a2d7f4615715fa6c98dc0d3c3422d547e816d480..66346f62145856bf895f414c54690ba02d02631b 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/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb index 031eee37a3b221a0a8d5c45d5f6fb52c505ab60b..d978054e87f2733762433b0c75f404744646e7ad 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 6c46cc24c62b9d10f6a784ba8e21fb0d1a9cf151..6278131a962db5275b0722d52b45dca322f2a248 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 2eb5363cee96ed1142892ef8dfb2f9a6615776e4..cd298cc0dce040b9431de8d7e8eef8ce69f02650 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/config/audit_events/types/email_created.yml b/ee/config/audit_events/types/email_created.yml new file mode 100644 index 0000000000000000000000000000000000000000..9079c432e8c91a3cb7578eff996d7ee66d4da842 --- /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 0000000000000000000000000000000000000000..eec34951d1ac51f8fafa2a21ec075c0de05ba5cc --- /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/smartcard_authentication_created.yml b/ee/config/audit_events/types/smartcard_authentication_created.yml new file mode 100644 index 0000000000000000000000000000000000000000..8da628941e1c9e146badfc3efc185d62a978238e --- /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/controllers/ee/registrations_controller_spec.rb b/ee/spec/controllers/ee/registrations_controller_spec.rb index 80a12e4383a2d25b742cfcb82ef5d0a47ecd89cd..15dff803e3a001b3225112ffc0a0e63f01a02677 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 8b60acf548ba6322e25cce19a2035728363d6a2e..c9b2fbf67a29cf25f41e5d92479cdb2dc5be1240 100644 --- a/ee/spec/controllers/projects_controller_spec.rb +++ b/ee/spec/controllers/projects_controller_spec.rb @@ -17,6 +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/lib/audit/merge_request_before_destroy_auditor_spec.rb b/ee/spec/lib/audit/merge_request_before_destroy_auditor_spec.rb index b7471bf60bbabb4c539ec3e7bb74e14b62e72eed..567ede1a55f7ae1b2bac66105d1f414b8f0e3364 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 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 4bfe6be069d3e818062396a57d6d4f1922110411..9005f07d34ef67d4e0fa202f30fe665be99b4077 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/requests/ee/verifies_with_email_spec.rb b/ee/spec/requests/ee/verifies_with_email_spec.rb index f33cec0706a43739963fa3880c221cd1ce3872f8..3972d987bc007d253ae53bc8a6669341f7633417 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/requests/smartcard_controller_spec.rb b/ee/spec/requests/smartcard_controller_spec.rb index 79b7576a8b5e43a20d1ae5174501ba97a528ff1f..65b8552d40b29e31321f453e88e8da06aee46ea0 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 diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index c3eed30423ae396090b48615632ed0e4edad5a0a..ac2e0128bd4df5ebcabf692d016b2ba7078bdfe5 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/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 3784f0f2c990fe37770c8745e005e417ae3d0fcf..0000000000000000000000000000000000000000 --- 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 715fafd955a345c60f753d9ddca8a5742eb60a17..5d806a29fc6f0a2f53d04a1694aa92460525a8b2 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/ee/notes/post_process_service_spec.rb b/ee/spec/services/ee/notes/post_process_service_spec.rb index c791f69c1ff45dc5a31c99eeab8eb2b16354e0dc..48d2daef7497aafb22cc196f3f0cefd5c12d5876 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,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 @@ -44,7 +47,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 diff --git a/ee/spec/services/emails/create_service_spec.rb b/ee/spec/services/emails/create_service_spec.rb index 983837e232ccfe304a56e745738c7b736bd8a8fb..ec9297c741444b912a01525dd97cb2dc352a997b 100644 --- a/ee/spec/services/emails/create_service_spec.rb +++ b/ee/spec/services/emails/create_service_spec.rb @@ -10,10 +10,25 @@ 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) + + 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/emails/destroy_service_spec.rb b/ee/spec/services/emails/destroy_service_spec.rb index b2e8f0b462c1398ea2b20713ed2e79004045cb9e..d44840a1517ef703c9c21c2b1fb194c1cd9a4661 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/projects/restore_service_spec.rb b/ee/spec/services/projects/restore_service_spec.rb index 856f0130e7b39d07a3c7659453c5523e927ca954..668fc50ba1e3f6b49d062be63fb55aaa1558dc99 100644 --- a/ee/spec/services/projects/restore_service_spec.rb +++ b/ee/spec/services/projects/restore_service_spec.rb @@ -102,6 +102,9 @@ 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( 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 34e3291b111be11e383907d7694d6b8dbf102bc3..0f03c7ea2bc66f973bece0b8232adafdabeebd2d 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