diff --git a/app/controllers/concerns/unconfirmed_user_concern.rb b/app/controllers/concerns/unconfirmed_user_concern.rb index 886765b3a6b..0ce93067cba 100644 --- a/app/controllers/concerns/unconfirmed_user_concern.rb +++ b/app/controllers/concerns/unconfirmed_user_concern.rb @@ -1,24 +1,39 @@ module UnconfirmedUserConcern - def with_unconfirmed_user + def find_user_with_confirmation_token @confirmation_token = params[:confirmation_token] @user = User.find_or_initialize_with_error_by(:confirmation_token, @confirmation_token) - @user = User.confirm_by_token(@confirmation_token) if @user.confirmed? - @password_form = PasswordForm.new(@user) + end + + def confirm_user_needs_sign_up_confirmation + return unless @user&.confirmed? + process_already_confirmed_user + end - yield if block_given? + def process_already_confirmed_user + track_user_already_confirmed_event + action_text = t('devise.confirmations.sign_in') unless user_signed_in? + flash[:error] = t('devise.confirmations.already_confirmed', action: action_text) + redirect_to user_signed_in? ? account_url : new_user_session_url + end + + def track_user_already_confirmed_event + hash = { + success: false, + errors: { email: [t('errors.messages.already_confirmed')] }, + user_id: @user.uuid, + } + analytics.track_event(Analytics::USER_REGISTRATION_EMAIL_CONFIRMATION, hash) end def validate_token - with_unconfirmed_user do - result = EmailConfirmationTokenValidator.new(@user).submit + result = EmailConfirmationTokenValidator.new(@user).submit - analytics.track_event(Analytics::USER_REGISTRATION_EMAIL_CONFIRMATION, result.to_h) + analytics.track_event(Analytics::USER_REGISTRATION_EMAIL_CONFIRMATION, result.to_h) - if result.success? - process_successful_confirmation - else - process_unsuccessful_confirmation - end + if result.success? + process_successful_confirmation + else + process_unsuccessful_confirmation end end @@ -31,41 +46,14 @@ def process_valid_confirmation_token session[:user_confirmation_token] = @confirmation_token end - def process_confirmed_user - create_user_event(:email_changed, @user) - - flash[:success] = t('devise.confirmations.confirmed') - redirect_to after_confirmation_url_for(@user) - EmailNotifier.new(@user).send_email_changed_email - end - - def after_confirmation_url_for(user) - if !user_signed_in? - new_user_session_url - elsif MfaPolicy.new(user, user_session[:signing_up]).sufficient_factors_enabled? - account_url - else - two_factor_options_url - end - end - def process_unsuccessful_confirmation - return process_already_confirmed_user if @user.confirmed? - @confirmation_token = params[:confirmation_token] flash[:error] = unsuccessful_confirmation_error redirect_to sign_up_email_resend_url(request_id: params[:_request_id]) end - def process_already_confirmed_user - action_text = t('devise.confirmations.sign_in') unless user_signed_in? - flash[:error] = t('devise.confirmations.already_confirmed', action: action_text) - - redirect_to user_signed_in? ? account_url : new_user_session_url - end - def unsuccessful_confirmation_error - if @user.confirmation_period_expired? + if @user&.confirmation_period_expired? @user.decorate.confirmation_period_expired_error else t('errors.messages.confirmation_invalid_token') diff --git a/app/controllers/sign_up/email_confirmations_controller.rb b/app/controllers/sign_up/email_confirmations_controller.rb index 62b85e0aa73..a8a790e8d1e 100644 --- a/app/controllers/sign_up/email_confirmations_controller.rb +++ b/app/controllers/sign_up/email_confirmations_controller.rb @@ -2,6 +2,9 @@ module SignUp class EmailConfirmationsController < ApplicationController include UnconfirmedUserConcern + before_action :find_user_with_confirmation_token + before_action :confirm_user_needs_sign_up_confirmation + def create validate_token rescue ActiveRecord::RecordNotUnique @@ -11,15 +14,11 @@ def create private def process_successful_confirmation - if !@user.confirmed? - process_valid_confirmation_token - request_id = params.fetch(:_request_id, '') - redirect_to sign_up_enter_password_url( - request_id: request_id, confirmation_token: @confirmation_token, - ) - else - process_confirmed_user - end + process_valid_confirmation_token + request_id = params.fetch(:_request_id, '') + redirect_to sign_up_enter_password_url( + request_id: request_id, confirmation_token: @confirmation_token, + ) end end end diff --git a/app/controllers/sign_up/passwords_controller.rb b/app/controllers/sign_up/passwords_controller.rb index f101f04855b..d84a02f292d 100644 --- a/app/controllers/sign_up/passwords_controller.rb +++ b/app/controllers/sign_up/passwords_controller.rb @@ -2,33 +2,31 @@ module SignUp class PasswordsController < ApplicationController include UnconfirmedUserConcern + before_action :find_user_with_confirmation_token + before_action :confirm_user_needs_sign_up_confirmation + def new + password_form # Memoize the password form to use in the view validate_token end def create - with_unconfirmed_user do - result = @password_form.submit(permitted_params) - analytics.track_event(Analytics::PASSWORD_CREATION, result.to_h) - store_sp_metadata_in_session unless sp_request_id.empty? + result = password_form.submit(permitted_params) + analytics.track_event(Analytics::PASSWORD_CREATION, result.to_h) + store_sp_metadata_in_session unless sp_request_id.empty? - if result.success? - process_successful_password_creation - else - process_unsuccessful_password_creation - end + if result.success? + process_successful_password_creation + else + process_unsuccessful_password_creation end end private def process_successful_confirmation - if !@user.confirmed? - process_valid_confirmation_token - render_page - else - process_confirmed_user - end + process_valid_confirmation_token + render_page end def render_page @@ -58,6 +56,10 @@ def store_sp_metadata_in_session StoreSpMetadataInSession.new(session: session, request_id: sp_request_id).call end + def password_form + @password_form ||= PasswordForm.new(@user) + end + def sp_request_id permitted_params.fetch(:request_id, '') end @@ -70,7 +72,7 @@ def process_unsuccessful_password_creation def sign_in_and_redirect_user sign_in @user user_session[:signing_up] = true - redirect_to after_confirmation_url_for(@user) + redirect_to two_factor_options_url end end end diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 131adedc8fd..69bc5ac1293 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -6,10 +6,6 @@ class UserMailer < ActionMailer::Base default from: email_with_name(Figaro.env.email_from, Figaro.env.email_from), reply_to: email_with_name(Figaro.env.email_from, Figaro.env.email_from) - def email_changed(old_email) - mail(to: old_email, subject: t('mailer.email_change_notice.subject')) - end - # :reek:ControlParameter # :reek:LongParameterList # :reek:TooManyStatements diff --git a/app/services/email_confirmation_token_validator.rb b/app/services/email_confirmation_token_validator.rb index 34b11ce1e4b..d46018c634d 100644 --- a/app/services/email_confirmation_token_validator.rb +++ b/app/services/email_confirmation_token_validator.rb @@ -27,7 +27,6 @@ def form_errors def extra_analytics_attributes { user_id: user.uuid, - existing_user: user.confirmed?, } end diff --git a/app/views/user_mailer/email_changed.html.slim b/app/views/user_mailer/email_changed.html.slim deleted file mode 100644 index a427a86574f..00000000000 --- a/app/views/user_mailer/email_changed.html.slim +++ /dev/null @@ -1,16 +0,0 @@ -p.lead == t('.intro', app: link_to(APP_NAME, Figaro.env.mailer_domain_name, class: 'gray')) - -table.spacer - tbody - tr - td.s10 height="10px" - |   -table.hr - tr - th - |   - -p == t('.help', - app: link_to(APP_NAME, Figaro.env.mailer_domain_name, class: 'gray'), - help_link: link_to(t('user_mailer.help_link_text'), MarketingSite.help_url), - contact_link: link_to(t('user_mailer.contact_link_text'), MarketingSite.contact_url)) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 94d048056b9..0249969092f 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -12,7 +12,6 @@ config.mailer_sender = email_with_name(Figaro.env.email_from, Figaro.env.email_from) config.paranoid = true config.password_length = 12..128 - config.reconfirmable = true config.reset_password_within = 6.hours config.secret_key = Figaro.env.secret_key_base config.sign_in_after_reset_password = false diff --git a/config/locales/mailer/en.yml b/config/locales/mailer/en.yml index 0e1b7951589..d7d44321123 100644 --- a/config/locales/mailer/en.yml +++ b/config/locales/mailer/en.yml @@ -2,8 +2,6 @@ en: mailer: about: About %{app} - email_change_notice: - subject: Change your email address email_reuse_notice: subject: This email address is already associated with an account. help: If you need help, visit %{link} diff --git a/config/locales/mailer/es.yml b/config/locales/mailer/es.yml index 890b1f860d2..28040188283 100644 --- a/config/locales/mailer/es.yml +++ b/config/locales/mailer/es.yml @@ -2,8 +2,6 @@ es: mailer: about: Acerca de %{app} - email_change_notice: - subject: Cambie su email email_reuse_notice: subject: Este email ya está asociado a una cuenta. help: Si necesitas ayuda, visita %{link} diff --git a/config/locales/mailer/fr.yml b/config/locales/mailer/fr.yml index 9cf8b66cb00..bba51228504 100644 --- a/config/locales/mailer/fr.yml +++ b/config/locales/mailer/fr.yml @@ -2,8 +2,6 @@ fr: mailer: about: À propos de %{app} - email_change_notice: - subject: Changez votre adresse courriel email_reuse_notice: subject: Cette adresse courriel est déjà associée à un compte. help: Si vous avez besoin d’aide, visitez le site %{link} diff --git a/config/locales/user_mailer/en.yml b/config/locales/user_mailer/en.yml index 88d4c3490a4..0254f522308 100644 --- a/config/locales/user_mailer/en.yml +++ b/config/locales/user_mailer/en.yml @@ -62,10 +62,6 @@ en: help: If you did not make this change, sign in to your profile and manage your email addresses. We recommend that you also change your password. subject: New email address added - email_changed: - help: If you did not want to change your email address, please visit the %{app} - %{help_link} or %{contact_link}. - intro: The email address for your %{app} account has been changed. email_confirmation_instructions: first_sentence: confirmed: Trying to change your email address? diff --git a/config/locales/user_mailer/es.yml b/config/locales/user_mailer/es.yml index 545586e694e..b2ba75cd052 100644 --- a/config/locales/user_mailer/es.yml +++ b/config/locales/user_mailer/es.yml @@ -64,9 +64,6 @@ es: help: Si no realizó este cambio, inicie sesión en su perfil y administre sus direcciones de correo electrónico. Le recomendamos que también cambie su contraseña. subject: Nueva dirección de correo electrónico añadida - email_changed: - help: Si no desea cambiar su email, visite el %{app} %{help_link} o el %{contact_link}. - intro: El email de su cuenta de %{app} ha sido cambiado. email_confirmation_instructions: first_sentence: confirmed: "¿Desea cambiar su email?" diff --git a/config/locales/user_mailer/fr.yml b/config/locales/user_mailer/fr.yml index 9059d09b378..5e271ab55b0 100644 --- a/config/locales/user_mailer/fr.yml +++ b/config/locales/user_mailer/fr.yml @@ -66,10 +66,6 @@ fr: profil et gérez vos adresses e-mail. Nous vous recommandons de changer également votre mot de passe. subject: Nouvelle adresse email ajoutée - email_changed: - help: Si vous préférez ne pas changer votre adresse courriel, veuillez visiter - le %{help_link} de %{app} ou %{contact_link}. - intro: L'adresse courriel de votre compte %{app} a été changée. email_confirmation_instructions: first_sentence: confirmed: Vous tentez de changer votre adresse courriel? diff --git a/spec/controllers/sign_up/email_confirmations_controller_spec.rb b/spec/controllers/sign_up/email_confirmations_controller_spec.rb index 4e08f635604..f7fcb48dd13 100644 --- a/spec/controllers/sign_up/email_confirmations_controller_spec.rb +++ b/spec/controllers/sign_up/email_confirmations_controller_spec.rb @@ -11,7 +11,6 @@ success: false, errors: { confirmation_token: [t('errors.messages.blank')] }, user_id: nil, - existing_user: false, } expect(@analytics).to receive(:track_event). @@ -28,7 +27,6 @@ success: false, errors: { confirmation_token: [t('errors.messages.blank')] }, user_id: nil, - existing_user: false, } expect(@analytics).to receive(:track_event). @@ -45,7 +43,6 @@ success: false, errors: { confirmation_token: [t('errors.messages.invalid')] }, user_id: nil, - existing_user: false, } expect(@analytics).to receive(:track_event). @@ -62,7 +59,6 @@ success: false, errors: { confirmation_token: [t('errors.messages.invalid')] }, user_id: nil, - existing_user: false, } expect(@analytics).to receive(:track_event). @@ -81,7 +77,6 @@ success: false, errors: { email: [t('errors.messages.already_confirmed')] }, user_id: user.uuid, - existing_user: true, } expect(@analytics).to receive(:track_event). @@ -101,7 +96,6 @@ success: false, errors: { confirmation_token: [t('errors.messages.expired')] }, user_id: user.uuid, - existing_user: false, } expect(@analytics).to receive(:track_event). @@ -125,33 +119,6 @@ success: true, errors: {}, user_id: user.uuid, - existing_user: false, - } - - expect(@analytics).to receive(:track_event). - with(Analytics::USER_REGISTRATION_EMAIL_CONFIRMATION, analytics_hash) - - get :create, params: { confirmation_token: 'foo' } - end - end - - describe 'User confirms new email' do - it 'tracks the event' do - user = create( - :user, - :signed_up, - confirmation_token: 'foo', - confirmation_sent_at: Time.zone.now, - unconfirmed_email: 'test@example.com', - ) - - stub_analytics - - analytics_hash = { - success: true, - errors: {}, - user_id: user.uuid, - existing_user: true, } expect(@analytics).to receive(:track_event). @@ -167,9 +134,9 @@ allow_any_instance_of(SignUp::EmailConfirmationsController). to receive(:validate_token).and_raise(ActiveRecord::RecordNotUnique) - get :create, params: { confirmation_token: 'foo' } + get :create, params: { confirmation_token: 'foo' } - expect(flash[:error]). + expect(flash[:error]). to eq t('devise.confirmations.already_confirmed', action: t('devise.confirmations.sign_in')) expect(response).to redirect_to root_url diff --git a/spec/controllers/sign_up/passwords_controller_spec.rb b/spec/controllers/sign_up/passwords_controller_spec.rb index 14ddce55616..9a4d315d218 100644 --- a/spec/controllers/sign_up/passwords_controller_spec.rb +++ b/spec/controllers/sign_up/passwords_controller_spec.rb @@ -4,7 +4,9 @@ describe '#create' do it 'tracks a valid password event' do token = 'new token' - user = create(:user, confirmation_token: token, confirmation_sent_at: Time.zone.now) + user = create( + :user, :unconfirmed, confirmation_token: token, confirmation_sent_at: Time.zone.now, + ) stub_analytics @@ -30,7 +32,9 @@ it 'tracks an invalid password event' do token = 'new token' - user = create(:user, confirmation_token: token, confirmation_sent_at: Time.zone.now) + user = create( + :user, :unconfirmed, confirmation_token: token, confirmation_sent_at: Time.zone.now + ) stub_analytics diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index cbb3b231d0f..814cba353ba 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -4,27 +4,6 @@ let(:user) { build(:user, :with_email) } let(:email_address) { user.email_addresses.first } - describe 'email_changed' do - let(:mail) { UserMailer.email_changed('old@email.com') } - - it_behaves_like 'a system email' - - it 'sends to the old email' do - expect(mail.to).to eq ['old@email.com'] - end - - it 'renders the subject' do - expect(mail.subject).to eq t('mailer.email_change_notice.subject') - end - - it 'renders the body' do - expect(mail.html_part.body).to have_content( - t('user_mailer.email_changed.intro', app: APP_NAME), - ) - expect_email_body_to_have_help_and_contact_links - end - end - describe 'email_deleted' do let(:mail) { UserMailer.email_deleted('old@email.com') } diff --git a/spec/services/email_confirmation_token_validator_spec.rb b/spec/services/email_confirmation_token_validator_spec.rb index 43b696bd3e9..2ffd012e449 100644 --- a/spec/services/email_confirmation_token_validator_spec.rb +++ b/spec/services/email_confirmation_token_validator_spec.rb @@ -13,7 +13,6 @@ errors = { confirmation_token: [t('errors.messages.invalid')] } extra = { user_id: user.uuid, - existing_user: false, } validator = EmailConfirmationTokenValidator.new(user).submit @@ -35,7 +34,6 @@ errors = { confirmation_token: [t('errors.messages.expired')] } extra = { user_id: user.uuid, - existing_user: false, } validator = EmailConfirmationTokenValidator.new(user).submit @@ -57,7 +55,6 @@ errors = { email: [t('errors.messages.already_confirmed')] } extra = { user_id: user.uuid, - existing_user: true, } validator = EmailConfirmationTokenValidator.new(user).submit @@ -77,7 +74,6 @@ extra = { user_id: user.uuid, - existing_user: false, } validator = EmailConfirmationTokenValidator.new(user).submit diff --git a/spec/views/layouts/user_mailer.html.slim_spec.rb b/spec/views/layouts/user_mailer.html.slim_spec.rb index 93ef358b3e7..fc21982aa49 100644 --- a/spec/views/layouts/user_mailer.html.slim_spec.rb +++ b/spec/views/layouts/user_mailer.html.slim_spec.rb @@ -2,7 +2,7 @@ describe 'layouts/user_mailer.html.slim' do before do - @mail = UserMailer.email_changed('foo@example.com') + @mail = UserMailer.email_added('foo@example.com') allow(view).to receive(:message).and_return(@mail) allow(view).to receive(:attachments).and_return(@mail.attachments)