Fix remember browser for first MFA (#3144)

* Fix remember browser for first MFA

* dry up revoke_remember_device code

* Fixing error when user not present

* pass user

* removed debug code

* tests

app/controllers/concerns/remember_device_concern.rb

@@ -38,8 +38,20 @@ def remember_device_expired_for_sp?
     )
   end
 
+  def revoke_remember_device(user)
+    return if sign_up_incomplete(user)
+    UpdateUser.new(
+      user: user,
+      attributes: { remember_device_revoked_at: Time.zone.now },
+    ).call
+  end
+
   private
 
+  def sign_up_incomplete(user)
+    !MfaPolicy.new(user).sufficient_factors_enabled?
+  end
+
   def handle_valid_remember_device_cookie
     user_session[:mfa_device_remembered] = true
     mark_user_session_authenticated(:device_remembered)

app/controllers/users/backup_code_setup_controller.rb

@@ -1,6 +1,7 @@
 module Users
   class BackupCodeSetupController < ApplicationController
     include MfaSetupConcern
+    include RememberDeviceConcern
 
     before_action :authenticate_user!
     before_action :confirm_user_authenticated_for_2fa_setup
@@ -59,13 +60,7 @@ def save_backup_codes
       mark_user_as_fully_authenticated
       generator.save(user_session[:backup_codes])
       create_user_event(:backup_codes_added)
-      revoke_remember_device
-    end
-
-    def revoke_remember_device
-      UpdateUser.new(
-        user: current_user, attributes: { remember_device_revoked_at: Time.zone.now },
-      ).call
+      revoke_remember_device(current_user)
     end
 
     def generator

app/controllers/users/piv_cac_authentication_setup_controller.rb

@@ -3,6 +3,7 @@ class PivCacAuthenticationSetupController < ApplicationController
     include UserAuthenticator
     include PivCacConcern
     include MfaSetupConcern
+    include RememberDeviceConcern
 
     before_action :authenticate_user!
     before_action :confirm_user_authenticated_for_2fa_setup,
@@ -37,7 +38,8 @@ def redirect_to_piv_cac_service
     private
 
     def remove_piv_cac
-      attributes = { x509_dn_uuid: nil, remember_device_revoked_at: Time.zone.now }
+      revoke_remember_device(current_user)
+      attributes = { x509_dn_uuid: nil }
       UpdateUser.new(user: current_user, attributes: attributes).call
     end
 

app/controllers/users/totp_setup_controller.rb

@@ -2,6 +2,7 @@ module Users
   class TotpSetupController < ApplicationController
     include RememberDeviceConcern
     include MfaSetupConcern
+    include RememberDeviceConcern
 
     before_action :authenticate_user!
     before_action :confirm_user_authenticated_for_2fa_setup
@@ -78,14 +79,15 @@ def should_show_totp_configured_message?
     def process_successful_disable
       analytics.track_event(Analytics::TOTP_USER_DISABLED)
       create_user_event(:authenticator_disabled)
-      revoke_remember_device
+      revoke_remember_device(current_user)
+      revoke_otp_secret_key
       flash[:success] = t('notices.totp_disabled')
     end
 
-    def revoke_remember_device
+    def revoke_otp_secret_key
       UpdateUser.new(
         user: current_user,
-        attributes: { otp_secret_key: nil, remember_device_revoked_at: Time.zone.now },
+        attributes: { otp_secret_key: nil},
       ).call
     end
 

app/controllers/users/webauthn_setup_controller.rb

@@ -2,6 +2,7 @@ module Users
   class WebauthnSetupController < ApplicationController
     include RememberDeviceConcern
     include MfaSetupConcern
+    include RememberDeviceConcern
 
     before_action :authenticate_user!
     before_action :confirm_user_authenticated_for_2fa_setup
@@ -56,17 +57,11 @@ def exclude_credentials
     def handle_successful_delete
       create_user_event(:webauthn_key_removed)
       WebauthnConfiguration.where(user_id: current_user.id, id: params[:id]).destroy_all
-      revoke_remember_device
+      revoke_remember_device(current_user)
       flash[:success] = t('notices.webauthn_deleted')
       track_delete(true)
     end
 
-    def revoke_remember_device
-      UpdateUser.new(
-        user: current_user, attributes: { remember_device_revoked_at: Time.zone.now },
-      ).call
-    end
-
     def handle_failed_delete
       track_delete(false)
     end

app/forms/two_factor_authentication/phone_deletion_form.rb

@@ -1,6 +1,7 @@
 module TwoFactorAuthentication
   class PhoneDeletionForm
     include ActiveModel::Model
+    include RememberDeviceConcern
 
     attr_reader :user, :configuration
 
@@ -32,17 +33,12 @@ def extra_analytics_attributes
     def configuration_destroyed
       if configuration.destroy != false
         user.phone_configurations.reload
