diff --git a/app/controllers/account_reset/confirm_request_controller.rb b/app/controllers/account_reset/confirm_request_controller.rb index d6172b22c0b..f3c554062ae 100644 --- a/app/controllers/account_reset/confirm_request_controller.rb +++ b/app/controllers/account_reset/confirm_request_controller.rb @@ -5,7 +5,10 @@ def show if email.blank? redirect_to root_url else - render :show, locals: { email: email } + render :show, locals: { + email: email, sms_phone: SmsLoginOptionPolicy.new(current_user).configured? + } + sign_out end end end diff --git a/app/controllers/account_reset/request_controller.rb b/app/controllers/account_reset/request_controller.rb index 3b16b6d6f7f..b6698c03ca7 100644 --- a/app/controllers/account_reset/request_controller.rb +++ b/app/controllers/account_reset/request_controller.rb @@ -6,13 +6,14 @@ class RequestController < ApplicationController before_action :confirm_two_factor_enabled before_action :confirm_user_not_verified - def show; end + def show + analytics.track_event(Analytics::ACCOUNT_RESET_VISIT) + end def create - analytics.track_event(Analytics::ACCOUNT_RESET, event: :request) - create_request - send_notifications - reset_session_with_email + analytics.track_event(Analytics::ACCOUNT_RESET, analytics_attributes) + AccountReset::CreateRequest.new(current_user).call + flash[:email] = current_user.email redirect_to account_reset_confirm_request_url end @@ -22,36 +23,24 @@ def check_account_reset_enabled redirect_to root_url unless FeatureManagement.account_reset_enabled? end - def confirm_user_not_verified - # IAL2 users should not be able to reset account to comply with AAL2 reqs - redirect_to account_url if decorated_user.identity_verified? - end - - def reset_session_with_email - email = current_user.email - sign_out - flash[:email] = email - end + def confirm_two_factor_enabled + return if current_user.two_factor_enabled? - def send_notifications - phone = current_user.phone - if phone - SmsAccountResetNotifierJob.perform_now( - phone: phone, - cancel_token: current_user.account_reset_request.request_token - ) - end - UserMailer.account_reset_request(current_user).deliver_later + redirect_to two_factor_options_url end - def create_request - AccountResetService.new(current_user).create_request + def confirm_user_not_verified + # IAL2 users should not be able to reset account to comply with AAL2 reqs + redirect_to account_url if decorated_user.identity_verified? end - def confirm_two_factor_enabled - return if current_user.two_factor_enabled? - - redirect_to phone_setup_url + def analytics_attributes + { + event: 'request', + sms_phone: SmsLoginOptionPolicy.new(current_user).configured?, + totp: AuthAppLoginOptionPolicy.new(current_user).configured?, + piv_cac: PivCacLoginOptionPolicy.new(current_user).configured?, + } end end end diff --git a/app/jobs/sms_account_reset_notifier_job.rb b/app/jobs/sms_account_reset_notifier_job.rb index 43d5045c07e..12c6cb4d0a7 100644 --- a/app/jobs/sms_account_reset_notifier_job.rb +++ b/app/jobs/sms_account_reset_notifier_job.rb @@ -2,13 +2,13 @@ class SmsAccountResetNotifierJob < ApplicationJob queue_as :sms include Rails.application.routes.url_helpers - def perform(phone:, cancel_token:) + def perform(phone:, token:) TwilioService::Utils.new.send_sms( to: phone, body: I18n.t( 'jobs.sms_account_reset_notifier_job.message', app: APP_NAME, - cancel_link: account_reset_cancel_url(token: cancel_token) + cancel_link: account_reset_cancel_url(token: token) ) ) end diff --git a/app/services/account_reset/create_request.rb b/app/services/account_reset/create_request.rb new file mode 100644 index 00000000000..11d24c91dc5 --- /dev/null +++ b/app/services/account_reset/create_request.rb @@ -0,0 +1,41 @@ +module AccountReset + class CreateRequest + def initialize(user) + @user = user + end + + def call + create_request + notify_user_by_email + notify_user_by_sms_if_applicable + end + + private + + attr_reader :user + + def create_request + request = AccountResetRequest.find_or_create_by(user: user) + request.update!( + request_token: SecureRandom.uuid, + requested_at: Time.zone.now, + cancelled_at: nil, + granted_at: nil, + granted_token: nil + ) + end + + def notify_user_by_email + UserMailer.account_reset_request(user).deliver_later + end + + def notify_user_by_sms_if_applicable + phone = user.phone + return unless phone + SmsAccountResetNotifierJob.perform_now( + phone: phone, + token: user.account_reset_request.request_token + ) + end + end +end diff --git a/app/services/account_reset_service.rb b/app/services/account_reset_service.rb index 43e5fad94cb..b59d8d57bc3 100644 --- a/app/services/account_reset_service.rb +++ b/app/services/account_reset_service.rb @@ -3,15 +3,6 @@ def initialize(user) @user_id = user.id end - def create_request - account_reset = account_reset_request - account_reset.update(request_token: SecureRandom.uuid, - requested_at: Time.zone.now, - cancelled_at: nil, - granted_at: nil, - granted_token: nil) - end - def self.report_fraud(token) account_reset = token.blank? ? nil : AccountResetRequest.find_by(request_token: token) return false unless account_reset diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 26a4dfe5f8e..ed492c35212 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -56,6 +56,7 @@ def browser # rubocop:disable Metrics/LineLength ACCOUNT_RESET = 'Account Reset'.freeze ACCOUNT_DELETION = 'Account Deletion Requested'.freeze + ACCOUNT_RESET_VISIT = 'Account deletion and reset visited'.freeze ACCOUNT_VISIT = 'Account Page Visited'.freeze EMAIL_AND_PASSWORD_AUTH = 'Email and Password Authentication'.freeze EMAIL_CHANGE_REQUEST = 'Email Change Request'.freeze diff --git a/app/views/account_reset/confirm_request/show.html.slim b/app/views/account_reset/confirm_request/show.html.slim index 7ba5b9833b4..54660aea114 100644 --- a/app/views/account_reset/confirm_request/show.html.slim +++ b/app/views/account_reset/confirm_request/show.html.slim @@ -6,3 +6,8 @@ h1.mt1.mb-12p.h3 = t('headings.verify_email') p == t('account_reset.confirm_request.instructions', email: email) + - if sms_phone + p + == t('account_reset.confirm_request.security_note') + p + == t('account_reset.confirm_request.close_window') diff --git a/config/locales/account_reset/en.yml b/config/locales/account_reset/en.yml index 97286aff899..c0af8a61f3c 100644 --- a/config/locales/account_reset/en.yml +++ b/config/locales/account_reset/en.yml @@ -9,10 +9,11 @@ en: title: You have deleted your account confirm_request: check_your_email: Check your email + close_window: You can close this window if you're done. instructions: We sent an email to %{email} to begin the account delete process. Follow the instructions in your email to complete the process. -

