From bd2bb46a0e994c9fbce155e54098b4a05269f3c6 Mon Sep 17 00:00:00 2001 From: Aaron Chapman Date: Mon, 29 Jul 2019 17:09:21 -0400 Subject: [PATCH] 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 --- .../concerns/remember_device_concern.rb | 12 ++++++++ .../users/backup_code_setup_controller.rb | 9 ++---- ...piv_cac_authentication_setup_controller.rb | 4 ++- .../users/totp_setup_controller.rb | 8 ++++-- .../users/webauthn_setup_controller.rb | 9 ++---- .../phone_deletion_form.rb | 8 ++---- app/forms/user_phone_form.rb | 8 ++---- app/forms/user_piv_cac_setup_form.rb | 4 ++- spec/features/remember_device/totp_spec.rb | 27 +++++++++++++++++- .../features/remember_device/webauthn_spec.rb | 28 ++++++++++++++++++- 10 files changed, 84 insertions(+), 33 deletions(-) diff --git a/app/controllers/concerns/remember_device_concern.rb b/app/controllers/concerns/remember_device_concern.rb index b2d3cd186f3..7332fd04a78 100644 --- a/app/controllers/concerns/remember_device_concern.rb +++ b/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) diff --git a/app/controllers/users/backup_code_setup_controller.rb b/app/controllers/users/backup_code_setup_controller.rb index 59758058ef0..ef5b1c1328a 100644 --- a/app/controllers/users/backup_code_setup_controller.rb +++ b/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 diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 04cfba02c9c..387dab6a1df 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/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 diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 0f65a36e502..486433925c1 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/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 diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 3cd6e6620ed..3a64bb7d6e1 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/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 diff --git a/app/forms/two_factor_authentication/phone_deletion_form.rb b/app/forms/two_factor_authentication/phone_deletion_form.rb index 7c0d4a0c862..b66d73ea708 100644 --- a/app/forms/two_factor_authentication/phone_deletion_form.rb +++ b/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 diff --git a/app/forms/user_phone_form.rb b/app/forms/user_phone_form.rb index f011498a0d0..01dd16477ca 100644 --- a/app/forms/user_phone_form.rb +++ b/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 diff --git a/app/forms/user_piv_cac_setup_form.rb b/app/forms/user_piv_cac_setup_form.rb index 178bbbfc5ad..d0d7dc1daaa 100644 --- a/app/forms/user_piv_cac_setup_form.rb +++ b/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 diff --git a/spec/features/remember_device/totp_spec.rb b/spec/features/remember_device/totp_spec.rb index 7f69d67ce2a..b64a54868f8 100644 --- a/spec/features/remember_device/totp_spec.rb +++ b/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 diff --git a/spec/features/remember_device/webauthn_spec.rb b/spec/features/remember_device/webauthn_spec.rb index 091e426680b..814942af154 100644 --- a/spec/features/remember_device/webauthn_spec.rb +++ b/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