diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 3cd9178a2..2d68555b1 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -50,22 +50,17 @@ }); }) .then(response => { - if (response.status === 403) { - // flask will have `flash`ed an error message up - window.location.reload(); - return; + if (!response.ok) { + throw Error(response.statusText); } - return response.arrayBuffer() - .then(cbor => { - return Promise.resolve(window.CBOR.decode(cbor)); - }) - .catch(() => { - throw Error(response.statusText); - }) - .then(data => { - window.location.assign(data.redirect_url); - }); + return response.arrayBuffer(); + }) + .then(cbor => { + return Promise.resolve(window.CBOR.decode(cbor)); + }) + .then(data => { + window.location.assign(data.redirect_url); }) .catch(error => { console.error(error); diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index b9c76865e..b7c93bef0 100644 --- a/app/assets/javascripts/registerSecurityKey.js +++ b/app/assets/javascripts/registerSecurityKey.js @@ -30,16 +30,7 @@ }) .then((response) => { if (!response.ok) { - return response.arrayBuffer() - .then((cbor) => { - return Promise.resolve(window.CBOR.decode(cbor)); - }) - .catch(() => { - throw Error(response.statusText); - }) - .then((text) => { - throw Error(text); - }); + throw Error(response.statusText); } window.location.reload(); @@ -48,7 +39,6 @@ console.error(error); // some browsers will show an error dialogue for some // errors; to be safe we always display an error message on the page. - const message = error.message || error; window.GOVUK.ErrorBanner.showBanner(); }); }); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 4d6d13148..47a4b2e3e 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -3,7 +3,6 @@ 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.models.user import User @@ -50,7 +49,7 @@ def webauthn_complete_register(): ) except RegistrationError as e: current_app.logger.info(f'User {current_user.id} could not register a new webauthn token - {e}') - return cbor.encode(str(e)), 400 + abort(400) current_user.create_webauthn_credential(credential) current_user.update(auth_type='webauthn_auth') @@ -102,24 +101,8 @@ def webauthn_complete_authentication(): user_id = session['user_details']['id'] user_to_login = User.from_id(user_id) - try: - _verify_webauthn_authentication(user_to_login) - redirect = _complete_webauthn_login_attempt(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 + _verify_webauthn_authentication(user_to_login) + redirect = _complete_webauthn_login_attempt(user_to_login) return cbor.encode({'redirect_url': redirect.location}), 200 @@ -142,6 +125,12 @@ def _verify_webauthn_authentication(user): signature=request_data['signature'] ) except ValueError as exc: + # 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 current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') user.complete_webauthn_login_attempt(is_successful=False) abort(403) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 3106ef79a..3c2c33f4c 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -181,7 +181,6 @@ def test_complete_register_handles_library_errors( ) assert response.status_code == 400 - assert cbor.decode(response.data) == 'error' def test_complete_register_handles_missing_state( @@ -289,8 +288,6 @@ 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 @@ -370,10 +367,6 @@ 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 diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index ea0275851..8be566a0c 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -25,7 +25,7 @@ describe('Authenticate with security key', () => { beforeEach(() => { // disable console.error() so we don't see it in test output // you might need to comment this out to debug some failures - jest.spyOn(console, 'error').mockImplementation(() => { }) + jest.spyOn(console, 'error').mockImplementation(() => {}) document.body.innerHTML = ` ` @@ -133,10 +133,17 @@ describe('Authenticate with security key', () => { expect(url.toString()).toEqual( 'https://www.notifications.service.gov.uk/webauthn/authenticate?next=%2Ffoo%3Fbar%3Dbaz' ) - - done() + return Promise.resolve({ + ok: true, arrayBuffer: () => Promise.resolve(window.CBOR.encode({ redirect_url: '/foo' })) + }) }) + jest.spyOn(window.location, 'assign').mockImplementation((href) => { + // ensure that the fetch mock implementation above was called + expect.assertions(1) + done() + }) + button.click() }) @@ -181,8 +188,8 @@ describe('Authenticate with security key', () => { }) test.each([ - ['network error'], - ['internal server error'], + ['network'], + ['server'], ])('errors if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch') .mockImplementationOnce((_url) => { @@ -210,12 +217,10 @@ describe('Authenticate with security key', () => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { // subsequent POST of credential data to server switch (errorType) { - case 'network error': + case 'network': return Promise.reject('error') - case 'internal server error': - // dont need this becuase we dont cbor return errors - const message = Promise.reject('encoding error') - return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) + case 'server': + return Promise.resolve({ ok: false, statusText: 'FORBIDDEN' }) } }) @@ -229,9 +234,8 @@ describe('Authenticate with security key', () => { test('reloads page if POSTing WebAuthn credentials returns 403', (done) => { - jest.spyOn(window, 'fetch') - .mockImplementationOnce((_url) => { - const webauthnOptions = window.CBOR.encode('someArbitraryOptions') + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { + const webauthnOptions = window.CBOR.encode('someArbitraryOptions') return Promise.resolve({ ok: true, arrayBuffer: () => webauthnOptions diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 4bd0ac925..a69d7d3c1 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -107,9 +107,8 @@ describe('Register security key', () => { }) test.each([ - ['network error'], - ['internal server error'], - ['bad request'], + ['network'], + ['server'], ])('errors if sending WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { @@ -129,14 +128,10 @@ describe('Register security key', () => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { // subsequent POST of credential data to server switch (errorType) { - case 'network error': + case 'network': return Promise.reject('error') - case 'bad request': - message = Promise.resolve(window.CBOR.encode('error')) - return Promise.resolve({ ok: false, arrayBuffer: () => message }) - case 'internal server error': - message = Promise.reject('encoding error') - return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) + case 'server': + return Promise.resolve({ ok: false, statusText: 'FORBIDDEN' }) } })