Skip to content

Skip delayed own user deletion when the user has been unblocked

Eugie Limpin requested to merge el-skip-own-account-deletion-when-user-is-unblocked into master

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

Change when user deletes their own account (User#delete_async)

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.

Changes when the job (DeleteUserWorker) to delete the user record runs after the delay (7 days)

In addition to skipping deletion when the user was banned, it now also

  1. 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.
  2. 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

  1. Update app/models/user.rb as follows to make testing easier

    index 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?

  2. 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)

  3. Tail application_json.log

    $ tail -f log/application_json.log

  4. Sign in with the new user, go to http://localhost:3000/-/profile/account and delete the account

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

  6. Go to http://localhost:3000/admin/sidekiq/scheduled and confirm that a DeleteUserWorker job is scheduled with the id of the user you deleted Screenshot_2023-11-15_at_12.37.04_PM

  7. Wait for the job to get executed

  8. Go back to http://localhost:3000/admin/users/todelete1115 and validate that the user still exists (page doesn't return 404)

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

Edited by Eugie Limpin

Merge request reports