From af06ddd9201439e918f6e3ca218954d9144e2169 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Thu, 16 Mar 2023 03:24:48 -0500 Subject: [PATCH 1/5] Refactor audit events for user create service --- ee/app/services/ee/users/create_service.rb | 16 +++++++++++----- ee/config/audit_events/types/user_created.yml | 9 +++++++++ ee/spec/services/ee/users/create_service_spec.rb | 4 ++++ .../audit_event_logging_shared_examples.rb | 6 ++++++ 4 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 ee/config/audit_events/types/user_created.yml diff --git a/ee/app/services/ee/users/create_service.rb b/ee/app/services/ee/users/create_service.rb index f5d0aa3ff6de81..50c1d3f2b5954f 100644 --- a/ee/app/services/ee/users/create_service.rb +++ b/ee/app/services/ee/users/create_service.rb @@ -15,11 +15,17 @@ def after_create_hook(user, reset_token) private def log_audit_event(user) - ::AuditEventService.new( - current_user, - user, - action: :create - ).for_user.security_event + ::Gitlab::Audit::Auditor.audit({ + name: "user_created", + author: current_user, + scope: user, + target: user, + target_details: user.full_path, + message: "User #{user.username} created", + additional_details: { + add: "user" + } + }) end def audit_required? diff --git a/ee/config/audit_events/types/user_created.yml b/ee/config/audit_events/types/user_created.yml new file mode 100644 index 00000000000000..866ab13286bc0c --- /dev/null +++ b/ee/config/audit_events/types/user_created.yml @@ -0,0 +1,9 @@ +--- +name: user_created +description: Event triggered when a user is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 +feature_category: compliance_management +milestone: '15.10' +saved_to_database: true +streamed: true diff --git a/ee/spec/services/ee/users/create_service_spec.rb b/ee/spec/services/ee/users/create_service_spec.rb index 6d00ee9c1ac493..6027cac50bdd62 100644 --- a/ee/spec/services/ee/users/create_service_spec.rb +++ b/ee/spec/services/ee/users/create_service_spec.rb @@ -21,6 +21,8 @@ context 'audit events' do include_examples 'audit event logging' do + let(:audit_event_name) { 'user_created' } + let(:fail_condition!) do expect_any_instance_of(User) .to receive(:save).and_return(false) @@ -33,7 +35,9 @@ entity_type: 'User', details: { add: 'user', + author_class: 'User', author_name: current_user.name, + custom_message: "User #{@resource.username} created", target_id: @resource.id, target_type: 'User', target_details: @resource.full_path diff --git a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb index 8a66143d4cc7c9..a29d30e5335222 100644 --- a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb @@ -8,6 +8,12 @@ context 'when operation succeeds' do it 'logs an audit event' do + if defined?(audit_event_name) + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including( + name: audit_event_name + )).and_call_original + end + expect { operation }.to change(AuditEvent, :count).by(1) end -- GitLab From 2c988a6789ee04f112650d8fa1fb782b47c8e949 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Tue, 4 Apr 2023 12:39:00 -0500 Subject: [PATCH 2/5] Refactor audit events for user destroy service * add auditing for scheduling user deletion * add audit event type for user_destroyed * update user_created event type --- ee/app/services/ee/users/destroy_service.rb | 21 +++++++++++- ee/config/audit_events/types/user_created.yml | 2 +- .../audit_events/types/user_destroyed.yml | 9 +++++ .../services/ee/users/destroy_service_spec.rb | 34 +++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 ee/config/audit_events/types/user_destroyed.yml diff --git a/ee/app/services/ee/users/destroy_service.rb b/ee/app/services/ee/users/destroy_service.rb index aeda51d9a095f5..fa46e4e0f5a1e1 100644 --- a/ee/app/services/ee/users/destroy_service.rb +++ b/ee/app/services/ee/users/destroy_service.rb @@ -7,11 +7,15 @@ module DestroyService override :execute def execute(user, options = {}) - super(user, options) do |delete_user| + result = super(user, options) do |delete_user| mirror_cleanup(delete_user) oncall_rotations_cleanup(delete_user) escalation_rules_cleanup(delete_user) end + + log_audit_event(user) if audit_required? + + result end def mirror_cleanup(user) @@ -51,6 +55,21 @@ def first_mirror_owner(user, mirror) mirror_owners.first end + + def log_audit_event(user) + ::Gitlab::Audit::Auditor.audit({ + name: "user_destroyed", + author: current_user, + scope: user, + target: user, + target_details: user.full_path, + message: "User #{user.username} scheduled for deletion" + }) + end + + def audit_required? + current_user.present? + end end end end diff --git a/ee/config/audit_events/types/user_created.yml b/ee/config/audit_events/types/user_created.yml index 866ab13286bc0c..7a4b5ab23daa6c 100644 --- a/ee/config/audit_events/types/user_created.yml +++ b/ee/config/audit_events/types/user_created.yml @@ -3,7 +3,7 @@ name: user_created description: Event triggered when a user is created introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 -feature_category: compliance_management +feature_category: user_management milestone: '15.10' saved_to_database: true streamed: true diff --git a/ee/config/audit_events/types/user_destroyed.yml b/ee/config/audit_events/types/user_destroyed.yml new file mode 100644 index 00000000000000..0959f5c252e795 --- /dev/null +++ b/ee/config/audit_events/types/user_destroyed.yml @@ -0,0 +1,9 @@ +--- +name: user_destroyed +description: Event triggered when a user is scheduled for removal from the instance +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 +feature_category: user_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/services/ee/users/destroy_service_spec.rb b/ee/spec/services/ee/users/destroy_service_spec.rb index 67b88e7ea25de7..90738e211b6ee3 100644 --- a/ee/spec/services/ee/users/destroy_service_spec.rb +++ b/ee/spec/services/ee/users/destroy_service_spec.rb @@ -7,6 +7,27 @@ subject(:service) { described_class.new(current_user) } + shared_examples 'auditable' do + before do + stub_licensed_features(extended_audit_events: true) + end + + it 'creates audit event record' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: "user_destroyed" + })).and_call_original + + expect { operation }.to change { AuditEvent.count }.by(1) + + audit_event = ::AuditEvent.last + details = audit_attributes.delete(:details) || {} + audit_attributes.each do |method, value| + expect(audit_event.public_send(method)).to eq(value) + end + expect(audit_event.details).to include(details) + end + end + describe '#execute' do let!(:user) { create(:user) } @@ -21,6 +42,17 @@ context 'when admin mode is enabled', :enable_admin_mode do context 'when project is a mirror' do let(:project) { create(:project, :mirror, mirror_user_id: user.id) } + let(:audit_attributes) do + { + author: current_user, + entity: user, + target_id: user.id, + target_details: user.full_path, + details: { + custom_message: "User #{user.full_path} scheduled for deletion" + } + } + end it 'disables mirror and does not assign a new mirror_user' do expect(::Gitlab::ErrorTracking).to receive(:track_exception) @@ -34,6 +66,8 @@ expect { operation }.to change { project.reload.mirror_user }.from(user).to(nil) .and change { project.reload.mirror }.from(true).to(false) end + + it_behaves_like 'auditable' end context 'when user has oncall rotations' do -- GitLab From 9d1bd9a1339310dd8d492cdf379452c1952af5dd Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 10 Apr 2023 14:48:40 -0500 Subject: [PATCH 3/5] Refactor audit events for user approve service --- ee/app/services/ee/users/approve_service.rb | 20 +++++++++++----- .../audit_events/types/user_approved.yml | 9 ++++++++ .../services/ee/users/approve_service_spec.rb | 23 +++++++++++++------ 3 files changed, 39 insertions(+), 13 deletions(-) create mode 100644 ee/config/audit_events/types/user_approved.yml diff --git a/ee/app/services/ee/users/approve_service.rb b/ee/app/services/ee/users/approve_service.rb index 2f32f1029fe5dd..6636cbac12e814 100644 --- a/ee/app/services/ee/users/approve_service.rb +++ b/ee/app/services/ee/users/approve_service.rb @@ -15,12 +15,20 @@ def after_approve_hook(user) end def log_audit_event(user) - ::AuditEventService.new( - current_user, - user, - action: :custom, - custom_message: _('Instance access request approved') - ).for_user.security_event + # ::AuditEventService.new( + # current_user, + # user, + # action: :custom, + # custom_message: _('Instance access request approved') + # ).for_user.security_event + ::Gitlab::Audit::Auditor.audit({ + name: 'user_approved', + message: _('Instance access request approved'), + author: current_user, + scope: user, + target: user, + target_details: user.username + }) end end end diff --git a/ee/config/audit_events/types/user_approved.yml b/ee/config/audit_events/types/user_approved.yml new file mode 100644 index 00000000000000..3545435ce32179 --- /dev/null +++ b/ee/config/audit_events/types/user_approved.yml @@ -0,0 +1,9 @@ +--- +name: user_approved +description: Event triggered when a user is approved for an instance +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 +feature_category: user_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/services/ee/users/approve_service_spec.rb b/ee/spec/services/ee/users/approve_service_spec.rb index fbe5b688890441..5770e798018444 100644 --- a/ee/spec/services/ee/users/approve_service_spec.rb +++ b/ee/spec/services/ee/users/approve_service_spec.rb @@ -19,18 +19,27 @@ end context 'when user approve operation succeeds' do - it 'logs an audit event' do - expect { operation }.to change { AuditEvent.count }.by(1) - end + it 'logs an audit event', :aggregate_failures do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: 'user_approved' + })).and_call_original - it 'logs the audit event info', :aggregate_failures do - operation + expect { operation }.to change { AuditEvent.count }.by(1) audit_event = AuditEvent.where(author_id: current_user.id).last expect(audit_event.ip_address).to eq(current_user.current_sign_in_ip) - expect(audit_event.details[:target_details]).to eq(user.username) - expect(audit_event.details[:custom_message]).to eq('Instance access request approved') + expect(audit_event.author).to eq(current_user) + expect(audit_event.entity).to eq(user) + expect(audit_event.attributes).to include({ + "target_id" => user.id, + "target_details" => user.username, + "target_type" => "User" + }) + expect(audit_event.details).to include({ + target_details: user.username, + custom_message: "Instance access request approved" + }) end end -- GitLab From 5478274704e1b6c62e600aff5fd4a76ed958a9f4 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 10 Apr 2023 22:07:27 -0500 Subject: [PATCH 4/5] Refactor audit events for user reject service --- ee/app/services/ee/users/approve_service.rb | 6 ----- ee/app/services/ee/users/reject_service.rb | 14 +++++++----- .../audit_events/types/user_rejected.yml | 9 ++++++++ .../services/ee/users/reject_service_spec.rb | 22 ++++++++++++++----- 4 files changed, 33 insertions(+), 18 deletions(-) create mode 100644 ee/config/audit_events/types/user_rejected.yml diff --git a/ee/app/services/ee/users/approve_service.rb b/ee/app/services/ee/users/approve_service.rb index 6636cbac12e814..0cd45abf1ac8c7 100644 --- a/ee/app/services/ee/users/approve_service.rb +++ b/ee/app/services/ee/users/approve_service.rb @@ -15,12 +15,6 @@ def after_approve_hook(user) end def log_audit_event(user) - # ::AuditEventService.new( - # current_user, - # user, - # action: :custom, - # custom_message: _('Instance access request approved') - # ).for_user.security_event ::Gitlab::Audit::Auditor.audit({ name: 'user_approved', message: _('Instance access request approved'), diff --git a/ee/app/services/ee/users/reject_service.rb b/ee/app/services/ee/users/reject_service.rb index 25bf7bd33eaccb..c559bc911670d4 100644 --- a/ee/app/services/ee/users/reject_service.rb +++ b/ee/app/services/ee/users/reject_service.rb @@ -15,12 +15,14 @@ def after_reject_hook(user) end def log_audit_event(user) - ::AuditEventService.new( - current_user, - user, - action: :custom, - custom_message: _('Instance access request rejected') - ).for_user.security_event + ::Gitlab::Audit::Auditor.audit({ + name: 'user_approved', + message: _('Instance access request rejected'), + author: current_user, + scope: user, + target: user, + target_details: user.username + }) end end end diff --git a/ee/config/audit_events/types/user_rejected.yml b/ee/config/audit_events/types/user_rejected.yml new file mode 100644 index 00000000000000..323a331f32e196 --- /dev/null +++ b/ee/config/audit_events/types/user_rejected.yml @@ -0,0 +1,9 @@ +--- +name: user_rejected +description: Event triggered when a user registration is rejected +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 +feature_category: user_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/services/ee/users/reject_service_spec.rb b/ee/spec/services/ee/users/reject_service_spec.rb index 6ab38bcfddb82e..ef57732f3f5740 100644 --- a/ee/spec/services/ee/users/reject_service_spec.rb +++ b/ee/spec/services/ee/users/reject_service_spec.rb @@ -17,17 +17,27 @@ end context 'when user is successfully rejected' do - it 'logs an audit event' do + it 'logs an audit event', :aggregate_failures do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: 'user_approved' + })).and_call_original + expect { reject_user }.to change { AuditEvent.count }.by(1) - end - it 'logs the audit event info', :aggregate_failures do - reject_user audit_event = AuditEvent.where(author_id: current_user.id).last expect(audit_event.ip_address).to eq(current_user.current_sign_in_ip) - expect(audit_event.details[:target_details]).to eq(user.username) - expect(audit_event.details[:custom_message]).to eq('Instance access request rejected') + expect(audit_event.author).to eq(current_user) + expect(audit_event.entity).to eq(user) + expect(audit_event.attributes).to include({ + "target_id" => user.id, + "target_details" => user.username, + "target_type" => "User" + }) + expect(audit_event.details).to include({ + target_details: user.username, + custom_message: "Instance access request rejected" + }) end end -- GitLab From 04789f00544c60860d741a96eb9e26664a196582 Mon Sep 17 00:00:00 2001 From: Aaron Huntsman Date: Mon, 10 Apr 2023 22:39:01 -0500 Subject: [PATCH 5/5] Refactor audit events for user block service --- ee/app/services/ee/users/block_service.rb | 14 ++++++----- ee/config/audit_events/types/user_blocked.yml | 9 +++++++ .../services/ee/users/block_service_spec.rb | 25 +++++++++++++------ 3 files changed, 35 insertions(+), 13 deletions(-) create mode 100644 ee/config/audit_events/types/user_blocked.yml diff --git a/ee/app/services/ee/users/block_service.rb b/ee/app/services/ee/users/block_service.rb index c7abd536f5ff66..c8c77ee2adac41 100644 --- a/ee/app/services/ee/users/block_service.rb +++ b/ee/app/services/ee/users/block_service.rb @@ -15,12 +15,14 @@ def after_block_hook(user) private def log_audit_event(user) - ::AuditEventService.new( - current_user, - user, - action: :custom, - custom_message: 'Blocked user' - ).for_user.security_event + ::Gitlab::Audit::Auditor.audit({ + name: 'user_approved', + message: 'Blocked user', + author: current_user, + scope: user, + target: user, + target_details: user.username + }) end end end diff --git a/ee/config/audit_events/types/user_blocked.yml b/ee/config/audit_events/types/user_blocked.yml new file mode 100644 index 00000000000000..b35e126c4b05d6 --- /dev/null +++ b/ee/config/audit_events/types/user_blocked.yml @@ -0,0 +1,9 @@ +--- +name: user_blocked +description: Event triggered when a user is blocked +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374107 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784 +feature_category: user_management +milestone: '15.11' +saved_to_database: true +streamed: false diff --git a/ee/spec/services/ee/users/block_service_spec.rb b/ee/spec/services/ee/users/block_service_spec.rb index 4a24d7fafa2874..89736c0fb6041d 100644 --- a/ee/spec/services/ee/users/block_service_spec.rb +++ b/ee/spec/services/ee/users/block_service_spec.rb @@ -19,16 +19,27 @@ end context 'when user block operation succeeds' do - it 'logs an audit event' do + it 'logs an audit event', :aggregate_failures do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ + name: 'user_approved' + })).and_call_original + expect { operation }.to change { AuditEvent.count }.by(1) - end - it 'logs the audit event info' do - operation + audit_event = AuditEvent.where(author_id: current_user.id).last - expect(AuditEvent.last).to have_attributes( - details: hash_including(custom_message: 'Blocked user') - ) + expect(audit_event.ip_address).to eq(current_user.current_sign_in_ip) + expect(audit_event.author).to eq(current_user) + expect(audit_event.entity).to eq(user) + expect(audit_event.attributes).to include({ + "target_id" => user.id, + "target_details" => user.username, + "target_type" => "User" + }) + expect(audit_event.details).to include({ + target_details: user.username, + custom_message: "Blocked user" + }) end end -- GitLab