diff --git a/app/controllers/concerns/phone_confirmation.rb b/app/controllers/concerns/phone_confirmation.rb index 01cdf412549..411492e1654 100644 --- a/app/controllers/concerns/phone_confirmation.rb +++ b/app/controllers/concerns/phone_confirmation.rb @@ -1,7 +1,7 @@ module PhoneConfirmation - def prompt_to_confirm_phone(phone:, context: 'confirmation', selected_delivery_method: nil) + def prompt_to_confirm_phone(phone:, selected_delivery_method: nil) user_session[:unconfirmed_phone] = phone - user_session[:context] = context + user_session[:context] = 'confirmation' redirect_to otp_send_url( otp_delivery_selection_form: { diff --git a/app/controllers/concerns/remember_device_concern.rb b/app/controllers/concerns/remember_device_concern.rb index 3bc01e76930..59542d5d7c1 100644 --- a/app/controllers/concerns/remember_device_concern.rb +++ b/app/controllers/concerns/remember_device_concern.rb @@ -2,7 +2,6 @@ module RememberDeviceConcern extend ActiveSupport::Concern def save_remember_device_preference - return if idv_context? return unless params[:remember_device] == 'true' cookies.encrypted[:remember_device] = { value: RememberDeviceCookie.new(user_id: current_user.id, created_at: Time.zone.now).to_json, diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index 28f1d05e06f..fe837ce757d 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -72,7 +72,7 @@ def reset_attempt_count_if_user_no_longer_locked_out def handle_valid_otp if authentication_context? handle_valid_otp_for_authentication_context - elsif idv_or_confirmation_context? || profile_context? + elsif confirmation_context? handle_valid_otp_for_confirmation_context end save_remember_device_preference @@ -128,7 +128,7 @@ def handle_valid_otp_for_authentication_context end def assign_phone - @updating_existing_number = old_phone.present? && !profile_context? + @updating_existing_number = old_phone.present? if @updating_existing_number && confirmation_context? phone_changed @@ -153,32 +153,10 @@ def phone_confirmed end def update_phone_attributes - if idv_or_profile_context? - update_idv_state - else - UpdateUser.new( - user: current_user, - attributes: { phone: user_session[:unconfirmed_phone], phone_confirmed_at: Time.zone.now } - ).call - end - end - - def update_idv_state - if idv_context? - confirm_idv_session_phone - elsif profile_context? - Idv::ProfileActivator.new(user: current_user).call - end - end - - def confirm_idv_session_phone - idv_session = Idv::Session.new( - user_session: user_session, - current_user: current_user, - issuer: sp_session[:issuer] - ) - idv_session.user_phone_confirmation = true - idv_session.params['phone_confirmed_at'] = Time.zone.now + UpdateUser.new( + user: current_user, + attributes: { phone: user_session[:unconfirmed_phone], phone_confirmed_at: Time.zone.now } + ).call end def reset_otp_session_data @@ -187,9 +165,7 @@ def reset_otp_session_data end def after_otp_verification_confirmation_url - if idv_context? - idv_review_url - elsif after_otp_action_required? + if after_otp_action_required? after_otp_action_url else after_sign_in_path_for(current_user) @@ -224,20 +200,17 @@ def direct_otp_code end def personal_key_unavailable? - idv_or_confirmation_context? || - profile_context? || - current_user.encrypted_recovery_code_digest.blank? + current_user.encrypted_recovery_code_digest.blank? end def unconfirmed_phone? - user_session[:unconfirmed_phone] && idv_or_confirmation_context? + user_session[:unconfirmed_phone] && confirmation_context? end # rubocop:disable MethodLength def phone_view_data { confirmation_for_phone_change: confirmation_for_phone_change?, - confirmation_for_idv: idv_context?, phone_number: display_phone_to_deliver_to, code_value: direct_otp_code, otp_delivery_preference: two_factor_authentication_method, @@ -245,7 +218,7 @@ def phone_view_data reenter_phone_number_path: reenter_phone_number_path, unconfirmed_phone: unconfirmed_phone?, totp_enabled: current_user.totp_enabled?, - remember_device_available: !idv_context?, + remember_device_available: true, account_reset_token: account_reset_token, }.merge(generic_data) end @@ -295,9 +268,7 @@ def decorated_user def reenter_phone_number_path locale = LinkLocaleResolver.locale - if idv_context? - idv_phone_path(locale: locale) - elsif current_user.phone.present? + if current_user.phone.present? manage_phone_path(locale: locale) else phone_setup_path(locale: locale) diff --git a/app/controllers/concerns/user_session_context.rb b/app/controllers/concerns/user_session_context.rb index ed40c55796b..a7021ddb1e6 100644 --- a/app/controllers/concerns/user_session_context.rb +++ b/app/controllers/concerns/user_session_context.rb @@ -16,20 +16,4 @@ def authentication_context? def confirmation_context? context == 'confirmation' end - - def idv_context? - context == 'idv' - end - - def idv_or_confirmation_context? - confirmation_context? || idv_context? - end - - def idv_or_profile_context? - idv_context? || profile_context? - end - - def profile_context? - context == 'profile' - end end diff --git a/app/controllers/concerns/verify_profile_concern.rb b/app/controllers/concerns/verify_profile_concern.rb index 709cf7603a7..598533a4fe9 100644 --- a/app/controllers/concerns/verify_profile_concern.rb +++ b/app/controllers/concerns/verify_profile_concern.rb @@ -10,7 +10,6 @@ def account_or_verify_profile_url end def account_or_verify_profile_route - return 'account' if idv_context? || profile_context? return 'account' unless profile_needs_verification? 'verify_account' end diff --git a/app/controllers/idv/otp_delivery_method_controller.rb b/app/controllers/idv/otp_delivery_method_controller.rb index 8978669f80c..b14e2db9c70 100644 --- a/app/controllers/idv/otp_delivery_method_controller.rb +++ b/app/controllers/idv/otp_delivery_method_controller.rb @@ -1,8 +1,8 @@ module Idv class OtpDeliveryMethodController < ApplicationController include IdvSession - include PhoneConfirmation + before_action :confirm_two_factor_authenticated before_action :confirm_phone_step_complete before_action :confirm_step_needed before_action :set_otp_delivery_method_presenter @@ -13,11 +13,8 @@ def new; end def create result = @otp_delivery_selection_form.submit(otp_delivery_selection_params) if result.success? - prompt_to_confirm_phone( - phone: @otp_delivery_selection_form.phone, - context: 'idv', - selected_delivery_method: @otp_delivery_selection_form.otp_delivery_preference - ) + save_delivery_preference_in_session + redirect_to idv_send_phone_otp_url else render :new end @@ -53,5 +50,10 @@ def set_otp_delivery_selection_form 'idv' ) end + + def save_delivery_preference_in_session + idv_session.phone_confirmation_otp_delivery_method = + @otp_delivery_selection_form.otp_delivery_preference + end end end diff --git a/app/controllers/idv/otp_verification_controller.rb b/app/controllers/idv/otp_verification_controller.rb new file mode 100644 index 00000000000..f769f18d8ae --- /dev/null +++ b/app/controllers/idv/otp_verification_controller.rb @@ -0,0 +1,113 @@ +module Idv + class OtpVerificationController < ApplicationController + include IdvSession + include TwoFactorAuthenticatable + + # Skip TwoFactorAuthenticatable before action that depends on MFA contexts + # to redirect to account url for signed in users. This controller does not + # use or maintain MFA contexts + skip_before_action :check_already_authenticated + + before_action :confirm_two_factor_authenticated + before_action :confirm_step_needed + before_action :handle_locked_out_user + before_action :confirm_otp_delivery_preference_selected + before_action :confirm_otp_sent, only: %i[show update] + before_action :set_code, only: %i[show update] + before_action :set_otp_verification_presenter, only: %i[show update] + + def new + result = send_phone_confirmation_otp_form.submit + analytics.track_event(Analytics::IDV_PHONE_CONFIRMATION_OTP_SENT, result.to_h) + if result.success? + redirect_to idv_otp_verification_url + elsif send_phone_confirmation_otp_form.user_locked_out? + handle_too_many_otp_sends + else + redirect_to idv_otp_delivery_method_url + end + end + + def show + # memoize the form so the ivar is available to the view + phone_confirmation_otp_verification_form + analytics.track_event(Analytics::IDV_PHONE_CONFIRMATION_OTP_VISIT) + end + + def update + result = phone_confirmation_otp_verification_form.submit(code: params[:code]) + analytics.track_event(Analytics::IDV_PHONE_CONFIRMATION_OTP_SUBMITTED, result.to_h) + if result.success? + redirect_to idv_review_url + else + handle_otp_confirmation_failure + end + end + + private + + def confirm_step_needed + return unless idv_session.user_phone_confirmation + redirect_to idv_review_url + end + + def handle_locked_out_user + reset_attempt_count_if_user_no_longer_locked_out + return unless decorated_user.locked_out? + handle_second_factor_locked_user 'generic' + false + end + + def confirm_otp_delivery_preference_selected + return if idv_session.params[:phone].present? && + idv_session.phone_confirmation_otp_delivery_method.present? + + redirect_to idv_otp_delivery_method_url + end + + def confirm_otp_sent + return if idv_session.phone_confirmation_otp.present? && + idv_session.phone_confirmation_otp_sent_at.present? + + redirect_to idv_send_phone_otp_url + end + + def set_code + return unless FeatureManagement.prefill_otp_codes? + @code = idv_session.phone_confirmation_otp + end + + def set_otp_verification_presenter + @presenter = OtpVerificationPresenter.new(idv_session: idv_session) + end + + def handle_otp_confirmation_failure + if decorated_user.locked_out? + handle_second_factor_locked_user('otp') + else + flash.now[:error] = t('devise.two_factor_authentication.invalid_otp') + render :show + end + end + + def send_phone_confirmation_otp_form + @send_phone_confirmation_otp_form ||= SendPhoneConfirmationOtpForm.new( + user: current_user, + idv_session: idv_session, + locale: user_locale + ) + end + + def phone_confirmation_otp_verification_form + @phone_confirmation_otp_verification_form ||= PhoneConfirmationOtpVerificationForm.new( + user: current_user, + idv_session: idv_session + ) + end + + def user_locale + available_locales = PhoneVerification::AVAILABLE_LOCALES + http_accept_language.language_region_compatible_from(available_locales) + end + end +end diff --git a/app/controllers/idv/review_controller.rb b/app/controllers/idv/review_controller.rb index 857f180cf55..88bd1dda2d3 100644 --- a/app/controllers/idv/review_controller.rb +++ b/app/controllers/idv/review_controller.rb @@ -15,11 +15,7 @@ def confirm_idv_steps_complete def confirm_idv_phone_confirmed return unless idv_session.address_verification_mechanism == 'phone' return if idv_session.phone_confirmed? - - prompt_to_confirm_phone( - phone: idv_session.params[:phone], - context: 'idv' - ) + redirect_to idv_otp_verification_path end def confirm_current_password diff --git a/app/controllers/idv/sessions_controller.rb b/app/controllers/idv/sessions_controller.rb index 660a261c37a..f55f46ddbdf 100644 --- a/app/controllers/idv/sessions_controller.rb +++ b/app/controllers/idv/sessions_controller.rb @@ -16,7 +16,6 @@ class SessionsController < ApplicationController delegate :attempts_exceeded?, to: :step, prefix: true def new - user_session[:context] = 'idv' set_idv_form @selected_state = user_session[:idv_jurisdiction] analytics.track_event(Analytics::IDV_BASIC_INFO_VISIT) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index c0be8f8155c..a7bfe5f4870 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -38,6 +38,7 @@ def redirect_to_account_or_verify_profile_url end def profile_or_identity_needs_verification? + return false unless @authorize_form.loa3_requested? profile_needs_verification? || identity_needs_verification? end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index c6bc5ab98ab..3bce7a68583 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -81,6 +81,7 @@ def redirect_to_account_or_verify_profile_url end def profile_or_identity_needs_verification? + return false unless loa3_requested? profile_needs_verification? || identity_needs_verification? end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 8e661ac75f5..de65c743a6e 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -26,7 +26,7 @@ def create private def confirm_two_factor_enabled - return if confirming_phone? || phone_enabled? + return if confirmation_context? || phone_enabled? if current_user.two_factor_enabled? && !phone_enabled? && user_signed_in? return redirect_to user_two_factor_authentication_url @@ -35,10 +35,6 @@ def confirm_two_factor_enabled redirect_to phone_setup_url end - def confirming_phone? - idv_context? || confirmation_context? - end - def phone_enabled? current_user.phone_enabled? end diff --git a/app/forms/idv/phone_confirmation_otp_verification_form.rb b/app/forms/idv/phone_confirmation_otp_verification_form.rb new file mode 100644 index 00000000000..a26586c67c7 --- /dev/null +++ b/app/forms/idv/phone_confirmation_otp_verification_form.rb @@ -0,0 +1,61 @@ +module Idv + class PhoneConfirmationOtpVerificationForm + attr_reader :user, :idv_session, :code + + def initialize(user:, idv_session:) + @user = user + @idv_session = idv_session + end + + def submit(code:) + @code = code + success = code_valid? + if success + idv_session.user_phone_confirmation = true + clear_second_factor_attempts + else + increment_second_factor_attempts + end + FormResponse.new(success: success, errors: {}, extra: extra_analytics_attributes) + end + + private + + def code_valid? + return false if code_expired? + code_matches? + end + + # Ignore duplicate method call on Time.zone :reek:DuplicateMethodCall + def code_expired? + sent_at_time = Time.zone.parse(idv_session.phone_confirmation_otp_sent_at) + expiration_time = sent_at_time + Figaro.env.otp_valid_for.to_i.minutes + Time.zone.now > expiration_time + end + + def code_matches? + Devise.secure_compare(code, idv_session.phone_confirmation_otp) + end + + def clear_second_factor_attempts + UpdateUser.new(user: user, attributes: { second_factor_attempts_count: 0 }).call + end + + def increment_second_factor_attempts + user.second_factor_attempts_count += 1 + attributes = {} + attributes[:second_factor_locked_at] = Time.zone.now if user.max_login_attempts? + + UpdateUser.new(user: user, attributes: attributes).call + end + + def extra_analytics_attributes + { + code_expired: code_expired?, + code_matches: code_matches?, + second_factor_attempts_count: user.second_factor_attempts_count, + second_factor_locked_at: user.second_factor_locked_at, + } + end + end +end diff --git a/app/forms/idv/send_phone_confirmation_otp_form.rb b/app/forms/idv/send_phone_confirmation_otp_form.rb new file mode 100644 index 00000000000..9310888d8be --- /dev/null +++ b/app/forms/idv/send_phone_confirmation_otp_form.rb @@ -0,0 +1,106 @@ +module Idv + # :reek:InstanceVariableAssumption + class SendPhoneConfirmationOtpForm + include ActiveModel::Model + + attr_accessor :user, :idv_session, :locale + + validates :otp_delivery_preference, inclusion: { in: %i[sms voice] } + + def initialize(user:, idv_session:, locale:) + self.user = user + self.idv_session = idv_session + self.locale = locale + end + + def submit + return handle_valid_otp_delivery_preference if valid? + FormResponse.new(success: valid?, errors: errors.messages, extra: extra_analytics_attributes) + end + + def user_locked_out? + @user_locked_out + end + + private + + def handle_valid_otp_delivery_preference + otp_rate_limiter.reset_count_and_otp_last_sent_at if user.decorate.no_longer_locked_out? + + return too_many_otp_sends_response if rate_limit_exceeded? + otp_rate_limiter.increment + return too_many_otp_sends_response if rate_limit_exceeded? + + send_otp + FormResponse.new(success: true, errors: {}, extra: extra_analytics_attributes) + end + + def too_many_otp_sends_response + FormResponse.new( + success: false, + errors: {}, + extra: extra_analytics_attributes + ) + end + + def rate_limit_exceeded? + if otp_rate_limiter.exceeded_otp_send_limit? + otp_rate_limiter.lock_out_user + return @user_locked_out = true + end + false + end + + def otp_rate_limiter + @otp_rate_limiter ||= OtpRateLimiter.new(user: user, phone: phone) + end + + def send_otp + idv_session.phone_confirmation_otp = PhoneConfirmationOtpGenerator.generate_otp + idv_session.phone_confirmation_otp_sent_at = Time.zone.now.to_s + if otp_delivery_preference == :sms + send_sms_otp + elsif otp_delivery_preference == :voice + send_voice_otp + end + end + + def send_sms_otp + SmsOtpSenderJob.perform_later( + code: idv_session.phone_confirmation_otp, + phone: phone, + otp_created_at: idv_session.phone_confirmation_otp_sent_at, + message: 'jobs.sms_otp_sender_job.verify_message', + locale: locale + ) + end + + def send_voice_otp + VoiceOtpSenderJob.perform_later( + code: idv_session.phone_confirmation_otp, + phone: phone, + otp_created_at: idv_session.phone_confirmation_otp_sent_at, + locale: locale + ) + end + + def phone + @phone ||= PhoneFormatter.format(idv_session.params[:phone]) + end + + def otp_delivery_preference + return :sms if PhoneNumberCapabilities.new(phone).sms_only? + @otp_delivery_preference ||= idv_session.phone_confirmation_otp_delivery_method.to_sym + end + + def extra_analytics_attributes + parsed_phone = Phonelib.parse(phone) + { + otp_delivery_preference: otp_delivery_preference, + country_code: parsed_phone.country_code, + area_code: parsed_phone.area_code, + rate_limit_exceeded: rate_limit_exceeded?, + } + end + end +end diff --git a/app/presenters/idv/otp_verification_presenter.rb b/app/presenters/idv/otp_verification_presenter.rb new file mode 100644 index 00000000000..3befebb1570 --- /dev/null +++ b/app/presenters/idv/otp_verification_presenter.rb @@ -0,0 +1,28 @@ +module Idv + class OtpVerificationPresenter + include ActionView::Helpers::TagHelper + include ActionView::Helpers::TranslationHelper + + attr_reader :idv_session + + def initialize(idv_session:) + @idv_session = idv_session + end + + def phone_number_message + t("instructions.mfa.#{otp_delivery_preference}.number_message", + number: content_tag(:strong, phone_number), + expiration: Figaro.env.otp_valid_for) + end + + private + + def phone_number + PhoneFormatter.format(idv_session.params[:phone]) + end + + def otp_delivery_preference + idv_session.phone_confirmation_otp_delivery_method + end + end +end diff --git a/app/presenters/two_factor_auth_code/phone_delivery_presenter.rb b/app/presenters/two_factor_auth_code/phone_delivery_presenter.rb index 286f71eb134..03befd982f4 100644 --- a/app/presenters/two_factor_auth_code/phone_delivery_presenter.rb +++ b/app/presenters/two_factor_auth_code/phone_delivery_presenter.rb @@ -33,8 +33,6 @@ def cancel_link locale = LinkLocaleResolver.locale if confirmation_for_phone_change || reauthn account_path(locale: locale) - elsif confirmation_for_idv - idv_cancel_path(locale: locale) else sign_out_path(locale: locale) end @@ -50,7 +48,6 @@ def cancel_link :account_reset_token, :confirmation_for_phone_change, :voice_otp_delivery_unsupported, - :confirmation_for_idv ) def account_reset_link diff --git a/app/services/analytics.rb b/app/services/analytics.rb index de4444d0039..ea1affe872c 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -71,6 +71,9 @@ def browser IDV_JURISDICTION_FORM = 'IdV: jurisdiction form submitted'.freeze IDV_PHONE_CONFIRMATION_FORM = 'IdV: phone confirmation form'.freeze IDV_PHONE_CONFIRMATION_VENDOR = 'IdV: phone confirmation vendor'.freeze + IDV_PHONE_CONFIRMATION_OTP_SENT = 'IdV: phone confirmation otp sent'.freeze + IDV_PHONE_CONFIRMATION_OTP_SUBMITTED = 'IdV: phone confirmation otp submitted'.freeze + IDV_PHONE_CONFIRMATION_OTP_VISIT = 'IdV: phone confirmation otp visitted'.freeze IDV_PHONE_RECORD_VISIT = 'IdV: phone of record visited'.freeze IDV_REVIEW_COMPLETE = 'IdV: review complete'.freeze IDV_REVIEW_VISIT = 'IdV: review info visited'.freeze diff --git a/app/services/idv/phone_confirmation_otp_generator.rb b/app/services/idv/phone_confirmation_otp_generator.rb new file mode 100644 index 00000000000..62f5f224f22 --- /dev/null +++ b/app/services/idv/phone_confirmation_otp_generator.rb @@ -0,0 +1,8 @@ +module Idv + class PhoneConfirmationOtpGenerator + def self.generate_otp + digits = Devise.direct_otp_length + SecureRandom.random_number(10**digits).to_s.rjust(digits, '0') + end + end +end diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index bdc4c12ecf9..a6e59634c29 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -8,6 +8,9 @@ class Session params vendor_phone_confirmation user_phone_confirmation + phone_confirmation_otp_delivery_method + phone_confirmation_otp_sent_at + phone_confirmation_otp pii profile_confirmation profile_id diff --git a/app/services/otp_delivery_preference_updater.rb b/app/services/otp_delivery_preference_updater.rb index 2713dbbb2a2..20ca377ed07 100644 --- a/app/services/otp_delivery_preference_updater.rb +++ b/app/services/otp_delivery_preference_updater.rb @@ -16,14 +16,10 @@ def call def should_update_user? return false unless user - otp_delivery_preference_changed? && !idv_context? + otp_delivery_preference_changed? end def otp_delivery_preference_changed? preference != user.otp_delivery_preference end - - def idv_context? - context == 'idv' - end end diff --git a/app/views/idv/otp_verification/show.html.slim b/app/views/idv/otp_verification/show.html.slim new file mode 100644 index 00000000000..0659c101805 --- /dev/null +++ b/app/views/idv/otp_verification/show.html.slim @@ -0,0 +1,30 @@ +- title t('titles.enter_2fa_code') + +h1.h3.my0 = t('devise.two_factor_authentication.header_text') + +p == @presenter.phone_number_message + += form_tag(:idv_otp_verification, method: :put, role: 'form', class: 'mt3') do + = label_tag 'code', \ + t('simple_form.required.html') + t('forms.two_factor.code'), \ + class: 'block bold' + .col-12.sm-col-5.mb2.sm-mb0.sm-mr-20p.inline-block + = text_field_tag(:code, '', value: @code, required: true, + autofocus: true, pattern: '[0-9]*', class: 'col-12 field monospace mfa', + 'aria-describedby': 'code-instructs', maxlength: Devise.direct_otp_length, + autocomplete: 'off', type: 'tel') + = submit_tag t('forms.buttons.submit.default'), class: 'btn btn-primary align-top sm-col-6 col-12' + br + br + = link_to(t('links.two_factor_authentication.get_another_code'), idv_send_phone_otp_path, + class: 'btn btn-link btn-border ico ico-refresh text-decoration-none', + form_class: 'inline-block') + +br + +p = link_to t('forms.two_factor.try_again'), idv_phone_path +p = link_to t('two_factor_authentication.choose_another_option'), idv_otp_delivery_method_url + +.mt3.border-top + .mt1 + = link_to t('links.cancel'), idv_cancel_path diff --git a/config/routes.rb b/config/routes.rb index d4e78b4bad5..1026bb6554e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -188,6 +188,9 @@ put '/phone' => 'phone#create' get '/phone/result' => 'phone#show' get '/phone/failure/:reason' => 'phone#failure', as: :phone_failure + get '/phone_confirmation/send' => 'otp_verification#new', as: :send_phone_otp + get '/phone_confirmation' => 'otp_verification#show', as: :otp_verification + put '/phone_confirmation' => 'otp_verification#update', as: :nil get '/review' => 'review#new' put '/review' => 'review#create' get '/session' => 'sessions#new' diff --git a/spec/controllers/concerns/user_session_context_spec.rb b/spec/controllers/concerns/user_session_context_spec.rb index 3d3129e1de5..cf73ff5fd3e 100644 --- a/spec/controllers/concerns/user_session_context_spec.rb +++ b/spec/controllers/concerns/user_session_context_spec.rb @@ -17,7 +17,6 @@ def user_session describe UserSessionContext do let(:controller) { DummyController.new } let(:confirmation) { { context: 'confirmation' } } - let(:idv) { { context: 'idv' } } after { controller.set({}) } @@ -35,31 +34,16 @@ def user_session it 'returns true when context is authentication, false otherwise' do expect(controller.authentication_context?).to be(true) expect(controller.confirmation_context?).to be(false) - expect(controller.idv_or_confirmation_context?).to be(false) end end describe '#confirmation_context?' do it 'returns true if context matches, false otherwise' do expect(controller.confirmation_context?).to be(false) - expect(controller.idv_or_confirmation_context?).to be(false) controller.set(confirmation) expect(controller.confirmation_context?).to be(true) - expect(controller.idv_or_confirmation_context?).to be(true) - end - end - - describe '#idv_context?' do - it 'returns true if context matches, false otherwise' do - expect(controller.idv_context?).to be(false) - expect(controller.idv_or_confirmation_context?).to be(false) - - controller.set(idv) - - expect(controller.idv_context?).to be(true) - expect(controller.idv_or_confirmation_context?).to be(true) end end end diff --git a/spec/controllers/idv/confirmations_controller_spec.rb b/spec/controllers/idv/confirmations_controller_spec.rb index f7c79a2a046..dcef2d33dba 100644 --- a/spec/controllers/idv/confirmations_controller_spec.rb +++ b/spec/controllers/idv/confirmations_controller_spec.rb @@ -22,7 +22,6 @@ def stub_idv_session idv_session.profile_id = profile.id idv_session.personal_key = profile.personal_key allow(subject).to receive(:idv_session).and_return(idv_session) - allow(subject).to receive(:user_session).and_return(context: 'idv') end let(:password) { 'sekrit phrase' } diff --git a/spec/controllers/idv/otp_delivery_method_controller_spec.rb b/spec/controllers/idv/otp_delivery_method_controller_spec.rb index 234c7d4218e..c8823f868ea 100644 --- a/spec/controllers/idv/otp_delivery_method_controller_spec.rb +++ b/spec/controllers/idv/otp_delivery_method_controller_spec.rb @@ -98,7 +98,8 @@ context 'user has selected sms' do it 'redirects to the otp send path for sms' do post :create, params: params - expect(response).to redirect_to otp_send_path(params) + expect(subject.idv_session.phone_confirmation_otp_delivery_method).to eq('sms') + expect(response).to redirect_to idv_send_phone_otp_path end end @@ -113,7 +114,8 @@ it 'redirects to the otp send path for voice' do post :create, params: params - expect(response).to redirect_to otp_send_path(params) + expect(subject.idv_session.phone_confirmation_otp_delivery_method).to eq('voice') + expect(response).to redirect_to idv_send_phone_otp_path end end diff --git a/spec/controllers/idv/otp_verification_controller_spec.rb b/spec/controllers/idv/otp_verification_controller_spec.rb new file mode 100644 index 00000000000..09645f4626b --- /dev/null +++ b/spec/controllers/idv/otp_verification_controller_spec.rb @@ -0,0 +1,167 @@ +require 'rails_helper' + +require 'rails_helper' + +describe Idv::OtpVerificationController do + let(:user) { build(:user) } + + let(:phone) { '5555555000' } + let(:user_phone_confirmation) { false } + let(:phone_confirmation_otp_delivery_method) { 'sms' } + let(:phone_confirmation_otp) { nil } + let(:phone_confirmation_otp_sent_at) { nil } + + before do + stub_analytics + allow(@analytics).to receive(:track_event) + + sign_in(user) + stub_verify_steps_one_and_two(user) + subject.idv_session.params[:phone] = '2255555000' + subject.idv_session.vendor_phone_confirmation = true + subject.idv_session.user_phone_confirmation = user_phone_confirmation + subject.idv_session.phone_confirmation_otp_delivery_method = + phone_confirmation_otp_delivery_method + subject.idv_session.phone_confirmation_otp = phone_confirmation_otp + subject.idv_session.phone_confirmation_otp_sent_at = phone_confirmation_otp_sent_at + end + + describe '#new' do + context 'the user has not selected a delivery method' do + let(:phone_confirmation_otp_delivery_method) { nil } + + it 'redirects to otp deleivery method selection' do + get :new + expect(response).to redirect_to(idv_otp_delivery_method_path) + end + end + + context 'the user has already confirmed their phone' do + let(:user_phone_confirmation) { true } + + it 'redirects to the review step' do + get :new + expect(response).to redirect_to(idv_review_path) + end + end + + context 'the delivery method is invalid' do + let(:phone_confirmation_otp_delivery_method) { 'noise' } + + it 'redirects to the otp delivery method selection' do + get :new + expect(response).to redirect_to(idv_otp_delivery_method_path) + end + end + + it 'tracks an analytics event' do + get :new + + expected_result = { + success: true, + errors: {}, + otp_delivery_preference: :sms, + country_code: '1', + area_code: '225', + rate_limit_exceeded: false, + } + + expect(@analytics).to have_received(:track_event).with( + Analytics::IDV_PHONE_CONFIRMATION_OTP_SENT, + expected_result + ) + end + end + + describe '#show' do + let(:phone_confirmation_otp) { '777777' } + let(:phone_confirmation_otp_sent_at) { Time.zone.now.to_s } + + context 'the user has not selected a delivery method' do + let(:phone_confirmation_otp_delivery_method) { nil } + + it 'redirects to otp deleivery method selection' do + get :show + expect(response).to redirect_to(idv_otp_delivery_method_path) + end + end + + context 'the user has not been sent an otp' do + let(:phone_confirmation_otp) { nil } + let(:phone_confirmation_otp_sent_at) { nil } + + it 'redirects to the otp send path' do + get :show + expect(response).to redirect_to(idv_send_phone_otp_path) + end + end + + context 'the user has already confirmed their phone' do + let(:user_phone_confirmation) { true } + + it 'redirects to the review step' do + get :show + expect(response).to redirect_to(idv_review_path) + end + end + + it 'tracks an analytics event' do + get :show + + expect(@analytics).to have_received(:track_event).with( + Analytics::IDV_PHONE_CONFIRMATION_OTP_VISIT + ) + end + end + + describe '#update' do + let(:phone_confirmation_otp) { '777777' } + let(:phone_confirmation_otp_sent_at) { Time.zone.now.to_s } + + context 'the user has not selected a delivery method' do + let(:phone_confirmation_otp_delivery_method) { nil } + + it 'redirects to otp deleivery method selection' do + put :update, params: { code: phone_confirmation_otp } + expect(response).to redirect_to(idv_otp_delivery_method_path) + end + end + + context 'the user has not been sent an otp' do + let(:phone_confirmation_otp) { nil } + let(:phone_confirmation_otp_sent_at) { nil } + + it 'redirects to the otp send path' do + put :update, params: { code: phone_confirmation_otp } + expect(response).to redirect_to(idv_send_phone_otp_path) + end + end + + context 'the user has already confirmed their phone' do + let(:user_phone_confirmation) { true } + + it 'redirects to the review step' do + put :update, params: { code: phone_confirmation_otp } + expect(response).to redirect_to(idv_review_path) + end + end + + it 'tracks an analytics event' do + put :update, params: { code: phone_confirmation_otp } + + expected_result = { + success: true, + errors: {}, + code_expired: false, + code_matches: true, + second_factor_attempts_count: 0, + second_factor_locked_at: nil, + } + + expect(@analytics).to have_received(:track_event).with( + Analytics::IDV_PHONE_CONFIRMATION_OTP_SUBMITTED, + expected_result + ) + end + end +end diff --git a/spec/controllers/idv/review_controller_spec.rb b/spec/controllers/idv/review_controller_spec.rb index f0a6f8c9e0b..62580dae5c6 100644 --- a/spec/controllers/idv/review_controller_spec.rb +++ b/spec/controllers/idv/review_controller_spec.rb @@ -130,9 +130,7 @@ def show it 'redirects to phone confirmation' do get :show - expect(response).to redirect_to otp_send_path( - otp_delivery_selection_form: { otp_delivery_preference: :sms } - ) + expect(response).to redirect_to idv_otp_verification_path end end end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index cae8cb32be7..38a437754ad 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -432,144 +432,5 @@ end end end - - context 'idv phone confirmation' do - before do - user = sign_in_as_user - idv_session = Idv::Session.new( - user_session: subject.user_session, current_user: user, issuer: nil - ) - idv_session.params = { 'phone' => '+1 (703) 555-5555' } - subject.user_session[:unconfirmed_phone] = '+1 (703) 555-5555' - subject.user_session[:context] = 'idv' - @previous_phone_confirmed_at = subject.current_user.phone_confirmed_at - allow(subject).to receive(:idv_session).and_return(idv_session) - stub_analytics - allow(@analytics).to receive(:track_event) - allow(subject).to receive(:create_user_event) - subject.current_user.create_direct_otp - allow(UserMailer).to receive(:phone_changed) - end - - context 'user enters a valid code' do - before do - post( - :create, - params: { - code: subject.current_user.direct_otp, - otp_delivery_preference: 'sms', - } - ) - end - - it 'resets otp session data' do - expect(subject.user_session[:unconfirmed_phone]).to be_nil - expect(subject.user_session[:context]).to eq 'authentication' - end - - it 'tracks the OTP verification event' do - properties = { - success: true, - errors: {}, - confirmation_for_phone_change: false, - context: 'idv', - multi_factor_auth_method: 'sms', - } - - expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH, properties) - - expect(subject).to have_received(:create_user_event).with(:phone_confirmed) - end - - it 'does not track a phone change event' do - expect(subject).to_not have_received(:create_user_event).with(:phone_changed) - end - - it 'updates idv session phone_confirmed_at attribute' do - expect(subject.user_session[:idv][:params]['phone_confirmed_at']).to_not be_nil - end - - it 'updates idv session user_phone_confirmation attributes' do - expect(subject.user_session[:idv][:user_phone_confirmation]).to eq(true) - end - - it 'does not update user phone attributes' do - expect(subject.current_user.reload.phone).to eq '+1 202-555-1212' - expect(subject.current_user.reload.phone_confirmed_at).to eq @previous_phone_confirmed_at - - configuration = subject.current_user.reload.phone_configuration - expect(configuration.phone).to eq '+1 202-555-1212' - expect(configuration.confirmed_at).to eq @previous_phone_confirmed_at - end - - it 'redirects to idv_review_path' do - expect(response).to redirect_to(idv_review_path) - end - - it 'does not call UserMailer' do - expect(UserMailer).to_not have_received(:phone_changed) - end - end - - context 'user enters an invalid code' do - before { post :create, params: { code: '999', otp_delivery_preference: 'sms' } } - - it 'increments second_factor_attempts_count' do - expect(subject.current_user.reload.second_factor_attempts_count).to eq 1 - end - - it 'does not clear session data' do - expect(subject.user_session[:unconfirmed_phone]).to eq('+1 (703) 555-5555') - end - - it 'does not update user phone or phone_confirmed_at attributes' do - expect(subject.current_user.phone).to eq('+1 202-555-1212') - expect(subject.current_user.phone_confirmed_at).to eq(@previous_phone_confirmed_at) - - configuration = subject.current_user.reload.phone_configuration - expect(configuration.phone).to eq '+1 202-555-1212' - expect(configuration.confirmed_at).to eq @previous_phone_confirmed_at - - expect(subject.idv_session.params['phone_confirmed_at']).to be_nil - end - - it 'renders :show' do - expect(response).to render_template(:show) - end - - it 'displays error flash notice' do - expect(flash[:error]).to eq t('devise.two_factor_authentication.invalid_otp') - end - - it 'tracks an event' do - properties = { - success: false, - errors: {}, - confirmation_for_phone_change: false, - context: 'idv', - multi_factor_auth_method: 'sms', - } - - expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH, properties) - end - end - - context 'with remember_device in the params' do - it 'ignores the param and does not save an encrypted cookie' do - post( - :create, - params: { - code: subject.current_user.direct_otp, - otp_delivery_preference: 'sms', - remember_device: 'true', - } - ) - - expect(cookies[:remember_device]).to be_nil - end - end - end end end diff --git a/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb b/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb index 4639c6bf133..bcda4e217ca 100644 --- a/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb +++ b/spec/features/idv/steps/phone_otp_delivery_selection_step_spec.rb @@ -13,7 +13,7 @@ choose_idv_otp_delivery_method_sms expect(page).to have_content(t('devise.two_factor_authentication.header_text')) - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + expect(current_path).to eq(idv_otp_verification_path) end end @@ -27,7 +27,7 @@ choose_idv_otp_delivery_method_voice expect(page).to have_content(t('devise.two_factor_authentication.header_text')) - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :voice)) + expect(current_path).to eq(idv_otp_verification_path) end end @@ -49,6 +49,19 @@ end end + it 'does not modify the otp column on the user model when sending an OTP' do + user = user_with_2fa + + start_idv_from_sp + complete_idv_steps_before_phone_otp_delivery_selection_step(user) + + old_direct_otp = user.direct_otp + choose_idv_otp_delivery_method_sms + user.reload + + expect(user.direct_otp).to eq(old_direct_otp) + end + context 'cancelling IdV' do it_behaves_like 'cancel at idv step', :phone_otp_delivery_selection it_behaves_like 'cancel at idv step', :phone_otp_delivery_selection, :oidc diff --git a/spec/features/idv/steps/phone_otp_verification_step_spec.rb b/spec/features/idv/steps/phone_otp_verification_step_spec.rb index 86b348aec14..c454d74e93f 100644 --- a/spec/features/idv/steps/phone_otp_verification_step_spec.rb +++ b/spec/features/idv/steps/phone_otp_verification_step_spec.rb @@ -3,6 +3,12 @@ feature 'phone otp verification step spec', :idv_job do include IdvStepHelper + let(:otp_code) { '777777' } + + before do + allow(Idv::PhoneConfirmationOtpGenerator).to receive(:generate_otp).and_return(otp_code) + end + it 'requires the user to enter the correct otp before continuing' do user = user_with_2fa @@ -11,25 +17,109 @@ # Attempt to bypass the step visit idv_review_path - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + expect(current_path).to eq(idv_otp_verification_path) # Enter an incorrect otp fill_in 'code', with: '000000' click_submit_default expect(page).to have_content(t('devise.two_factor_authentication.invalid_otp')) - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + expect(current_path).to eq(idv_otp_verification_path) # Enter the correct code - enter_correct_otp_code_for_user(user) + fill_in 'code', with: '777777' + click_submit_default expect(page).to have_content(t('idv.titles.session.review')) expect(page).to have_current_path(idv_review_path) end + it 'rate limits the number of OTPs that can be sent' do + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + + user = user_with_2fa + + start_idv_from_sp + complete_idv_steps_before_phone_otp_verification_step + + max_attempts.times do + click_link t('links.two_factor_authentication.get_another_code') + end + + expect(page).to have_content t('titles.account_locked') + expect(page).to have_content t( + 'devise.two_factor_authentication.max_otp_requests_reached' + ) + + expect_rate_limit_circumvention_to_be_disallowed(user) + expect_rate_limit_to_expire(user) + end + + it 'limits the number of OTP attempts' do + user = user_with_2fa + + start_idv_from_sp + complete_idv_steps_before_phone_otp_verification_step(user) + + 3.times do + fill_in('code', with: 'bad-code') + click_button t('forms.buttons.submit.default') + end + + expect(page).to have_content t('titles.account_locked') + expect(page). + to have_content t('devise.two_factor_authentication.max_otp_login_attempts_reached') + + expect_rate_limit_circumvention_to_be_disallowed(user) + expect_rate_limit_to_expire(user) + end + + it 'rejects OTPs after they are expired' do + expiration_minutes = Figaro.env.otp_valid_for.to_i + 1 + + start_idv_from_sp + complete_idv_steps_before_phone_otp_verification_step + + Timecop.travel(expiration_minutes.minutes.from_now) do + fill_in(:code, with: otp_code) + click_button t('forms.buttons.submit.default') + + expect(page).to have_content(t('devise.two_factor_authentication.invalid_otp')) + expect(page).to have_current_path(idv_otp_verification_path) + end + end + context 'cancelling IdV' do it_behaves_like 'cancel at idv step', :phone_otp_verification it_behaves_like 'cancel at idv step', :phone_otp_verification, :oidc it_behaves_like 'cancel at idv step', :phone_otp_verification, :saml end + + def expect_rate_limit_circumvention_to_be_disallowed(user) + # Attempting to send another OTP does not send an OTP and shows lockout message + allow(SmsOtpSenderJob).to receive(:perform_now) + allow(SmsOtpSenderJob).to receive(:perform_later) + + start_idv_from_sp + complete_idv_steps_before_phone_otp_verification_step(user) + + expect(page).to have_content t('titles.account_locked') + expect(SmsOtpSenderJob).to_not have_received(:perform_now) + expect(SmsOtpSenderJob).to_not have_received(:perform_later) + end + + def expect_rate_limit_to_expire(user) + # Returning after session and lockout expires allows you to try again + retry_minutes = Figaro.env.lockout_period_in_minutes.to_i + 1 + Timecop.travel retry_minutes.minutes.from_now do + start_idv_from_sp + complete_idv_steps_before_phone_otp_verification_step(user) + + fill_in(:code, with: otp_code) + click_submit_default + + expect(page).to have_content(t('idv.titles.session.review')) + expect(current_path).to eq(idv_review_path) + end + end end diff --git a/spec/features/idv/steps/phone_step_spec.rb b/spec/features/idv/steps/phone_step_spec.rb index a4b775e522d..0328ea70b83 100644 --- a/spec/features/idv/steps/phone_step_spec.rb +++ b/spec/features/idv/steps/phone_step_spec.rb @@ -71,6 +71,8 @@ end it 'is not re-entrant after confirming OTP' do + allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) + user = user_with_2fa start_idv_from_sp @@ -78,7 +80,7 @@ fill_out_phone_form_ok click_idv_continue choose_idv_otp_delivery_method_sms - enter_correct_otp_code_for_user(user) + click_submit_default visit idv_phone_path expect(page).to have_content(t('idv.titles.session.review')) diff --git a/spec/features/idv/steps/usps_step_spec.rb b/spec/features/idv/steps/usps_step_spec.rb index e8e1686fa0a..fdd31b3cb4b 100644 --- a/spec/features/idv/steps/usps_step_spec.rb +++ b/spec/features/idv/steps/usps_step_spec.rb @@ -43,6 +43,7 @@ def complete_idv_and_return_to_usps_step click_continue click_acknowledge_personal_key visit root_path + click_on t('idv.buttons.cancel') first(:link, t('links.sign_out')).click sign_in_live_with_2fa(user) click_on t('idv.messages.usps.resend') diff --git a/spec/features/idv/usps_disabled_spec.rb b/spec/features/idv/usps_disabled_spec.rb index ee42002642b..b63c7cab28f 100644 --- a/spec/features/idv/usps_disabled_spec.rb +++ b/spec/features/idv/usps_disabled_spec.rb @@ -18,6 +18,8 @@ end it 'allows verification without the option to confirm address with usps' do + allow(Idv::PhoneConfirmationOtpGenerator).to receive(:generate_otp).and_return('777777') + user = user_with_2fa start_idv_from_sp complete_idv_steps_before_phone_step(user) @@ -32,7 +34,8 @@ expect(page).to_not have_content(t('idv.form.activate_by_mail')) choose_idv_otp_delivery_method_sms - enter_correct_otp_code_for_user(user) + fill_in(:code, with: '777777') + click_submit_default fill_in 'Password', with: user.password click_continue click_acknowledge_personal_key diff --git a/spec/features/two_factor_authentication/remember_device_spec.rb b/spec/features/two_factor_authentication/remember_device_spec.rb index 2f31bb951a0..5bdacf382ea 100644 --- a/spec/features/two_factor_authentication/remember_device_spec.rb +++ b/spec/features/two_factor_authentication/remember_device_spec.rb @@ -87,7 +87,7 @@ def remember_device_and_sign_out_user end it 'requires 2FA and does not offer the option to remember device' do - expect(current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + expect(current_path).to eq(idv_otp_verification_path) expect(page).to_not have_content( t('forms.messages.remember_device', duration: Figaro.env.remember_device_expiration_days!) ) diff --git a/spec/forms/idv/phone_confirmation_otp_verification_form_spec.rb b/spec/forms/idv/phone_confirmation_otp_verification_form_spec.rb new file mode 100644 index 00000000000..549c7ca0447 --- /dev/null +++ b/spec/forms/idv/phone_confirmation_otp_verification_form_spec.rb @@ -0,0 +1,107 @@ +require 'rails_helper' + +describe Idv::PhoneConfirmationOtpVerificationForm do + let(:user) { create(:user, :signed_up) } + let(:idv_session) { double(Idv::Session) } + let(:phone_confirmation_otp) { '123456' } + let(:phone_confirmation_otp_sent_at) { Time.zone.now.to_s } + + before do + allow(idv_session).to receive(:phone_confirmation_otp). + and_return(phone_confirmation_otp) + allow(idv_session).to receive(:phone_confirmation_otp_sent_at). + and_return(phone_confirmation_otp_sent_at) + end + + describe '#submit' do + def try_submit(code) + described_class.new( + user: user, idv_session: idv_session + ).submit(code: code) + end + + context 'when the code matches' do + it 'returns a successful result' do + expect(idv_session).to receive(:user_phone_confirmation=).with(true) + + result = try_submit(phone_confirmation_otp) + + expect(result.success?).to eq(true) + end + + it 'clears the second factor attempts' do + expect(idv_session).to receive(:user_phone_confirmation=).with(true) + + user.update(second_factor_attempts_count: 4) + + try_submit(phone_confirmation_otp) + + expect(user.reload.second_factor_attempts_count).to eq(0) + end + end + + context 'when the code does not match' do + it 'returns an unsuccessful result' do + expect(idv_session).to_not receive(:user_phone_confirmation=) + + result = try_submit('xxxxxx') + + expect(result.success?).to eq(false) + end + + it 'increments second factor attempts' do + 2.times do + try_submit('xxxxxx') + end + + user.reload + + expect(user.second_factor_attempts_count).to eq(2) + expect(user.second_factor_locked_at).to eq(nil) + + try_submit('xxxxxx') + + expect(user.second_factor_attempts_count).to eq(3) + expect(user.second_factor_locked_at).to be_within(1.second).of(Time.zone.now) + end + end + + context 'when the code is expired' do + let(:phone_confirmation_otp_sent_at) { 11.minutes.ago.to_s } + + it 'returns an unsuccessful result' do + expect(idv_session).to_not receive(:user_phone_confirmation=) + + result = try_submit(phone_confirmation_otp) + + expect(result.success?).to eq(false) + end + + it 'increment second factor attempts and locks out user after too many' do + 2.times do + try_submit(phone_confirmation_otp) + end + + user.reload + + expect(user.second_factor_attempts_count).to eq(2) + expect(user.second_factor_locked_at).to eq(nil) + + try_submit(phone_confirmation_otp) + + expect(user.second_factor_attempts_count).to eq(3) + expect(user.second_factor_locked_at).to be_within(1.second).of(Time.zone.now) + end + end + + it 'handles nil and empty codes' do + result = try_submit(nil) + + expect(result.success?).to eq(false) + + result = try_submit('') + + expect(result.success?).to eq(false) + end + end +end diff --git a/spec/forms/idv/send_phone_confirmation_otp_form_spec.rb b/spec/forms/idv/send_phone_confirmation_otp_form_spec.rb new file mode 100644 index 00000000000..24995fa0671 --- /dev/null +++ b/spec/forms/idv/send_phone_confirmation_otp_form_spec.rb @@ -0,0 +1,164 @@ +require 'rails_helper' + +describe Idv::SendPhoneConfirmationOtpForm do + let(:phone) { '2255555000' } + let(:parsed_phone) { '+1 225-555-5000' } + let(:otp_delivery_preference) { 'sms' } + let(:phone_confirmation_otp) { '777777' } + let(:idv_session) { Idv::Session.new(user_session: {}, current_user: user, issuer: '') } + + let(:user) { create(:user, :signed_up) } + + let(:exceeded_otp_send_limit) { false } + let(:otp_rate_limiter) { OtpRateLimiter.new(user: user, phone: phone) } + + before do + # Setup Idv::Session + idv_session.params[:phone] = phone + idv_session.phone_confirmation_otp_delivery_method = otp_delivery_preference + + # Mock Idv::PhoneConfirmationOtpGenerator + allow(Idv::PhoneConfirmationOtpGenerator).to receive(:generate_otp). + and_return(phone_confirmation_otp) + + # Mock OtpRateLimiter + allow(OtpRateLimiter).to receive(:new).with(user: user, phone: parsed_phone). + and_return(otp_rate_limiter) + allow(otp_rate_limiter).to receive(:exceeded_otp_send_limit?). + and_return(exceeded_otp_send_limit) + end + + subject { described_class.new(user: user, idv_session: idv_session, locale: 'en') } + + describe '#submit' do + context 'with sms' do + it 'sends an sms' do + allow(SmsOtpSenderJob).to receive(:perform_later) + + result = subject.submit + + expect(result.success?).to eq(true) + + sent_at = Time.zone.parse(idv_session.phone_confirmation_otp_sent_at) + + expect(idv_session.phone_confirmation_otp).to eq(phone_confirmation_otp) + expect(sent_at).to be_within(1.second).of(Time.zone.now) + expect(SmsOtpSenderJob).to have_received(:perform_later).with( + otp_created_at: idv_session.phone_confirmation_otp_sent_at, + code: phone_confirmation_otp, + phone: parsed_phone, + message: 'jobs.sms_otp_sender_job.verify_message', + locale: 'en' + ) + end + end + + context 'with voice' do + let(:otp_delivery_preference) { 'voice' } + + it 'makes a phone call' do + allow(VoiceOtpSenderJob).to receive(:perform_later) + + result = subject.submit + + expect(result.success?).to eq(true) + + sent_at = Time.zone.parse(idv_session.phone_confirmation_otp_sent_at) + + expect(idv_session.phone_confirmation_otp).to eq(phone_confirmation_otp) + expect(sent_at).to be_within(1.second).of(Time.zone.now) + expect(VoiceOtpSenderJob).to have_received(:perform_later).with( + otp_created_at: idv_session.phone_confirmation_otp_sent_at, + code: phone_confirmation_otp, + phone: parsed_phone, + locale: 'en' + ) + end + + context 'with a number that does not support voice' do + let(:phone) { '+81543543643' } + let(:parsed_phone) { '+81 54-354-3643' } + + it 'sends an sms' do + allow(SmsOtpSenderJob).to receive(:perform_later) + + result = subject.submit + + expect(result.success?).to eq(true) + + sent_at = Time.zone.parse(idv_session.phone_confirmation_otp_sent_at) + + expect(idv_session.phone_confirmation_otp).to eq(phone_confirmation_otp) + expect(sent_at).to be_within(1.second). + of(Time.zone.now) + expect(SmsOtpSenderJob).to have_received(:perform_later).with( + otp_created_at: idv_session.phone_confirmation_otp_sent_at, + code: phone_confirmation_otp, + phone: parsed_phone, + message: 'jobs.sms_otp_sender_job.verify_message', + locale: 'en' + ) + end + end + end + + context 'when the user has requested too many otps' do + let(:exceeded_otp_send_limit) { true } + + it 'does not make a phone call or send an sms' do + expect(SmsOtpSenderJob).to_not receive(:perform_later) + expect(SmsOtpSenderJob).to_not receive(:perform_now) + expect(VoiceOtpSenderJob).to_not receive(:perform_later) + expect(VoiceOtpSenderJob).to_not receive(:perform_now) + + result = subject.submit + + expect(result.success?).to eq(false) + expect(idv_session.phone_confirmation_otp).to be_nil + expect(idv_session.phone_confirmation_otp_sent_at).to be_nil + end + end + + context 'with an invalid delivery method' do + let(:otp_delivery_preference) { 'noise' } + + it 'does not make a phone call or send an sms' do + expect(SmsOtpSenderJob).to_not receive(:perform_later) + expect(SmsOtpSenderJob).to_not receive(:perform_now) + expect(VoiceOtpSenderJob).to_not receive(:perform_later) + expect(VoiceOtpSenderJob).to_not receive(:perform_now) + + result = subject.submit + + expect(result.success?).to eq(false) + expect(idv_session.phone_confirmation_otp).to be_nil + expect(idv_session.phone_confirmation_otp_sent_at).to be_nil + end + end + end + + describe '#user_locked_out?' do + before do + allow(otp_rate_limiter).to receive(:exceeded_otp_send_limit?). + and_return(exceeded_otp_send_limit) + end + + context 'the user is locked out' do + let(:exceeded_otp_send_limit) { true } + + it 'returns true' do + subject.submit + + expect(subject.user_locked_out?).to eq(true) + end + end + + context 'the user is not locked out' do + it 'returns false' do + subject.submit + + expect(subject.user_locked_out?).to be_falsey + end + end + end +end diff --git a/spec/presenters/two_factor_auth_code/phone_delivery_presenter_spec.rb b/spec/presenters/two_factor_auth_code/phone_delivery_presenter_spec.rb index 878b0759c4b..78d5a898edf 100644 --- a/spec/presenters/two_factor_auth_code/phone_delivery_presenter_spec.rb +++ b/spec/presenters/two_factor_auth_code/phone_delivery_presenter_spec.rb @@ -7,11 +7,10 @@ let(:data) do { confirmation_for_phone_change: false, - confirmation_for_idv: false, phone_number: '5555559876', code_value: '999999', otp_delivery_preference: 'sms', - reenter_phone_number_path: '/idv/phone', + reenter_phone_number_path: '/manage/phone', unconfirmed_phone: true, totp_enabled: false, personal_key_unavailable: true, @@ -45,11 +44,6 @@ data[:confirmation_for_phone_change] = true expect(presenter.cancel_link).to eq account_path end - - it 'returns the verification cancel path during identity verification' do - data[:confirmation_for_idv] = true - expect(presenter.cancel_link).to eq idv_cancel_path - end end describe '#phone_number_message' do @@ -66,7 +60,7 @@ def presenter_with_locale(locale) TwoFactorAuthCode::PhoneDeliveryPresenter.new( data: data.clone.merge(reenter_phone_number_path: - "#{locale == :en ? nil : '/' + locale.to_s}/idv/phone"), + "#{locale == :en ? nil : '/' + locale.to_s}/manage/phone"), view: view ) end diff --git a/spec/services/otp_delivery_preference_updater_spec.rb b/spec/services/otp_delivery_preference_updater_spec.rb index d8b28dc88b4..aa48a0e1e1b 100644 --- a/spec/services/otp_delivery_preference_updater_spec.rb +++ b/spec/services/otp_delivery_preference_updater_spec.rb @@ -40,38 +40,6 @@ 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( diff --git a/spec/views/two_factor_authentication/otp_verification/show.html.slim_spec.rb b/spec/views/two_factor_authentication/otp_verification/show.html.slim_spec.rb index 30aa35e91b6..2f39b20e64e 100644 --- a/spec/views/two_factor_authentication/otp_verification/show.html.slim_spec.rb +++ b/spec/views/two_factor_authentication/otp_verification/show.html.slim_spec.rb @@ -7,7 +7,7 @@ phone_number: '***-***-1212', code_value: '12777', unconfirmed_user: false, - reenter_phone_number_path: idv_phone_path, + reenter_phone_number_path: manage_phone_path, } end @@ -255,7 +255,7 @@ render - expect(rendered).to have_link(t('forms.two_factor.try_again'), href: idv_phone_path) + expect(rendered).to have_link(t('forms.two_factor.try_again'), href: manage_phone_path) end end @@ -270,7 +270,7 @@ render - expect(rendered).to have_link(t('forms.two_factor.try_again'), href: idv_phone_path) + expect(rendered).to have_link(t('forms.two_factor.try_again'), href: manage_phone_path) end end end