Remove handle_valid_otp method

app/controllers/concerns/remember_device_concern.rb

@@ -74,7 +74,6 @@ def handle_valid_remember_device_cookie(remember_device_cookie:)
     mark_user_session_authenticated(:device_remembered)
     handle_valid_remember_device_analytics(cookie_created_at: remember_device_cookie.created_at)
     redirect_to after_otp_verification_confirmation_url unless reauthn?
-    reset_otp_session_data
   end
 
   def handle_valid_remember_device_analytics(cookie_created_at:)

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -93,27 +93,11 @@ def reset_attempt_count_if_user_no_longer_locked_out
     ).call
   end
 
-  def handle_valid_otp(next_url:, auth_method: nil)
-    handle_valid_otp_for_context(auth_method: auth_method)
-    handle_remember_device
-    next_url ||= after_otp_verification_confirmation_url
-    reset_otp_session_data
-    redirect_to next_url
-  end
-
   def handle_remember_device
     save_user_opted_remember_device_pref
     save_remember_device_preference
   end
 
-  def handle_valid_otp_for_context(auth_method:)
-    if UserSessionContext.authentication_or_reauthentication_context?(context)
-      handle_valid_verification_for_authentication_context(auth_method: auth_method)
-    elsif UserSessionContext.confirmation_context?(context)
-      handle_valid_verification_for_confirmation_context(auth_method: auth_method)
-    end
-  end
-
   # Method will be renamed in the next refactor.
   # You can pass in any "type" with a corresponding I18n key in
   # two_factor_authentication.invalid_#{type}
@@ -186,11 +170,6 @@ def reset_second_factor_attempts_count
     UpdateUser.new(user: current_user, attributes: { second_factor_attempts_count: 0 }).call
   end
 
-  def reset_otp_session_data
-    user_session.delete(:unconfirmed_phone)
-    user_session[:context] = 'authentication'
-  end
-
   def after_otp_verification_confirmation_url
     return @next_mfa_setup_path if @next_mfa_setup_path
     return account_url if @updating_existing_number

app/controllers/two_factor_authentication/backup_code_verification_controller.rb

@@ -77,7 +77,6 @@ def backup_code_params
 
     def handle_valid_backup_code
       redirect_to after_otp_verification_confirmation_url
-      reset_otp_session_data
     end
 
     def check_sp_required_mfa

app/controllers/two_factor_authentication/otp_verification_controller.rb

@@ -21,8 +21,17 @@ def create
       result = otp_verification_form.submit
       post_analytics(result)
       if result.success?
-        handle_valid_confirmation_otp if UserSessionContext.confirmation_context?(context)
-        handle_valid_otp(next_url: nil, auth_method: params[:otp_delivery_preference])
+        if UserSessionContext.confirmation_context?(context)
+          handle_valid_confirmation_otp
+        else
+          handle_valid_verification_for_authentication_context(
+            auth_method: params[:otp_delivery_preference],
+          )
+        end
+
+        handle_remember_device
+        reset_otp_session_data
+        redirect_to after_otp_verification_confirmation_url
       else
         handle_invalid_otp(context: context, type: 'otp')
       end
@@ -34,6 +43,9 @@ def handle_valid_confirmation_otp
       assign_phone
       track_mfa_added
       @next_mfa_setup_path = next_setup_path
+      handle_valid_verification_for_confirmation_context(
+        auth_method: params[:otp_delivery_preference],
+      )
       flash[:success] = t('notices.phone_confirmed')
     end
 
@@ -247,5 +259,10 @@ def selected_otp_make_default_number
     def direct_otp_code
       current_user.direct_otp if FeatureManagement.prefill_otp_codes?
     end
+
+    def reset_otp_session_data
+      user_session.delete(:unconfirmed_phone)
+      user_session[:context] = 'authentication'
+    end
   end
 end

app/controllers/two_factor_authentication/personal_key_verification_controller.rb

@@ -99,7 +99,6 @@ def handle_valid_otp
       else
         redirect_to authentication_methods_setup_url
       end
-      reset_otp_session_data
     end
   end
 end

app/controllers/two_factor_authentication/piv_cac_verification_controller.rb

@@ -54,7 +54,6 @@ def handle_valid_piv_cac
         auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC,
       )
       redirect_to after_otp_verification_confirmation_url
-      reset_otp_session_data
     end
 
     def handle_invalid_piv_cac

app/controllers/two_factor_authentication/totp_verification_controller.rb

@@ -23,7 +23,11 @@ def create
       irs_attempts_api_tracker.mfa_login_totp(success: result.success?)
 
       if result.success?
