From 0b27d7e0a927856f350d0b7feaa08e8a1bae5041 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 11 Aug 2021 18:02:50 +0100 Subject: [PATCH] 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() })