Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 1 addition & 6 deletions app/javascript/packages/webauthn/converters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion config/locales/two_factor_authentication/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions config/locales/two_factor_authentication/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions config/locales/two_factor_authentication/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
mock_webauthn_verification_challenge
sign_in_user(user)
uncheck(:remember_device)
mock_press_button_on_hardware_key_on_verification
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click

sign_in_user(user)
Expand Down
4 changes: 2 additions & 2 deletions spec/features/remember_device/webauthn_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click
user
end
Expand Down Expand Up @@ -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
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }
first(:link, t('links.sign_out')).click
user
end
Expand Down
4 changes: 2 additions & 2 deletions spec/features/two_factor_authentication/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }

expect(page).to have_current_path(account_path)
end
Expand All @@ -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
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }

expect(page).to have_current_path(account_path)
end
Expand Down
57 changes: 35 additions & 22 deletions spec/features/webauthn/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,22 @@
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)
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }

expect(page).to have_current_path(account_path)
end

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)
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)
Expand All @@ -41,24 +31,47 @@
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)
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)
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)
mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button }

expect(page).to have_content(t('errors.general'))

mock_press_button_on_hardware_key_on_verification
mock_successful_webauthn_authentication { click_webauthn_authenticate_button }

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)
mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button }

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)
mock_cancelled_webauthn_authentication { click_webauthn_authenticate_button }

expect(page).to have_content(t('two_factor_authentication.webauthn_platform_header_text'))
end
end
end
75 changes: 66 additions & 9 deletions spec/support/features/webauthn_helper.rb
Original file line number Diff line number Diff line change
@@ -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(
Expand Down Expand Up @@ -45,24 +46,76 @@ def mock_press_button_on_hardware_key_on_setup
end
end

def mock_press_button_on_hardware_key_on_verification
def mock_cancelled_webauthn_authentication
if javascript_enabled?
page.evaluate_script(<<~JS)
navigator.credentials.get = () => Promise.reject(new DOMException('', 'NotAllowedError'));
JS

yield

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
end

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')

# 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
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
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?
Expand Down Expand Up @@ -137,4 +190,8 @@ def signature
B3/YJKhlr2AA5uw59+aFzk
HEREDOC
end

def platform_authenticator?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make the tests clearer if we passed in platform: true to these helpers instead of inferring from the URL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have a soft preference for the current approach, for convenience and since the helpers already make many other assumptions of "just working" for both platform authenticators and security keys. Open to revisiting though.

Rack::Utils.parse_nested_query(URI(current_url).query)['platform'] == 'true'
end
end