-        handle_valid_otp(next_url: nil, auth_method: 'authenticator')
+        handle_valid_verification_for_authentication_context(
+          auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP,
+        )
+        handle_remember_device
+        redirect_to after_otp_verification_confirmation_url
       else
         handle_invalid_otp(context: context, type: 'totp')
       end

app/controllers/two_factor_authentication/webauthn_verification_controller.rb

@@ -40,10 +40,17 @@ def handle_webauthn_result(result)
     end
 
     def handle_valid_webauthn
-      handle_valid_verification_for_authentication_context(auth_method: 'webauthn')
+      if form.webauthn_configuration.platform_authenticator
+        handle_valid_verification_for_authentication_context(
+          auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM,
+        )
+      else
+        handle_valid_verification_for_authentication_context(
+          auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN,
+        )
+      end
       handle_remember_device
       redirect_to after_otp_verification_confirmation_url
-      reset_otp_session_data
     end
 
     def handle_invalid_webauthn
@@ -96,9 +103,9 @@ def credential_ids
     def analytics_properties
       auth_method = if form&.webauthn_configuration&.platform_authenticator ||
                        params[:platform].to_s == 'true'
-                      'webauthn_platform'
+                      TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM
                     else
-                      'webauthn'
+                      TwoFactorAuthenticatable::AuthMethod::WEBAUTHN
                     end
       {
         context: context,

app/controllers/users/piv_cac_login_controller.rb

@@ -74,10 +74,10 @@ def process_valid_submission
         presented: true,
       )
 
-      handle_valid_otp(
-        next_url: next_step,
+      handle_valid_verification_for_authentication_context(
         auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC,
       )
+      redirect_to next_step
     end
 
     def next_step
@@ -105,13 +105,5 @@ def process_invalid_submission
     def process_token_with_error
       redirect_to login_piv_cac_error_url(error: piv_cac_login_form.error_type)
     end
-
-    def mark_user_session_authenticated(authentication_type)
-      user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
-      user_session[:authn_at] = Time.zone.now
-      analytics.user_marked_authed(
-        authentication_type: authentication_type,
-      )
-    end
   end
 end

spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb

@@ -41,6 +41,11 @@
           with(:mfa_login_backup_code, success: true)
 
         post :create, params: payload
+
+        expect(subject.user_session[:auth_method]).to eq(
+          TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE,
+        )
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
 
       it 'tracks the valid authentication event when there are exisitng codes' do
@@ -120,6 +125,8 @@
 
         expect(response).to render_template(:show)
         expect(flash[:error]).to eq t('two_factor_authentication.invalid_backup_code')
+        expect(subject.user_session[:auth_method]).to eq nil
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
       end
 
       it 'tracks the max attempts event' do

spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb

@@ -161,6 +161,11 @@
       it 'displays flash error message' do
         expect(flash[:error]).to eq t('two_factor_authentication.invalid_otp')
       end
+
+      it 'does not set auth_method and requires 2FA' do
+        expect(subject.user_session[:auth_method]).to eq nil
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
+      end
     end
 
     context 'when the user enters an invalid OTP during reauthentication context' do
@@ -278,6 +283,9 @@
           code: subject.current_user.reload.direct_otp,
           otp_delivery_preference: 'sms',
         }
+
+        expect(subject.user_session[:auth_method]).to eq TwoFactorAuthenticatable::AuthMethod::SMS
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
 
       it 'tracks the attempt event with reauthentication parameter true' do
@@ -393,6 +401,7 @@
       let(:user) { create(:user, :fully_registered) }
       before do
         sign_in_as_user(user)
+        subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = false
         subject.user_session[:unconfirmed_phone] = '+1 (703) 555-5555'
         subject.user_session[:context] = 'confirmation'
 
@@ -417,7 +426,7 @@
         @previous_phone = MfaContext.new(subject.current_user).phone_configurations.first&.phone
       end
 
-      context 'user has an existing phone number' do
+      context 'user is fully authenticated and has an existing phone number' do
         context 'user enters a valid code' do
           before do
             subject.user_session[:mfa_selections] = ['sms']

spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb

@@ -67,6 +67,11 @@
           with('User marked authenticated', authentication_type: :valid_2fa)
 
         post :create, params: payload
+
+        expect(subject.user_session[:auth_method]).to eq(
+          TwoFactorAuthenticatable::AuthMethod::PERSONAL_KEY,
+        )
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
     end
 
@@ -136,6 +141,9 @@
         expect(subject).to receive(:handle_invalid_otp).and_call_original
 
         post :create, params: payload
