Skip to content
Snippets Groups Projects

Link project_bot user deletion audit event to its resource if possible

Merged Smriti Garg requested to merge smriti-492873/audit_events_for_bot_user_deletion into master
All threads resolved!

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 without current_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)

Screenshots or screen recordings

Before After
Screenshot_from_2024-09-11_16-41-37 Screenshot_from_2024-10-04_12-44-04

How to set up and validate locally

  1. Create project access token or group access token. (Under the hood it creates a user with project_bot type)
  2. Revoke the token or simulate token expiration.
  3. Visit the group/project's /audit_events page. You should see the event about the project_bot deletion with the reason in the message.
Edited by Bogdan Denkovych

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Smriti Garg added 1 commit

    added 1 commit

    • 2d3298e5 - Moved code to non-ee directory

    Compare with previous version

  • Smriti Garg added 1 commit

    added 1 commit

    Compare with previous version

  • Smriti Garg added 510 commits

    added 510 commits

    Compare with previous version

  • Smriti Garg added 1 commit

    added 1 commit

    Compare with previous version

  • Smriti Garg added 1 commit

    added 1 commit

    Compare with previous version

  • Smriti Garg added 1 commit

    added 1 commit

    Compare with previous version

  • added 1 commit

    • 6261f929 - Consolidate implementation of `project_bot_user_destroyed` audit event

    Compare with previous version

  • Ghost User
  • Ghost User
    • 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.
      • Rename bot_user_destroyed to project_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 use project_bot instead of bot. 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.

      Screenshot_from_2024-09-30_23-58-44

      Edited by Bogdan Denkovych
  • Bogdan Denkovych removed review request for @bdenkovych

    removed review request for @bdenkovych

  • Smriti Garg added 1 commit

    added 1 commit

    • b2dcb4d7 - Added condition to not to log any event for normal user in case current_user is nil

    Compare with previous version

  • Smriti Garg requested review from @atevans

    requested review from @atevans

  • Andrew Evans
  • Andrew Evans
  • Andrew Evans
  • Andrew Evans
  • Andrew Evans
    • 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 :smile_cat:

  • Andrew Evans approved this merge request

    approved this merge request

  • 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.

  • :tools: Generated by gitlab_quality-test_tooling.


    :snail: 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: :white_check_mark: test report for 2194b238

    expand 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: :x: test report for 6261f929

    expand 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   | ❌     |
    +--------+--------+--------+---------+-------+-------+--------+
  • Bogdan Denkovych changed title from Added audit event for tracking bot user deletion to Scope project_bot user deletion to its resource if it can be identified

    changed title from Added audit event for tracking bot user deletion to Scope project_bot user deletion to its resource if it can be identified

  • Bogdan Denkovych changed title from Scope project_bot user deletion to its resource if it can be identified to Link project_bot user deletion audit event to its resource if possible

    changed title from Scope project_bot user deletion to its resource if it can be identified to Link project_bot user deletion audit event to its resource if possible

  • mentioned in commit 5bcbd776

  • Bogdan Denkovych added 1499 commits

    added 1499 commits

    • 6261f929...245d6439 - 1498 commits from branch master
    • 5bcbd776 - Link project_bot user deletion audit event to its resource if possible

    Compare with previous version

  • mentioned in commit 2457f300

  • added 1 commit

    • 2457f300 - Link project_bot user deletion audit event to its resource if possible

    Compare with previous version

  • Bogdan Denkovych reset approvals from @atevans by pushing to the branch

    reset approvals from @atevans by pushing to the branch

  • I had to rebase the branch to be able to amend this MR.

  • mentioned in commit 50f8a53a

  • added 1 commit

    • 50f8a53a - Link project_bot user deletion audit event to its resource if possible

    Compare with previous version

  • added 1 commit

    • 2194b238 - Keep using `user_destroyed` event type for project bot deletion

    Compare with previous version

  • Bogdan Denkovych changed the description

    changed the description

  • Bogdan Denkovych changed the description

    changed the description

  • Bogdan Denkovych changed the description

    changed the description

  • requested review from @sam.figueroa

  • Bogdan Denkovych changed the description

    changed the description

  • mentioned in issue #488166 (closed)

  • Bogdan Denkovych changed the description

    changed the description

  • SAM FIGUEROA approved this merge request

    approved this merge request

  • SAM FIGUEROA removed review request for @sam.figueroa

    removed review request for @sam.figueroa

  • Andrew Evans approved this merge request

    approved this merge request

  • Andrew Evans resolved all threads

    resolved all threads

  • Andrew Evans enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • Andrew Evans added this merge request to the merge train at position 12

    added this merge request to the merge train at position 12

  • merged

  • Andrew Evans mentioned in commit 6f90cc6c

    mentioned in commit 6f90cc6c

  • Smriti Garg mentioned in commit cba1c715

    mentioned in commit cba1c715

  • added workflowstaging label and removed workflowcanary label

  • Smriti Garg mentioned in commit 9c644d5e

    mentioned in commit 9c644d5e

  • mentioned in issue #471683 (closed)

  • Bogdan Denkovych mentioned in merge request !171988 (merged)

    mentioned in merge request !171988 (merged)

  • Bogdan Denkovych mentioned in merge request !172375 (merged)

    mentioned in merge request !172375 (merged)

  • Please register or sign in to reply
    Loading