As a security measure, we also sent a text to your registered phone - number.

You can close this window if you are done. + security_note: As a security measure, we also sent a text to your registered + phone number. delete_account: are_you_sure: Are you sure you want to delete your account? cancel: Cancel diff --git a/config/locales/account_reset/es.yml b/config/locales/account_reset/es.yml index 0ec383a9f6f..e4344e0de0c 100644 --- a/config/locales/account_reset/es.yml +++ b/config/locales/account_reset/es.yml @@ -9,11 +9,12 @@ es: title: Has eliminado tu cuenta confirm_request: check_your_email: Consultar su correo electrónico + close_window: Puede cerrar esta ventana si ha terminado. instructions: Enviamos un correo electrónico a %{email} para comenzar el proceso de eliminación de cuenta. Siga las instrucciones en su - correo electrónico para completar el proceso.

Como medida de seguridad, - también enviamos un mensaje de texto a su registro número de teléfono.

- Puede cerrar esta ventana si ha terminado. + correo electrónico para completar el proceso. + security_note: Como medida de seguridad, también enviamos un mensaje de texto + a su registro número de teléfono. delete_account: are_you_sure: "¿Seguro que quieres eliminar tu cuenta?" cancel: Cancelar diff --git a/config/locales/account_reset/fr.yml b/config/locales/account_reset/fr.yml index f151c7c61e4..089b9e588df 100644 --- a/config/locales/account_reset/fr.yml +++ b/config/locales/account_reset/fr.yml @@ -9,11 +9,12 @@ fr: title: Vous avez supprimé votre compte confirm_request: check_your_email: Vérifiez votre email + close_window: Vous pouvez fermer cette fenêtre si vous avez terminé. instructions: Nous avons envoyé un e-mail à %{email} pour commencer le compte. Supprimer le processus. Suivez les instructions dans votre e-mail - pour terminer le processus.

Par mesure de sécurité, nous avons également - envoyé un SMS sur votre téléphone enregistré nombre.