-        update_remember_device_revoked_at
+        revoke_remember_device(user)
         true
       else
         errors.add(:configuration, :not_destroyed, message: 'cannot delete phone')
         false
       end
     end
-
-    def update_remember_device_revoked_at
-      attributes = { remember_device_revoked_at: Time.zone.now }
-      UpdateUser.new(user: user, attributes: attributes).call
-    end
   end
 end

app/forms/user_phone_form.rb

@@ -2,6 +2,7 @@ class UserPhoneForm
   include ActiveModel::Model
   include FormPhoneValidator
   include OtpDeliveryPreferenceValidator
+  include RememberDeviceConcern
 
   validates :otp_delivery_preference, inclusion: { in: %w[voice sms] }
 
@@ -25,7 +26,7 @@ def submit(params)
     success = valid?
     self.phone = submitted_phone unless success
 
-    update_remember_device_revoked_at if success
+    revoke_remember_device(user) if success
 
     FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes)
   end
@@ -96,11 +97,6 @@ def default_phone_configuration?
     phone_configuration == user.default_phone_configuration
   end
 
-  def update_remember_device_revoked_at
-    attributes = { remember_device_revoked_at: Time.zone.now }
-    UpdateUser.new(user: user, attributes: attributes).call
-  end
-
   def formatted_user_phone
     phone_configuration&.formatted_phone
   end

app/forms/user_piv_cac_setup_form.rb

@@ -1,6 +1,7 @@
 class UserPivCacSetupForm
   include ActiveModel::Model
   include PivCacFormHelpers
+  include RememberDeviceConcern
 
   attr_accessor :x509_dn_uuid, :x509_dn, :token, :user, :nonce, :error_type
 
@@ -21,7 +22,8 @@ def submit
   private
 
   def process_valid_submission
-    attributes = { x509_dn_uuid: x509_dn_uuid, remember_device_revoked_at: Time.zone.now }
+    revoke_remember_device(user)
+    attributes = { x509_dn_uuid: x509_dn_uuid }
     UpdateUser.new(user: user, attributes: attributes).call
     true
   rescue PG::UniqueViolation

spec/features/remember_device/totp_spec.rb

@@ -20,7 +20,7 @@ def remember_device_and_sign_out_user
     it_behaves_like 'remember device'
   end
 
-  context 'sign up' do
+  context 'sign up with remember_device last' do
     def remember_device_and_sign_out_user
       user = sign_up_and_set_password
       user.password = Features::SessionHelper::VALID_PASSWORD
@@ -45,6 +45,31 @@ def remember_device_and_sign_out_user
     it_behaves_like 'remember device'
   end
 
+  context 'sign up with remember_device first' do
+    def remember_device_and_sign_out_user
+      user = sign_up_and_set_password
+      user.password = Features::SessionHelper::VALID_PASSWORD
+
+      select_2fa_option('auth_app')
+      fill_in :code, with: totp_secret_from_page
+      check :remember_device
+      click_submit_default
+
+      click_continue
+
+      select_2fa_option('phone')
+      fill_in :user_phone_form_phone, with: '2025551212'
+      click_send_security_code
+      fill_in_code_with_last_phone_otp
+      click_submit_default
+
+      first(:link, t('links.sign_out')).click
+      user
+    end
+
+    it_behaves_like 'remember device'
+  end
+
   context 'update totp' do
     after do
       Timecop.return

spec/features/remember_device/webauthn_spec.rb

@@ -33,7 +33,7 @@ def remember_device_and_sign_out_user
     it_behaves_like 'remember device'
   end
 
-  context 'sign up' do
+  context 'sign up with remember_device last' do
     def remember_device_and_sign_out_user
       mock_webauthn_setup_challenge
       user = sign_up_and_set_password
@@ -59,6 +59,32 @@ def remember_device_and_sign_out_user
     it_behaves_like 'remember device'
   end
 
+  context 'sign up with remember_device first' do
+    def remember_device_and_sign_out_user
+      mock_webauthn_setup_challenge
+      user = sign_up_and_set_password
+      user.password = Features::SessionHelper::VALID_PASSWORD
+
+      select_2fa_option('webauthn')
+      fill_in_nickname_and_click_continue
+      check :remember_device
+      mock_press_button_on_hardware_key_on_setup
+
+      click_continue
+
+      select_2fa_option('phone')
+      fill_in :user_phone_form_phone, with: '2025551313'
+      click_send_security_code
+      fill_in_code_with_last_phone_otp
+      click_submit_default
+
+      first(:link, t('links.sign_out')).click
+      user
+    end
+
+    it_behaves_like 'remember device'
+  end
+
   context 'update webauthn' do
     def remember_device_and_sign_out_user
       mock_webauthn_setup_challenge