From 50f8a53ae09002e8db0646faea6970a75c2b91a7 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Thu, 3 Oct 2024 11:19:50 +0300 Subject: [PATCH 1/2] Link project_bot user deletion audit event to its resource if possible Changelog: changed EE: true MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167021 --- .../resource_access_tokens/create_service.rb | 2 +- .../resource_access_tokens/revoke_service.rb | 2 +- app/services/users/destroy_service.rb | 3 + app/workers/remove_expired_members_worker.rb | 6 +- .../inactive_tokens_deletion_cron_worker.rb | 7 +- doc/user/compliance/audit_event_types.md | 1 + ee/app/services/ee/users/destroy_service.rb | 29 ++-- .../types/project_bot_user_destroyed.yml | 10 ++ .../services/ee/users/destroy_service_spec.rb | 128 +++++++++++++++--- .../remove_expired_members_worker_spec.rb | 42 ++++++ ...active_tokens_deletion_cron_worker_spec.rb | 42 ++++++ .../create_service_spec.rb | 22 ++- .../revoke_service_spec.rb | 5 +- ...ccess_tokens_controller_shared_examples.rb | 6 +- 14 files changed, 268 insertions(+), 37 deletions(-) create mode 100644 ee/config/audit_events/types/project_bot_user_destroyed.yml create mode 100644 ee/spec/workers/remove_expired_members_worker_spec.rb create mode 100644 ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index a69f3405996433..230e17a568e33f 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -75,7 +75,7 @@ def create_user end def delete_failed_user(user) - DeleteUserWorker.perform_async(current_user.id, user.id, hard_delete: true, skip_authorization: true) + DeleteUserWorker.perform_async(current_user.id, user.id, hard_delete: true, skip_authorization: true, reason_for_deletion: "Access token creation failed") end def default_user_params diff --git a/app/services/resource_access_tokens/revoke_service.rb b/app/services/resource_access_tokens/revoke_service.rb index b6c402428d10f8..04aa73b0568e05 100644 --- a/app/services/resource_access_tokens/revoke_service.rb +++ b/app/services/resource_access_tokens/revoke_service.rb @@ -38,7 +38,7 @@ def execute attr_reader :current_user, :access_token, :bot_user, :resource def destroy_bot_user - DeleteUserWorker.perform_async(current_user.id, bot_user.id, skip_authorization: true) + DeleteUserWorker.perform_async(current_user.id, bot_user.id, skip_authorization: true, reason_for_deletion: "Access token revoked") end def can_destroy_token? diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index b62bedf73832aa..775774baad93f9 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -58,6 +58,9 @@ def execute(user, options = {}) # Load the records. Groups are unavailable after membership is destroyed. solo_owned_groups = user.solo_owned_groups.load + # Load the project_bot user resource. It is unavailable after membership is destroyed. + options[:project_bot_resource] ||= user.resource_bot_resource + user.members.each_batch { |batch| batch.destroy_all } # rubocop:disable Cop/DestroyAll solo_owned_groups.each do |group| diff --git a/app/workers/remove_expired_members_worker.rb b/app/workers/remove_expired_members_worker.rb index ca6ee9566e94e4..677be19c64deca 100644 --- a/app/workers/remove_expired_members_worker.rb +++ b/app/workers/remove_expired_members_worker.rb @@ -54,7 +54,11 @@ def process_member(member) expired_user = member.user if expired_user.project_bot? - Users::DestroyService.new(nil).execute(expired_user, skip_authorization: true) + Users::DestroyService.new(nil).execute(expired_user, { + skip_authorization: true, + project_bot_resource: member.source, + reason_for_deletion: "Membership expired" + }) end @updated_count += 1 diff --git a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb index d0789845ebc03b..e6e469639a1b52 100644 --- a/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb +++ b/app/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker.rb @@ -38,7 +38,12 @@ def initiate_users_deletion(users) DeleteUserWorker.bulk_perform_async_with_contexts( users, - arguments_proc: ->(user) { [admin_bot_id, user.id, { skip_authorization: true }] }, + arguments_proc: ->(user) { + [ + admin_bot_id, user.id, + { skip_authorization: true, reason_for_deletion: "No active token assigned" } + ] + }, context_proc: ->(user) { { user: user } } ) end diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index b2ee7b3390fd76..ef8d8289412b94 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -559,6 +559,7 @@ Audit event types belong to the following product categories. | [`ban_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116103) | Triggered when a user is banned, unbanned, blocked, or unblocked | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`change_membership_state`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/87924) | Triggered when a user's membership is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.1](https://gitlab.com/gitlab-org/gitlab/-/issues/362200) | Group | | [`password_reset_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129079) | Triggered when a password reset fails for a user | **{dotted-circle}** No | **{check-circle}** Yes | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/377762) | User | +| [`project_bot_user_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167021) | Event triggered when a bot user is scheduled for removal from the instance | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/488166) | Group, Project | | [`unban_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116221) | Event triggered on user unban action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`unblock_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115727) | Event triggered on user unblock action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`user_activate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121708) | Event triggered on user activate action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.1](https://gitlab.com/gitlab-org/gitlab/-/issues/13473) | User | diff --git a/ee/app/services/ee/users/destroy_service.rb b/ee/app/services/ee/users/destroy_service.rb index fa46e4e0f5a1e1..6285efd5572db5 100644 --- a/ee/app/services/ee/users/destroy_service.rb +++ b/ee/app/services/ee/users/destroy_service.rb @@ -13,7 +13,7 @@ def execute(user, options = {}) escalation_rules_cleanup(delete_user) end - log_audit_event(user) if audit_required? + log_audit_event(user, options) result end @@ -56,15 +56,26 @@ 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, + def log_audit_event(user, options) + audit_context = { + author: current_user || ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), target: user, - target_details: user.full_path, - message: "User #{user.username} scheduled for deletion" - }) + target_details: user.full_path + } + + if user.project_bot? && options[:project_bot_resource] + audit_context[:name] = 'project_bot_user_destroyed' + audit_context[:scope] = options[:project_bot_resource] + audit_context[:message] = "Bot user #{user.username} scheduled for deletion" + else + audit_context[:name] = 'user_destroyed' + audit_context[:scope] = user + audit_context[:message] = "User #{user.username} scheduled for deletion" + end + + audit_context[:message] += ". Reason: #{options[:reason_for_deletion]}" if options[:reason_for_deletion] + + ::Gitlab::Audit::Auditor.audit(audit_context) end def audit_required? diff --git a/ee/config/audit_events/types/project_bot_user_destroyed.yml b/ee/config/audit_events/types/project_bot_user_destroyed.yml new file mode 100644 index 00000000000000..b1d7c1c881cf6d --- /dev/null +++ b/ee/config/audit_events/types/project_bot_user_destroyed.yml @@ -0,0 +1,10 @@ +--- +name: project_bot_user_destroyed +description: Event triggered when a bot user is scheduled for removal from the instance +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/488166 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167021 +feature_category: user_management +milestone: '17.5' +saved_to_database: true +streamed: false +scope: [Group, Project] diff --git a/ee/spec/services/ee/users/destroy_service_spec.rb b/ee/spec/services/ee/users/destroy_service_spec.rb index 90738e211b6ee3..4a73f3b3ec7040 100644 --- a/ee/spec/services/ee/users/destroy_service_spec.rb +++ b/ee/spec/services/ee/users/destroy_service_spec.rb @@ -7,21 +7,21 @@ subject(:service) { described_class.new(current_user) } - shared_examples 'auditable' do + shared_examples 'auditable' do |audit_name:| before do stub_licensed_features(extended_audit_events: true) end - it 'creates audit event record' do + it "creates #{audit_name} audit event record", :aggregate_failures do expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including({ - name: "user_destroyed" + name: audit_name })).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| + details = expected_audit_attributes.delete(:details) || {} + expected_audit_attributes.each do |method, value| expect(audit_event.public_send(method)).to eq(value) end expect(audit_event.details).to include(details) @@ -40,19 +40,113 @@ end 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" + context 'for audit event' do + it_behaves_like 'auditable', audit_name: 'user_destroyed' do + let(:author) { current_user } + + let(:expected_audit_attributes) do + { + author_id: author.id, + entity: user, + details: { + author_class: author.class.to_s, + author_name: author.name, + custom_message: "User #{user.username} scheduled for deletion", + target_details: user.full_path, + target_id: user.id, + target_type: user.class.to_s + } } - } + end + end + + context 'when current_user is nil' do + let_it_be(:current_user) { nil } + + subject(:operation) { service.execute(user, { skip_authorization: true }) } + + it_behaves_like 'auditable', audit_name: 'user_destroyed' do + let(:author) { ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)') } + + let(:expected_audit_attributes) do + { + author_id: author.id, + details: { + author_class: author.class.to_s, + author_name: author.name + } + } + end + end + end + + context 'when reason_for_deletion is provided' do + let(:reason_for_deletion) { 'Reason for deletion!' } + + subject(:operation) { service.execute(user, { reason_for_deletion: reason_for_deletion }) } + + it_behaves_like 'auditable', audit_name: 'user_destroyed' do + let(:expected_audit_attributes) do + { + details: { + custom_message: "User #{user.username} scheduled for deletion. Reason: #{reason_for_deletion}" + } + } + end + end + end + + context 'when user is a project_bot' do + let(:user) { create(:user, :project_bot) } + + context 'when project_bot belongs to resource' do + let!(:resource) { create(:group, maintainers: user) } + + it_behaves_like 'auditable', audit_name: 'project_bot_user_destroyed' do + let(:author) { current_user } + + let(:expected_audit_attributes) do + { + author_id: author.id, + entity: resource, + details: { + author_class: author.class.to_s, + author_name: author.name, + custom_message: "Bot user #{user.username} scheduled for deletion", + target_details: user.full_path, + target_id: user.id, + target_type: user.class.to_s + } + } + end + end + end + + context 'when project_bot is orphaned record' do + it_behaves_like 'auditable', audit_name: 'user_destroyed' do + let(:author) { current_user } + + let(:expected_audit_attributes) do + { + author_id: author.id, + entity: user, + details: { + author_class: author.class.to_s, + author_name: author.name, + custom_message: "User #{user.username} scheduled for deletion", + target_details: user.full_path, + target_id: user.id, + target_type: user.class.to_s + } + } + end + end + end end + end + + context 'when project is a mirror' do + let(:project) { create(:project, :mirror, mirror_user_id: user.id) } it 'disables mirror and does not assign a new mirror_user' do expect(::Gitlab::ErrorTracking).to receive(:track_exception) @@ -66,8 +160,6 @@ 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 diff --git a/ee/spec/workers/remove_expired_members_worker_spec.rb b/ee/spec/workers/remove_expired_members_worker_spec.rb new file mode 100644 index 00000000000000..ec92d7ae3ae604 --- /dev/null +++ b/ee/spec/workers/remove_expired_members_worker_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoveExpiredMembersWorker, feature_category: :system_access do + let(:worker) { described_class.new } + + describe '#perform' do + context 'for project bots' do + let(:project) { create(:project) } + let!(:project_bot) { create(:resource_access_token, resource: project).user } + + context 'when project bot membership is expired' do + before do + project_bot.members.first.update_column(:expires_at, 1.second.ago) + end + + it "logs project_bot_user_destroyed audit event", :aggregate_failures do + expect { worker.perform }.to change { + AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").count + }.by(1) + + audit_event = AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").last + + author = ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)') + expect(audit_event.author_id).to eq(author.id) + expect(audit_event.entity).to eq(project) + expect(audit_event.details).to eq({ + event_name: 'project_bot_user_destroyed', + author_class: author.class.to_s, + author_name: author.name, + custom_message: + "Bot user #{project_bot.username} scheduled for deletion. Reason: Membership expired", + target_details: project_bot.full_path, + target_id: project_bot.id, + target_type: project_bot.class.to_s + }) + end + end + end + end +end diff --git a/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000000..31f2a0622b5a92 --- /dev/null +++ b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ResourceAccessTokens::InactiveTokensDeletionCronWorker, feature_category: :system_access do + subject(:worker) { described_class.new } + + describe '#perform' do + context 'when project_bot is associated with resource' do + let(:resource) { create(:group) } + let!(:project_bot) do + create( + :resource_access_token, + resource: resource, + expires_at: ApplicationSetting::INACTIVE_RESOURCE_ACCESS_TOKENS_DELETE_AFTER_DAYS.days.ago - 1.day + ).user + end + + it "logs project_bot_user_destroyed audit event", :freeze_time, :sidekiq_inline, :aggregate_failures do + expect { worker.perform }.to change { + AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").count + }.by(1) + + audit_event = AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").last + + author = Users::Internal.admin_bot + expect(audit_event.author_id).to eq(author.id) + expect(audit_event.entity).to eq(resource) + expect(audit_event.details).to eq({ + event_name: 'project_bot_user_destroyed', + author_class: author.class.to_s, + author_name: author.name, + custom_message: + "Bot user #{project_bot.username} scheduled for deletion. Reason: No active token assigned", + target_details: project_bot.full_path, + target_id: project_bot.id, + target_type: project_bot.class.to_s + }) + end + end + end +end diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 3f5845d51ea8a7..85d7c8bf46f47f 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -31,6 +31,20 @@ end end + shared_examples 'deletes failed project bot' do + it 'calls DeleteUserWorker for the project bot' do + expect_next_instance_of(User) do |project_bot| + project_bot.id = User.maximum(:id) + 1 + expect(DeleteUserWorker).to receive(:perform_async).with( + user.id, project_bot.id, + { hard_delete: true, skip_authorization: true, reason_for_deletion: "Access token creation failed" } + ).and_call_original + end + + subject + end + end + shared_examples 'correct error message' do it 'returns correct error message' do expect(subject.error?).to be true @@ -279,18 +293,16 @@ it_behaves_like 'token creation fails' it_behaves_like 'correct error message' + it_behaves_like 'deletes failed project bot' end end context "when access provisioning fails" do - let_it_be(:bot_user) { create(:user, :project_bot) } - - let(:unpersisted_member) { build(:project_member, source: resource, user: bot_user) } + let(:unpersisted_member) { build(:project_member, source: resource) } let(:error_message) { 'Could not provision maintainer access to the access token. ERROR: error message' } before do allow_next_instance_of(ResourceAccessTokens::CreateService) do |service| - allow(service).to receive(:create_user).and_return(bot_user) allow(service).to receive(:create_membership).and_return(unpersisted_member) end @@ -303,6 +315,7 @@ it_behaves_like 'token creation fails' it_behaves_like 'correct error message' + it_behaves_like 'deletes failed project bot' end context 'with MAINTAINER access_level, in string format' do @@ -310,6 +323,7 @@ it_behaves_like 'token creation fails' it_behaves_like 'correct error message' + it_behaves_like 'deletes failed project bot' end end end diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index cec8fb3b902a7c..4d5b237c6a71af 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -45,7 +45,10 @@ it { expect(subject.message).to eq("Access token #{access_token.name} has been revoked and the bot user has been scheduled for deletion.") } it 'calls delete user worker' do - expect(DeleteUserWorker).to receive(:perform_async).with(user.id, resource_bot.id, skip_authorization: true) + expect(DeleteUserWorker).to receive(:perform_async).with( + user.id, resource_bot.id, + { skip_authorization: true, reason_for_deletion: "Access token revoked" } + ) subject end diff --git a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb index da5b41f18d6bf7..1f5c41cd41fc00 100644 --- a/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb +++ b/spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb @@ -197,7 +197,11 @@ def created_token end it 'calls delete user worker' do - expect(DeleteUserWorker).to receive(:perform_async).with(user.id, access_token_user.id, skip_authorization: true) + expect(DeleteUserWorker).to receive(:perform_async).with( + user.id, + access_token_user.id, + skip_authorization: true, reason_for_deletion: "Access token revoked" + ) subject end -- GitLab From 2194b23832afabe1b3f610104cf334f2ed94bd24 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Fri, 4 Oct 2024 12:32:23 +0300 Subject: [PATCH 2/2] Keep using `user_destroyed` event type for project bot deletion --- doc/user/compliance/audit_event_types.md | 3 +-- ee/app/services/ee/users/destroy_service.rb | 21 ++++++------------- .../types/project_bot_user_destroyed.yml | 10 --------- .../audit_events/types/user_destroyed.yml | 2 +- .../services/ee/users/destroy_service_spec.rb | 4 ++-- .../remove_expired_members_worker_spec.rb | 12 +++++------ ...active_tokens_deletion_cron_worker_spec.rb | 15 +++++++------ .../create_service_spec.rb | 2 +- .../revoke_service_spec.rb | 2 +- 9 files changed, 27 insertions(+), 44 deletions(-) delete mode 100644 ee/config/audit_events/types/project_bot_user_destroyed.yml diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index ef8d8289412b94..83e79e1fde83f1 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -559,7 +559,6 @@ Audit event types belong to the following product categories. | [`ban_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116103) | Triggered when a user is banned, unbanned, blocked, or unblocked | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`change_membership_state`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/87924) | Triggered when a user's membership is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.1](https://gitlab.com/gitlab-org/gitlab/-/issues/362200) | Group | | [`password_reset_failed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129079) | Triggered when a password reset fails for a user | **{dotted-circle}** No | **{check-circle}** Yes | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/377762) | User | -| [`project_bot_user_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167021) | Event triggered when a bot user is scheduled for removal from the instance | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [17.5](https://gitlab.com/gitlab-org/gitlab/-/issues/488166) | Group, Project | | [`unban_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/116221) | Event triggered on user unban action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`unblock_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115727) | Event triggered on user unblock action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/377620) | User | | [`user_activate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121708) | Event triggered on user activate action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.1](https://gitlab.com/gitlab-org/gitlab/-/issues/13473) | User | @@ -567,7 +566,7 @@ Audit event types belong to the following product categories. | [`user_blocked`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | Event triggered when a user is blocked | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | | [`user_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | Event triggered when a user is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.10](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | | [`user_deactivate`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/117776) | Event triggered on user deactivate action | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/13473) | User | -| [`user_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | Event triggered when a user is scheduled for removal from the instance | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User | +| [`user_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113784) | Event triggered when a user is scheduled for removal from the instance | **{check-circle}** Yes | **{dotted-circle}** No | GitLab [15.11](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) | User, Group, Project | | [`user_email_changed_and_user_signed_in`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106090) | audit when user emailed changed and user signed in | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369331) | User | | [`user_impersonation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79340) | Triggered when an instance administrator starts or stops impersonating a user | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [14.8](https://gitlab.com/gitlab-org/gitlab/-/issues/300961) | User, Group | | [`user_password_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106086) | audit when user password is updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369330) | User | diff --git a/ee/app/services/ee/users/destroy_service.rb b/ee/app/services/ee/users/destroy_service.rb index 6285efd5572db5..ff78ff85e60a5c 100644 --- a/ee/app/services/ee/users/destroy_service.rb +++ b/ee/app/services/ee/users/destroy_service.rb @@ -58,28 +58,19 @@ def first_mirror_owner(user, mirror) def log_audit_event(user, options) audit_context = { + name: 'user_destroyed', author: current_user || ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)'), + scope: user, target: user, - target_details: user.full_path + target_details: user.full_path, + message: "User #{user.username} scheduled for deletion" } - if user.project_bot? && options[:project_bot_resource] - audit_context[:name] = 'project_bot_user_destroyed' - audit_context[:scope] = options[:project_bot_resource] - audit_context[:message] = "Bot user #{user.username} scheduled for deletion" - else - audit_context[:name] = 'user_destroyed' - audit_context[:scope] = user - audit_context[:message] = "User #{user.username} scheduled for deletion" - end - audit_context[:message] += ". Reason: #{options[:reason_for_deletion]}" if options[:reason_for_deletion] - ::Gitlab::Audit::Auditor.audit(audit_context) - end + audit_context[:scope] = options[:project_bot_resource] if user.project_bot? && options[:project_bot_resource] - def audit_required? - current_user.present? + ::Gitlab::Audit::Auditor.audit(audit_context) end end end diff --git a/ee/config/audit_events/types/project_bot_user_destroyed.yml b/ee/config/audit_events/types/project_bot_user_destroyed.yml deleted file mode 100644 index b1d7c1c881cf6d..00000000000000 --- a/ee/config/audit_events/types/project_bot_user_destroyed.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: project_bot_user_destroyed -description: Event triggered when a bot user is scheduled for removal from the instance -introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/488166 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167021 -feature_category: user_management -milestone: '17.5' -saved_to_database: true -streamed: false -scope: [Group, Project] diff --git a/ee/config/audit_events/types/user_destroyed.yml b/ee/config/audit_events/types/user_destroyed.yml index b470133b6bf7b3..46afde330e2738 100644 --- a/ee/config/audit_events/types/user_destroyed.yml +++ b/ee/config/audit_events/types/user_destroyed.yml @@ -7,4 +7,4 @@ feature_category: user_management milestone: '15.11' saved_to_database: true streamed: false -scope: [User] +scope: [User, Group, Project] diff --git a/ee/spec/services/ee/users/destroy_service_spec.rb b/ee/spec/services/ee/users/destroy_service_spec.rb index 4a73f3b3ec7040..a59a230b610ea7 100644 --- a/ee/spec/services/ee/users/destroy_service_spec.rb +++ b/ee/spec/services/ee/users/destroy_service_spec.rb @@ -102,7 +102,7 @@ context 'when project_bot belongs to resource' do let!(:resource) { create(:group, maintainers: user) } - it_behaves_like 'auditable', audit_name: 'project_bot_user_destroyed' do + it_behaves_like 'auditable', audit_name: 'user_destroyed' do let(:author) { current_user } let(:expected_audit_attributes) do @@ -112,7 +112,7 @@ details: { author_class: author.class.to_s, author_name: author.name, - custom_message: "Bot user #{user.username} scheduled for deletion", + custom_message: "User #{user.username} scheduled for deletion", target_details: user.full_path, target_id: user.id, target_type: user.class.to_s diff --git a/ee/spec/workers/remove_expired_members_worker_spec.rb b/ee/spec/workers/remove_expired_members_worker_spec.rb index ec92d7ae3ae604..d018582065e8c1 100644 --- a/ee/spec/workers/remove_expired_members_worker_spec.rb +++ b/ee/spec/workers/remove_expired_members_worker_spec.rb @@ -10,27 +10,27 @@ let(:project) { create(:project) } let!(:project_bot) { create(:resource_access_token, resource: project).user } - context 'when project bot membership is expired' do + context 'for audit event' do before do project_bot.members.first.update_column(:expires_at, 1.second.ago) end - it "logs project_bot_user_destroyed audit event", :aggregate_failures do + it "logs user_destroyed audit event linked to the project bot resource", :aggregate_failures do expect { worker.perform }.to change { - AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").count + AuditEvent.where("details LIKE ?", "%user_destroyed%").count }.by(1) - audit_event = AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").last + audit_event = AuditEvent.where("details LIKE ?", "%user_destroyed%").last author = ::Gitlab::Audit::UnauthenticatedAuthor.new(name: '(System)') expect(audit_event.author_id).to eq(author.id) expect(audit_event.entity).to eq(project) expect(audit_event.details).to eq({ - event_name: 'project_bot_user_destroyed', + event_name: 'user_destroyed', author_class: author.class.to_s, author_name: author.name, custom_message: - "Bot user #{project_bot.username} scheduled for deletion. Reason: Membership expired", + "User #{project_bot.username} scheduled for deletion. Reason: Membership expired", target_details: project_bot.full_path, target_id: project_bot.id, target_type: project_bot.class.to_s diff --git a/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb index 31f2a0622b5a92..8fb37097aba2bf 100644 --- a/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb @@ -6,7 +6,7 @@ subject(:worker) { described_class.new } describe '#perform' do - context 'when project_bot is associated with resource' do + context 'for audit event' do let(:resource) { create(:group) } let!(:project_bot) do create( @@ -16,22 +16,25 @@ ).user end - it "logs project_bot_user_destroyed audit event", :freeze_time, :sidekiq_inline, :aggregate_failures do + it( + "logs user_destroyed audit event linked to the project bot resource", + :freeze_time, :sidekiq_inline, :aggregate_failures + ) do expect { worker.perform }.to change { - AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").count + AuditEvent.where("details LIKE ?", "%user_destroyed%").count }.by(1) - audit_event = AuditEvent.where("details LIKE ?", "%project_bot_user_destroyed%").last + audit_event = AuditEvent.where("details LIKE ?", "%user_destroyed%").last author = Users::Internal.admin_bot expect(audit_event.author_id).to eq(author.id) expect(audit_event.entity).to eq(resource) expect(audit_event.details).to eq({ - event_name: 'project_bot_user_destroyed', + event_name: 'user_destroyed', author_class: author.class.to_s, author_name: author.name, custom_message: - "Bot user #{project_bot.username} scheduled for deletion. Reason: No active token assigned", + "User #{project_bot.username} scheduled for deletion. Reason: No active token assigned", target_details: project_bot.full_path, target_id: project_bot.id, target_type: project_bot.class.to_s diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 85d7c8bf46f47f..930cee21499eba 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -37,7 +37,7 @@ project_bot.id = User.maximum(:id) + 1 expect(DeleteUserWorker).to receive(:perform_async).with( user.id, project_bot.id, - { hard_delete: true, skip_authorization: true, reason_for_deletion: "Access token creation failed" } + hard_delete: true, skip_authorization: true, reason_for_deletion: "Access token creation failed" ).and_call_original end diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 4d5b237c6a71af..96ac7ef7dc7ae1 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -47,7 +47,7 @@ it 'calls delete user worker' do expect(DeleteUserWorker).to receive(:perform_async).with( user.id, resource_bot.id, - { skip_authorization: true, reason_for_deletion: "Access token revoked" } + skip_authorization: true, reason_for_deletion: "Access token revoked" ) subject -- GitLab