From e730655cd1daafbad929a91a9e5d3bbf1ca8f525 Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Tue, 17 Jul 2018 16:13:10 -0400 Subject: [PATCH] LG-484 Display a message to the user when an account reset link is expired **Why**: Currently when a link to delete account is expired the user has no indication that the link was expired after clicking on it. Moreover when there is a pending request for account reset and the token is expired, a cancel link still shows. The user can cancel and create a new request. However we should remove the cancel link when the token expires so that they can immediately create a new request. **How**: Display a flash message to the user when the token is expired. When there is a pending request check for a valid token rather than the existence of a token. --- .../delete_account_controller.rb | 25 ++++++++++++++++--- app/models/account_reset_request.rb | 8 ++++-- .../two_factor_login_options_presenter.rb | 6 ++++- config/locales/devise/en.yml | 2 ++ config/locales/devise/es.yml | 2 ++ config/locales/devise/fr.yml | 2 ++ config/locales/user_mailer/en.yml | 2 +- config/locales/user_mailer/es.yml | 2 +- config/locales/user_mailer/fr.yml | 4 +-- .../delete_account_controller_spec.rb | 18 +++++++++++++ spec/models/account_reset_request_spec.rb | 23 +++++++++++++++++ ...two_factor_login_options_presenter_spec.rb | 11 +++++--- 12 files changed, 92 insertions(+), 13 deletions(-) diff --git a/app/controllers/account_reset/delete_account_controller.rb b/app/controllers/account_reset/delete_account_controller.rb index d6a13530dc8..88614da8654 100644 --- a/app/controllers/account_reset/delete_account_controller.rb +++ b/app/controllers/account_reset/delete_account_controller.rb @@ -38,12 +38,31 @@ def check_granted_token def prevent_parameter_leak token = params[:token] return if token.blank? - if AccountResetRequest.find_by(granted_token: token)&.granted_token_valid? + remove_token_from_url(token) + end + + def remove_token_from_url(token) + ar = AccountResetRequest.find_by(granted_token: token) + if ar&.granted_token_valid? session[:granted_token] = token redirect_to url_for - else - redirect_to root_url + return end + handle_expired_token(ar) if ar&.granted_token_expired? + redirect_to root_url + end + + def handle_expired_token(ar) + analytics.track_event(Analytics::ACCOUNT_RESET, + event: :delete, + token_valid: true, + expired: true, + user_id: ar&.user&.uuid) + flash[:error] = link_expired + end + + def link_expired + t('devise.two_factor_authentication.account_reset.link_expired') end end end diff --git a/app/models/account_reset_request.rb b/app/models/account_reset_request.rb index 8d1452f4ff9..0f123999a2d 100644 --- a/app/models/account_reset_request.rb +++ b/app/models/account_reset_request.rb @@ -7,7 +7,11 @@ def self.from_valid_granted_token(granted_token) end def granted_token_valid? - !granted_token.nil? && - ((Time.zone.now - granted_at) <= Figaro.env.account_reset_token_valid_for_days.to_i.days) + granted_token.present? && !granted_token_expired? + end + + def granted_token_expired? + granted_at.present? && + ((Time.zone.now - granted_at) > Figaro.env.account_reset_token_valid_for_days.to_i.days) end end diff --git a/app/presenters/two_factor_login_options_presenter.rb b/app/presenters/two_factor_login_options_presenter.rb index 0775f3b99c1..ae06a833f18 100644 --- a/app/presenters/two_factor_login_options_presenter.rb +++ b/app/presenters/two_factor_login_options_presenter.rb @@ -46,7 +46,7 @@ def options end def account_reset_or_cancel_link - account_reset_token ? account_reset_cancel_link : account_reset_link + account_reset_token_valid? ? account_reset_cancel_link : account_reset_link end private @@ -71,6 +71,10 @@ def account_reset_token current_user&.account_reset_request&.request_token end + def account_reset_token_valid? + current_user&.account_reset_request&.granted_token_valid? + end + def configured_2fa_types POSSIBLE_OPTIONS.each_with_object([]) do |option, result| result << option if POLICIES[option].new(@current_user).configured? diff --git a/config/locales/devise/en.yml b/config/locales/devise/en.yml index 33f2bbc98c2..604dcd6697a 100644 --- a/config/locales/devise/en.yml +++ b/config/locales/devise/en.yml @@ -146,6 +146,8 @@ en: code below. please_try_again_html: Please try again in %{time_remaining}. account_reset: + link_expired: The link to delete your login.gov account has expired. Please + create another request to delete your account. successful_cancel: Thank you. Your request to delete your login.gov account has been cancelled. link: deleting your account diff --git a/config/locales/devise/es.yml b/config/locales/devise/es.yml index 242c09a1ac1..0ac7dda0eac 100644 --- a/config/locales/devise/es.yml +++ b/config/locales/devise/es.yml @@ -151,6 +151,8 @@ es: el código de seguridad a continuación. please_try_again_html: Inténtelo de nuevo en %{time_remaining}. account_reset: + link_expired: El enlace para eliminar su cuenta de login.gov ha caducado. + Crea otra solicitud para eliminar tu cuenta. successful_cancel: Gracias. Su solicitud para eliminar su cuenta de login.gov ha sido cancelada. link: eliminando su cuenta diff --git a/config/locales/devise/fr.yml b/config/locales/devise/fr.yml index a9f2ea7d1f7..95de0f925ab 100644 --- a/config/locales/devise/fr.yml +++ b/config/locales/devise/fr.yml @@ -158,6 +158,8 @@ fr: le code de sécurité ci-dessous. please_try_again_html: Veuillez essayer de nouveau dans %{time_remaining}. account_reset: + link_expired: Le lien pour supprimer votre compte login.gov a expiré. Veuillez + créer une autre demande pour supprimer votre compte. successful_cancel: Je vous remercie. Votre demande de suppression de votre compte login.gov a été annulée. link: supprimer votre compte diff --git a/config/locales/user_mailer/en.yml b/config/locales/user_mailer/en.yml index 21e09a1bfdf..6a3ada6da59 100644 --- a/config/locales/user_mailer/en.yml +++ b/config/locales/user_mailer/en.yml @@ -9,7 +9,7 @@ en: link_text: Create your account subject: Your login.gov password reset request account_reset_cancel: - intro: This email confirms you have cancelled the request to delete your login.gov + intro: This email confirms you have cancelled your request to delete your login.gov account. subject: Delete your account help: '' diff --git a/config/locales/user_mailer/es.yml b/config/locales/user_mailer/es.yml index a7349619979..4b71d82bc41 100644 --- a/config/locales/user_mailer/es.yml +++ b/config/locales/user_mailer/es.yml @@ -6,7 +6,7 @@ es: link_text: NOT TRANSLATED YET subject: NOT TRANSLATED YET account_reset_cancel: - intro: Este correo electrónico confirma que ha cancelado la solicitud para eliminar + intro: Este correo electrónico confirma que ha cancelado su solicitud para eliminar su cuenta de login.gov. subject: Eliminar su cuenta help: '' diff --git a/config/locales/user_mailer/fr.yml b/config/locales/user_mailer/fr.yml index 18556d9a1fd..98da0e47e97 100644 --- a/config/locales/user_mailer/fr.yml +++ b/config/locales/user_mailer/fr.yml @@ -6,8 +6,8 @@ fr: link_text: NOT TRANSLATED YET subject: NOT TRANSLATED YET account_reset_cancel: - intro: Cet e-mail confirme que vous avez annulé la demande de suppression de - votre compte login.gov. + intro: Cet e-mail confirme que vous avez annulé votre demande de suppression + de votre compte login.gov. subject: Supprimer votre compte help: '' account_reset_request: diff --git a/spec/controllers/account_reset/delete_account_controller_spec.rb b/spec/controllers/account_reset/delete_account_controller_spec.rb index c9c5030b982..14b4f241440 100644 --- a/spec/controllers/account_reset/delete_account_controller_spec.rb +++ b/spec/controllers/account_reset/delete_account_controller_spec.rb @@ -57,5 +57,23 @@ expect(response).to render_template(:show) end + + it 'displays a flash and redirects to root if the token is expired' do + user = create(:user) + AccountResetService.new(user).create_request + AccountResetService.new(user).grant_request + + stub_analytics + expect(@analytics).to receive(:track_event). + with(Analytics::ACCOUNT_RESET, + event: :delete, token_valid: true, expired: true, user_id: user.uuid) + + Timecop.travel(Time.zone.now + 2.days) do + get :show, params: { token: AccountResetRequest.all[0].granted_token } + end + + expect(response).to redirect_to(root_url) + expect(flash[:error]).to eq t('devise.two_factor_authentication.account_reset.link_expired') + end end end diff --git a/spec/models/account_reset_request_spec.rb b/spec/models/account_reset_request_spec.rb index fdc19d3fd93..404c7259986 100644 --- a/spec/models/account_reset_request_spec.rb +++ b/spec/models/account_reset_request_spec.rb @@ -28,6 +28,29 @@ end end + describe '#granted_token_expired?' do + it 'returns false if the token does not exist' do + subject.granted_token = nil + subject.granted_at = nil + + expect(subject.granted_token_expired?).to eq(false) + end + + it 'returns true if the token is expired' do + subject.granted_token = '123' + subject.granted_at = Time.zone.now - 7.days + + expect(subject.granted_token_expired?).to eq(true) + end + + it 'returns false if the token is valid' do + subject.granted_token = '123' + subject.granted_at = Time.zone.now + + expect(subject.granted_token_expired?).to eq(false) + end + end + describe '.from_valid_granted_token' do it 'returns nil if the token does not exist' do expect(AccountResetRequest.from_valid_granted_token('123')).to eq(nil) diff --git a/spec/presenters/two_factor_login_options_presenter_spec.rb b/spec/presenters/two_factor_login_options_presenter_spec.rb index 7220d2926ba..c39f5d4bcd5 100644 --- a/spec/presenters/two_factor_login_options_presenter_spec.rb +++ b/spec/presenters/two_factor_login_options_presenter_spec.rb @@ -19,9 +19,13 @@ t('two_factor_authentication.login_options_title') end - it 'supplies a cancel link when there is an account reset token' do + it 'supplies a cancel link when the token is valid' do + allow_any_instance_of(TwoFactorLoginOptionsPresenter).to \ + receive(:account_reset_token_valid?).and_return(true) + allow_any_instance_of(TwoFactorLoginOptionsPresenter).to \ receive(:account_reset_token).and_return('foo') + expect(presenter.account_reset_or_cancel_link).to eq \ t('devise.two_factor_authentication.account_reset.pending_html', cancel_link: view.link_to( @@ -30,9 +34,10 @@ )) end - it 'supplies a reset link when there is no account reset token' do + it 'supplies a reset link when the token is not valid' do allow_any_instance_of(TwoFactorLoginOptionsPresenter).to \ - receive(:account_reset_token).and_return(nil) + receive(:account_reset_token_valid?).and_return(false) + expect(presenter.account_reset_or_cancel_link).to eq \ t('devise.two_factor_authentication.account_reset.text_html', link: view.link_to(