Vous pouvez - fermer cette fenêtre si vous avez terminé. + pour terminer le processus. + security_note: Par mesure de sécurité, nous avons également envoyé un SMS à + votre numéro de téléphone enregistré. delete_account: are_you_sure: Êtes-vous sûr de vouloir supprimer votre compte? cancel: Annuler diff --git a/spec/controllers/account_reset/confirm_request_controller_spec.rb b/spec/controllers/account_reset/confirm_request_controller_spec.rb index fc582f00a8e..4db995aa123 100644 --- a/spec/controllers/account_reset/confirm_request_controller_spec.rb +++ b/spec/controllers/account_reset/confirm_request_controller_spec.rb @@ -2,17 +2,7 @@ RSpec.describe AccountReset::ConfirmRequestController do describe '#show' do - context 'email in session' do - it 'renders the page and deletes the email from the session' do - allow(controller).to receive(:flash).and_return(email: 'test@example.com') - - get :show - - expect(response).to render_template(:show) - end - end - - context 'no email in session' do + context 'no email in flash' do it 'redirects to the new user registration path' do get :show diff --git a/spec/controllers/account_reset/delete_account_controller_spec.rb b/spec/controllers/account_reset/delete_account_controller_spec.rb index 14b4f241440..d219b7ad611 100644 --- a/spec/controllers/account_reset/delete_account_controller_spec.rb +++ b/spec/controllers/account_reset/delete_account_controller_spec.rb @@ -1,10 +1,12 @@ require 'rails_helper' describe AccountReset::DeleteAccountController do + include AccountResetHelper + describe '#delete' do it 'logs a good token to the analytics' do user = create(:user) - AccountResetService.new(user).create_request + create_account_reset_request_for(user) AccountResetService.new(user).grant_request session[:granted_token] = AccountResetRequest.all[0].granted_token @@ -33,7 +35,7 @@ describe '#show' do it 'prevents parameter leak' do user = create(:user) - AccountResetService.new(user).create_request + create_account_reset_request_for(user) AccountResetService.new(user).grant_request get :show, params: { token: AccountResetRequest.all[0].granted_token } @@ -49,7 +51,7 @@ it 'renders the page' do user = create(:user) - AccountResetService.new(user).create_request + create_account_reset_request_for(user) AccountResetService.new(user).grant_request session[:granted_token] = AccountResetRequest.all[0].granted_token @@ -60,7 +62,7 @@ it 'displays a flash and redirects to root if the token is expired' do user = create(:user) - AccountResetService.new(user).create_request + create_account_reset_request_for(user) AccountResetService.new(user).grant_request stub_analytics diff --git a/spec/controllers/account_reset/report_fraud_controller_spec.rb b/spec/controllers/account_reset/report_fraud_controller_spec.rb index 383378f1fae..56114cf0104 100644 --- a/spec/controllers/account_reset/report_fraud_controller_spec.rb +++ b/spec/controllers/account_reset/report_fraud_controller_spec.rb @@ -1,10 +1,12 @@ require 'rails_helper' describe AccountReset::ReportFraudController do + include AccountResetHelper + describe '#update' do it 'logs a good token to the analytics' do user = create(:user) - AccountResetService.new(user).create_request + create_account_reset_request_for(user) stub_analytics expect(@analytics).to receive(:track_event). diff --git a/spec/controllers/account_reset/request_controller_spec.rb b/spec/controllers/account_reset/request_controller_spec.rb index 9fb3678f3a7..9c85c8bd757 100644 --- a/spec/controllers/account_reset/request_controller_spec.rb +++ b/spec/controllers/account_reset/request_controller_spec.rb @@ -3,52 +3,122 @@ describe AccountReset::RequestController do describe '#show' do it 'renders the page' do - sign_in_before_2fa + user = build(:user, :with_authentication_app) + stub_sign_in_before_2fa(user) get :show expect(response).to render_template(:show) end - it 'redirects to root without 2fa' do + it 'redirects to root if user not signed in' do get :show expect(response).to redirect_to root_url end - it 'redirects to phone setup url if 2fa not setup' do - user = create(:user) - sign_in_before_2fa(user) + it 'redirects to root if feature is not enabled' do + allow(FeatureManagement).to receive(:account_reset_enabled?).and_return(false) + user = build(:user, :with_authentication_app) + stub_sign_in_before_2fa(user) + + get :show + + expect(response).to redirect_to root_url + end + + it 'redirects to 2FA setup url if 2FA not set up' do + stub_sign_in_before_2fa get :show - expect(response).to redirect_to phone_setup_url + expect(response).to redirect_to two_factor_options_url + end + + it 'logs the visit to analytics' do + user = build(:user, :with_authentication_app) + stub_sign_in_before_2fa(user) + stub_analytics + + expect(@analytics).to receive(:track_event).with(Analytics::ACCOUNT_RESET_VISIT) + + get :show end end describe '#create' do - it 'logs the request in the analytics' do + it 'logs totp user in the analytics' do + user = build(:user, :with_authentication_app) + stub_sign_in_before_2fa(user) + + stub_analytics + attributes = { + event: 'request', + sms_phone: false, + totp: true, + piv_cac: false, + } + expect(@analytics).to receive(:track_event). + with(Analytics::ACCOUNT_RESET, attributes) + + post :create + end + + it 'logs sms user in the analytics' do TwilioService::Utils.telephony_service = FakeSms - sign_in_before_2fa + user = build(:user, :signed_up) + stub_sign_in_before_2fa(user) + + stub_analytics + attributes = { + event: 'request', + sms_phone: true, + totp: false, + piv_cac: false, + } + expect(@analytics).to receive(:track_event). + with(Analytics::ACCOUNT_RESET, attributes) + + post :create + end + + it 'logs PIV/CAC user in the analytics' do + user = build(:user, :with_piv_or_cac) + stub_sign_in_before_2fa(user) stub_analytics + attributes = { + event: 'request', + sms_phone: false, + totp: false, + piv_cac: true, + } expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, event: :request) + with(Analytics::ACCOUNT_RESET, attributes) + + post :create + end + it 'redirects to root if user not signed in' do post :create + + expect(response).to redirect_to root_url end - it 'redirects to root without 2fa' do + it 'redirects to root if feature is not enabled' do + allow(FeatureManagement).to receive(:account_reset_enabled?).and_return(false) + user = build(:user, :with_authentication_app) + stub_sign_in_before_2fa(user) + post :create expect(response).to redirect_to root_url end - it 'redirects to phone setup url if 2fa not setup' do - user = create(:user) - sign_in_before_2fa(user) + it 'redirects to 2FA setup url if 2FA not set up' do + stub_sign_in_before_2fa post :create - expect(response).to redirect_to phone_setup_url + expect(response).to redirect_to two_factor_options_url end end end diff --git a/spec/features/account_reset/delete_account_spec.rb b/spec/features/account_reset/delete_account_spec.rb index e9635fdf13c..7daa30108fe 100644 --- a/spec/features/account_reset/delete_account_spec.rb +++ b/spec/features/account_reset/delete_account_spec.rb @@ -13,6 +13,14 @@ click_link t('two_factor_authentication.login_options_link_text') click_link t('devise.two_factor_authentication.account_reset.link') click_button t('account_reset.request.yes_continue') + + expect(page). + to have_content strip_tags( + t('account_reset.confirm_request.instructions', email: user.email) + ) + expect(page).to have_content t('account_reset.confirm_request.security_note') + expect(page).to have_content t('account_reset.confirm_request.close_window') + reset_email Timecop.travel(Time.zone.now + 2.days) do @@ -40,6 +48,28 @@ end end + context 'as an LOA1 user without a phone' do + it 'does not tell the user that an SMS was sent to their registered phone' do + user = create(:user, :with_authentication_app) + signin(user.email, user.password) + click_link t('two_factor_authentication.login_options_link_text') + click_link t('devise.two_factor_authentication.account_reset.link') + click_button t('account_reset.request.yes_continue') + + expect(page). + to have_content strip_tags( + t('account_reset.confirm_request.instructions', email: user.email) + ) + expect(page).to_not have_content t('account_reset.confirm_request.security_note') + expect(page).to have_content t('account_reset.confirm_request.close_window') + + # user should now be signed out + visit account_path + + expect(page).to have_current_path(new_user_session_path) + end + end + context 'as an LOA3 user' do let(:user) do create( diff --git a/spec/jobs/sms_account_reset_notifier_job_spec.rb b/spec/jobs/sms_account_reset_notifier_job_spec.rb index d80cdfe9ee6..822ce2935fa 100644 --- a/spec/jobs/sms_account_reset_notifier_job_spec.rb +++ b/spec/jobs/sms_account_reset_notifier_job_spec.rb @@ -14,7 +14,7 @@ subject(:perform) do SmsAccountResetNotifierJob.perform_now( phone: '+1 (888) 555-5555', - cancel_token: 'UUID1' + token: 'UUID1' ) end diff --git a/spec/services/account_reset_service_spec.rb b/spec/services/account_reset_service_spec.rb index 55f63f306d2..a7b50bb112d 100644 --- a/spec/services/account_reset_service_spec.rb +++ b/spec/services/account_reset_service_spec.rb @@ -4,35 +4,11 @@ include AccountResetHelper let(:user) { create(:user) } - let(:subject) { AccountResetService.new(user) } let(:user2) { create(:user) } - let(:subject2) { AccountResetService.new(user2) } - - describe '#create_request' do - it 'creates a new account reset request on the user' do - subject.create_request - arr = user.account_reset_request - expect(arr.request_token).to be_present - expect(arr.requested_at).to be_present - expect(arr.cancelled_at).to be_nil - expect(arr.granted_at).to be_nil - expect(arr.granted_token).to be_nil - end - - it 'creates a new account reset request in the db' do - subject.create_request - arr = AccountResetRequest.find_by(user_id: user.id) - expect(arr.request_token).to be_present - expect(arr.requested_at).to be_present - expect(arr.cancelled_at).to be_nil - expect(arr.granted_at).to be_nil - expect(arr.granted_token).to be_nil - end - end describe '#report_fraud' do it 'removes tokens from the request' do - subject.create_request + create_account_reset_request_for(user) AccountResetService.report_fraud(user.account_reset_request.request_token) arr = AccountResetRequest.find_by(user_id: user.id) expect(arr.request_token).to_not be_present @@ -60,9 +36,8 @@ describe '#grant_request' do it 'adds a notified at timestamp and granted token to the user' do - rd = subject - rd.create_request - rd.grant_request + create_account_reset_request_for(user) + AccountResetService.new(user).grant_request arr = AccountResetRequest.find_by(user_id: user.id) expect(arr.granted_at).to be_present expect(arr.granted_token).to be_present @@ -72,7 +47,7 @@ describe '.grant_tokens_and_send_notifications' do context 'after waiting the full wait period' do it 'does not send notifications when the notifications were already sent' do - subject.create_request + create_account_reset_request_for(user) after_waiting_the_full_wait_period do AccountResetService.grant_tokens_and_send_notifications @@ -82,7 +57,7 @@ end it 'does not send notifications when the request was cancelled' do - subject.create_request + create_account_reset_request_for(user) cancel_request_for(user) after_waiting_the_full_wait_period do @@ -92,7 +67,7 @@ end it 'sends notifications after a request is granted' do - subject.create_request + create_account_reset_request_for(user) after_waiting_the_full_wait_period do notifications_sent = AccountResetService.grant_tokens_and_send_notifications @@ -102,8 +77,8 @@ end it 'sends 2 notifications after 2 requests are granted' do - subject.create_request - subject2.create_request + create_account_reset_request_for(user) + create_account_reset_request_for(user2) after_waiting_the_full_wait_period do notifications_sent = AccountResetService.grant_tokens_and_send_notifications @@ -115,14 +90,14 @@ context 'after not waiting the full wait period' do it 'does not send notifications after a request' do - subject.create_request + create_account_reset_request_for(user) notifications_sent = AccountResetService.grant_tokens_and_send_notifications expect(notifications_sent).to eq(0) end it 'does not send notifications when the request was cancelled' do - subject.create_request + create_account_reset_request_for(user) cancel_request_for(user) notifications_sent = AccountResetService.grant_tokens_and_send_notifications diff --git a/spec/support/account_reset_helper.rb b/spec/support/account_reset_helper.rb index ff32b8695ce..9176d3078c1 100644 --- a/spec/support/account_reset_helper.rb +++ b/spec/support/account_reset_helper.rb @@ -1,6 +1,6 @@ module AccountResetHelper def create_account_reset_request_for(user) - AccountResetService.new(user).create_request + AccountReset::CreateRequest.new(user).call account_reset_request = AccountResetRequest.find_by(user_id: user.id) account_reset_request.request_token end diff --git a/spec/views/account_reset/confirm_request/show.html.slim_spec.rb b/spec/views/account_reset/confirm_request/show.html.slim_spec.rb index 471a851cd0d..74321b6fa50 100644 --- a/spec/views/account_reset/confirm_request/show.html.slim_spec.rb +++ b/spec/views/account_reset/confirm_request/show.html.slim_spec.rb @@ -3,6 +3,7 @@ describe 'account_reset/confirm_request/show.html.slim' do before do allow(view).to receive(:email).and_return('foo@bar.com') + allow(view).to receive(:sms_phone).and_return(true) end it 'has a localized title' do @@ -10,13 +11,4 @@ render end - - it 'contains the user email' do - email = 'foo@bar.com' - session[:email] = email - - render - - expect(rendered).to have_content(email) - end end