diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 9a39236e3..2d68555b1 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -7,6 +7,9 @@ .on('click', function (event) { event.preventDefault(); + // hide any existing error prompt + window.GOVUK.ErrorBanner.hideBanner(); + fetch('/webauthn/authenticate') .then(response => { if (!response.ok) { @@ -47,29 +50,23 @@ }); }) .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); // some browsers will show an error dialogue for some - // errors; to be safe we always pop up an alert - var message = error.message || error; - alert('Error during authentication.\n\n' + message); + // errors; to be safe we always display an error message on the page. + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/assets/javascripts/errorBanner.js b/app/assets/javascripts/errorBanner.js new file mode 100644 index 000000000..35cdf0c83 --- /dev/null +++ b/app/assets/javascripts/errorBanner.js @@ -0,0 +1,17 @@ +(function (window) { + "use strict"; + + /* + This module is intended to be used to show and hide an error banner based on a javascript trigger. You should make + sure the banner has an appropriate aria-live attribute, and a tabindex of -1 so that screenreaders and keyboard users + are alerted to the change respectively. + + This may behave in unexpected ways if you have more than one element with the `banner-dangerous` class on your page. + */ + window.GOVUK.ErrorBanner = { + hideBanner: () => $('.banner-dangerous').addClass('govuk-!-display-none'), + showBanner: () => $('.banner-dangerous') + .removeClass('govuk-!-display-none') + .trigger('focus'), + }; +})(window); diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index 2e3280e40..b7c93bef0 100644 --- a/app/assets/javascripts/registerSecurityKey.js +++ b/app/assets/javascripts/registerSecurityKey.js @@ -7,6 +7,9 @@ .on('click', function(event) { event.preventDefault(); + // hide any existing error prompt + window.GOVUK.ErrorBanner.hideBanner(); + fetch('/webauthn/register') .then((response) => { if (!response.ok) { @@ -27,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(); @@ -44,9 +38,8 @@ .catch((error) => { console.error(error); // some browsers will show an error dialogue for some - // errors; to be safe we always pop up an alert - var message = error.message || error; - alert('Error during registration.\n\n' + message); + // errors; to be safe we always display an error message on the page. + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 426852ff4..c02256964 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -232,6 +232,7 @@ def user_profile_disable_platform_admin_view(): @main.route("/user-profile/security-keys", methods=['GET']) +@user_is_logged_in def user_profile_security_keys(): if not current_user.can_use_webauthn: abort(403) @@ -251,6 +252,7 @@ def user_profile_security_keys(): methods=['GET'], endpoint="user_profile_confirm_delete_security_key" ) +@user_is_logged_in def user_profile_manage_security_key(key_id): if not current_user.can_use_webauthn: abort(403) @@ -282,6 +284,7 @@ def user_profile_manage_security_key(key_id): @main.route("/user-profile/security-keys//delete", methods=['POST']) +@user_is_logged_in def user_profile_delete_security_key(key_id): if not current_user.can_use_webauthn: abort(403) 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/app/templates/views/two-factor-webauthn.html b/app/templates/views/two-factor-webauthn.html index 0221e553e..fe311c3e8 100644 --- a/app/templates/views/two-factor-webauthn.html +++ b/app/templates/views/two-factor-webauthn.html @@ -21,6 +21,21 @@ {% block maincolumn_content %} + {{ govukErrorMessage({ + "classes": "banner-dangerous govuk-!-display-none", + "html": ( + 'There’s a problem with your security key' + + '

Check you have the right key and try again. ' + + 'If this does not work, ' + + 'contact us." + + '

' + ), + "attributes": { + "aria-live": "polite", + "tabindex": '-1' + } + }) }} +
{{ page_header(page_title) }} diff --git a/app/templates/views/user-profile/security-keys.html b/app/templates/views/user-profile/security-keys.html index e1a77aa50..3026156a4 100644 --- a/app/templates/views/user-profile/security-keys.html +++ b/app/templates/views/user-profile/security-keys.html @@ -46,6 +46,22 @@ {% endset %}
+ + {{ govukErrorMessage({ + "classes": "banner-dangerous govuk-!-display-none", + "html": ( + 'There’s a problem with your security key' + + '

Check you have the right key and try again. ' + + 'If this does not work, ' + + 'contact us." + + '

' + ), + "attributes": { + "aria-live": "polite", + "tabindex": '-1' + } + }) }} + {% if credentials %}
{{ page_header(page_title) }} diff --git a/gulpfile.js b/gulpfile.js index 22ebff748..35b51964e 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -182,6 +182,7 @@ const javascripts = () => { paths.src + 'javascripts/registerSecurityKey.js', paths.src + 'javascripts/authenticateSecurityKey.js', paths.src + 'javascripts/updateStatus.js', + paths.src + 'javascripts/errorBanner.js', paths.src + 'javascripts/homepage.js', paths.src + 'javascripts/main.js', ]) 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 2b9503331..8be566a0c 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -5,6 +5,10 @@ beforeAll(() => { // populate missing values to allow consistent jest.spyOn() window.fetch = () => { } window.navigator.credentials = { get: () => { } } + window.GOVUK.ErrorBanner = { + showBanner: () => {}, + hideBanner: () => {} + }; }) afterAll(() => { @@ -21,10 +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(() => { }) - - // ensure window.alert() is implemented to simplify errors - jest.spyOn(window, 'alert').mockImplementation(() => { }) + jest.spyOn(console, 'error').mockImplementation(() => {}) document.body.innerHTML = ` ` @@ -89,9 +90,9 @@ describe('Authenticate with security key', () => { done() }) - // this will make the test fail if the alert is called - jest.spyOn(window, 'alert').mockImplementation((msg) => { - done(msg) + // this will make the test fail if the error banner is displayed + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -132,17 +133,24 @@ 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() }) test.each([ ['network'], ['server'], - ])('alerts if fetching WebAuthn fails (%s error)', (errorType, done) => { + ])('errors if fetching WebAuthn fails (%s error)', (errorType, done) => { jest.spyOn(window, 'fetch').mockImplementation((_url) => { if (errorType == 'network') { return Promise.reject('error') @@ -151,15 +159,14 @@ describe('Authenticate with security key', () => { } }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during authentication.\n\nerror') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) button.click() }) - test('alerts if comms with the authenticator fails', (done) => { + test('errors if comms with the authenticator fails', (done) => { jest.spyOn(window, 'fetch') .mockImplementationOnce((_url) => { const webauthnOptions = window.CBOR.encode('someArbitraryOptions') @@ -173,8 +180,7 @@ describe('Authenticate with security key', () => { return Promise.reject(new DOMException('error')) }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during authentication.\n\nerror') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -182,9 +188,9 @@ describe('Authenticate with security key', () => { }) test.each([ - ['network error'], - ['internal server error'], - ])('alerts if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { + ['network'], + ['server'], + ])('errors if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch') .mockImplementationOnce((_url) => { const webauthnOptions = window.CBOR.encode('someArbitraryOptions') @@ -211,17 +217,15 @@ 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' }) } }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during authentication.\n\nerror') + + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -230,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 @@ -266,8 +269,8 @@ describe('Authenticate with security key', () => { done() }) - // this will make the test fail if the alert is called - jest.spyOn(window, 'alert').mockImplementation((msg) => { + // this will make the test fail if the error banner is displayed + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { done(msg) }) diff --git a/tests/javascripts/errorBanner.test.js b/tests/javascripts/errorBanner.test.js new file mode 100644 index 000000000..c32195543 --- /dev/null +++ b/tests/javascripts/errorBanner.test.js @@ -0,0 +1,37 @@ +beforeAll(() => { + require('../../app/assets/javascripts/errorBanner.js') +}); + +afterAll(() => { + require('./support/teardown.js'); +}); + +describe("Error Banner", () => { + afterEach(() => { + document.body.innerHTML = ''; + }); + + describe("The `hideBanner` method", () => { + test("Will hide the element", () => { + document.body.innerHTML = ` + `; + window.GOVUK.ErrorBanner.hideBanner(); + expect(document.querySelector('.banner-dangerous').classList).toContain('govuk-!-display-none') + }); + }); + + describe("The `showBanner` method", () => { + beforeEach(() => { + document.body.innerHTML = ` + `; + + window.GOVUK.ErrorBanner.showBanner('Some Err'); + }); + + test("Will show the element", () => { + expect(document.querySelector('.banner-dangerous').classList).not.toContain('govuk-!-display-none') + }); + }); +}); diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 1886935fd..a69d7d3c1 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -4,7 +4,11 @@ beforeAll(() => { // populate missing values to allow consistent jest.spyOn() window.fetch = () => {} - window.navigator.credentials = { create: () => {} } + window.navigator.credentials = { create: () => { } } + window.GOVUK.ErrorBanner = { + showBanner: () => { }, + hideBanner: () => { } + }; }) afterAll(() => { @@ -23,9 +27,6 @@ describe('Register security key', () => { // you might need to comment this out to debug some failures jest.spyOn(console, 'error').mockImplementation(() => {}) - // ensure window.alert() is implemented to simplify errors - jest.spyOn(window, 'alert').mockImplementation(() => {}) - document.body.innerHTML = ` Register a key @@ -78,9 +79,9 @@ describe('Register security key', () => { done() }) - // this will make the test fail if the alert is called - jest.spyOn(window, 'alert').mockImplementation((msg) => { - done(msg) + // this will make the test fail if the error banner is displayed + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -89,7 +90,7 @@ describe('Register security key', () => { test.each([ ['network'], ['server'], - ])('alerts if fetching WebAuthn options fails (%s error)', (errorType, done) => { + ])('errors if fetching WebAuthn options fails (%s error)', (errorType, done) => { jest.spyOn(window, 'fetch').mockImplementation((_url) => { if (errorType == 'network') { return Promise.reject('error') @@ -98,8 +99,7 @@ describe('Register security key', () => { } }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during registration.\n\nerror') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -107,10 +107,9 @@ describe('Register security key', () => { }) test.each([ - ['network error'], - ['internal server error'], - ['bad request'], - ])('alerts if sending WebAuthn credentials fails (%s)', (errorType, done) => { + ['network'], + ['server'], + ])('errors if sending WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { // initial fetch of options from the server @@ -129,26 +128,21 @@ 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' }) } }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during registration.\n\nerror') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) button.click() }) - test('alerts if comms with the authenticator fails', (done) => { + test('errors if comms with the authenticator fails', (done) => { jest.spyOn(window.navigator.credentials, 'create').mockImplementation(() => { return Promise.reject(new DOMException('error')) }) @@ -162,8 +156,7 @@ describe('Register security key', () => { }) }) - jest.spyOn(window, 'alert').mockImplementation((msg) => { - expect(msg).toEqual('Error during registration.\n\nerror') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() })