From fe46c7bf7a366c9345092468f93ad0207568dc60 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 6 Jul 2018 08:59:37 -0400 Subject: [PATCH] Handle Twilio errors more gracefully **Why**: So users don't get 500 errors when signing in. This is a follow up to the previous commit which adds the following: - Rescue Twilio errors when signing in and display the OTP verification page with an error message, allowing the user to try the phone call option, or signing in with their personal key - Capture the country code and context along with Twilio errors so we can more easily see which countries are affected - Don't change the user's OTP delivery preference if the last preference they used resulted in a Twilio error. For example, if a user normally uses voice, but for some reason didn't get the phone call, and then tries to send an SMS to a landline, resulting in an error message, we should leave their preference as voice so they won't get an error the next time they sign in. - Add the phone number country to the Exception Notification emails --- .reek | 3 + .rubocop.yml | 2 +- .../two_factor_authentication_controller.rb | 60 +++++++++++-- app/forms/otp_delivery_selection_form.rb | 23 +---- app/models/anonymous_user.rb | 4 + .../otp_delivery_preference_updater.rb | 29 ++++++ app/services/phone_verification.rb | 19 +++- .../exception_notifier/_session.text.erb | 4 +- config/application.yml.example | 4 +- config/initializers/figaro.rb | 1 + config/locales/errors/en.yml | 2 + config/locales/errors/es.yml | 2 + config/locales/errors/fr.yml | 12 ++- ...o_factor_authentication_controller_spec.rb | 25 +++++- .../change_factor_spec.rb | 19 ++++ .../two_factor_authentication/sign_in_spec.rb | 40 +++++++++ .../forms/otp_delivery_selection_form_spec.rb | 66 +++++--------- .../otp_delivery_preference_updater_spec.rb | 89 +++++++++++++++++++ spec/services/phone_verification_spec.rb | 16 ++++ spec/support/fake_adapter.rb | 18 ++++ 20 files changed, 354 insertions(+), 84 deletions(-) create mode 100644 app/services/otp_delivery_preference_updater.rb create mode 100644 spec/services/otp_delivery_preference_updater_spec.rb diff --git a/.reek b/.reek index 4883301e48b..54bff8852c2 100644 --- a/.reek +++ b/.reek @@ -6,6 +6,7 @@ ControlParameter: - OpenidConnectRedirector#initialize - NoRetryJobs#call - PhoneFormatter#self.format + - Users::TwoFactorAuthenticationController#invalid_phone_number DuplicateMethodCall: exclude: - ApplicationController#disable_caching @@ -47,6 +48,7 @@ FeatureEnvy: - Utf8Sanitizer#remote_ip - Idv::Proofer#validate_vendors - PersonalKeyGenerator#create_legacy_recovery_code + - TwoFactorAuthenticationController#capture_analytics_for_exception InstanceVariableAssumption: exclude: - User @@ -105,6 +107,7 @@ TooManyStatements: - Upaya::RandomTools#self.random_weighted_sample - UserFlowFormatter#stop - Upaya::QueueConfig#self.choose_queue_adapter + - Users::TwoFactorAuthenticationController#send_code TooManyMethods: exclude: - Users::ConfirmationsController diff --git a/.rubocop.yml b/.rubocop.yml index 9cb21bca7e5..9af4903ae03 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -62,7 +62,7 @@ Metrics/ClassLength: - app/controllers/openid_connect/authorization_controller.rb - app/controllers/users/confirmations_controller.rb - app/controllers/users/sessions_controller.rb - - app/controllers/devise/two_factor_authentication_controller.rb + - app/controllers/users/two_factor_authentication_controller.rb - app/decorators/service_provider_session_decorator.rb - app/decorators/user_decorator.rb - app/services/analytics.rb diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 2c93a0fd861..b24e0eeaf25 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -4,6 +4,7 @@ class TwoFactorAuthenticationController < ApplicationController before_action :check_remember_device_preference + # rubocop:disable Metrics/MethodLength def show if current_user.piv_cac_enabled? redirect_to login_two_factor_piv_cac_url @@ -14,7 +15,10 @@ def show else redirect_to two_factor_options_url end + rescue Twilio::REST::RestError, PhoneVerification::VerifyError => exception + invalid_phone_number(exception, action: 'show') end + # rubocop:enable Metrics/MethodLength def send_code result = otp_delivery_selection_form.submit(delivery_params) @@ -22,11 +26,12 @@ def send_code if result.success? handle_valid_otp_delivery_preference(user_selected_otp_delivery_preference) + update_otp_delivery_preference_if_needed else handle_invalid_otp_delivery_preference(result) end rescue Twilio::REST::RestError, PhoneVerification::VerifyError => exception - invalid_phone_number(exception) + invalid_phone_number(exception, action: 'send_code') end private @@ -44,19 +49,56 @@ def validate_otp_delivery_preference_and_send_code end end + def update_otp_delivery_preference_if_needed + OtpDeliveryPreferenceUpdater.new( + user: current_user, + preference: delivery_params[:otp_delivery_preference], + context: otp_delivery_selection_form.context + ).call + end + def handle_invalid_otp_delivery_preference(result) flash[:error] = result.errors[:phone].first preference = current_user.otp_delivery_preference redirect_to login_two_factor_url(otp_delivery_preference: preference) end - def invalid_phone_number(exception) - code = exception.code - analytics.track_event( - Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: exception.message, code: code + def invalid_phone_number(exception, action:) + capture_analytics_for_exception(exception) + + if action == 'show' + redirect_to_otp_verification_with_error + else + flash[:error] = error_message(exception.code) + redirect_back(fallback_location: account_url) + end + end + + def redirect_to_otp_verification_with_error + flash[:error] = t('errors.messages.phone_unsupported') + redirect_to login_two_factor_url( + otp_delivery_preference: current_user.otp_delivery_preference, reauthn: reauthn? ) - flash[:error] = error_message(code) - redirect_back(fallback_location: account_url) + end + + # rubocop:disable Metrics/MethodLength + def capture_analytics_for_exception(exception) + attributes = { + error: exception.message, + code: exception.code, + context: context, + country: parsed_phone.country, + } + if exception.is_a?(PhoneVerification::VerifyError) + attributes[:status] = exception.status + attributes[:response] = exception.response + end + analytics.track_event(Analytics::TWILIO_PHONE_VALIDATION_FAILED, attributes) + end + # rubocop:enable Metrics/MethodLength + + def parsed_phone + @parsed_phone ||= Phonelib.parse(phone_to_deliver_to) end def error_message(code) @@ -68,7 +110,9 @@ def twilio_errors end def otp_delivery_selection_form - OtpDeliverySelectionForm.new(current_user, phone_to_deliver_to, context) + @otp_delivery_selection_form ||= OtpDeliverySelectionForm.new( + current_user, phone_to_deliver_to, context + ) end def reauthn_param diff --git a/app/forms/otp_delivery_selection_form.rb b/app/forms/otp_delivery_selection_form.rb index 4971439a99f..75b34f515f2 100644 --- a/app/forms/otp_delivery_selection_form.rb +++ b/app/forms/otp_delivery_selection_form.rb @@ -2,7 +2,7 @@ class OtpDeliverySelectionForm include ActiveModel::Model include OtpDeliveryPreferenceValidator - attr_reader :otp_delivery_preference, :phone + attr_reader :otp_delivery_preference, :phone, :context validates :otp_delivery_preference, inclusion: { in: %w[sms voice] } validates :phone, presence: true @@ -20,7 +20,6 @@ def submit(params) @success = valid? change_otp_delivery_preference_to_sms if unsupported_phone? - update_otp_delivery_preference if should_update_user? FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) end @@ -29,7 +28,7 @@ def submit(params) attr_writer :otp_delivery_preference attr_accessor :resend - attr_reader :success, :user, :context + attr_reader :success, :user def change_otp_delivery_preference_to_sms user_attributes = { otp_delivery_preference: 'sms' } @@ -43,24 +42,6 @@ def unsupported_phone? error_messages[:phone].first != I18n.t('errors.messages.missing_field') end - def update_otp_delivery_preference - user_attributes = { otp_delivery_preference: otp_delivery_preference } - UpdateUser.new(user: user, attributes: user_attributes).call - end - - def idv_context? - context == 'idv' - end - - def otp_delivery_preference_changed? - otp_delivery_preference != user.otp_delivery_preference - end - - def should_update_user? - return false if unsupported_phone? - success && otp_delivery_preference_changed? && !idv_context? - end - def extra_analytics_attributes { otp_delivery_preference: otp_delivery_preference, diff --git a/app/models/anonymous_user.rb b/app/models/anonymous_user.rb index 95582e98bae..83eab07e127 100644 --- a/app/models/anonymous_user.rb +++ b/app/models/anonymous_user.rb @@ -6,4 +6,8 @@ def uuid def second_factor_locked_at nil end + + def phone + nil + end end diff --git a/app/services/otp_delivery_preference_updater.rb b/app/services/otp_delivery_preference_updater.rb new file mode 100644 index 00000000000..2713dbbb2a2 --- /dev/null +++ b/app/services/otp_delivery_preference_updater.rb @@ -0,0 +1,29 @@ +class OtpDeliveryPreferenceUpdater + def initialize(user:, preference:, context:) + @user = user + @preference = preference + @context = context + end + + def call + user_attributes = { otp_delivery_preference: preference } + UpdateUser.new(user: user, attributes: user_attributes).call if should_update_user? + end + + private + + attr_reader :user, :preference, :context + + def should_update_user? + return false unless user + otp_delivery_preference_changed? && !idv_context? + end + + def otp_delivery_preference_changed? + preference != user.otp_delivery_preference + end + + def idv_context? + context == 'idv' + end +end diff --git a/app/services/phone_verification.rb b/app/services/phone_verification.rb index 94abe87be27..1ecc430869a 100644 --- a/app/services/phone_verification.rb +++ b/app/services/phone_verification.rb @@ -18,9 +18,18 @@ def initialize(phone:, code:, locale: nil) @locale = locale end + # rubocop:disable Style/GuardClause def send_sms - raise VerifyError.new(code: error_code, message: error_message) unless start_request.success? + unless start_request.success? + raise VerifyError.new( + code: error_code, + message: error_message, + status: start_request.response_code, + response: start_request.response_body + ) + end end + # rubocop:enable Style/GuardClause private @@ -36,6 +45,8 @@ def error_message def response_body @response_body ||= JSON.parse(start_request.response_body) + rescue JSON::ParserError + {} end def start_request @@ -73,11 +84,13 @@ def country_code end class VerifyError < StandardError - attr_reader :code, :message + attr_reader :code, :message, :status, :response - def initialize(code:, message:) + def initialize(code:, message:, status:, response:) @code = code @message = message + @status = status + @response = response end end end diff --git a/app/views/exception_notifier/_session.text.erb b/app/views/exception_notifier/_session.text.erb index 65583037bfc..496e1aa9abc 100644 --- a/app/views/exception_notifier/_session.text.erb +++ b/app/views/exception_notifier/_session.text.erb @@ -16,6 +16,8 @@ Referer: <%= @request.referer %> <% session['user_return_to'] = session['user_return_to']&.split('?')&.first %> Session: <%= session %> -User UUID: <%= @kontroller.analytics_user.uuid %> +<% user = @kontroller.analytics_user %> +User UUID: <%= user.uuid %> +User's Country (based on phone): <%= Phonelib.parse(user.phone).country %> Visitor ID: <%= @request.cookies['ahoy_visitor'] %> diff --git a/config/application.yml.example b/config/application.yml.example index c744ae5d7c3..bd67a6eb170 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -155,6 +155,7 @@ development: piv_cac_service_url: 'https://localhost:8443/' piv_cac_verify_token_url: 'https://localhost:8443/' pkcs11_lib: '/opt/cloudhsm/lib/libcloudhsm_pkcs11.so' + programmable_sms_countries: 'US,CA,MX' proofer_mock_fallback: 'true' rack_mini_profiler: 'off' reauthn_window: '120' @@ -277,7 +278,7 @@ production: piv_cac_agencies: '["DOD"]' piv_cac_enabled: 'false' pkcs11_lib: '/opt/cloudhsm/lib/libcloudhsm_pkcs11.so' - programmable_sms_countries: 'US,CA' + programmable_sms_countries: 'US,CA,MX' proofer_mock_fallback: 'true' reauthn_window: '120' recaptcha_enabled_percent: '0' @@ -401,6 +402,7 @@ test: piv_cac_verify_token_secret: '3ac13bfa23e22adae321194c083e783faf89469f6f85dcc0802b27475c94b5c3891b5657bd87d0c1ad65de459166440512f2311018db90d57b15d8ab6660748f' piv_cac_verify_token_url: 'https://localhost:8443/' pkcs11_lib: '/opt/cloudhsm/lib/libcloudhsm_pkcs11.so' + programmable_sms_countries: 'US,CA,MX' proofer_mock_fallback: 'true' programmable_sms_countries: 'US,CA' reauthn_window: '120' diff --git a/config/initializers/figaro.rb b/config/initializers/figaro.rb index ed1d5602f6d..fef45c0cbf7 100644 --- a/config/initializers/figaro.rb +++ b/config/initializers/figaro.rb @@ -30,6 +30,7 @@ 'password_max_attempts', 'password_pepper', 'password_strength_enabled', + 'programmable_sms_countries', 'queue_health_check_dead_interval_seconds', 'reauthn_window', 'recovery_code_length', diff --git a/config/locales/errors/en.yml b/config/locales/errors/en.yml index 0302c404593..5dfe8ea8114 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -40,6 +40,8 @@ en: otp_incorrect: Incorrect code. Did you type it correctly? password_incorrect: Incorrect password personal_key_incorrect: Incorrect personal key + phone_unsupported: Sorry, we are unable to send SMS at this time. Please try + the phone call option below, or use your personal key. requires_phone: requires you to enter your phone number. twilio_inbound_sms_invalid: The inbound Twilio SMS message failed validation. unauthorized_authn_context: Unauthorized authentication context diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index bb8fc94bd65..3f5cd91d7ed 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -38,6 +38,8 @@ es: otp_incorrect: El código es incorrecto. ¿Lo escribió correctamente? password_incorrect: La contraseña es incorrecta personal_key_incorrect: La clave personal es incorrecta + phone_unsupported: Lo sentimos, no podemos enviar SMS en este momento. Pruebe + la opción de llamada telefónica a continuación o use su clave personal. requires_phone: requiere que ingrese su número de teléfono. twilio_inbound_sms_invalid: El mensaje de Twilio SMS de entrada falló la validación. unauthorized_authn_context: Contexto de autenticación no autorizado diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index 4746589909b..7b14eebc229 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -7,7 +7,7 @@ fr: invalid_totp: Code non valide. Veuillez essayer de nouveau. max_password_attempts_reached: Vous avez inscrit des mots de passe incorrects un trop grand nombre de fois. Vous pouvez réinitialiser votre mot de passe en - utilisant le lien « Vous avez oublié votre mot de passe? ». + utilisant le lien « Vous avez oublié votre mot de passe? ». messages: already_confirmed: a déjà été confirmé, veuillez essayer de vous connecter blank: Veuillez remplir ce champ. @@ -15,7 +15,7 @@ fr: confirmation_invalid_token: Lien de confirmation non valide. Le lien est expiré ou vous avez déjà confirmé votre compte. confirmation_period_expired: Lien de confirmation expiré. Vous pouvez cliquer - sur « Envoyer les instructions de confirmation de nouveau » pour en obtenir + sur « Envoyer les instructions de confirmation de nouveau » pour en obtenir un autre. expired: est expiré, veuillez en demander un nouveau format_mismatch: Veuillez vous assurer de respecter le format requis. @@ -33,12 +33,16 @@ fr: not_found: introuvable not_locked: n'a pas été verrouillé not_saved: - one: '1 erreur a interdit la sauvegarde de cette %{resource} :' - other: "%{count} des erreurs ont empêché la sauvegarde de cette %{resource} :" + one: '1 erreur a interdit la sauvegarde de cette %{resource} :' + other: "%{count} des erreurs ont empêché la sauvegarde de cette %{resource} + :" otp_failed: NOT TRANSLATED YET otp_incorrect: Code non valide. L'avez-vous inscrit correctement? password_incorrect: Mot de passe incorrect personal_key_incorrect: Clé personnelle incorrecte + phone_unsupported: Désolé, nous ne sommes pas en mesure d'envoyer des SMS pour + le moment. S'il vous plaît essayez l'option d'appel téléphonique ci-dessous, + ou utilisez votre clé personnelle. requires_phone: vous demande d'entrer votre numéro de téléphone. twilio_inbound_sms_invalid: Le message SMS Twilio entrant a échoué à la validation. unauthorized_authn_context: Contexte d'authentification non autorisé diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index c87b128c9da..85ef7036ebc 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -332,11 +332,18 @@ def index } twilio_error = "[HTTP 400] : error message\n\n" + twilio_error_hash = { + error: twilio_error, + code: '', + context: 'confirmation', + country: 'US', + } + expect(@analytics).to receive(:track_event). with(Analytics::OTP_DELIVERY_SELECTION, analytics_hash) expect(@analytics).to receive(:track_event). - with(Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: twilio_error, code: '') + with(Analytics::TWILIO_PHONE_VALIDATION_FAILED, twilio_error_hash) get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } end @@ -345,7 +352,11 @@ def index stub_analytics code = 60_033 error_message = 'error' - verify_error = PhoneVerification::VerifyError.new(code: code, message: error_message) + response = '{"error_code":"60004"}' + status = 400 + verify_error = PhoneVerification::VerifyError.new( + code: code, message: error_message, status: status, response: response + ) allow(SmsOtpSenderJob).to receive(:perform_now).and_raise(verify_error) analytics_hash = { @@ -357,12 +368,20 @@ def index country_code: '1', area_code: '202', } + twilio_error_hash = { + error: error_message, + code: code, + context: 'confirmation', + country: 'US', + status: status, + response: response, + } expect(@analytics).to receive(:track_event). with(Analytics::OTP_DELIVERY_SELECTION, analytics_hash) expect(@analytics).to receive(:track_event). - with(Analytics::TWILIO_PHONE_VALIDATION_FAILED, error: error_message, code: code) + with(Analytics::TWILIO_PHONE_VALIDATION_FAILED, twilio_error_hash) get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } } end diff --git a/spec/features/two_factor_authentication/change_factor_spec.rb b/spec/features/two_factor_authentication/change_factor_spec.rb index c4ca885c936..e0125e2acca 100644 --- a/spec/features/two_factor_authentication/change_factor_spec.rb +++ b/spec/features/two_factor_authentication/change_factor_spec.rb @@ -175,6 +175,25 @@ end end + context 'with SMS and number that Verify does not think is valid' do + it 'rescues the VerifyError' do + allow(SmsOtpSenderJob).to receive(:perform_later) + PhoneVerification.adapter = FakeAdapter + allow(FakeAdapter).to receive(:post).and_return(FakeAdapter::ErrorResponse.new) + + user = create(:user, :signed_up, phone: '+17035551212') + visit new_user_session_path + sign_in_live_with_2fa(user) + visit manage_phone_path + select 'Morocco', from: 'user_phone_form_international_code' + fill_in 'user_phone_form_phone', with: '+212 661-289325' + click_button t('forms.buttons.submit.confirm_change') + + expect(current_path).to eq manage_phone_path + expect(page).to have_content t('errors.messages.invalid_phone_number') + end + end + def complete_2fa_confirmation complete_2fa_confirmation_without_entering_otp click_submit_default diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index 6d25a053a85..11ec4f488a9 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -562,6 +562,45 @@ def submit_prefilled_otp_code expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') end end + + context 'with SMS and international number that Verify does not think is valid' do + it 'rescues the VerifyError' do + allow(SmsOtpSenderJob).to receive(:perform_later) do |*args| + SmsOtpSenderJob.perform_now(*args) + end + PhoneVerification.adapter = FakeAdapter + allow(FakeAdapter).to receive(:post).and_return(FakeAdapter::ErrorResponse.new) + + user = create(:user, :signed_up, phone: '+212 661-289324') + sign_in_user(user) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + expect(page). + to have_content t('errors.messages.phone_unsupported') + end + end + + context 'user with Voice preference sends SMS, causing a Twilio error' do + it 'does not change their OTP delivery preference' do + allow(Figaro.env).to receive(:programmable_sms_countries).and_return('CA') + allow(VoiceOtpSenderJob).to receive(:perform_later) + allow(SmsOtpSenderJob).to receive(:perform_later) do |*args| + SmsOtpSenderJob.perform_now(*args) + end + PhoneVerification.adapter = FakeAdapter + allow(FakeAdapter).to receive(:post).and_return(FakeAdapter::ErrorResponse.new) + + user = create(:user, :signed_up, phone: '+17035551212', otp_delivery_preference: 'voice') + sign_in_user(user) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'voice') + + click_link t('links.two_factor_authentication.sms') + + expect(page).to have_content t('errors.messages.invalid_phone_number') + expect(user.reload.otp_delivery_preference).to eq 'voice' + end + end end describe 'when the user is not piv/cac enabled' do @@ -621,6 +660,7 @@ def submit_prefilled_otp_code end end + # TODO: readd profile redirect, modal tests describe 'signing in when user does not already have personal key' do # For example, when migrating users from another DB it 'displays personal key and redirects to profile' do diff --git a/spec/forms/otp_delivery_selection_form_spec.rb b/spec/forms/otp_delivery_selection_form_spec.rb index d281a5dcda7..d1a50d84ae0 100644 --- a/spec/forms/otp_delivery_selection_form_spec.rb +++ b/spec/forms/otp_delivery_selection_form_spec.rb @@ -66,56 +66,38 @@ end end - context 'with authentication context' do - context 'when otp_delivery_preference is the same as the user otp_delivery_preference' do - it 'does not update the user' do - user = build_stubbed(:user, otp_delivery_preference: 'sms') - form = OtpDeliverySelectionForm.new(user, phone_to_deliver_to, 'authentication') - - expect(UpdateUser).to_not receive(:new) - - form.submit(otp_delivery_preference: 'sms') - end - end - - context 'when otp_delivery_preference is different from the user otp_delivery_preference' do - it 'updates the user' do - user = build_stubbed(:user, otp_delivery_preference: 'voice') - form = OtpDeliverySelectionForm.new(user, phone_to_deliver_to, 'authentication') - attributes = { otp_delivery_preference: 'sms' } + context 'with voice preference and unsupported phone' do + it 'changes the otp_delivery_preference to sms' do + user = build_stubbed(:user, otp_delivery_preference: 'voice') + form = OtpDeliverySelectionForm.new( + user, + '+12423270143', + 'authentication' + ) + attributes = { otp_delivery_preference: 'sms' } - updated_user = instance_double(UpdateUser) - allow(UpdateUser).to receive(:new). - with(user: user, attributes: attributes).and_return(updated_user) + updated_user = instance_double(UpdateUser) + allow(UpdateUser).to receive(:new). + with(user: user, attributes: attributes).and_return(updated_user) - expect(updated_user).to receive(:call) + expect(updated_user).to receive(:call) - form.submit(otp_delivery_preference: 'sms') - end + form.submit(otp_delivery_preference: 'voice') end end - context 'with idv context' do - context 'when otp_delivery_preference is the same as the user otp_delivery_preference' do - it 'does not update the user' do - user = build_stubbed(:user, otp_delivery_preference: 'sms') - form = OtpDeliverySelectionForm.new(user, phone_to_deliver_to, 'idv') - - expect(UpdateUser).to_not receive(:new) - - form.submit(otp_delivery_preference: 'sms') - end - end - - context 'when otp_delivery_preference is different from the user otp_delivery_preference' do - it 'does not update the user' do - user = build_stubbed(:user, otp_delivery_preference: 'voice') - form = OtpDeliverySelectionForm.new(user, phone_to_deliver_to, 'idv') + context 'with voice preference and supported phone' do + it 'does not change the otp_delivery_preference to sms' do + user = build_stubbed(:user, otp_delivery_preference: 'voice') + form = OtpDeliverySelectionForm.new( + user, + '+17035551212', + 'authentication' + ) - expect(UpdateUser).to_not receive(:new) + expect(UpdateUser).to_not receive(:new) - form.submit(otp_delivery_preference: 'sms') - end + form.submit(otp_delivery_preference: 'voice') end end end diff --git a/spec/services/otp_delivery_preference_updater_spec.rb b/spec/services/otp_delivery_preference_updater_spec.rb new file mode 100644 index 00000000000..d8b28dc88b4 --- /dev/null +++ b/spec/services/otp_delivery_preference_updater_spec.rb @@ -0,0 +1,89 @@ +require 'rails_helper' + +describe OtpDeliveryPreferenceUpdater do + subject do + OtpDeliveryPreferenceUpdater.new( + user: build_stubbed(:user, otp_delivery_preference: 'sms'), + preference: 'sms', + context: 'authentication' + ) + end + + describe '#call' do + context 'with authentication context' do + context 'when otp_delivery_preference is the same as the user otp_delivery_preference' do + it 'does not update the user' do + expect(UpdateUser).to_not receive(:new) + + subject.call + end + end + + context 'when otp_delivery_preference is different from the user otp_delivery_preference' do + it 'updates the user' do + user = build_stubbed(:user, otp_delivery_preference: 'voice') + updater = OtpDeliveryPreferenceUpdater.new( + user: user, + preference: 'sms', + context: 'authentication' + ) + attributes = { otp_delivery_preference: 'sms' } + + updated_user = instance_double(UpdateUser) + allow(UpdateUser).to receive(:new). + with(user: user, attributes: attributes).and_return(updated_user) + + expect(updated_user).to receive(:call) + + updater.call + end + end + end + + context 'with idv context' do + context 'when otp_delivery_preference is the same as the user otp_delivery_preference' do + it 'does not update the user' do + user = build_stubbed(:user, otp_delivery_preference: 'sms') + updater = OtpDeliveryPreferenceUpdater.new( + user: user, + preference: 'sms', + context: 'idv' + ) + + expect(UpdateUser).to_not receive(:new) + + updater.call + end + end + + context 'when otp_delivery_preference is different from the user otp_delivery_preference' do + it 'does not update the user' do + user = build_stubbed(:user, otp_delivery_preference: 'voice') + updater = OtpDeliveryPreferenceUpdater.new( + user: user, + preference: 'sms', + context: 'idv' + ) + + expect(UpdateUser).to_not receive(:new) + + updater.call + end + end + end + + context 'when user is nil' do + it 'does not update the user' do + updater = OtpDeliveryPreferenceUpdater.new( + user: nil, + preference: 'sms', + context: 'idv' + ) + + expect(UpdateUser).to_not receive(:new) + + updater.call + end + end + end +end diff --git a/spec/services/phone_verification_spec.rb b/spec/services/phone_verification_spec.rb index dbbc692fd1a..86db9f3fe67 100644 --- a/spec/services/phone_verification_spec.rb +++ b/spec/services/phone_verification_spec.rb @@ -45,5 +45,21 @@ expect(error).to be_a(PhoneVerification::VerifyError) end end + + it 'raises VerifyError when response body is not valid JSON' do + PhoneVerification.adapter = FakeAdapter + phone = '17035551212' + code = '123456' + + allow(FakeAdapter).to receive(:post).and_return(FakeAdapter::EmptyResponse.new) + + expect { PhoneVerification.new(phone: phone, code: code).send_sms }.to raise_error do |error| + expect(error.code).to eq 0 + expect(error.message).to eq '' + expect(error.status).to eq 400 + expect(error.response).to eq '' + expect(error).to be_a(PhoneVerification::VerifyError) + end + end end end diff --git a/spec/support/fake_adapter.rb b/spec/support/fake_adapter.rb index f721126e560..628551296c9 100644 --- a/spec/support/fake_adapter.rb +++ b/spec/support/fake_adapter.rb @@ -20,5 +20,23 @@ def response_body message: 'Invalid number', }.to_json end + + def response_code + 400 + end + end + + class EmptyResponse + def success? + false + end + + def response_body + '' + end + + def response_code + 400 + end end end