From 7cd9a04445dbceba06b473dbd66d9e40a59a571d Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Mon, 24 Jan 2022 16:57:01 +0000 Subject: [PATCH 1/9] Save audit events for start/stop user impersonation to group level Adds additional audit events to all of the groups that a user belongs to to inform group owners that their users have been impersonated by instance administrators. EE: true Changelog: added --- doc/administration/audit_events.md | 2 +- .../controllers/ee/admin/users_controller.rb | 6 ++- .../controllers/ee/application_controller.rb | 6 ++- ...impersonation_group_audit_event_service.rb | 38 +++++++++++++ .../user_impersonation_event_create_worker.rb | 18 +++++++ .../admin/users_controller_spec.rb | 14 ++++- ...sonation_group_audit_event_service_spec.rb | 54 +++++++++++++++++++ ..._impersonation_event_create_worker_spec.rb | 9 ++++ 8 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb create mode 100644 ee/app/workers/audit_events/user_impersonation_event_create_worker.rb create mode 100644 ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb create mode 100644 ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 02dc1489294ad4..d46933a1135818 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -51,7 +51,7 @@ There are two kinds of events logged: When a user is being [impersonated](../user/admin_area/index.md#user-impersonation), their actions are logged as audit events as usual, with two additional details: 1. Usual audit events include information about the impersonating administrator. These are visible in their respective Audit Event pages depending on their type (Group/Project/User). -1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events. +1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events if the user belongs to a group. ![audit events](img/impersonated_audit_events_v13_8.png) diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb index 8ed0f690ee1ad3..fde9ad83c9bab4 100644 --- a/ee/app/controllers/ee/admin/users_controller.rb +++ b/ee/app/controllers/ee/admin/users_controller.rb @@ -44,8 +44,10 @@ def log_impersonation_event end def log_audit_event - AuditEvents::ImpersonationAuditEventService.new(current_user, request.remote_ip, 'Started Impersonation') - .for_user(full_path: user.username, entity_id: user.id).security_event + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator_id: current_user.id, + user_id: user.id, + request: request, + action: :started) end def allowed_user_params diff --git a/ee/app/controllers/ee/application_controller.rb b/ee/app/controllers/ee/application_controller.rb index f36c0626f9857f..a1afa52a8d1444 100644 --- a/ee/app/controllers/ee/application_controller.rb +++ b/ee/app/controllers/ee/application_controller.rb @@ -38,8 +38,10 @@ def log_impersonation_event end def log_audit_event - AuditEvents::ImpersonationAuditEventService.new(impersonator, request.remote_ip, 'Stopped Impersonation') - .for_user(full_path: current_user.username, entity_id: current_user.id).security_event + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator: impersonator, + user: current_user, + request: request, + action: :stopped) end def set_current_ip_address(&block) 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 new file mode 100644 index 00000000000000..9065b8b045dda5 --- /dev/null +++ b/ee/app/services/audit_events/user_impersonation_group_audit_event_service.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +# Creates audit events at both the instance level +# and for all of a user's groups when the user is impersonated. +module AuditEvents + class UserImpersonationGroupAuditEventService + def initialize(impersonator:, user:, request:, action: :started) + @impersonator = impersonator + @user = user + @request = request + @action = action.to_s + end + + def execute + log_instance_audit_event + log_groups_audit_events + end + + def log_instance_audit_event + AuditEvents::ImpersonationAuditEventService.new(@impersonator, @request.remote_ip, "#{@action.capitalize} Impersonation") + .for_user(full_path: @user.username, entity_id: @user.id).security_event + end + + def log_groups_audit_events + @user.groups.each do |group| + audit_context = { + name: "user_impersonation", + author: @impersonator, + scope: group, + target: @user, + message: "Instance administrator #{@action} impersonation of #{@user.username}" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end +end diff --git a/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb new file mode 100644 index 00000000000000..06590dc66ef25b --- /dev/null +++ b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module AuditEvents + class UserImpersonationEventCreateWorker + include ApplicationWorker + + idempotent! + data_consistency :always + feature_category :audit_events + + def perform(impersonator_id:, user_id:, request:, action:) + ::AuditEvents::UserImpersonationGroupAuditEventService.new(impersonator: User.find_by_id(impersonator.id), + user: User.find_by_id(user_id), + request: request, + action: action).execute + end + end +end diff --git a/ee/spec/controllers/admin/users_controller_spec.rb b/ee/spec/controllers/admin/users_controller_spec.rb index 724e9721ea683f..510acb6e7716c2 100644 --- a/ee/spec/controllers/admin/users_controller_spec.rb +++ b/ee/spec/controllers/admin/users_controller_spec.rb @@ -108,8 +108,18 @@ stub_licensed_features(extended_audit_events: true) end - it 'creates an AuditEvent record' do - expect { post :impersonate, params: { id: user.username } }.to change { AuditEvent.count }.by(1) + it 'enqueues a new worker' do + expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with( + impersonator_id: current_user.id, + user_id: user.id, + request: anything, + action: :started + ).once + + post :impersonate, params: { id: user.username } end + # it 'creates an AuditEvent record' do + # expect { post :impersonate, params: { id: user.username } }.to change { AuditEvent.count }.by(1) + # 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 new file mode 100644 index 00000000000000..090fe435116fbd --- /dev/null +++ b/ee/spec/services/audit_events/user_impersonation_group_audit_event_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::UserImpersonationGroupAuditEventService do + let!(:group) { create(:group) } + let!(:user) { create(:user) } + let!(:admin) { create(:admin) } + let!(:request) { double(ActionDispatch::Request, remote_ip: '111.112.11.2') } + + let(:service) { described_class.new(impersonator: admin, user: user, request: request, action: :started) } + + before do + stub_licensed_features(audit_events: true) + stub_licensed_features(admin_audit_log: true) + stub_licensed_features(extended_audit_events: true) + end + + context 'when user belongs to a single group' do + before do + group.add_developer(user) + end + + it 'creates audit events for both the instance and group level' do + expect { service.execute }.to change { AuditEvent.count }.by(2) + end + end + + context 'when user belongs to multiple groups' do + let!(:group2) { create(:group) } + let!(:group3) { create(:group) } + + before do + group.add_developer(user) + group2.add_developer(user) + group3.add_developer(user) + end + + it 'creates audit events for both the instance and group level' do + expect { service.execute }.to change { AuditEvent.count }.by(4) + end + end + + context 'when user does not belong to any group' do + it 'creates audit events at the instance level' do + expect { service.execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.details[:custom_message]).to eq("Started Impersonation") + expect(event.author).to eq admin + expect(event.target_id).to eq user.id + end + end +end diff --git a/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb b/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb new file mode 100644 index 00000000000000..e605a08a24f5e0 --- /dev/null +++ b/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::UserImpersonationEventCreateWorker do + describe "#perform" do + subject(:worker) { described_class.new } + end +end -- GitLab From 1b486c8e3624500ddcca29851d1e06f93c3e7e65 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Fri, 4 Feb 2022 11:54:09 +0000 Subject: [PATCH 2/9] Make backend reviewer suggestions --- config/sidekiq_queues.yml | 2 ++ ee/app/controllers/ee/admin/users_controller.rb | 5 +---- ee/app/controllers/ee/application_controller.rb | 5 +---- ...er_impersonation_group_audit_event_service.rb | 6 +++--- ee/app/workers/all_queues.yml | 9 +++++++++ .../user_impersonation_event_create_worker.rb | 6 +++--- .../admin/impersonations_controller_spec.rb | 6 ++++-- .../controllers/admin/users_controller_spec.rb | 10 +--------- ...personation_group_audit_event_service_spec.rb | 9 +++++++-- ...ser_impersonation_event_create_worker_spec.rb | 16 ++++++++++++++++ 10 files changed, 47 insertions(+), 27 deletions(-) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index c197e77fbf78fd..dd23baa00ffadc 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -43,6 +43,8 @@ - 1 - - audit_events_audit_event_streaming - 1 +- - audit_events_user_impersonation_event_create + - 1 - - authorized_keys - 2 - - authorized_project_update diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb index fde9ad83c9bab4..da3e3b616694bb 100644 --- a/ee/app/controllers/ee/admin/users_controller.rb +++ b/ee/app/controllers/ee/admin/users_controller.rb @@ -44,10 +44,7 @@ def log_impersonation_event end def log_audit_event - ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator_id: current_user.id, - user_id: user.id, - request: request, - action: :started) + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(current_user.id, user.id, request.remote_ip, :started) end def allowed_user_params diff --git a/ee/app/controllers/ee/application_controller.rb b/ee/app/controllers/ee/application_controller.rb index a1afa52a8d1444..62cc4712313212 100644 --- a/ee/app/controllers/ee/application_controller.rb +++ b/ee/app/controllers/ee/application_controller.rb @@ -38,10 +38,7 @@ def log_impersonation_event end def log_audit_event - ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator: impersonator, - user: current_user, - request: request, - action: :stopped) + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator.id, current_user.id, request.remote_ip, :stopped) end def set_current_ip_address(&block) 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 9065b8b045dda5..0cdd69ec98d1d5 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 @@ -4,10 +4,10 @@ # and for all of a user's groups when the user is impersonated. module AuditEvents class UserImpersonationGroupAuditEventService - def initialize(impersonator:, user:, request:, action: :started) + def initialize(impersonator:, user:, remote_ip:, action: :started) @impersonator = impersonator @user = user - @request = request + @remote_ip = remote_ip @action = action.to_s end @@ -17,7 +17,7 @@ def execute end def log_instance_audit_event - AuditEvents::ImpersonationAuditEventService.new(@impersonator, @request.remote_ip, "#{@action.capitalize} Impersonation") + AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip, "#{@action.capitalize} Impersonation") .for_user(full_path: @user.username, entity_id: @user.id).security_event end diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 3d428042928516..75cb8a5abbb8b8 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -903,6 +903,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: audit_events_user_impersonation_event_create + :worker_name: AuditEvents::UserImpersonationEventCreateWorker + :feature_category: :audit_events + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: ci_batch_reset_minutes :worker_name: Ci::BatchResetMinutesWorker :feature_category: :continuous_integration diff --git a/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb index 06590dc66ef25b..c4a908bb8f7c7d 100644 --- a/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb +++ b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb @@ -8,10 +8,10 @@ class UserImpersonationEventCreateWorker data_consistency :always feature_category :audit_events - def perform(impersonator_id:, user_id:, request:, action:) - ::AuditEvents::UserImpersonationGroupAuditEventService.new(impersonator: User.find_by_id(impersonator.id), + def perform(impersonator_id, user_id, remote_ip, action) + ::AuditEvents::UserImpersonationGroupAuditEventService.new(impersonator: User.find_by_id(impersonator_id), user: User.find_by_id(user_id), - request: request, + remote_ip: remote_ip, action: action).execute end end diff --git a/ee/spec/controllers/admin/impersonations_controller_spec.rb b/ee/spec/controllers/admin/impersonations_controller_spec.rb index 6ef610a4639143..8b4d261c0452c5 100644 --- a/ee/spec/controllers/admin/impersonations_controller_spec.rb +++ b/ee/spec/controllers/admin/impersonations_controller_spec.rb @@ -18,8 +18,10 @@ stub_licensed_features(extended_audit_events: true) end - it 'creates an AuditEvent record' do - expect { delete :destroy }.to change { AuditEvent.count }.by(1) + it 'enqueues a new worker' do + expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(impersonator.id, user.id, anything, :stopped).once + + delete :destroy end end end diff --git a/ee/spec/controllers/admin/users_controller_spec.rb b/ee/spec/controllers/admin/users_controller_spec.rb index 510acb6e7716c2..e06c3b04d90021 100644 --- a/ee/spec/controllers/admin/users_controller_spec.rb +++ b/ee/spec/controllers/admin/users_controller_spec.rb @@ -109,17 +109,9 @@ end it 'enqueues a new worker' do - expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with( - impersonator_id: current_user.id, - user_id: user.id, - request: anything, - action: :started - ).once + expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(admin.id, user.id, anything, :started).once post :impersonate, params: { id: user.username } end - # it 'creates an AuditEvent record' do - # expect { post :impersonate, params: { id: user.username } }.to change { AuditEvent.count }.by(1) - # 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 090fe435116fbd..10271bb5e207a5 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 @@ -6,9 +6,8 @@ let!(:group) { create(:group) } let!(:user) { create(:user) } let!(:admin) { create(:admin) } - let!(:request) { double(ActionDispatch::Request, remote_ip: '111.112.11.2') } - let(:service) { described_class.new(impersonator: admin, user: user, request: request, action: :started) } + let(:service) { described_class.new(impersonator: admin, user: user, remote_ip: '111.112.11.2', action: :started) } before do stub_licensed_features(audit_events: true) @@ -23,6 +22,9 @@ it 'creates audit events for both the instance and group level' do expect { service.execute }.to change { AuditEvent.count }.by(2) + + event = AuditEvent.first + expect(event.details[:custom_message]).to eq("Started Impersonation") end end @@ -38,6 +40,9 @@ it 'creates audit events for both the instance and group level' do expect { service.execute }.to change { AuditEvent.count }.by(4) + + event = AuditEvent.first + expect(event.details[:custom_message]).to eq("Started Impersonation") end end diff --git a/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb b/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb index e605a08a24f5e0..cfa0cc18720cc1 100644 --- a/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb +++ b/ee/spec/workers/audit_events/user_impersonation_event_create_worker_spec.rb @@ -4,6 +4,22 @@ RSpec.describe AuditEvents::UserImpersonationEventCreateWorker do describe "#perform" do + let_it_be(:impersonator) { create(:admin) } + let_it_be(:user) { create(:user) } + + let(:action) { :started } + subject(:worker) { described_class.new } + + it 'invokes the UserImpersonationGroupAuditEventService' do + expect(::AuditEvents::UserImpersonationGroupAuditEventService).to receive(:new).with( + impersonator: impersonator, + user: user, + remote_ip: '111.112.11.2', + action: action + ).and_call_original + + subject.perform(impersonator.id, user.id, '111.112.11.2', action) + end end end -- GitLab From d36313d7bdfdaeb56c8034459011a3f2752a6130 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Tue, 8 Feb 2022 08:32:57 +0000 Subject: [PATCH 3/9] Make changes based on appsec review --- doc/administration/audit_events.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index d46933a1135818..3ebd2a57314f79 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -51,7 +51,7 @@ There are two kinds of events logged: When a user is being [impersonated](../user/admin_area/index.md#user-impersonation), their actions are logged as audit events as usual, with two additional details: 1. Usual audit events include information about the impersonating administrator. These are visible in their respective Audit Event pages depending on their type (Group/Project/User). -1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events if the user belongs to a group. +2. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events for all groups the user belongs to. ![audit events](img/impersonated_audit_events_v13_8.png) @@ -103,6 +103,7 @@ From there, you can see the following actions: - Group CI/CD variable added, removed, or protected status changed. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/30857) in GitLab 13.3. - Compliance framework created, updated, or deleted. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/340649) in GitLab 14.5. - Event streaming destination created, updated, or deleted. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/344664) in GitLab 14.6. +- Instance administrator started or stopped impersonation of a group member. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/300961) in GitLab 14.8. Group events can also be accessed via the [Group Audit Events API](../api/audit_events.md#group-audit-events) -- GitLab From ec12928697ddaa671c7f3506d5905fba2801a598 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Tue, 8 Feb 2022 13:10:54 +0000 Subject: [PATCH 4/9] Additional backend review changes --- doc/administration/audit_events.md | 2 +- ee/app/controllers/ee/admin/users_controller.rb | 2 +- ee/app/controllers/ee/application_controller.rb | 2 +- .../user_impersonation_group_audit_event_service.rb | 2 +- ee/spec/controllers/admin/impersonations_controller_spec.rb | 2 +- ee/spec/controllers/admin/users_controller_spec.rb | 2 +- .../user_impersonation_group_audit_event_service_spec.rb | 6 ++++++ 7 files changed, 12 insertions(+), 6 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 3ebd2a57314f79..49a7ee5c5c5563 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -51,7 +51,7 @@ There are two kinds of events logged: When a user is being [impersonated](../user/admin_area/index.md#user-impersonation), their actions are logged as audit events as usual, with two additional details: 1. Usual audit events include information about the impersonating administrator. These are visible in their respective Audit Event pages depending on their type (Group/Project/User). -2. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events for all groups the user belongs to. +1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events for all groups the user belongs to. ![audit events](img/impersonated_audit_events_v13_8.png) diff --git a/ee/app/controllers/ee/admin/users_controller.rb b/ee/app/controllers/ee/admin/users_controller.rb index da3e3b616694bb..93430223e48a38 100644 --- a/ee/app/controllers/ee/admin/users_controller.rb +++ b/ee/app/controllers/ee/admin/users_controller.rb @@ -44,7 +44,7 @@ def log_impersonation_event end def log_audit_event - ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(current_user.id, user.id, request.remote_ip, :started) + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(current_user.id, user.id, request.remote_ip, 'started') end def allowed_user_params diff --git a/ee/app/controllers/ee/application_controller.rb b/ee/app/controllers/ee/application_controller.rb index 62cc4712313212..8bbdea699999d5 100644 --- a/ee/app/controllers/ee/application_controller.rb +++ b/ee/app/controllers/ee/application_controller.rb @@ -38,7 +38,7 @@ def log_impersonation_event end def log_audit_event - ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator.id, current_user.id, request.remote_ip, :stopped) + ::AuditEvents::UserImpersonationEventCreateWorker.perform_async(impersonator.id, current_user.id, request.remote_ip, 'stopped') end def set_current_ip_address(&block) 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 0cdd69ec98d1d5..bd525889d9975e 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 @@ -17,7 +17,7 @@ def execute end def log_instance_audit_event - AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip, "#{@action.capitalize} Impersonation") + AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip,"#{@action.capitalize} Impersonation") .for_user(full_path: @user.username, entity_id: @user.id).security_event end diff --git a/ee/spec/controllers/admin/impersonations_controller_spec.rb b/ee/spec/controllers/admin/impersonations_controller_spec.rb index 8b4d261c0452c5..bcb6177b0f54da 100644 --- a/ee/spec/controllers/admin/impersonations_controller_spec.rb +++ b/ee/spec/controllers/admin/impersonations_controller_spec.rb @@ -19,7 +19,7 @@ end it 'enqueues a new worker' do - expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(impersonator.id, user.id, anything, :stopped).once + expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(impersonator.id, user.id, anything, 'stopped').once delete :destroy end diff --git a/ee/spec/controllers/admin/users_controller_spec.rb b/ee/spec/controllers/admin/users_controller_spec.rb index e06c3b04d90021..94a597f4d81d78 100644 --- a/ee/spec/controllers/admin/users_controller_spec.rb +++ b/ee/spec/controllers/admin/users_controller_spec.rb @@ -109,7 +109,7 @@ end it 'enqueues a new worker' do - expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(admin.id, user.id, anything, :started).once + expect(AuditEvents::UserImpersonationEventCreateWorker).to receive(:perform_async).with(admin.id, user.id, anything, 'started').once post :impersonate, params: { id: user.username } 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 10271bb5e207a5..b2850b47103063 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 @@ -25,6 +25,9 @@ event = AuditEvent.first expect(event.details[:custom_message]).to eq("Started Impersonation") + + group_audit_event = AuditEvent.last + expect(group_audit_event.details[:custom_message]).to eq("Instance administrator started impersonation of #{user.username}") end end @@ -43,6 +46,9 @@ event = AuditEvent.first expect(event.details[:custom_message]).to eq("Started Impersonation") + + group_audit_event = AuditEvent.last + expect(group_audit_event.details[:custom_message]).to eq("Instance administrator started impersonation of #{user.username}") end end -- GitLab From 55c8d3cc2925477e157a124c420e9b92e40aebe4 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Tue, 8 Feb 2022 14:42:23 +0000 Subject: [PATCH 5/9] Apply 1 suggestion(s) to 1 file(s) --- .../user_impersonation_group_audit_event_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 bd525889d9975e..0cdd69ec98d1d5 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 @@ -17,7 +17,7 @@ def execute end def log_instance_audit_event - AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip,"#{@action.capitalize} Impersonation") + AuditEvents::ImpersonationAuditEventService.new(@impersonator, @remote_ip, "#{@action.capitalize} Impersonation") .for_user(full_path: @user.username, entity_id: @user.id).security_event end -- GitLab From 5a46a62b545510498d967b63842a96bb9200efc5 Mon Sep 17 00:00:00 2001 From: Evan Read Date: Wed, 9 Feb 2022 09:50:52 +0000 Subject: [PATCH 6/9] Apply 1 suggestion(s) to 1 file(s) --- doc/administration/audit_events.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 49a7ee5c5c5563..2f16ca8bafcf77 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -51,7 +51,10 @@ There are two kinds of events logged: When a user is being [impersonated](../user/admin_area/index.md#user-impersonation), their actions are logged as audit events as usual, with two additional details: 1. Usual audit events include information about the impersonating administrator. These are visible in their respective Audit Event pages depending on their type (Group/Project/User). -1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the instance Audit Events and group Audit Events for all groups the user belongs to. +1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in + the: + - Instance audit events. + - Group audit events for all groups the user belongs to (GitLab 14.8 and later). ![audit events](img/impersonated_audit_events_v13_8.png) -- GitLab From 1630ae3aaa541393c530688aa0d866c5feb95d02 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Wed, 9 Feb 2022 13:24:18 +0000 Subject: [PATCH 7/9] Add backend maintainer suggestions --- ee/app/workers/all_queues.yml | 2 +- .../audit_events/user_impersonation_event_create_worker.rb | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 75cb8a5abbb8b8..e076009673ade0 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -910,7 +910,7 @@ :urgency: :low :resource_boundary: :unknown :weight: 1 - :idempotent: true + :idempotent: :tags: [] - :name: ci_batch_reset_minutes :worker_name: Ci::BatchResetMinutesWorker diff --git a/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb index c4a908bb8f7c7d..97b86e00485459 100644 --- a/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb +++ b/ee/app/workers/audit_events/user_impersonation_event_create_worker.rb @@ -1,11 +1,10 @@ # frozen_string_literal: true module AuditEvents - class UserImpersonationEventCreateWorker + class UserImpersonationEventCreateWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker - idempotent! - data_consistency :always + data_consistency :sticky feature_category :audit_events def perform(impersonator_id, user_id, remote_ip, action) -- GitLab From a647d0f4f2af523a272388772dff5485dbc01e79 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Thu, 10 Feb 2022 09:44:35 +0000 Subject: [PATCH 8/9] limit number of groups events are saved for --- doc/administration/audit_events.md | 2 +- .../user_impersonation_group_audit_event_service.rb | 2 +- .../user_impersonation_group_audit_event_service_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 2f16ca8bafcf77..d4902a18cac015 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -54,7 +54,7 @@ When a user is being [impersonated](../user/admin_area/index.md#user-impersonati 1. Extra audit events are recorded for the start and stop of the administrator's impersonation session. These are visible in the: - Instance audit events. - - Group audit events for all groups the user belongs to (GitLab 14.8 and later). + - Group audit events for all groups the user belongs to (GitLab 14.8 and later). This is limited to 20 groups for performance reasons. ![audit events](img/impersonated_audit_events_v13_8.png) 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 0cdd69ec98d1d5..f1b9b48b532fb3 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 @@ -22,7 +22,7 @@ def log_instance_audit_event end def log_groups_audit_events - @user.groups.each do |group| + @user.groups.first(20).each do |group| audit_context = { name: "user_impersonation", author: @impersonator, 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 b2850b47103063..5b525216ebbd01 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 @@ -3,9 +3,9 @@ require 'spec_helper' RSpec.describe AuditEvents::UserImpersonationGroupAuditEventService do - let!(:group) { create(:group) } - let!(:user) { create(:user) } - let!(:admin) { create(:admin) } + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } + let_it_be(:admin) { create(:admin) } let(:service) { described_class.new(impersonator: admin, user: user, remote_ip: '111.112.11.2', action: :started) } -- GitLab From ad64a6360a16fb0c8a4156d4c9c249d584d2b0c4 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 10 Feb 2022 11:48:54 +0000 Subject: [PATCH 9/9] Add a note about group limit for impersonation audit events --- .../user_impersonation_group_audit_event_service.rb | 2 ++ 1 file changed, 2 insertions(+) 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 f1b9b48b532fb3..6e496de010c254 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 @@ -22,6 +22,8 @@ def log_instance_audit_event end def log_groups_audit_events + # Limited to 20 groups because we can't batch insert audit events + # https://gitlab.com/gitlab-org/gitlab/-/issues/352483 @user.groups.first(20).each do |group| audit_context = { name: "user_impersonation", -- GitLab