diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 163c71b05..9ed4b45fd 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -86,6 +86,7 @@ def two_factor_sms(): @main.route('/two-factor-webauthn', methods=['GET']) @redirect_to_sign_in def two_factor_webauthn(): + # TODO: Return a sensible error page if the user isn't platform admin or doesn't have webauthn redirect_url = request.args.get('next') return render_template('views/two-factor-webauthn.html', redirect_url=redirect_url) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 2bfdf98ac..d0f2eb8d6 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -3,6 +3,7 @@ from fido2.client import ClientData from fido2.ctap2 import AuthenticatorData from flask import abort, current_app, flash, redirect, request, session, url_for from flask_login import current_user +from werkzeug.exceptions import Forbidden from app.main import main from app.main.views.two_factor import log_in_user @@ -95,9 +96,25 @@ def webauthn_complete_authentication(): if not user_to_login.platform_admin: abort(403) - _complete_webauthn_authentication(user_to_login) + try: + _complete_webauthn_authentication(user_to_login) + redirect = _verify_webauthn_login(user_to_login) + except Forbidden: + # We don't expect to reach this case in normal situations - normally errors (such as using the wrong + # security key) will be caught in the browser inside `window.navigator.credentials.get`, and the js will + # error first meaning it doesn't send the POST request to this method. If this method is called but the key + # couldn't be authenticated, something went wrong along the way, probably: + # * The browser didn't implement the webauthn standard correctly, and let something through it shouldn't have + # * The key itself is in some way corrupted, or of lower security standard + flash('Security key not recognised') + + # flash sets the error message in the user's session cookie, and flask renders it next time `render_template` + # is called. In authenticateSecurityKey.js we refresh the page if this POST returns a 403. + # we can't use `abort(403)` here, and just return an empty body instead as our 403 error handler would return + # an error page response containing the flash, but our javascript ignores the body of the error response and + # just looks at the error code + return '', 403 - redirect = _verify_webauthn_login(user_to_login) return cbor.encode({'redirect_url': redirect.location}), 200 @@ -119,7 +136,6 @@ def _complete_webauthn_authentication(user): ) except ValueError as exc: current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') - flash('Security key not recognised') user.verify_webauthn_login(is_successful=False) abort(403) @@ -138,7 +154,7 @@ def _verify_webauthn_login(user): logged_in, _ = user.verify_webauthn_login() if not logged_in: # user account is locked as too many failed logins - flash('Security key not recognised') + abort(403) if not is_less_than_days_ago(user.email_access_validated_at, 90): diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 54e1254cf..3c951b7dc 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -292,6 +292,8 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( assert 'user_id' not in session # webauthn state reset so can't replay assert 'webauthn_authentication_state' not in session + # make sure there's an error message to show when the page reloads + assert '_flashes' in session assert mock_verify_webauthn_login.called is False # make sure we incremented the failed login count @@ -362,6 +364,10 @@ def test_verify_webauthn_login_signs_user_in_doesnt_sign_user_in_if_api_rejects( resp = client.post(url_for('main.webauthn_complete_authentication')) + with client.session_transaction() as session: + # make sure there's an error message to show when the page reloads + assert '_flashes' in session + assert resp.status_code == 403