From e864100be7aa2493f8643791ba8990c628d59902 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 May 2021 14:30:11 +0100 Subject: [PATCH] make sure error message flashes work properly flashes are consumed by the jinja template calling get_flashed_messages in flash_messages.html. When you call `abort(403)` the 403 error page is rendered, with the flashed message on it. However, the webauthn endpoints just return that page to the ajax `fetch`, which ignores the response and just reloads the page. Instead of calling abort, we can just return an empty response body and the 403 error code, so that the flashed messages stay in the session and will be rendered when the `GET /two-factor-webauthn` request happens after the js reloads the page. --- app/main/views/two_factor.py | 1 + app/main/views/webauthn_credentials.py | 24 +++++++++++++++---- .../main/views/test_webauthn_credentials.py | 6 +++++ 3 files changed, 27 insertions(+), 4 deletions(-) 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