Link project_bot user deletion audit event to its resource if possible
What does this MR do and why?
Related to #488166 (closed)
Currently, when deleting user with any user type, GitLab creates user_destroyed
audit event. That audit event is linked to User
. Meaning that only instance admins can see that audit event.
The goal of this MR to allow resource(group/project) owners see user_destroyed
audit events that are related to the resource project_bot user deletion by linking that audit event to the resource if possible.
That MR also
- improves
user_destroyed
audit event creation by mentioning reason of the user deletion in the message - ensures
user_destroyed
audit event is created even withoutcurrent_user
- I think we and users could benefit from having audit events about user deletions without
current_user
(when user is deleted from rails console for instance). See !167021 (comment 2137711286)
- I think we and users could benefit from having audit events about user deletions without
Screenshots or screen recordings
Before | After |
---|---|
![]() |
![]() |
How to set up and validate locally
- Create project access token or group access token. (Under the hood it creates a user with project_bot type)
- Revoke the token or simulate token expiration.
- Visit the group/project's
/audit_events
page. You should see the event about the project_bot deletion with the reason in the message.
Merge request reports
Activity
assigned to @sgarg_gitlab
added pipelinetier-1 label
- A deleted user
added backend documentation featureaddition typefeature labels
1 Warning 2194b238: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
-
doc/user/compliance/audit_event_types.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Category Reviewer Maintainer backend @adie.po
(UTC+0, 5.5 hours behind author)
@minac
(UTC+2, 3.5 hours behind author)
groupauthentication Reviewer review is optional for groupauthentication @eduardosanz
(UTC+2, 3.5 hours behind author)
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger-
changed milestone to %16.5
added groupauthentication maintenanceusability typemaintenance labels and removed typefeature label
removed featureaddition label
added devopsgovern sectionsec labels
changed milestone to %17.5
requested review from @bdenkovych
- Resolved by Bogdan Denkovych
@bdenkovych Could you please provide this MR first backend review. I tried addressing your comments from earlier MR !165938 (comment 2102148093) here.
mentioned in merge request !165938 (closed)
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
added 510 commits
-
5e203150...dd373570 - 502 commits from branch
master
- 29eeed2e - Added audit event for tracking bot user deletion
- 45073650 - Corrected reason placement
- ac4e7e34 - Message string split to avoid rubocop error
- 4e096e3f - Corrected issue and MR link
- 235aaef0 - Fix for foss pipeline failure
- 01bd274a - Moved code to non-ee directory
- d6db863b - Fix for spec failures
- 40984298 - Review comments included
Toggle commit list-
5e203150...dd373570 - 502 commits from branch
added 1 commit
- 6261f929 - Consolidate implementation of `project_bot_user_destroyed` audit event
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
@sgarg_gitlab As we discussed on Slack, for efficiency, I pushed commit to this MR with my suggestions to address concerns I've had regarding implementation. Please improve it further, add more specs or consider reverting changes you disagree with.
The main points of the changes:
-
User account deactivation audit events, specifically existing
user_destroyed
event we "inherit", are EE feature. We should ensure that we create audit event with the new type only in EE. - Since we can't create the audit event with the new type when project_bot is orphaned record because it should be scoped to Group|Project, we should retain creation of
user_destroyed
instance event for those records.- For that reason, it is better to keep code close that create
user_destroyed
event and the new event.
- For that reason, it is better to keep code close that create
- Rename
bot_user_destroyed
toproject_bot_user_destroyed
. Since there are lots of bot user types and general name could make impression that this event is used for all bot user types. I see in the table there are lots of existing event names that useproject_bot
instead ofbot
. I think for the same reason. - More thorough specs and move specs that test creation of new audit event to ee/.
Please review this latest version of the MR, amend it further and pass it to another groupauthentication member for final review since I've pushed the git-commit to this MR and my approval won't count.
Edited by Bogdan Denkovych -
User account deactivation audit events, specifically existing
removed review request for @bdenkovych
assigned to @bdenkovych
added 1 commit
- b2dcb4d7 - Added condition to not to log any event for normal user in case current_user is nil
requested review from @atevans
- Resolved by Andrew Evans
- Resolved by Andrew Evans
- Resolved by Andrew Evans
- Resolved by Andrew Evans
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Bogdan Denkovych
- Resolved by Andrew Evans
I was able to verify the functionality locally for the revoked token, membership expired, and inactive token cron worker events. Overall the code looks good, and the new audit event will be an improvement for users. I left some minor suggestions, but do not consider them blocking. If y'all decide to make changes based on the suggestions, please re-ping me for re-approval
added pipeline:mr-approved label
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-1 label
Before you set this MR to auto-merge
This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.
Before you set this MR to auto-merge, please check the following:
- You are the last maintainer of this merge request
- The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
- This pipeline is recent enough (created in the last 8 hours)
If all the criteria above apply, please set auto-merge for this merge request.
See pipeline tiers and merging a merge request for more details.
Generated bygitlab_quality-test_tooling
.
Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.Click to expand
Job File Name Duration Expected duration #7979418350 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.42 s < 50.13 s #7985129112 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.38 s < 50.13 s #7987125074 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.26 s < 50.13 s #7993932767 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.48 s < 50.13 s #7994931670 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 69.67 s < 50.13 s #7998126589 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.75 s < 50.13 s #8025165955 spec/features/admin/users/users_spec.rb#L177
Admin::Users GET /admin/users when blocking/unblocking a user shows confirmation and allows blocking and unblocking 66.45 s < 50.13 s - A deleted user
added rspec:slow test detected label
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 2194b238expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Verify | 5 | 0 | 0 | 0 | 5 | ✅ | | Plan | 4 | 0 | 0 | 0 | 4 | ✅ | | Govern | 17 | 0 | 0 | 0 | 17 | ✅ | | Data Stores | 13 | 0 | 0 | 0 | 13 | ✅ | | Package | 1 | 0 | 0 | 0 | 1 | ✅ | | Analytics | 1 | 0 | 0 | 0 | 1 | ✅ | | Create | 3 | 0 | 0 | 0 | 3 | ✅ | | Secure | 2 | 0 | 0 | 0 | 2 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 46 | 0 | 0 | 0 | 46 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-test-on-omnibus:
test report for 6261f929expand test summary
+-------------------------------------------------------------+ | suites summary | +--------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +--------+--------+--------+---------+-------+-------+--------+ | Govern | 107 | 1 | 6 | 1 | 114 | ❌ | | Create | 408 | 0 | 60 | 0 | 468 | ✅ | +--------+--------+--------+---------+-------+-------+--------+ | Total | 515 | 1 | 66 | 1 | 582 | ❌ | +--------+--------+--------+---------+-------+-------+--------+
removed pipeline:run-e2e-omnibus-once label
mentioned in commit 5bcbd776
added 1499 commits
-
6261f929...245d6439 - 1498 commits from branch
master
- 5bcbd776 - Link project_bot user deletion audit event to its resource if possible
-
6261f929...245d6439 - 1498 commits from branch
mentioned in commit 2457f300
added 1 commit
- 2457f300 - Link project_bot user deletion audit event to its resource if possible
reset approvals from @atevans by pushing to the branch
mentioned in commit 50f8a53a
added 1 commit
- 50f8a53a - Link project_bot user deletion audit event to its resource if possible
added 1 commit
- 2194b238 - Keep using `user_destroyed` event type for project bot deletion
requested review from @sam.figueroa
mentioned in issue #488166 (closed)
removed review request for @sam.figueroa
added this merge request to the merge train at position 12
mentioned in commit 6f90cc6c
mentioned in commit cba1c715
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
mentioned in commit 9c644d5e
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in issue #471683 (closed)
mentioned in merge request kubitus-project/kubitus-installer!3429 (merged)
mentioned in merge request !171988 (merged)
mentioned in merge request !172375 (merged)