diff --git a/app/mailers/previews/devise_mailer_preview.rb b/app/mailers/previews/devise_mailer_preview.rb index 53f960e64a7cadc1661688c8d8ccf4442b872297..3fbee8fc3ffbad7953d5b7851376ec042d0c8827 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 e26ae717278a42418e2833243004790a8cbec2ae..754841a11d1636dd6364fd17316528dd18dce320 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 5fca57520b8d0777e2469ca0ef1ffda067e72e61..2fc7036967367b36c45af77482a00d941ffe5e09 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 97fdf0249da4d44a3aae83ed8a1d74f2eb9b4ff2..1576bdf4d0ee347265e7ea588e4c6b79cacb3ee2 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 32e88047a9c922c8a1df17e135ce1b3cfdf5d6e5..b6f409e2919cb7372eb321f75410a60f2f500372 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 7a3c7037cffee5211993443ad45bfe40af042bb7..6da75945c8f4e8e20905c6b72671151f74c85449 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 0000000000000000000000000000000000000000..205233fae7190fb0e2a24b2b9457bfc8a711202d --- /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 a8bcdf10fee10639cd5bbead8a4c223bdbcbe45d..400b2904426564b3cdb79825fe5eb4d877870711 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 0000000000000000000000000000000000000000..105e1babc98d51eb4ee31b8c4de3e95e91b80332 --- /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 0000000000000000000000000000000000000000..96f07c3363af0c5f6440851f21b41cfa294443fa --- /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 b20061c7731b61dc8db8fddac292a50496758552..cbcccb6e78d4e2de837b8b95c450687ba6a97796 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 2c5e9ee0e07d4058d7f1f726a58f18e9b273b24b..9cb0468d710c09d2888a2b977f22be1aeee5fbcc 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 60c05b095b12e47abf66f27160e324d415200cf1..0a2f4cb53c3690da4376a5f8b3ce33fd7f757695 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 f5813ea7f200cba8273bbb108d748a7c16c39865..c5636bb5022079c64b1987c7aa49d6a30ac19199 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 fe79b36e9c987d98b549ccefadf71c99cda304a6..1eb98d90467daf9e100434f87c0332c47c658b0a 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 ecb8f72470daa0a23d73cf2f877014838b46620e..b73e525a324fd719b1dab4ac15c631e1dc0aee52 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 0000000000000000000000000000000000000000..cd6ede8ce2e5d0e8417e4e413f572e90e128d6fa --- /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