Skip to content

Unconfirmed user deletion skips users who have signed in

Jessie Young requested to merge jy-refine-user-deletion-for-sign-in-count into master

What does this MR do and why?

  • If an instance has always had hard email confirmation turned on, then all users who are unconfirmed would have never signed in.
  • If an instance had email confirmed off or "soft" at some point in the past, there may be users who signed in and were active on the platform for a time even though they have never confirmed their email.
  • If a user wanted to sign in again, they would need to confirm their email. But we still want to remove any users with a sign_in_count > 0 here because we don't want to risk accidentally deleting dormant users rather than inactive users.
  • Refinement of worker added via !122600 (merged)
  • Internal discussion on this logic: #352514 (comment 1432041522)
  • This also adds some validations on how many days to wait after deleting unconfirmed users so that when email confirmation is set to 'soft' this setting isn't used to delete users who are still in the window to confirm their accounts.
  • This logic is all behind the delete_unconfirmed_users_setting feature flag

Changelog: changed

Database Migration

How to set up and validate locally

This requires a Premium license.

  1. In rails console enable feature flag and needed settings

    Feature.enable(:delete_unconfirmed_users_setting)
Gitlab::CurrentSettings.update(delete_unconfirmed_users: true)
Gitlab::CurrentSettings.update(unconfirmed_users_delete_after_days: 30)
Gitlab::CurrentSettings.update(email_confirmation_setting: 'hard')

  2. Create a user who is unconfirmed and created more than 30 days ago

    FactoryBot.create(:user, :unconfirmed, created_at: 1.year.ago, email: 'deleteme@example.com', username: 'unconfirmed_person')
FactoryBot.create(:user, :unconfirmed, created_at: 1.year.ago, email: 'donotdeleteme@example.com', username: 'unconfirmed_person_who_signed_in_once', sign_in_count: 1)

  3. Wait for the hourly cron job or kick it off manually

    Users::UnconfirmedUsersDeletionCronWorker.new.perform

  4. The user who never signed in should have a ghost migration user (these are processed async to delete the user)

    Users::GhostUserMigration.where(user_id: User.find_by_username('unconfirmed_person').id).first
=> present
Users::GhostUserMigration.where(user_id: User.find_by_username('unconfirmed_person_who_signed_in_once').id).first
=> nil

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 Jessie Young

Merge request reports