From c96a1dc0b77e954a90fb8858d5dd0f8497b7aba7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 20 Jul 2021 12:14:10 +0100 Subject: [PATCH 1/5] add new error banner module for showing users js errors this ensures it's reusable by other components, and easier to unit test by isolating the separate concerns note: this is not in Modules since that's designed for classes that are then bound to an element in the DOM as indicated by a data-module attribute. This will just live at the window.GOVUK level since we want there to only ever be one `.banner-dangerous` warning. --- app/assets/javascripts/errorBanner.js | 18 ++++++++++++ gulpfile.js | 1 + tests/javascripts/errorBanner.test.js | 41 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 app/assets/javascripts/errorBanner.js create mode 100644 tests/javascripts/errorBanner.test.js diff --git a/app/assets/javascripts/errorBanner.js b/app/assets/javascripts/errorBanner.js new file mode 100644 index 000000000..2b3c259cc --- /dev/null +++ b/app/assets/javascripts/errorBanner.js @@ -0,0 +1,18 @@ +(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: (errorMessage) => $('.banner-dangerous') + .html(`Error: ${errorMessage}`) + .removeClass('govuk-!-display-none') + .trigger('focus'), + }; +})(window); 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/javascripts/errorBanner.test.js b/tests/javascripts/errorBanner.test.js new file mode 100644 index 000000000..e80b49811 --- /dev/null +++ b/tests/javascripts/errorBanner.test.js @@ -0,0 +1,41 @@ +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 set a specific error message on the element", () => { + expect(document.querySelector('.banner-dangerous').textContent).toEqual('Error: Some Err') + }); + + test("Will show the element", () => { + expect(document.querySelector('.banner-dangerous').classList).not.toContain('govuk-!-display-none') + }); + }); +}); From 0b27d7e0a927856f350d0b7feaa08e8a1bae5041 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 11 Aug 2021 18:02:50 +0100 Subject: [PATCH 2/5] show error message in banner rather than an alert the banner is a nicer user experience, and consistent with how we display errors elsewhere in notify. For now pass through the error message from JS, but we'll probably want to change that since the erorr messages themselves are often a bit cryptic and unhelpful --- .../javascripts/authenticateSecurityKey.js | 8 +++-- app/assets/javascripts/registerSecurityKey.js | 9 +++-- app/templates/views/two-factor-webauthn.html | 9 +++++ .../views/user-profile/security-keys.html | 10 ++++++ .../authenticateSecurityKey.test.js | 34 ++++++++++--------- tests/javascripts/registerSecurityKey.test.js | 31 +++++++++-------- 6 files changed, 64 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 9a39236e3..5ce70578a 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) { @@ -67,9 +70,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 authentication.\n\n' + message); + // errors; to be safe we always display an error message on the page. + window.GOVUK.ErrorBanner.showBanner('Something went wrong'); }); }); }; diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index 2e3280e40..39005c29c 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) { @@ -44,9 +47,9 @@ .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. + const message = error.message || error; + window.GOVUK.ErrorBanner.showBanner('Something went wrong'); }); }); }; diff --git a/app/templates/views/two-factor-webauthn.html b/app/templates/views/two-factor-webauthn.html index 0221e553e..9fd6a339a 100644 --- a/app/templates/views/two-factor-webauthn.html +++ b/app/templates/views/two-factor-webauthn.html @@ -21,6 +21,15 @@ {% block maincolumn_content %} + {{ govukErrorMessage({ + "classes": "banner-dangerous govuk-!-display-none", + "text": "There was a javascript error.", + "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..b20c7881b 100644 --- a/app/templates/views/user-profile/security-keys.html +++ b/app/templates/views/user-profile/security-keys.html @@ -46,6 +46,16 @@ {% endset %}
+ + {{ govukErrorMessage({ + "classes": "banner-dangerous govuk-!-display-none", + "text": "There was a javascript error.", + "attributes": { + "aria-live": "polite", + "tabindex": '-1' + } + }) }} + {% if credentials %}
{{ page_header(page_title) }} diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 2b9503331..8b83af2aa 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(() => { @@ -23,9 +27,6 @@ describe('Authenticate with 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 = ` ` @@ -89,8 +90,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) }) @@ -142,7 +143,7 @@ describe('Authenticate with security key', () => { 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 +152,15 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') 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 +174,8 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') done() }) @@ -184,7 +185,7 @@ describe('Authenticate with security key', () => { test.each([ ['network error'], ['internal server error'], - ])('alerts if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { + ])('errors if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch') .mockImplementationOnce((_url) => { const webauthnOptions = window.CBOR.encode('someArbitraryOptions') @@ -220,8 +221,9 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') done() }) @@ -266,8 +268,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/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 1886935fd..03bbbd075 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,8 +79,8 @@ describe('Register 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) }) @@ -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,8 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') done() }) @@ -110,7 +111,7 @@ describe('Register security key', () => { ['network error'], ['internal server error'], ['bad request'], - ])('alerts if sending WebAuthn credentials fails (%s)', (errorType, done) => { + ])('errors if sending WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { // initial fetch of options from the server @@ -140,15 +141,15 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') 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 +163,8 @@ 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((msg) => { + expect(msg).toEqual('Something went wrong') done() }) From b7e50fc638f18db0ad2a8fe4725a6d86638e049f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 13 Aug 2021 18:21:52 +0100 Subject: [PATCH 3/5] redirect non logged in users previously it'd show an error because non logged in users don't have the can_use_webauthn attribute. now we can just bounce them to the sign-in page --- app/main/views/user_profile.py | 3 +++ 1 file changed, 3 insertions(+) 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) From 2c55f4d0cef91007fbba4da4aa4b1cca9a781d12 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 13 Sep 2021 16:31:58 +0100 Subject: [PATCH 4/5] hard-code html error message for errorBanner turns out that we're only using errorBanner with a static message, and it's also full of rich html content. This means that it's probably better to put it in the html templates with other content, rather than hidden away in js files if we can help it. Since there are two places, had to dupe the error message but i think that's fine as i don't anticipate this error message being used in significantly more places. making it a string is a bit gross and means we don't get nice syntax highlighting on it, but as it needs to be passed in to a jinja macro that's the way it has to go unfortunately. --- app/assets/javascripts/authenticateSecurityKey.js | 2 +- app/assets/javascripts/errorBanner.js | 3 +-- app/assets/javascripts/registerSecurityKey.js | 2 +- app/templates/views/two-factor-webauthn.html | 8 +++++++- app/templates/views/user-profile/security-keys.html | 8 +++++++- tests/javascripts/authenticateSecurityKey.test.js | 13 +++++-------- tests/javascripts/errorBanner.test.js | 4 ---- tests/javascripts/registerSecurityKey.test.js | 13 +++++-------- 8 files changed, 27 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 5ce70578a..3cd9178a2 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -71,7 +71,7 @@ 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. - window.GOVUK.ErrorBanner.showBanner('Something went wrong'); + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/assets/javascripts/errorBanner.js b/app/assets/javascripts/errorBanner.js index 2b3c259cc..35cdf0c83 100644 --- a/app/assets/javascripts/errorBanner.js +++ b/app/assets/javascripts/errorBanner.js @@ -10,8 +10,7 @@ */ window.GOVUK.ErrorBanner = { hideBanner: () => $('.banner-dangerous').addClass('govuk-!-display-none'), - showBanner: (errorMessage) => $('.banner-dangerous') - .html(`Error: ${errorMessage}`) + showBanner: () => $('.banner-dangerous') .removeClass('govuk-!-display-none') .trigger('focus'), }; diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index 39005c29c..b9c76865e 100644 --- a/app/assets/javascripts/registerSecurityKey.js +++ b/app/assets/javascripts/registerSecurityKey.js @@ -49,7 +49,7 @@ // 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('Something went wrong'); + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/templates/views/two-factor-webauthn.html b/app/templates/views/two-factor-webauthn.html index 9fd6a339a..fe311c3e8 100644 --- a/app/templates/views/two-factor-webauthn.html +++ b/app/templates/views/two-factor-webauthn.html @@ -23,7 +23,13 @@ {{ govukErrorMessage({ "classes": "banner-dangerous govuk-!-display-none", - "text": "There was a javascript error.", + "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' diff --git a/app/templates/views/user-profile/security-keys.html b/app/templates/views/user-profile/security-keys.html index b20c7881b..3026156a4 100644 --- a/app/templates/views/user-profile/security-keys.html +++ b/app/templates/views/user-profile/security-keys.html @@ -49,7 +49,13 @@ {{ govukErrorMessage({ "classes": "banner-dangerous govuk-!-display-none", - "text": "There was a javascript error.", + "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' diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 8b83af2aa..ea0275851 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -91,8 +91,8 @@ describe('Authenticate with security key', () => { }) // this will make the test fail if the error banner is displayed - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - done(msg) + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -152,8 +152,7 @@ describe('Authenticate with security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -174,8 +173,7 @@ describe('Authenticate with security key', () => { return Promise.reject(new DOMException('error')) }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -222,8 +220,7 @@ describe('Authenticate with security key', () => { }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) diff --git a/tests/javascripts/errorBanner.test.js b/tests/javascripts/errorBanner.test.js index e80b49811..c32195543 100644 --- a/tests/javascripts/errorBanner.test.js +++ b/tests/javascripts/errorBanner.test.js @@ -30,10 +30,6 @@ describe("Error Banner", () => { window.GOVUK.ErrorBanner.showBanner('Some Err'); }); - test("Will set a specific error message on the element", () => { - expect(document.querySelector('.banner-dangerous').textContent).toEqual('Error: 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 03bbbd075..4bd0ac925 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -80,8 +80,8 @@ describe('Register security key', () => { }) // this will make the test fail if the error banner is displayed - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - done(msg) + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -99,8 +99,7 @@ describe('Register security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -141,8 +140,7 @@ describe('Register security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -163,8 +161,7 @@ describe('Register security key', () => { }) }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) From a96bfdb16e1e73ce2107506e6c81acbe1a69fa00 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Sep 2021 14:31:45 +0100 Subject: [PATCH 5/5] remove server-side error messages for webauthn since we are hard-coding a generic error message on the front-end, we have no need to do anything on the back end. This is also nice as it standardises the two flows to behave more like each other (rather than previously where one would `flash` an error message and the other would return CBOR for the js to decode). Note that the register flow returns 400 while the auth flow returns 403. The js for both just checks `response.ok` so will handle both. The JS completely discards any body returned if the status isn't 200 now. --- .../javascripts/authenticateSecurityKey.js | 23 ++++++-------- app/assets/javascripts/registerSecurityKey.js | 12 +------- app/main/views/webauthn_credentials.py | 29 ++++++------------ .../main/views/test_webauthn_credentials.py | 7 ----- .../authenticateSecurityKey.test.js | 30 +++++++++++-------- tests/javascripts/registerSecurityKey.test.js | 15 ++++------ 6 files changed, 41 insertions(+), 75 deletions(-) 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' }) } })