From 37cb5408bfacdfe72f6fa292631964a77b72ef69 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 11:57:36 -0400 Subject: [PATCH] LG-454 Refactor AccountReset::CancelController (#2385) * LG-454 Refactor AccountReset::CancelController **Why**: It was not adhering to our controller design convention, and was breaking the analytics format contract. **How**: - Create a `AccountReset::Cancel` class that validates the token, and if successful, notifies the user via email and SMS if the user has a phone, then updates the user's account reset request to reflect that it was cancelled. - Rename `cancel` to `create` in the controller to adhere to our CRUD-only guideline. Benefits: - The controller is a lot leaner and cleaner. - It keeps all the actions in one place, as opposed to having some actions in the controller (the notifications) and some in `AccountResetService` (updating the request in the DB). - It respects the Analytics API, and logs all error scenarios, including missing tokens. --- .../account_reset/cancel_controller.rb | 26 ++-- app/services/account_reset/cancel.rb | 67 +++++++++++ app/services/account_reset_service.rb | 9 -- config/locales/errors/en.yml | 3 + config/locales/errors/es.yml | 3 + config/locales/errors/fr.yml | 3 + config/routes.rb | 2 +- .../account_reset/cancel_controller_spec.rb | 83 +++++++------ .../account_reset/cancel_request_spec.rb | 54 +++++++++ spec/services/account_reset/cancel_spec.rb | 112 ++++++++++++++++++ spec/services/account_reset_service_spec.rb | 35 +----- spec/support/account_reset_helper.rb | 12 ++ 12 files changed, 316 insertions(+), 93 deletions(-) create mode 100644 app/services/account_reset/cancel.rb create mode 100644 spec/features/account_reset/cancel_request_spec.rb create mode 100644 spec/services/account_reset/cancel_spec.rb create mode 100644 spec/support/account_reset_helper.rb diff --git a/app/controllers/account_reset/cancel_controller.rb b/app/controllers/account_reset/cancel_controller.rb index 8cbe8659e7f..44f66f8cb90 100644 --- a/app/controllers/account_reset/cancel_controller.rb +++ b/app/controllers/account_reset/cancel_controller.rb @@ -1,30 +1,20 @@ module AccountReset class CancelController < ApplicationController - def cancel - account_reset = AccountResetService.cancel_request(params[:token]) - if account_reset - handle_success(account_reset.user) - else - handle_failure - end + def create + result = AccountReset::Cancel.new(params[:token]).call + + analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h) + + handle_success if result.success? + redirect_to root_url end private - def handle_success(user) - analytics.track_event(Analytics::ACCOUNT_RESET, - event: :cancel, token_valid: true, user_id: user.uuid) + def handle_success sign_out if current_user - UserMailer.account_reset_cancel(user.email).deliver_later - phone = user.phone - SmsAccountResetCancellationNotifierJob.perform_now(phone: phone) if phone.present? flash[:success] = t('devise.two_factor_authentication.account_reset.successful_cancel') end - - def handle_failure - return if params[:token].blank? - analytics.track_event(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: false) - end end end diff --git a/app/services/account_reset/cancel.rb b/app/services/account_reset/cancel.rb new file mode 100644 index 00000000000..77bba914934 --- /dev/null +++ b/app/services/account_reset/cancel.rb @@ -0,0 +1,67 @@ +module AccountReset + class Cancel + include ActiveModel::Model + + validates :token, presence: { message: I18n.t('errors.account_reset.cancel_token_missing') } + validate :valid_token + + def initialize(token) + @token = token + end + + def call + @success = valid? + + if success + notify_user_via_email_of_account_reset_cancellation + notify_user_via_phone_of_account_reset_cancellation if phone.present? + update_account_reset_request + end + + FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) + end + + private + + attr_reader :success, :token + + def valid_token + return if account_reset_request + + errors.add(:token, I18n.t('errors.account_reset.cancel_token_invalid')) if token + end + + def notify_user_via_email_of_account_reset_cancellation + UserMailer.account_reset_cancel(user.email).deliver_later + end + + def notify_user_via_phone_of_account_reset_cancellation + SmsAccountResetCancellationNotifierJob.perform_now(phone: phone) + end + + def update_account_reset_request + account_reset_request.update!(cancelled_at: Time.zone.now, + request_token: nil, + granted_token: nil) + end + + def account_reset_request + @account_reset_request ||= AccountResetRequest.find_by(request_token: token) + end + + def user + account_reset_request&.user || AnonymousUser.new + end + + def phone + user.phone + end + + def extra_analytics_attributes + { + event: 'cancel', + user_id: user.uuid, + } + end + end +end diff --git a/app/services/account_reset_service.rb b/app/services/account_reset_service.rb index 901bdc5ee18..43bc7d4f638 100644 --- a/app/services/account_reset_service.rb +++ b/app/services/account_reset_service.rb @@ -12,15 +12,6 @@ def create_request granted_token: nil) end - def self.cancel_request(token) - account_reset = token.blank? ? nil : AccountResetRequest.find_by(request_token: token) - return false unless account_reset - account_reset.update(cancelled_at: Time.zone.now, - request_token: nil, - granted_token: nil) - account_reset - 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/config/locales/errors/en.yml b/config/locales/errors/en.yml index 45f350fd4ee..0ec588520c6 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -1,6 +1,9 @@ --- en: errors: + account_reset: + cancel_token_invalid: cancel token is invalid + cancel_token_missing: cancel token is missing confirm_password_incorrect: Incorrect password. invalid_authenticity_token: Oops, something went wrong. Please try again. invalid_totp: Invalid code. Please try again. diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index 70f7cd81f22..303a54eaa22 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -1,6 +1,9 @@ --- es: errors: + account_reset: + cancel_token_invalid: NOT TRANSLATED YET + cancel_token_missing: NOT TRANSLATED YET confirm_password_incorrect: La contraseña es incorrecta. invalid_authenticity_token: "¡Oops! Algo salió mal. Inténtelo de nuevo." invalid_totp: El código es inválido. Vuelva a intentarlo. diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index b6615b07d3b..038d0fd5486 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -1,6 +1,9 @@ --- fr: errors: + account_reset: + cancel_token_invalid: NOT TRANSLATED YET + cancel_token_missing: NOT TRANSLATED YET confirm_password_incorrect: Mot de passe incorrect. invalid_authenticity_token: Oups, une erreur s'est produite. Veuillez essayer de nouveau. diff --git a/config/routes.rb b/config/routes.rb index de8be2635cd..0317f73f01d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -58,7 +58,7 @@ get '/account_reset/request' => 'account_reset/request#show' post '/account_reset/request' => 'account_reset/request#create' - get '/account_reset/cancel' => 'account_reset/cancel#cancel' + get '/account_reset/cancel' => 'account_reset/cancel#create' get '/account_reset/report_fraud' => 'account_reset/report_fraud#update' get '/account_reset/confirm_request' => 'account_reset/confirm_request#show' get '/account_reset/delete_account' => 'account_reset/delete_account#show' diff --git a/spec/controllers/account_reset/cancel_controller_spec.rb b/spec/controllers/account_reset/cancel_controller_spec.rb index 529b375fb76..a3b2a5c3777 100644 --- a/spec/controllers/account_reset/cancel_controller_spec.rb +++ b/spec/controllers/account_reset/cancel_controller_spec.rb @@ -1,67 +1,84 @@ require 'rails_helper' describe AccountReset::CancelController do - let(:user) { create(:user, :signed_up, phone: '+1 (703) 555-0000') } + include AccountResetHelper + + let(:user) { create(:user, :signed_up) } before do TwilioService::Utils.telephony_service = FakeSms end - describe '#cancel' do + describe '#create' do it 'logs a good token to the analytics' do - AccountResetService.new(user).create_request + token = create_account_reset_request_for(user) stub_analytics + analytics_hash = { + success: true, + errors: {}, + event: 'cancel', + user_id: user.uuid, + } + expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, - event: :cancel, token_valid: true, user_id: user.uuid) + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } end it 'logs a bad token to the analytics' do stub_analytics + analytics_hash = { + success: false, + errors: { token: [t('errors.account_reset.cancel_token_invalid')] }, + event: 'cancel', + user_id: 'anonymous-uuid', + } + expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: false) + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: 'FOO' } + post :create, params: { token: 'FOO' } end - it 'redirects to the root' do - post :cancel - expect(response).to redirect_to root_url - end + it 'logs a missing token to the analytics' do + stub_analytics + analytics_hash = { + success: false, + errors: { token: [t('errors.account_reset.cancel_token_missing')] }, + event: 'cancel', + user_id: 'anonymous-uuid', + } - it 'sends an SMS if there is a phone' do - AccountResetService.new(user).create_request - allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + expect(@analytics).to receive(:track_event). + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create + end - expect(SmsAccountResetCancellationNotifierJob).to have_received(:perform_now).with( - phone: user.phone - ) + it 'redirects to the root without a flash message when the token is missing or invalid' do + post :create + expect(response).to redirect_to root_url end - it 'does not send an SMS if there is no phone' do - AccountResetService.new(user).create_request - allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) - user.phone = nil - user.save! - user.phone_configuration.destroy + it 'redirects to the root with a flash message when the token is valid' do + token = create_account_reset_request_for(user) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } - expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + expect(flash[:success]). + to eq t('devise.two_factor_authentication.account_reset.successful_cancel') + expect(response).to redirect_to root_url end - it 'sends an email' do - AccountResetService.new(user).create_request + it 'signs the user out if signed in and if the token is valid' do + stub_sign_in(user) + + token = create_account_reset_request_for(user) - @mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) - allow(UserMailer).to receive(:account_reset_cancel).with(user.email). - and_return(@mailer) + expect(controller).to receive(:sign_out) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } end end end diff --git a/spec/features/account_reset/cancel_request_spec.rb b/spec/features/account_reset/cancel_request_spec.rb new file mode 100644 index 00000000000..d2524fac848 --- /dev/null +++ b/spec/features/account_reset/cancel_request_spec.rb @@ -0,0 +1,54 @@ +require 'rails_helper' + +describe 'Account Reset Request: Cancellation' do + context 'user cancels right away from the first email' do + it 'cancels the request and does not delete the user', email: true do + TwilioService::Utils.telephony_service = FakeSms + user = create(:user, :signed_up) + 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') + open_last_email + click_email_link_matching(/cancel\?token/) + + expect(page).to have_current_path new_user_session_path + expect(page). + to have_content t('devise.two_factor_authentication.account_reset.successful_cancel') + + signin(user.email, user.password) + + expect(page). + to have_current_path login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'false') + end + end + + context 'user cancels from the second email after the request has been granted' do + it 'cancels the request and does not delete the user', email: true do + TwilioService::Utils.telephony_service = FakeSms + user = create(:user, :signed_up) + 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') + reset_email + + Timecop.travel(Time.zone.now + 2.days) do + AccountResetService.grant_tokens_and_send_notifications + open_last_email + click_email_link_matching(/cancel\?token/) + + expect(page).to have_current_path new_user_session_path + expect(page). + to have_content t('devise.two_factor_authentication.account_reset.successful_cancel') + + signin(user.email, user.password) + + expect(page). + to have_current_path( + login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'false') + ) + end + end + end +end diff --git a/spec/services/account_reset/cancel_spec.rb b/spec/services/account_reset/cancel_spec.rb new file mode 100644 index 00000000000..f59132af926 --- /dev/null +++ b/spec/services/account_reset/cancel_spec.rb @@ -0,0 +1,112 @@ +require 'rails_helper' + +describe AccountReset::Cancel do + include AccountResetHelper + + let(:user) { create(:user, :signed_up) } + + before { TwilioService::Utils.telephony_service = FakeSms } + + it 'validates presence of token' do + request = AccountReset::Cancel.new(nil).call + + expect(request.success?).to eq false + end + + it 'validates validity of token' do + request = AccountReset::Cancel.new('foo').call + + expect(request.success?).to eq false + end + + context 'when the token is valid' do + context 'when the user has a phone enabled for SMS' do + it 'notifies the user via SMS of the account reset cancellation' do + token = create_account_reset_request_for(user) + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + + AccountReset::Cancel.new(token).call + + expect(SmsAccountResetCancellationNotifierJob). + to have_received(:perform_now).with(phone: user.phone) + end + end + + context 'when the user does not have a phone enabled for SMS' do + it 'does not notify the user via SMS' do + token = create_account_reset_request_for(user) + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + user.update!(phone: nil) + + AccountReset::Cancel.new(token).call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + it 'notifies the user via email of the account reset cancellation' do + token = create_account_reset_request_for(user) + + @mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) + expect(UserMailer).to receive(:account_reset_cancel).with(user.email). + and_return(@mailer) + + AccountReset::Cancel.new(token).call + end + + it 'updates the account_reset_request' do + token = create_account_reset_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + + AccountReset::Cancel.new(token).call + account_reset_request.reload + + expect(account_reset_request.request_token).to_not be_present + expect(account_reset_request.granted_token).to_not be_present + expect(account_reset_request.requested_at).to be_present + expect(account_reset_request.cancelled_at).to be_present + end + end + + context 'when the token is not valid' do + context 'when the user has a phone enabled for SMS' do + it 'does not notify the user via SMS of the account reset cancellation' do + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + + AccountReset::Cancel.new('foo').call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + context 'when the user does not have a phone enabled for SMS' do + it 'does not notify the user via SMS' do + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + user.update!(phone: nil) + + AccountReset::Cancel.new('foo').call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + it 'does not notify the user via email of the account reset cancellation' do + expect(UserMailer).to_not receive(:account_reset_cancel) + + AccountReset::Cancel.new('foo').call + end + + it 'does not update the account_reset_request' do + create_account_reset_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + + AccountReset::Cancel.new('foo').call + account_reset_request.reload + + expect(account_reset_request.request_token).to be_present + expect(account_reset_request.granted_token).to_not be_present + expect(account_reset_request.requested_at).to be_present + expect(account_reset_request.cancelled_at).to_not be_present + end + end +end diff --git a/spec/services/account_reset_service_spec.rb b/spec/services/account_reset_service_spec.rb index 6ad96b17490..55f63f306d2 100644 --- a/spec/services/account_reset_service_spec.rb +++ b/spec/services/account_reset_service_spec.rb @@ -1,17 +1,13 @@ require 'rails_helper' describe AccountResetService do - include Rails.application.routes.url_helpers + include AccountResetHelper let(:user) { create(:user) } let(:subject) { AccountResetService.new(user) } let(:user2) { create(:user) } let(:subject2) { AccountResetService.new(user2) } - before do - allow(Figaro.env).to receive(:account_reset_wait_period_days).and_return('1') - end - describe '#create_request' do it 'creates a new account reset request on the user' do subject.create_request @@ -34,31 +30,6 @@ end end - describe '#cancel_request' do - it 'removes tokens from a account reset request' do - subject.create_request - cancel = AccountResetService.cancel_request(user.account_reset_request.request_token) - arr = AccountResetRequest.find_by(user_id: user.id) - expect(arr.request_token).to_not be_present - expect(arr.granted_token).to_not be_present - expect(arr.requested_at).to be_present - expect(arr.cancelled_at).to be_present - expect(arr).to eq(cancel) - end - - it 'does not raise an error for a cancel request with a blank token' do - AccountResetService.cancel_request('') - end - - it 'does not raise an error for a cancel request with a nil token' do - AccountResetService.cancel_request('') - end - - it 'does not raise an error for a cancel request with a bad token' do - AccountResetService.cancel_request('ABC') - end - end - describe '#report_fraud' do it 'removes tokens from the request' do subject.create_request @@ -112,7 +83,7 @@ it 'does not send notifications when the request was cancelled' do subject.create_request - AccountResetService.cancel_request(AccountResetRequest.all[0].request_token) + cancel_request_for(user) after_waiting_the_full_wait_period do notifications_sent = AccountResetService.grant_tokens_and_send_notifications @@ -152,7 +123,7 @@ it 'does not send notifications when the request was cancelled' do subject.create_request - AccountResetService.cancel_request(AccountResetRequest.all[0].request_token) + cancel_request_for(user) notifications_sent = AccountResetService.grant_tokens_and_send_notifications expect(notifications_sent).to eq(0) diff --git a/spec/support/account_reset_helper.rb b/spec/support/account_reset_helper.rb new file mode 100644 index 00000000000..ff32b8695ce --- /dev/null +++ b/spec/support/account_reset_helper.rb @@ -0,0 +1,12 @@ +module AccountResetHelper + def create_account_reset_request_for(user) + AccountResetService.new(user).create_request + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + account_reset_request.request_token + end + + def cancel_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + account_reset_request.update(cancelled_at: Time.zone.now) + end +end