From 99aecbaacd332abc7f3a94adc7b07fa3a6995955 Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Tue, 30 Apr 2024 18:38:47 +0300 Subject: [PATCH] Delete users unconfirmed secondary emails after 3 days Related to https://gitlab.com/gitlab-org/gitlab/-/issues/367823 Changelog: added --- app/mailers/previews/devise_mailer_preview.rb | 7 ++ app/models/application_setting.rb | 2 + app/models/email.rb | 2 + ...firmation_instructions_secondary.html.haml | 2 + ...nfirmation_instructions_secondary.text.erb | 2 + app/workers/all_queues.yml | 9 ++ ...d_secondary_emails_deletion_cron_worker.rb | 29 +++++++ config/initializers/1_settings.rb | 3 + ...n_created_at_where_confirmed_at_is_null.rb | 17 ++++ db/schema_migrations/20240501103038 | 1 + db/structure.sql | 2 + doc/user/profile/index.md | 4 + locale/gitlab.pot | 3 + spec/mailers/devise_mailer_spec.rb | 33 ++++++++ spec/models/application_setting_spec.rb | 6 ++ spec/models/email_spec.rb | 65 ++++++++++++-- ...ondary_emails_deletion_cron_worker_spec.rb | 84 +++++++++++++++++++ 17 files changed, 265 insertions(+), 6 deletions(-) create mode 100644 app/workers/users/unconfirmed_secondary_emails_deletion_cron_worker.rb create mode 100644 db/migrate/20240501103038_index_emails_on_created_at_where_confirmed_at_is_null.rb create mode 100644 db/schema_migrations/20240501103038 create mode 100644 spec/workers/users/unconfirmed_secondary_emails_deletion_cron_worker_spec.rb diff --git a/app/mailers/previews/devise_mailer_preview.rb b/app/mailers/previews/devise_mailer_preview.rb index 53f960e64a7cad..3fbee8fc3ffbad 100644 --- a/app/mailers/previews/devise_mailer_preview.rb +++ b/app/mailers/previews/devise_mailer_preview.rb @@ -12,6 +12,13 @@ def confirmation_instructions_for_new_email DeviseMailer.confirmation_instructions(user, 'faketoken', {}) end + def confirmation_instructions_for_secondary_email + user = User.last + secondary_email = user.emails.build(email: 'unconfirmed@example.com') + + DeviseMailer.confirmation_instructions(secondary_email, 'faketoken', {}) + end + def reset_password_instructions DeviseMailer.reset_password_instructions(unsaved_user, 'faketoken', {}) end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index e26ae717278a42..754841a11d1636 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -35,6 +35,8 @@ class ApplicationSetting < MainClusterwide::ApplicationRecord PACKAGE_REGISTRY_SETTINGS = [:nuget_skip_metadata_url_validation].freeze + USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS = 3 + enum whats_new_variant: { all_tiers: 0, current_tier: 1, disabled: 2 }, _prefix: true enum email_confirmation_setting: { off: 0, soft: 1, hard: 2 }, _prefix: true diff --git a/app/models/email.rb b/app/models/email.rb index 5fca57520b8d07..2fc7036967367b 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -11,6 +11,8 @@ class Email < MainClusterwide::ApplicationRecord validate :unique_email, if: ->(email) { email.email_changed? } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :unconfirmed, -> { where(confirmed_at: nil) } + scope :unconfirmed_and_created_before, ->(created_cut_off) { unconfirmed.where('created_at < ?', created_cut_off) } after_commit :update_invalid_gpg_signatures, if: -> { previous_changes.key?('confirmed_at') } diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml index 97fdf0249da4d4..1576bdf4d0ee34 100644 --- a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml +++ b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml @@ -6,3 +6,5 @@ %p = _('If this email was added in error, you can remove it here:') = link_to _("Emails"), profile_emails_url + %p + = format(_('Confirm this email address within %{cut_off_days} days, otherwise the email address is removed.'), cut_off_days: ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS) diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb index 32e88047a9c922..b6f409e2919cb7 100644 --- a/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb +++ b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb @@ -5,3 +5,5 @@ <%= confirmation_url(@resource, confirmation_token: @token) %> <%= _("If this email was added in error, you can remove it here: %{profile_emails_url}") % { profile_emails_url: profile_emails_url } %> + +<%= format(_('Confirm this email address within %{cut_off_days} days, otherwise the email address is removed.'), cut_off_days: ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS) %> diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 7a3c7037cffee5..6da75945c8f4e8 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -975,6 +975,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: cronjob:users_unconfirmed_secondary_emails_deletion_cron + :worker_name: Users::UnconfirmedSecondaryEmailsDeletionCronWorker + :feature_category: :user_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:x509_issuer_crl_check :worker_name: X509IssuerCrlCheckWorker :feature_category: :source_code_management diff --git a/app/workers/users/unconfirmed_secondary_emails_deletion_cron_worker.rb b/app/workers/users/unconfirmed_secondary_emails_deletion_cron_worker.rb new file mode 100644 index 00000000000000..205233fae7190f --- /dev/null +++ b/app/workers/users/unconfirmed_secondary_emails_deletion_cron_worker.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Users + class UnconfirmedSecondaryEmailsDeletionCronWorker + include ApplicationWorker + include CronjobQueue # rubocop:disable Scalability/CronWorkerContext -- This worker does not perform work scoped to a context + + deduplicate :until_executed + idempotent! + data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- This is a cron job + feature_category :user_management + + BATCH_SIZE = 1000 + + def perform + loop do + records_deleted = Email.unconfirmed_and_created_before(created_cut_off).limit(BATCH_SIZE).delete_all + + break if records_deleted == 0 + end + end + + private + + def created_cut_off + ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS.days.ago + end + end +end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index a8bcdf10fee106..400b2904426564 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -870,6 +870,9 @@ Settings.cron_jobs['users_delete_unconfirmed_users_worker'] ||= {} Settings.cron_jobs['users_delete_unconfirmed_users_worker']['cron'] ||= '0 * * * *' Settings.cron_jobs['users_delete_unconfirmed_users_worker']['job_class'] = 'Users::UnconfirmedUsersDeletionCronWorker' + Settings.cron_jobs['users_unconfirmed_secondary_emails_deletion_cron_worker'] ||= {} + Settings.cron_jobs['users_unconfirmed_secondary_emails_deletion_cron_worker']['cron'] ||= '0 * * * *' + Settings.cron_jobs['users_unconfirmed_secondary_emails_deletion_cron_worker']['job_class'] = 'Users::UnconfirmedSecondaryEmailsDeletionCronWorker' Settings.cron_jobs['package_metadata_advisories_sync_worker'] ||= {} Settings.cron_jobs['package_metadata_advisories_sync_worker']['cron'] ||= "*/5 * * * *" Settings.cron_jobs['package_metadata_advisories_sync_worker']['job_class'] = 'PackageMetadata::AdvisoriesSyncWorker' diff --git a/db/migrate/20240501103038_index_emails_on_created_at_where_confirmed_at_is_null.rb b/db/migrate/20240501103038_index_emails_on_created_at_where_confirmed_at_is_null.rb new file mode 100644 index 00000000000000..105e1babc98d51 --- /dev/null +++ b/db/migrate/20240501103038_index_emails_on_created_at_where_confirmed_at_is_null.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class IndexEmailsOnCreatedAtWhereConfirmedAtIsNull < Gitlab::Database::Migration[2.2] + milestone '17.0' + + disable_ddl_transaction! + + INDEX_NAME = 'index_emails_on_created_at_where_confirmed_at_is_null' + + def up + add_concurrent_index :emails, :created_at, where: 'confirmed_at IS NULL', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :emails, name: INDEX_NAME + end +end diff --git a/db/schema_migrations/20240501103038 b/db/schema_migrations/20240501103038 new file mode 100644 index 00000000000000..96f07c3363af0c --- /dev/null +++ b/db/schema_migrations/20240501103038 @@ -0,0 +1 @@ +393c51beee226eadf2e9d4c5142669e2efb8f7a445bd18d6e6121727d241d9ac \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index b20061c7731b61..cbcccb6e78d4e2 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25517,6 +25517,8 @@ CREATE INDEX index_elasticsearch_indexed_namespaces_on_created_at ON elasticsear CREATE UNIQUE INDEX index_emails_on_confirmation_token ON emails USING btree (confirmation_token); +CREATE INDEX index_emails_on_created_at_where_confirmed_at_is_null ON emails USING btree (created_at) WHERE (confirmed_at IS NULL); + CREATE UNIQUE INDEX index_emails_on_email ON emails USING btree (email); CREATE INDEX index_emails_on_user_id ON emails USING btree (user_id); diff --git a/doc/user/profile/index.md b/doc/user/profile/index.md index 2c5e9ee0e07d40..9cb0468d710c09 100644 --- a/doc/user/profile/index.md +++ b/doc/user/profile/index.md @@ -76,12 +76,16 @@ NOTE: ## Delete emails from your user profile +> - Automatic deletion of unverified secondary email addresses [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151562) in GitLab 17.0. + You can delete a secondary email address from your account. You cannot delete your primary email address. If the deleted email address is used for any user emails, those user emails are sent to the primary email address instead. +Unverified secondary email addresses are automatically deleted after three days. + NOTE: Because of [issue 438600](https://gitlab.com/gitlab-org/gitlab/-/issues/438600), group notifications are still sent to the deleted email address. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 60c05b095b12e4..0a2f4cb53c3690 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -13711,6 +13711,9 @@ msgstr "" msgid "Confirm password" msgstr "" +msgid "Confirm this email address within %{cut_off_days} days, otherwise the email address is removed." +msgstr "" + msgid "Confirm user" msgstr "" diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index f5813ea7f200cb..c5636bb5022079 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -54,6 +54,39 @@ expect(subject.body.encoded).to have_text user.email end end + + context 'for secondary email' do + let(:secondary_email) { create(:email) } + + subject { described_class.confirmation_instructions(secondary_email, 'faketoken', opts) } + + it_behaves_like 'it validates recipients' + + it 'has the correct subject and body', :aggregate_failures do + is_expected.to have_subject I18n.t('devise.mailer.confirmation_instructions.subject') + + is_expected.to have_text_part_content( + format(_("%{name}, confirm your email address now!"), name: secondary_email.user.name) + ) + is_expected.to have_html_part_content( + format(_("%{name}, confirm your email address now!"), name: secondary_email.user.name) + ) + + is_expected.to have_text_part_content( + secondary_email.email + ) + is_expected.to have_html_part_content( + secondary_email.email + ) + + is_expected.to have_text_part_content( + format(_('Confirm this email address within %{cut_off_days} days, otherwise the email address is removed.'), cut_off_days: ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS) + ) + is_expected.to have_html_part_content( + format(_('Confirm this email address within %{cut_off_days} days, otherwise the email address is removed.'), cut_off_days: ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS) + ) + end + end end describe '#password_change_by_admin' do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index fe79b36e9c987d..1eb98d90467daf 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -37,6 +37,12 @@ it { expect(setting.nuget_skip_metadata_url_validation).to eq(false) } end + describe 'USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS' do + subject { described_class::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS } + + it { is_expected.to eq(3) } + end + describe 'validations' do let(:http) { 'http://example.com' } let(:https) { 'https://example.com' } diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb index ecb8f72470daa0..b73e525a324fd7 100644 --- a/spec/models/email_spec.rb +++ b/spec/models/email_spec.rb @@ -42,14 +42,67 @@ end describe 'scopes' do - let(:user) { create(:user, :unconfirmed) } + let_it_be(:unconfirmed_user) { create(:user, :unconfirmed) } + let_it_be(:confirmed_user) { create(:user) } + + let_it_be(:unconfirmed_primary_email) { unconfirmed_user.email } + let_it_be(:confirmed_primary_email) { described_class.find_by_email(confirmed_user.email) } + + let_it_be(:unconfirmed_secondary_email) { create(:email, user: confirmed_user) } + let_it_be(:confirmed_secondary_email) { create(:email, :confirmed, user: confirmed_user) } + + describe '.confirmed' do + it 'returns confirmed emails' do + expect(described_class.confirmed).to contain_exactly( + # after user's primary email is confirmed it is stored to 'emails' table + confirmed_primary_email, + confirmed_secondary_email + ) + end + end + + describe '.unconfirmed' do + it 'returns unconfirmed secondary emails' do + expect(described_class.unconfirmed).to contain_exactly( + # excludes `unconfirmed_primary_email` because + # user's primary email is not stored to 'emails' table till it is confirmed + unconfirmed_secondary_email + ) + end + end + + describe '.unconfirmed_and_created_before' do + let(:created_cut_off) { 3.days.ago } + + let!(:unconfirmed_secondary_email_created_before_cut_off) do + create(:email, created_at: created_cut_off - 1.second) + end + + let!(:unconfirmed_secondary_email_created_at_cut_off) do + create(:email, created_at: created_cut_off) + end - it 'scopes confirmed emails' do - create(:email, :confirmed, user: user) - create(:email, user: user) + let!(:unconfirmed_secondary_email_created_after_cut_off) do + create(:email, created_at: created_cut_off + 1.second) + end - expect(user.emails.count).to eq 2 - expect(user.emails.confirmed.count).to eq 1 + let!(:confirmed_secondary_email_created_before_cut_off) do + create(:email, :confirmed, created_at: created_cut_off - 1.second) + end + + let!(:confirmed_secondary_email_created_at_cut_off) do + create(:email, :confirmed, created_at: created_cut_off) + end + + let!(:confirmed_secondary_email_created_after_cut_off) do + create(:email, :confirmed, created_at: created_cut_off + 1.second) + end + + it 'returns unconfirmed secondary emails created before timestamp passed in' do + expect(described_class.unconfirmed_and_created_before(created_cut_off)).to contain_exactly( + unconfirmed_secondary_email_created_before_cut_off + ) + end end end diff --git a/spec/workers/users/unconfirmed_secondary_emails_deletion_cron_worker_spec.rb b/spec/workers/users/unconfirmed_secondary_emails_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000000..cd6ede8ce2e5d0 --- /dev/null +++ b/spec/workers/users/unconfirmed_secondary_emails_deletion_cron_worker_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::UnconfirmedSecondaryEmailsDeletionCronWorker, feature_category: :user_management do + subject(:worker) { described_class.new } + + it_behaves_like 'an idempotent worker' + + describe '#perform' do + it 'deletes unconfirmed secondary emails created before the cutoff', :aggregate_failures, :freeze_time do + cut_off = ApplicationSetting::USERS_UNCONFIRMED_SECONDARY_EMAILS_DELETE_AFTER_DAYS.days.ago + + unconfirmed_secondary_email_created_before_cut_off_1 = create( + :email, + created_at: cut_off - 1.second + ) + + unconfirmed_secondary_email_created_before_cut_off_2 = create( + :email, + created_at: cut_off - 1.day + ) + + unconfirmed_secondary_email_created_at_cut_off = create( + :email, + created_at: cut_off + ) + + unconfirmed_secondary_email_created_after_cut_off_1 = create( + :email, + created_at: cut_off + 1.second + ) + + unconfirmed_secondary_email_created_after_cut_off_2 = create( + :email, + created_at: cut_off + 1.day + ) + + confirmed_secondary_email_created_before_cut_off_1 = create( + :email, + :confirmed, + created_at: cut_off - 1.second + ) + + confirmed_secondary_email_created_before_cut_off_2 = create( + :email, + :confirmed, + created_at: cut_off - 1.day + ) + + confirmed_secondary_email_created_at_cut_off = create( + :email, + :confirmed, + created_at: cut_off + ) + + confirmed_secondary_email_created_after_cut_off_1 = create( + :email, + :confirmed, + created_at: cut_off + 1.second + ) + + confirmed_secondary_email_created_after_cut_off_2 = create( + :email, + :confirmed, + created_at: cut_off + 1.day + ) + + expect { worker.perform }.to change { Email.count }.by(-2) + + expect(Email.exists?(unconfirmed_secondary_email_created_before_cut_off_1.id)).to eq(false) + expect(Email.exists?(unconfirmed_secondary_email_created_before_cut_off_2.id)).to eq(false) + expect(Email.exists?(unconfirmed_secondary_email_created_at_cut_off.id)).to eq(true) + expect(Email.exists?(unconfirmed_secondary_email_created_after_cut_off_1.id)).to eq(true) + expect(Email.exists?(unconfirmed_secondary_email_created_after_cut_off_2.id)).to eq(true) + + expect(Email.exists?(confirmed_secondary_email_created_before_cut_off_1.id)).to eq(true) + expect(Email.exists?(confirmed_secondary_email_created_before_cut_off_2.id)).to eq(true) + expect(Email.exists?(confirmed_secondary_email_created_at_cut_off.id)).to eq(true) + expect(Email.exists?(confirmed_secondary_email_created_after_cut_off_1.id)).to eq(true) + expect(Email.exists?(confirmed_secondary_email_created_after_cut_off_2.id)).to eq(true) + end + end +end -- GitLab