From 3afb61e6783bc3bfcfb46bb69bf729127af0d396 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 12 Jul 2023 09:45:47 -0400 Subject: [PATCH 1/4] Try stubbing WebAuthn authenticate result in JS changelog: Internal, Automated Testing, Improve realism of WebAuthn tests --- .../packages/webauthn/converters.ts | 7 +-- .../user_opted_preference_spec.rb | 2 +- .../features/remember_device/webauthn_spec.rb | 4 +- .../two_factor_authentication/sign_in_spec.rb | 4 +- spec/features/webauthn/sign_in_spec.rb | 33 ++++------- spec/support/features/webauthn_helper.rb | 57 ++++++++++++++++--- 6 files changed, 65 insertions(+), 42 deletions(-) diff --git a/app/javascript/packages/webauthn/converters.ts b/app/javascript/packages/webauthn/converters.ts index 0bb810830f5..756bf7f9dd8 100644 --- a/app/javascript/packages/webauthn/converters.ts +++ b/app/javascript/packages/webauthn/converters.ts @@ -5,12 +5,7 @@ * @return Converted string. */ export const base64ToArrayBuffer = (base64: string): ArrayBuffer => - Uint8Array.from( - window - .atob(base64) - .split('') - .map((char) => char.charCodeAt(0)), - ).buffer; + Uint8Array.from(atob(base64), (c) => c.charCodeAt(0)).buffer; /** * Converts an array buffer to a base64-encoded string. diff --git a/spec/features/remember_device/user_opted_preference_spec.rb b/spec/features/remember_device/user_opted_preference_spec.rb index b8875cc159f..cbf9b866225 100644 --- a/spec/features/remember_device/user_opted_preference_spec.rb +++ b/spec/features/remember_device/user_opted_preference_spec.rb @@ -118,7 +118,7 @@ mock_webauthn_verification_challenge sign_in_user(user) uncheck(:remember_device) - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify first(:link, t('links.sign_out')).click sign_in_user(user) diff --git a/spec/features/remember_device/webauthn_spec.rb b/spec/features/remember_device/webauthn_spec.rb index 425de13cb1b..627c5fe149f 100644 --- a/spec/features/remember_device/webauthn_spec.rb +++ b/spec/features/remember_device/webauthn_spec.rb @@ -25,7 +25,7 @@ def remember_device_and_sign_out_user mock_webauthn_verification_challenge sign_in_user(user) check t('forms.messages.remember_device') - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify first(:link, t('links.sign_out')).click user end @@ -88,7 +88,7 @@ def remember_device_and_sign_out_user mock_webauthn_verification_challenge sign_in_user(user) check t('forms.messages.remember_device') - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify first(:link, t('links.sign_out')).click user end diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index f1267cb3858..f61d31332fa 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -548,7 +548,7 @@ def attempt_to_bypass_2fa mock_webauthn_verification_challenge sign_in_user(webauthn_configuration.user) - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify expect(page).to have_current_path(account_path) end @@ -563,7 +563,7 @@ def attempt_to_bypass_2fa mock_webauthn_verification_challenge sign_in_user(webauthn_configuration.user) - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify expect(page).to have_current_path(account_path) end diff --git a/spec/features/webauthn/sign_in_spec.rb b/spec/features/webauthn/sign_in_spec.rb index b208054249b..043c1aae0fa 100644 --- a/spec/features/webauthn/sign_in_spec.rb +++ b/spec/features/webauthn/sign_in_spec.rb @@ -7,23 +7,13 @@ allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') end - let(:webauthn_configuration) do - create( - :webauthn_configuration, - credential_id: credential_id, - credential_public_key: credential_public_key, - user: user, - ) - end - let(:user) do - create(:user, :with_backup_code) - end + let(:user) { create(:user, :with_webauthn, with: { credential_id:, credential_public_key: }) } it 'allows the user to sign in if webauthn is successful' do mock_webauthn_verification_challenge - sign_in_user(webauthn_configuration.user) - mock_press_button_on_hardware_key_on_verification + sign_in_user(user) + click_webauthn_authenticate_button_and_verify expect(page).to have_current_path(account_path) end @@ -31,8 +21,8 @@ it 'does not allow the user to sign in if the challenge/secret is incorrect' do # Not calling `mock_challenge` here means the challenge won't match the signature that is set # when the button is pressed. - sign_in_user(webauthn_configuration.user) - mock_press_button_on_hardware_key_on_verification + sign_in_user(user) + click_webauthn_authenticate_button_and_verify expect(page).to have_content(t('errors.general')) expect(page).to have_current_path(login_two_factor_webauthn_path) @@ -41,23 +31,22 @@ it 'does not allow the user to sign in if the hardware button has not been pressed' do mock_webauthn_verification_challenge - sign_in_user(webauthn_configuration.user) - click_button t('two_factor_authentication.webauthn_use_key') + sign_in_user(user) + click_webauthn_authenticate_button_and_cancel expect(page).to have_content(t('errors.general')) expect(page).to have_current_path(login_two_factor_webauthn_path) end - it 'does not show error after successful challenge/secret reattempt' do + it 'does not show error after successful challenge/secret reattempt', :js do mock_webauthn_verification_challenge - sign_in_user(webauthn_configuration.user) - # click the next button or cancel from the browser dialog - click_button t('two_factor_authentication.webauthn_use_key') + sign_in_user(user) + click_webauthn_authenticate_button_and_cancel expect(page).to have_content(t('errors.general')) - mock_press_button_on_hardware_key_on_verification + click_webauthn_authenticate_button_and_verify expect(page).to_not have_content(t('errors.general')) end diff --git a/spec/support/features/webauthn_helper.rb b/spec/support/features/webauthn_helper.rb index d3b88033508..6015a4244cc 100644 --- a/spec/support/features/webauthn_helper.rb +++ b/spec/support/features/webauthn_helper.rb @@ -45,24 +45,59 @@ def mock_press_button_on_hardware_key_on_setup end end - def mock_press_button_on_hardware_key_on_verification + def click_webauthn_authenticate_button_and_cancel + if javascript_enabled? + page.evaluate_script(<<~JS) + navigator.credentials.get = () => Promise.reject(new DOMException('', 'NotAllowedError')); + JS + click_webauthn_authenticate_button + expect(page).to have_content(t('errors.general'), wait: 5) + else + find('#webauthn-button').click + end + end + + def click_webauthn_authenticate_button_and_verify # this is required because the domain is embedded in the supplied attestation object allow(WebauthnSetupForm).to receive(:domain_name).and_return('localhost:3000') - # simulate javascript that is triggered when the hardware key button is pressed - set_hidden_field('credential_id', credential_id) - set_hidden_field('authenticator_data', authenticator_data) - set_hidden_field('signature', signature) - set_hidden_field('client_data_json', verification_client_data_json) - - # submit form if javascript_enabled? - first('form').evaluate_script('this.submit()') + page.evaluate_script(<<~JS) + base64ToArrayBuffer = (base64) => Uint8Array.from(atob(base64), (c) => c.charCodeAt(0)).buffer; + JS + page.evaluate_script(<<~JS) + navigator.credentials.get = () => Promise.resolve({ + rawId: base64ToArrayBuffer(#{credential_id.to_json}), + response: { + authenticatorData: base64ToArrayBuffer(#{authenticator_data.to_json}), + clientDataJSON: base64ToArrayBuffer(#{verification_client_data_json.to_json}), + signature: base64ToArrayBuffer(#{signature.to_json}), + }, + }); + JS + original_path = current_path + click_webauthn_authenticate_button + expect(page).not_to have_current_path(original_path, wait: 5) else + # simulate javascript that is triggered when the hardware key button is pressed + set_hidden_field('credential_id', credential_id) + set_hidden_field('authenticator_data', authenticator_data) + set_hidden_field('signature', signature) + set_hidden_field('client_data_json', verification_client_data_json) + + # submit form find('#webauthn-button').click end end + def click_webauthn_authenticate_button + if platform_authenticator? + click_button t('two_factor_authentication.webauthn_platform_use_key') + else + click_button t('two_factor_authentication.webauthn_use_key') + end + end + def set_hidden_field(id, value) input = first("input##{id}", visible: false) if javascript_enabled? @@ -137,4 +172,8 @@ def signature B3/YJKhlr2AA5uw59+aFzk HEREDOC end + + def platform_authenticator? + Rack::Utils.parse_nested_query(URI(current_url).query)['platform'] == 'true' + end end From 7d6b9d1dc9b237bec667e02de06a28fd444f6e60 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 12 Jul 2023 10:35:00 -0400 Subject: [PATCH 2/4] Add cancellation regression specs for WebAuthn --- .../webauthn_verification_controller.rb | 2 +- .../locales/two_factor_authentication/en.yml | 2 +- .../locales/two_factor_authentication/es.yml | 4 ++-- .../locales/two_factor_authentication/fr.yml | 4 ++-- .../webauthn_verification_controller_spec.rb | 2 +- spec/features/webauthn/sign_in_spec.rb | 24 +++++++++++++++++++ spec/support/features/webauthn_helper.rb | 18 +++++++++++++- 7 files changed, 48 insertions(+), 8 deletions(-) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 768dcc93e2a..9215afd96f8 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -55,7 +55,7 @@ def handle_invalid_webauthn is_platform_auth = params[:platform].to_s == 'true' if is_platform_auth flash[:error] = t( - 'two_factor_authentication.webauthn_error.multiple_methods', + 'two_factor_authentication.webauthn_error.try_again', link: view_context.link_to( t('two_factor_authentication.webauthn_error.additional_methods_link'), login_two_factor_options_path, diff --git a/config/locales/two_factor_authentication/en.yml b/config/locales/two_factor_authentication/en.yml index 1bf1fc26a9e..1aabcb08e46 100644 --- a/config/locales/two_factor_authentication/en.yml +++ b/config/locales/two_factor_authentication/en.yml @@ -186,7 +186,7 @@ en: webauthn_authenticating: Authenticating your credentials… webauthn_error: additional_methods_link: choose another authentication method - multiple_methods: Face or touch unlock was unsuccessful. Please try again or %{link}. + try_again: Face or touch unlock was unsuccessful. Please try again or %{link}. webauthn_header_text: Connect your security key webauthn_piv_available: Use your PIV or CAC webauthn_platform_header_text: Use face or touch unlock diff --git a/config/locales/two_factor_authentication/es.yml b/config/locales/two_factor_authentication/es.yml index ced88a84db3..899d31db3bf 100644 --- a/config/locales/two_factor_authentication/es.yml +++ b/config/locales/two_factor_authentication/es.yml @@ -200,8 +200,8 @@ es: webauthn_authenticating: Autenticando sus credenciales… webauthn_error: additional_methods_link: elija otro método de autenticación - multiple_methods: El desbloqueo facial o táctil no fue exitoso. Por favor, - inténtelo de nuevo o %{link}. + try_again: El desbloqueo facial o táctil no fue exitoso. Por favor, inténtelo de + nuevo o %{link}. webauthn_header_text: Conecte su llave de seguridad webauthn_piv_available: Utilice su PIV o CAC webauthn_platform_header_text: Usar desbloqueo facial o táctil diff --git a/config/locales/two_factor_authentication/fr.yml b/config/locales/two_factor_authentication/fr.yml index 5c58f46a4c4..2cda712c74e 100644 --- a/config/locales/two_factor_authentication/fr.yml +++ b/config/locales/two_factor_authentication/fr.yml @@ -206,8 +206,8 @@ fr: webauthn_authenticating: Authentification de vos informations d’identification… webauthn_error: additional_methods_link: choisir une autre méthode d’authentification - multiple_methods: Le déverrouillage facial ou tactile n’a pas fonctionné. - Veuillez réessayer ou %{link}. + try_again: Le déverrouillage facial ou tactile n’a pas fonctionné. Veuillez + réessayer ou %{link}. webauthn_header_text: Connectez votre clé de sécurité webauthn_piv_available: Utilisez votre PIV ou CAC webauthn_platform_header_text: Utilisez le déverrouillage facial ou tactile diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index 1831bf9f7ba..d45ef2aa8c9 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -219,7 +219,7 @@ it 'displays flash error message' do patch :confirm, params: params expect(flash[:error]).to eq t( - 'two_factor_authentication.webauthn_error.multiple_methods', + 'two_factor_authentication.webauthn_error.try_again', link: view_context.link_to( t('two_factor_authentication.webauthn_error.additional_methods_link'), login_two_factor_options_path, diff --git a/spec/features/webauthn/sign_in_spec.rb b/spec/features/webauthn/sign_in_spec.rb index 043c1aae0fa..283a8df1e0d 100644 --- a/spec/features/webauthn/sign_in_spec.rb +++ b/spec/features/webauthn/sign_in_spec.rb @@ -50,4 +50,28 @@ expect(page).to_not have_content(t('errors.general')) end + + it 'maintains correct platform attachment content if cancelled', :js do + mock_webauthn_verification_challenge + + sign_in_user(user) + click_webauthn_authenticate_button_and_cancel + + expect(page).to have_content(t('two_factor_authentication.webauthn_header_text')) + end + + context 'platform authenticator' do + let(:user) do + create(:user, :with_webauthn_platform, with: { credential_id:, credential_public_key: }) + end + + it 'maintains correct platform attachment content if cancelled', :js do + mock_webauthn_verification_challenge + + sign_in_user(user) + click_webauthn_authenticate_button_and_cancel + + expect(page).to have_content(t('two_factor_authentication.webauthn_platform_header_text')) + end + end end diff --git a/spec/support/features/webauthn_helper.rb b/spec/support/features/webauthn_helper.rb index 6015a4244cc..f1a4976bf93 100644 --- a/spec/support/features/webauthn_helper.rb +++ b/spec/support/features/webauthn_helper.rb @@ -1,5 +1,6 @@ module WebAuthnHelper include JavascriptDriverHelper + include ActionView::Helpers::UrlHelper def mock_webauthn_setup_challenge allow(WebAuthn::Credential).to receive(:options_for_create).and_return( @@ -51,7 +52,22 @@ def click_webauthn_authenticate_button_and_cancel navigator.credentials.get = () => Promise.reject(new DOMException('', 'NotAllowedError')); JS click_webauthn_authenticate_button - expect(page).to have_content(t('errors.general'), wait: 5) + if platform_authenticator? + expect(page).to have_content( + strip_tags( + t( + 'two_factor_authentication.webauthn_error.try_again', + link: link_to( + t('two_factor_authentication.webauthn_error.additional_methods_link'), + login_two_factor_options_path, + ), + ), + ), + wait: 5, + ) + else + expect(page).to have_content(t('errors.general'), wait: 5) + end else find('#webauthn-button').click end From 8d1a1c274185095e2c59748ab98d3b7e249e2dcd Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 13 Jul 2023 09:03:37 -0400 Subject: [PATCH 3/4] Add spacing Co-authored-by: Zach Margolis --- spec/support/features/webauthn_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/support/features/webauthn_helper.rb b/spec/support/features/webauthn_helper.rb index f1a4976bf93..866bcc8cb4e 100644 --- a/spec/support/features/webauthn_helper.rb +++ b/spec/support/features/webauthn_helper.rb @@ -51,7 +51,9 @@ def click_webauthn_authenticate_button_and_cancel page.evaluate_script(<<~JS) navigator.credentials.get = () => Promise.reject(new DOMException('', 'NotAllowedError')); JS + click_webauthn_authenticate_button + if platform_authenticator? expect(page).to have_content( strip_tags( From 200b0bb53b90480c7a1e0637cc70c268b3649790 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 13 Jul 2023 09:13:21 -0400 Subject: [PATCH 4/4] Change webauthn helper to block yield Two separate concerns, where mocking involves setup & teardown. Avoids some readability issues with similar named methods See: https://github.com/18F/identity-idp/pull/8761/files#r1261446915 --- .../remember_device/user_opted_preference_spec.rb | 2 +- spec/features/remember_device/webauthn_spec.rb | 4 ++-- .../two_factor_authentication/sign_in_spec.rb | 4 ++-- spec/features/webauthn/sign_in_spec.rb | 14 +++++++------- spec/support/features/webauthn_helper.rb | 10 +++++----- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/spec/features/remember_device/user_opted_preference_spec.rb b/spec/features/remember_device/user_opted_preference_spec.rb index cbf9b866225..99f5218b8eb 100644 --- a/spec/features/remember_device/user_opted_preference_spec.rb +++ b/spec/features/remember_device/user_opted_preference_spec.rb @@ -118,7 +118,7 @@ mock_webauthn_verification_challenge sign_in_user(user) uncheck(:remember_device) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } first(:link, t('links.sign_out')).click sign_in_user(user) diff --git a/spec/features/remember_device/webauthn_spec.rb b/spec/features/remember_device/webauthn_spec.rb index 627c5fe149f..3d4b7f78fc1 100644 --- a/spec/features/remember_device/webauthn_spec.rb +++ b/spec/features/remember_device/webauthn_spec.rb @@ -25,7 +25,7 @@ def remember_device_and_sign_out_user mock_webauthn_verification_challenge sign_in_user(user) check t('forms.messages.remember_device') - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } first(:link, t('links.sign_out')).click user end @@ -88,7 +88,7 @@ def remember_device_and_sign_out_user mock_webauthn_verification_challenge sign_in_user(user) check t('forms.messages.remember_device') - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } first(:link, t('links.sign_out')).click user end diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index f61d31332fa..9be513f14ca 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -548,7 +548,7 @@ def attempt_to_bypass_2fa mock_webauthn_verification_challenge sign_in_user(webauthn_configuration.user) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_current_path(account_path) end @@ -563,7 +563,7 @@ def attempt_to_bypass_2fa mock_webauthn_verification_challenge sign_in_user(webauthn_configuration.user) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_current_path(account_path) end diff --git a/spec/features/webauthn/sign_in_spec.rb b/spec/features/webauthn/sign_in_spec.rb index 283a8df1e0d..572a1b1b417 100644 --- a/spec/features/webauthn/sign_in_spec.rb +++ b/spec/features/webauthn/sign_in_spec.rb @@ -13,7 +13,7 @@ mock_webauthn_verification_challenge sign_in_user(user) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_current_path(account_path) end @@ -22,7 +22,7 @@ # Not calling `mock_challenge` here means the challenge won't match the signature that is set # when the button is pressed. sign_in_user(user) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_content(t('errors.general')) expect(page).to have_current_path(login_two_factor_webauthn_path) @@ -32,7 +32,7 @@ mock_webauthn_verification_challenge sign_in_user(user) - click_webauthn_authenticate_button_and_cancel + mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_content(t('errors.general')) expect(page).to have_current_path(login_two_factor_webauthn_path) @@ -42,11 +42,11 @@ mock_webauthn_verification_challenge sign_in_user(user) - click_webauthn_authenticate_button_and_cancel + mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_content(t('errors.general')) - click_webauthn_authenticate_button_and_verify + mock_successful_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to_not have_content(t('errors.general')) end @@ -55,7 +55,7 @@ mock_webauthn_verification_challenge sign_in_user(user) - click_webauthn_authenticate_button_and_cancel + mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_content(t('two_factor_authentication.webauthn_header_text')) end @@ -69,7 +69,7 @@ mock_webauthn_verification_challenge sign_in_user(user) - click_webauthn_authenticate_button_and_cancel + mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button } expect(page).to have_content(t('two_factor_authentication.webauthn_platform_header_text')) end diff --git a/spec/support/features/webauthn_helper.rb b/spec/support/features/webauthn_helper.rb index 866bcc8cb4e..a23da3877c5 100644 --- a/spec/support/features/webauthn_helper.rb +++ b/spec/support/features/webauthn_helper.rb @@ -46,14 +46,14 @@ def mock_press_button_on_hardware_key_on_setup end end - def click_webauthn_authenticate_button_and_cancel + def mock_cancelled_webauthn_authentication if javascript_enabled? page.evaluate_script(<<~JS) navigator.credentials.get = () => Promise.reject(new DOMException('', 'NotAllowedError')); JS - click_webauthn_authenticate_button - + yield + if platform_authenticator? expect(page).to have_content( strip_tags( @@ -75,7 +75,7 @@ def click_webauthn_authenticate_button_and_cancel end end - def click_webauthn_authenticate_button_and_verify + def mock_successful_webauthn_authentication # this is required because the domain is embedded in the supplied attestation object allow(WebauthnSetupForm).to receive(:domain_name).and_return('localhost:3000') @@ -94,7 +94,7 @@ def click_webauthn_authenticate_button_and_verify }); JS original_path = current_path - click_webauthn_authenticate_button + yield expect(page).not_to have_current_path(original_path, wait: 5) else # simulate javascript that is triggered when the hardware key button is pressed