From c426857ac5c4885e329892f56302b64b40942cad Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 17 Sep 2024 09:35:46 -0400 Subject: [PATCH 1/4] LG-12213: Fix redirect when selecting Face/Touch on unsupported device changelog: Bug Fixes, Two-Factor Authentication, Fix redirect when choosing to authenticate with Face/Touch Unlock on unsupported device if stronger authenticator exists --- .../webauthn_verification_controller.rb | 12 ------ .../two_factor_authentication_controller.rb | 12 +++--- ...o_factor_authentication_controller_spec.rb | 31 +++++++++----- spec/features/webauthn/hidden_spec.rb | 41 ++++++++++++++++--- 4 files changed, 64 insertions(+), 32 deletions(-) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 20b912feda6..c58a1967315 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -7,7 +7,6 @@ class WebauthnVerificationController < ApplicationController include NewDeviceConcern before_action :check_sp_required_mfa - before_action :check_if_device_supports_platform_auth, only: :show before_action :confirm_webauthn_enabled, only: :show def show @@ -23,17 +22,6 @@ def confirm private - def check_if_device_supports_platform_auth - return unless user_session.has_key?(:platform_authenticator_available) - if platform_authenticator? && !device_supports_webauthn_platform? - redirect_to login_two_factor_options_url - end - end - - def device_supports_webauthn_platform? - user_session.delete(:platform_authenticator_available) == true - end - def handle_webauthn_result(result) handle_verification_for_authentication_context( result:, diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index b56ccf6524b..57a5444489e 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -338,17 +338,19 @@ def otp_rate_limiter def redirect_url if !mobile? && TwoFactorAuthentication::PivCacPolicy.new(current_user).enabled? login_two_factor_piv_cac_url + elsif TwoFactorAuthentication::WebauthnPolicy.new(current_user).platform_enabled? + if user_session[:platform_authenticator_available] == false + login_two_factor_options_url + else + login_two_factor_webauthn_url(platform: true) + end elsif TwoFactorAuthentication::WebauthnPolicy.new(current_user).enabled? - login_two_factor_webauthn_url(webauthn_params) + login_two_factor_webauthn_url elsif TwoFactorAuthentication::AuthAppPolicy.new(current_user).enabled? login_two_factor_authenticator_url end end - def webauthn_params - { platform: current_user.webauthn_configurations.platform_authenticators.present? } - end - def handle_too_many_short_term_otp_sends(method:, default:) analytics.rate_limit_reached( limiter_type: short_term_otp_rate_limiter.rate_limit_type, diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 0c1cd63d7ab..2b4eeb055ce 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -176,28 +176,39 @@ def index end end - context 'when user is webauthn enabled' do + context 'when user has webauthn' do + let(:user) { create(:user, :with_webauthn) } + before do - stub_sign_in_before_2fa(create(:user, :with_webauthn)) + stub_sign_in_before_2fa(user) end - it 'renders the :webauthn view' do + it 'redirects to webauthn verification' do get :show - expect(response).to redirect_to login_two_factor_webauthn_path(platform: false) + expect(response).to redirect_to login_two_factor_webauthn_path end - context 'when platform_authenticator' do - before do - controller.current_user.webauthn_configurations. - first.update!(platform_authenticator: true) - end + context 'when user has platform webauthn' do + let(:user) { create(:user, :with_webauthn_platform) } - it 'passes the platform parameter if the user has a platform autheticator' do + it 'redirects to webauthn verification with the platform parameter' do get :show expect(response).to redirect_to login_two_factor_webauthn_path(platform: true) end + + context 'when session value indicates no device platform support available' do + before do + controller.user_session[:platform_authenticator_available] = false + end + + it 'redirects to mfa options page' do + get :show + + expect(response).to redirect_to login_two_factor_options_path + end + end end end diff --git a/spec/features/webauthn/hidden_spec.rb b/spec/features/webauthn/hidden_spec.rb index 1ad7393b430..f90ac3bcb7f 100644 --- a/spec/features/webauthn/hidden_spec.rb +++ b/spec/features/webauthn/hidden_spec.rb @@ -93,15 +93,46 @@ end context 'with device that doesnt support authenticator' do - it 'redirects to options page on sign in and shows the option' do - email ||= user.email_addresses.first.email - password = user.password + it 'redirects to options page and allows them to choose authenticator' do + email = user.email_addresses.first.email + visit new_user_session_path set_hidden_field('platform_authenticator_available', 'false') - fill_in_credentials_and_submit(email, password) - continue_as(email, password) + fill_in_credentials_and_submit(email, user.password) + continue_as(email, user.password) + + # Redirected to options page expect(current_path).to eq(login_two_factor_options_path) + + # Can choose authenticator expect(webauthn_option_hidden?).to eq(false) + choose t('two_factor_authentication.login_options.webauthn_platform') + click_continue + expect(current_url).to eq(login_two_factor_webauthn_url(platform: true)) + end + + context 'if the webauthn credential is not their default mfa method when signing in' do + let(:user) do + create(:user, :fully_registered, :with_piv_or_cac, :with_webauthn_platform) + end + + it 'allows them to choose authenticator if they change from their default method' do + email = user.email_addresses.first.email + + visit new_user_session_path + set_hidden_field('platform_authenticator_available', 'false') + fill_in_credentials_and_submit(email, user.password) + continue_as(email, user.password) + + # Redirected to default MFA method + expect(current_path).to eq(login_two_factor_piv_cac_path) + + # Can change to authenticator if they choose + click_on t('two_factor_authentication.login_options_link_text') + choose t('two_factor_authentication.login_options.webauthn_platform') + click_continue + expect(current_url).to eq(login_two_factor_webauthn_url(platform: true)) + end end end end From 7fcefe0731fe411375e4d26f0135a1717a28e535 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 17 Sep 2024 10:47:19 -0400 Subject: [PATCH 2/4] Remove platform=false expectations from spec --- spec/features/remember_device/revocation_spec.rb | 2 +- spec/support/shared_examples/remember_device.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/remember_device/revocation_spec.rb b/spec/features/remember_device/revocation_spec.rb index 1c83a366484..51a4073f2a0 100644 --- a/spec/features/remember_device/revocation_spec.rb +++ b/spec/features/remember_device/revocation_spec.rb @@ -62,7 +62,7 @@ def expect_mfa_to_be_required_for_user(user) elsif TwoFactorAuthentication::WebauthnPolicy.new(user).platform_enabled? login_two_factor_webauthn_path(platform: true) elsif TwoFactorAuthentication::WebauthnPolicy.new(user).enabled? - login_two_factor_webauthn_path(platform: false) + login_two_factor_webauthn_path elsif TwoFactorAuthentication::AuthAppPolicy.new(user).enabled? login_two_factor_authenticator_path elsif TwoFactorAuthentication::PhonePolicy.new(user).enabled? diff --git a/spec/support/shared_examples/remember_device.rb b/spec/support/shared_examples/remember_device.rb index 79719d93e92..8cadc69f64f 100644 --- a/spec/support/shared_examples/remember_device.rb +++ b/spec/support/shared_examples/remember_device.rb @@ -70,7 +70,7 @@ def expect_mfa_to_be_required_for_user(user) elsif TwoFactorAuthentication::WebauthnPolicy.new(user).platform_enabled? login_two_factor_webauthn_path(platform: true) elsif TwoFactorAuthentication::WebauthnPolicy.new(user).enabled? - login_two_factor_webauthn_path(platform: false) + login_two_factor_webauthn_path elsif TwoFactorAuthentication::AuthAppPolicy.new(user).enabled? login_two_factor_authenticator_path elsif TwoFactorAuthentication::PhonePolicy.new(user).enabled? From c5e487fdc5fa0064efcf7f0fde8a2402c3373a2c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 17 Sep 2024 11:16:35 -0400 Subject: [PATCH 3/4] Remove redundant continue_as --- spec/features/webauthn/hidden_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/features/webauthn/hidden_spec.rb b/spec/features/webauthn/hidden_spec.rb index f90ac3bcb7f..bdc19fdab3b 100644 --- a/spec/features/webauthn/hidden_spec.rb +++ b/spec/features/webauthn/hidden_spec.rb @@ -99,7 +99,6 @@ visit new_user_session_path set_hidden_field('platform_authenticator_available', 'false') fill_in_credentials_and_submit(email, user.password) - continue_as(email, user.password) # Redirected to options page expect(current_path).to eq(login_two_factor_options_path) @@ -122,7 +121,6 @@ visit new_user_session_path set_hidden_field('platform_authenticator_available', 'false') fill_in_credentials_and_submit(email, user.password) - continue_as(email, user.password) # Redirected to default MFA method expect(current_path).to eq(login_two_factor_piv_cac_path) From 4eda06dfcb8c3a3b1e4d36c1c7f1eb1a6745c504 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 17 Sep 2024 11:17:33 -0400 Subject: [PATCH 4/4] Inline email parameter --- spec/features/webauthn/hidden_spec.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/features/webauthn/hidden_spec.rb b/spec/features/webauthn/hidden_spec.rb index bdc19fdab3b..57b52cbb92b 100644 --- a/spec/features/webauthn/hidden_spec.rb +++ b/spec/features/webauthn/hidden_spec.rb @@ -94,11 +94,9 @@ context 'with device that doesnt support authenticator' do it 'redirects to options page and allows them to choose authenticator' do - email = user.email_addresses.first.email - visit new_user_session_path set_hidden_field('platform_authenticator_available', 'false') - fill_in_credentials_and_submit(email, user.password) + fill_in_credentials_and_submit(user.email, user.password) # Redirected to options page expect(current_path).to eq(login_two_factor_options_path) @@ -116,11 +114,9 @@ end it 'allows them to choose authenticator if they change from their default method' do - email = user.email_addresses.first.email - visit new_user_session_path set_hidden_field('platform_authenticator_available', 'false') - fill_in_credentials_and_submit(email, user.password) + fill_in_credentials_and_submit(user.email, user.password) # Redirected to default MFA method expect(current_path).to eq(login_two_factor_piv_cac_path)