diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index a69f3405996433a2f94ca7d50513a9ebdf1e8762..230e17a568e33fb43f2744cf7675519c89cd26fa 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 b6c402428d10f8b3e5605ef1b89a7eb41dadbac3..04aa73b0568e0596a35f78bf35e73ad227490cba 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 b62bedf73832aa1df1f305473fcbd54ca8596d02..775774baad93f900bf3363f67f2b76b1ec724dc5 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 ca6ee9566e94e4b2741f60b0ae4c39c20d52829d..677be19c64deca22183f27cb8a23cab14f8f3b53 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 d0789845ebc03bf04c7d9b3e6413692ceae52500..e6e469639a1b5269609c4b6bf6ad6927a575c657 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 b2ee7b3390fd769b7d1b3d8acb441ef6ec335af0..83e79e1fde83f1e91e0d420fbe94c5d27d72a5fe 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -566,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 fa46e4e0f5a1e18ee5d904e0c6fe70a15208cea8..ff78ff85e60a5c4e8ded0e15dc398ce7823bb6b0 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,19 +56,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, + 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, message: "User #{user.username} scheduled for deletion" - }) - end + } + + audit_context[:message] += ". Reason: #{options[:reason_for_deletion]}" if options[:reason_for_deletion] + + 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/user_destroyed.yml b/ee/config/audit_events/types/user_destroyed.yml index b470133b6bf7b391ee89dd3cf6b819aabe0f1a2d..46afde330e2738d0b4b2c17d8ce05707a600808c 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 90738e211b6ee31ef6383ac096a5819a4690ce10..a59a230b610ea74d8230ed8c1b1d8f5a43b83db4 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: '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: "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 0000000000000000000000000000000000000000..d018582065e8c19fb46d8977eae9c9b4991e4380 --- /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 'for audit event' do + before do + project_bot.members.first.update_column(:expires_at, 1.second.ago) + end + + it "logs user_destroyed audit event linked to the project bot resource", :aggregate_failures do + expect { worker.perform }.to change { + AuditEvent.where("details LIKE ?", "%user_destroyed%").count + }.by(1) + + 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: 'user_destroyed', + author_class: author.class.to_s, + author_name: author.name, + custom_message: + "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 0000000000000000000000000000000000000000..8fb37097aba2bfc2bad687feacf7d2cef774f82a --- /dev/null +++ b/ee/spec/workers/resource_access_tokens/inactive_tokens_deletion_cron_worker_spec.rb @@ -0,0 +1,45 @@ +# 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 'for audit event' 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 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 ?", "%user_destroyed%").count + }.by(1) + + 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: 'user_destroyed', + author_class: author.class.to_s, + author_name: author.name, + custom_message: + "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 3f5845d51ea8a78f953693a0c28929cdc4710a02..930cee21499ebab732e6766e09bf257ac251d9d7 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 cec8fb3b902a7cb18b3794585976f3106318c1b8..96ac7ef7dc7ae1d71a2b1631acedf40d1102f53b 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 da5b41f18d6bf768c3c70ffb4e7cb3e28874eaad..1f5c41cd41fc003841b9dfd4dd180ae41c2480f8 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