Skip delayed own user deletion when the user has been unblocked
What does this MR do and why?
This MR resolves https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/423+.
It updates what happens when a user deletes their own account (User#delete_async
) and what happens when the job (DeleteUserWorker
) to delete the user record runs after the delay (7 days).
User#delete_async
)
Change when user deletes their own account (In addition to blocking the user, a custom attribute is now created (key: deleted_own_account_at
, value: Time.zone.now.to_s
) for the user. This record is used in DeleteUserWorker
to identify users who initiated deletion of their own accounts.
DeleteUserWorker
) to delete the user record runs after the delay (7 days)
Changes when the job (In addition to skipping deletion when the user was banned, it now also
- Skips deletion when the user deleted their own account AND they are not blocked. This happens when the user is unblocked before the job ran, for example, as part of Trust & Safety team Account Reinstatement process.
- Destroys the
deleted_own_account_at
custom attribute and creates a (key:skippped_account_deletion_at
, value:Time.zone.now.to_s
) custom attribute for the user.
Sidekiq Compatibility across Updates
header | header |
---|---|
An older version of the application publishes a job, which is executed by an upgraded Sidekiq node. | No deleted_own_account_at custom attribute was created for the user so user.deleted_own_account? will return false and deletion will proceed. Same behavior as the one before this MR. |
A job is queued before an upgrade, but executed after an upgrade. | Same as above |
A job is queued by a node running the newer version of the application, but executed on a node running an older version of the application | A deleted_own_account_at custom attribute is created for the user. Old code executes (return if delete_user.banned? && ::Feature.enabled?(:delay_delete_own_user) ) which will make the deletion proceed. Same behavior as the one before this MR. |
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
-
Update
app/models/user.rb
as follows to make testing easierindex 665c14760895..43a075d4aead 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1639,7 +1639,7 @@ def remove_key_cache end # rubocop: enable CodeReuse/ServiceClass - DELETION_DELAY_IN_DAYS = 7.days + DELETION_DELAY_IN_DAYS = 2.minutes def delete_async(deleted_by:, params: {}) if should_delay_delete?(deleted_by) @@ -2325,8 +2325,8 @@ def should_delay_delete?(deleted_by) is_deleting_own_record = deleted_by.id == id is_deleting_own_record && - ::Feature.enabled?(:delay_delete_own_user) && - has_possible_spam_contributions? + ::Feature.enabled?(:delay_delete_own_user) # && + # has_possible_spam_contributions? end def pbkdf2?
-
Enable FF and create a user to use for testing
$ rails c > Feature.enable(:delay_delete_own_user) => true > FactoryBot.create(:user, username: 'todelete1115', email: 'todelete1115@ex.com', password: 'strongpassword1', confirmed_at: Time.now)
-
Tail
application_json.log
$ tail -f log/application_json.log
-
Sign in with the new user, go to http://localhost:3000/-/profile/account and delete the account
-
Login with
root
(admin), go to http://localhost:3000/admin/users/todelete1115 (or w/e username you used in step 2), and unblock the user -
Go to http://localhost:3000/admin/sidekiq/scheduled and confirm that a
DeleteUserWorker
job is scheduled with the id of the user you deleted -
Wait for the job to get executed
-
Go back to http://localhost:3000/admin/users/todelete1115 and validate that the user still exists (page doesn't return 404)
-
Validate that the following is logged in
log/application_json.log
{"severity":"INFO","time":"2023-11-15T04:38:41.629Z","correlation_id":"561ad5dfea84a964e20c24fa2575b8fe","meta.caller_id":"DeleteUserWorker","meta.remote_ip":"127.0.0.1","meta.feature_category":"user_management","meta.user":"todelete1115","meta.user_id":79,"meta.client_id":"user/79","meta.root_caller_id":"RegistrationsController#destroy","message":"Skipped delayed own account deletion.","reason":"User has been unblocked."}
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.