diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index e57a3e28be9..a1b64bce45b 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -253,6 +253,7 @@ def authenticator_view_data two_factor_authentication_method: two_factor_authentication_method, user_email: current_user.email, remember_device_available: false, + phone_enabled: current_user.phone_enabled?, }.merge(generic_data) end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index b0d0f07070b..de65c743a6e 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -26,11 +26,19 @@ def create private def confirm_two_factor_enabled - return if confirmation_context? || current_user.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 + end redirect_to phone_setup_url end + def phone_enabled? + current_user.phone_enabled? + end + def confirm_voice_capability return if two_factor_authentication_method == 'sms' diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index e60a21c435f..32ea41c4d2c 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -57,6 +57,7 @@ def piv_cac_view_data user_email: current_user.email, remember_device_available: false, totp_enabled: current_user.totp_enabled?, + phone_enabled: current_user.phone_enabled?, piv_cac_nonce: piv_cac_nonce, }.merge(generic_data) end diff --git a/app/controllers/users/phone_setup_controller.rb b/app/controllers/users/phone_setup_controller.rb index 08f47a27176..ead14b0dc7c 100644 --- a/app/controllers/users/phone_setup_controller.rb +++ b/app/controllers/users/phone_setup_controller.rb @@ -6,6 +6,7 @@ class PhoneSetupController < ApplicationController before_action :authenticate_user before_action :authorize_user + before_action :confirm_two_factor_authenticated, if: :two_factor_enabled? def index @user_phone_form = UserPhoneForm.new(current_user) @@ -28,6 +29,10 @@ def create private + def two_factor_enabled? + current_user.two_factor_enabled? + end + def user_phone_form_params params.require(:user_phone_form).permit( :international_code, diff --git a/app/presenters/two_factor_auth_code/authenticator_delivery_presenter.rb b/app/presenters/two_factor_auth_code/authenticator_delivery_presenter.rb index 6bf128a47eb..ee9e22e0f5c 100644 --- a/app/presenters/two_factor_auth_code/authenticator_delivery_presenter.rb +++ b/app/presenters/two_factor_auth_code/authenticator_delivery_presenter.rb @@ -30,9 +30,10 @@ def cancel_link private - attr_reader :user_email, :two_factor_authentication_method + attr_reader :user_email, :two_factor_authentication_method, :phone_enabled def otp_fallback_options + return unless phone_enabled t( 'devise.two_factor_authentication.totp_fallback.text_html', sms_link: sms_link, diff --git a/app/presenters/two_factor_auth_code/piv_cac_authentication_presenter.rb b/app/presenters/two_factor_auth_code/piv_cac_authentication_presenter.rb index ef09966c1ce..8efbed8b36f 100644 --- a/app/presenters/two_factor_auth_code/piv_cac_authentication_presenter.rb +++ b/app/presenters/two_factor_auth_code/piv_cac_authentication_presenter.rb @@ -40,9 +40,10 @@ def piv_cac_service_link private - attr_reader :user_email, :two_factor_authentication_method, :totp_enabled, :piv_cac_nonce + attr_reader :user_email, :two_factor_authentication_method, :totp_enabled, :phone_enabled def otp_fallback_options + return unless phone_enabled t( 'devise.two_factor_authentication.totp_fallback.text_html', sms_link: sms_link, diff --git a/spec/factories/users.rb b/spec/factories/users.rb index c69990d0a18..6410f18f62e 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -15,6 +15,17 @@ x509_dn_uuid { SecureRandom.uuid } end + trait :with_personal_key do + after :build do |user| + user.personal_key = PersonalKeyGenerator.new(user).create + end + end + + trait :with_authentication_app do + with_personal_key + otp_secret_key 'abc123' + end + trait :admin do role :admin end @@ -25,9 +36,7 @@ trait :signed_up do with_phone - after :build do |user| - user.personal_key = PersonalKeyGenerator.new(user).create - end + with_personal_key end trait :unconfirmed do diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index ab18e115208..b4b2edf38f0 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -526,9 +526,9 @@ def submit_prefilled_otp_code end end - describe 'when the user is TOTP enabled' do + describe 'when the user is TOTP enabled and phone enabled' do it 'allows SMS and Voice fallbacks' do - user = create(:user, :signed_up, otp_secret_key: 'foo') + user = create(:user, :with_authentication_app, :with_phone) sign_in_before_2fa(user) click_link t('devise.two_factor_authentication.totp_fallback.sms_link_text') diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 07bfc725e28..f954c9ccfae 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -431,4 +431,34 @@ expect(page.response_headers['Content-Security-Policy']). to(include('style-src \'self\'')) end + + context 'user is totp_enabled but not phone_enabled' do + before do + user = create(:user, :with_authentication_app) + signin(user.email, user.password) + end + + it 'requires 2FA before allowing access to phone setup form' do + visit phone_setup_path + + expect(page).to have_current_path login_two_factor_authenticator_path + end + + it 'does not redirect to phone setup form when visiting /login/two_factor/sms' do + visit login_two_factor_path(otp_delivery_preference: 'sms') + + expect(page).to have_current_path login_two_factor_authenticator_path + end + + it 'does not redirect to phone setup form when visiting /login/two_factor/voice' do + visit login_two_factor_path(otp_delivery_preference: 'voice') + + expect(page).to have_current_path login_two_factor_authenticator_path + end + + it 'does not display OTP Fallback text and links' do + expect(page). + to_not have_content t('devise.two_factor_authentication.totp_fallback.sms_link_text') + end + end end diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index adbe4f299b2..4d2ae4ccb76 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -212,4 +212,34 @@ to(include('style-src \'self\' \'unsafe-inline\'')) end end + + describe 'user is partially authenticated and phone 2fa is not configured' do + context 'with piv/cac enabled' do + let(:user) do + create(:user, :with_piv_or_cac) + end + + before(:each) do + sign_in_user(user) + end + + scenario 'can not access phone_setup' do + expect(page).to have_current_path login_two_factor_piv_cac_path + visit phone_setup_path + expect(page).to have_current_path login_two_factor_piv_cac_path + end + + scenario 'can not access phone_setup via login/two_factor/sms' do + expect(page).to have_current_path login_two_factor_piv_cac_path + visit login_two_factor_path(otp_delivery_preference: :sms) + expect(page).to have_current_path login_two_factor_piv_cac_path + end + + scenario 'can not access phone_setup via login/two_factor/voice' do + expect(page).to have_current_path login_two_factor_piv_cac_path + visit login_two_factor_path(otp_delivery_preference: :voice) + expect(page).to have_current_path login_two_factor_piv_cac_path + end + end + end end diff --git a/spec/presenters/two_factor_auth_code/piv_cac_authentication_presenter_spec.rb b/spec/presenters/two_factor_auth_code/piv_cac_authentication_presenter_spec.rb index a3d8a1f2181..615ab590732 100644 --- a/spec/presenters/two_factor_auth_code/piv_cac_authentication_presenter_spec.rb +++ b/spec/presenters/two_factor_auth_code/piv_cac_authentication_presenter_spec.rb @@ -35,8 +35,24 @@ def presenter_with(arguments = {}, view = ActionController::Base.new.view_contex end describe '#fallback_links' do - it 'has two options' do - expect(presenter.fallback_links.count).to eq 2 + context 'with phone enabled' do + let(:presenter) do + presenter_with(reauthn: reauthn, user_email: user_email, phone_enabled: true) + end + + it 'has two options' do + expect(presenter.fallback_links.count).to eq 2 + end + end + + context 'with phone disabled' do + let(:presenter) do + presenter_with(reauthn: reauthn, user_email: user_email, phone_enabled: false) + end + + it 'has one option' do + expect(presenter.fallback_links.count).to eq 1 + end end end diff --git a/spec/views/two_factor_authentication/totp_verification/show.html.slim_spec.rb b/spec/views/two_factor_authentication/totp_verification/show.html.slim_spec.rb index 1b7e4724516..adece4b916a 100644 --- a/spec/views/two_factor_authentication/totp_verification/show.html.slim_spec.rb +++ b/spec/views/two_factor_authentication/totp_verification/show.html.slim_spec.rb @@ -5,7 +5,8 @@ let(:presenter_data) do attributes_for(:generic_otp_presenter).merge( two_factor_authentication_method: 'authenticator', - user_email: view.current_user.email + user_email: view.current_user.email, + phone_enabled: user.phone_enabled? ) end