+
+        expect(subject.user_session[:auth_method]).to eq nil
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
       end
 
       it 're-renders the personal key entry screen' do

spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb

@@ -68,6 +68,10 @@
 
         get :show, params: { token: 'good-token' }
 
+        expect(subject.user_session[:auth_method]).to eq(
+          TwoFactorAuthenticatable::AuthMethod::PIV_CAC,
+        )
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
         expect(response).to redirect_to account_path
         expect(subject.user_session[:decrypted_x509]).to eq(
           {
@@ -147,6 +151,11 @@
       it 'resets the piv/cac session information' do
         expect(subject.user_session[:decrypted_x509]).to be_nil
       end
+
+      it 'does not set auth_method and requires 2FA' do
+        expect(subject.user_session[:auth_method]).to eq nil
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
+      end
     end
 
     context 'when the user presents a different piv/cac' do

spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb

@@ -15,14 +15,16 @@
         Db::AuthAppConfiguration.create(user, @secret, nil, 'foo')
       end
 
-      it 'redirects to the profile' do
+      it 'redirects to the profile and sets auth_method' do
         cfg = subject.current_user.auth_app_configurations.first
         expect(Db::AuthAppConfiguration).to receive(:authenticate).and_return(cfg)
         expect(subject.current_user.reload.second_factor_attempts_count).to eq 0
 
         post :create, params: { code: generate_totp_code(@secret) }
 
         expect(response).to redirect_to account_path
+        expect(subject.user_session[:auth_method]).to eq TwoFactorAuthenticatable::AuthMethod::TOTP
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
 
       it 'resets the second_factor_attempts_count' do
@@ -74,6 +76,11 @@
       it 'displays flash error message' do
         expect(flash[:error]).to eq t('two_factor_authentication.invalid_otp')
       end
+
+      it 'does not set auth_method and still requires 2FA' do
+        expect(subject.user_session[:auth_method]).to eq nil
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
+      end
     end
 
     context 'when the user has reached the max number of TOTP attempts' do

spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb

@@ -91,6 +91,11 @@
         )
 
         patch :confirm, params: params
+
+        expect(subject.user_session[:auth_method]).to eq(
+          TwoFactorAuthenticatable::AuthMethod::WEBAUTHN,
+        )
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
 
       it 'tracks a valid platform authenticator submission' do
@@ -117,6 +122,10 @@
         )
 
         patch :confirm, params: params
+        expect(subject.user_session[:auth_method]).to eq(
+          TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM,
+        )
+        expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false
       end
 
       it 'tracks an invalid submission' do
@@ -173,6 +182,8 @@
           it 'redirects to webauthn show page' do
             patch :confirm, params: params
             expect(response).to redirect_to login_two_factor_webauthn_url(platform: true)
+            expect(subject.user_session[:auth_method]).to eq nil
+            expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq true
           end
 
           it 'displays flash error message' do

spec/controllers/users/piv_cac_login_controller_spec.rb

@@ -146,14 +146,6 @@
           end
 
           describe 'it handles the otp_context' do
-            it 'sets the otp user_session' do
-              expect(controller.user_session[:auth_method]).
-                to eq TwoFactorAuthenticatable::AuthMethod::PIV_CAC
-
-              expect(controller.user_session[:unconfirmed_phone]).to be nil
-              expect(controller.user_session[:context]).to eq 'authentication'
-            end
-
             it 'tracks the user_marked_authed event' do
               expect(@analytics).to have_received(:track_event).with(
                 'User marked authenticated',

spec/support/controller_helper.rb

@@ -4,14 +4,13 @@ module ControllerHelper
   def sign_in_as_user(user = create(:user, :fully_registered, password: VALID_PASSWORD))
     @request.env['devise.mapping'] = Devise.mappings[:user]
     sign_in user
+    controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = true
     user
   end
 
   def sign_in_before_2fa(user = create(:user, :fully_registered))
     sign_in_as_user(user)
     controller.current_user.create_direct_otp
-    allow(controller).to receive(:user_fully_authenticated?).and_return(false)
-    allow(controller).to receive(:signed_in_url).and_return(account_url)
   end
 
   def stub_sign_in(user = build(:user, password: VALID_PASSWORD))
@@ -32,6 +31,7 @@ def stub_sign_in_before_2fa(user = build(:user, password: VALID_PASSWORD))
     allow(controller).to receive(:current_user).and_return(user)
     allow(controller).to receive(:user_fully_authenticated?).and_return(false)
     allow(controller).to receive(:signed_in_url).and_return(account_url)
+    controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = true
   end
 
   def stub_idv_steps_before_verify_step(user)