Don't set up 2fa without authenticating first

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
 

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'
 

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

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,

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,

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,

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

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')

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

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

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
 

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