Merge pull request #3973 from alphagov/error-handling-js-178466639

show error banner rather than alert when registering an invalid key

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) {
+              if (!response.ok) {
-                // flask will have `flash`ed an error message up
+                throw Error(response.statusText);
-                window.location.reload();
-                return;
               }
 
-              return response.arrayBuffer()
+              return response.arrayBuffer();
-                .then(cbor => {
+            })
-                  return Promise.resolve(window.CBOR.decode(cbor));
+            .then(cbor => {
-                })
+              return Promise.resolve(window.CBOR.decode(cbor));
-                .catch(() => {
+            })
-                  throw Error(response.statusText);
+            .then(data => {
-                })
+              window.location.assign(data.redirect_url);
-                .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
+              // errors; to be safe we always display an error message on the page.
-              var message = error.message || error;
+              window.GOVUK.ErrorBanner.showBanner();
-              alert('Error during authentication.\n\n' + message);
             });
         });
     };

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);

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()
+                throw Error(response.statusText);
-                  .then((cbor) => {
-                    return Promise.resolve(window.CBOR.decode(cbor));
-                  })
-                  .catch(() => {
-                    throw Error(response.statusText);
-                  })
-                  .then((text) => {
-                    throw Error(text);
-                  });
               }
 
               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
+              // errors; to be safe we always display an error message on the page.
-              var message = error.message || error;
+              window.GOVUK.ErrorBanner.showBanner();
-              alert('Error during registration.\n\n' + message);
             });
         });
     };

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/<uuid:key_id>/delete", methods=['POST'])
+@user_is_logged_in
 def user_profile_delete_security_key(key_id):
     if not current_user.can_use_webauthn:
         abort(403)

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)
-        _verify_webauthn_authentication(user_to_login)
+    redirect = _complete_webauthn_login_attempt(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
 
     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)

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' +
+        '<p class="govuk-body">Check you have the right key and try again. ' +
+        'If this does not work, ' +
+        '<a class="govuk-link govuk-link--no-visited-state" href=' + url_for('main.support') + ">contact us</a>." +
+        '</p>'
+      ),
+      "attributes": {
+        "aria-live": "polite",
+        "tabindex": '-1'
+      }
+    }) }}
+
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-one-half">
       {{ page_header(page_title) }}

app/templates/views/user-profile/security-keys.html

@@ -46,6 +46,22 @@
 {% endset %}
 
   <div class="govuk-grid-row">
+
+    {{ govukErrorMessage({
+      "classes": "banner-dangerous govuk-!-display-none",
+      "html": (
+        'There’s a problem with your security key' +
+        '<p class="govuk-body">Check you have the right key and try again. ' +
+        'If this does not work, ' +
+        '<a class="govuk-link govuk-link--no-visited-state" href=' + url_for('main.support') + ">contact us</a>." +
+        '</p>'
+      ),
+      "attributes": {
+        "aria-live": "polite",
+        "tabindex": '-1'
+      }
+    }) }}
+
     {% if credentials %}
       <div class="govuk-grid-column-five-sixths">
         {{ page_header(page_title) }}

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',
   ])

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
 
 

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(() => { })
+    jest.spyOn(console, 'error').mockImplementation(() => {})
-
-    // ensure window.alert() is implemented to simplify errors
-    jest.spyOn(window, 'alert').mockImplementation(() => { })
 
     document.body.innerHTML = `
       <button type="submit" data-module="authenticate-security-key" data-csrf-token="abc123"></button>`
@@ -89,9 +90,9 @@ describe('Authenticate with security key', () => {
       done()
     })
 
-    // this will make the test fail if the alert is called
+    // this will make the test fail if the error banner is displayed
-    jest.spyOn(window, 'alert').mockImplementation((msg) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      done(msg)
+      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'
         )
-
+        return Promise.resolve({
-        done()
+          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) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      expect(msg).toEqual('Error during authentication.\n\nerror')
       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) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      expect(msg).toEqual('Error during authentication.\n\nerror')
       done()
     })
 
@@ -182,9 +188,9 @@ describe('Authenticate with security key', () => {
   })
 
   test.each([
-    ['network error'],
+    ['network'],
-    ['internal server error'],
+    ['server'],
-  ])('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')
@@ -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':
+        case 'server':
-          // dont need this becuase we dont cbor return errors
+          return Promise.resolve({ ok: false, statusText: 'FORBIDDEN' })
-          const message = Promise.reject('encoding error')
-          return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' })
       }
     })
 
-    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')
+    jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => {
-      .mockImplementationOnce((_url) => {
+      const webauthnOptions = window.CBOR.encode('someArbitraryOptions')
-        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
+    // this will make the test fail if the error banner is displayed
-    jest.spyOn(window, 'alert').mockImplementation((msg) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => {
       done(msg)
     })
 

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 = `
+      <span class="govuk-error-message banner-dangerous js-error-visible">
+      </span>`;
+      window.GOVUK.ErrorBanner.hideBanner();
+      expect(document.querySelector('.banner-dangerous').classList).toContain('govuk-!-display-none')
+    });
+  });
+
+  describe("The `showBanner` method", () => {
+    beforeEach(() => {
+      document.body.innerHTML = `
+        <span class="govuk-error-message banner-dangerous js-error-visible govuk-!-display-none">
+        </span>`;
+
+      window.GOVUK.ErrorBanner.showBanner('Some Err');
+    });
+
+    test("Will show the element", () => {
+      expect(document.querySelector('.banner-dangerous').classList).not.toContain('govuk-!-display-none')
+    });
+  });
+});

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 = `
       <a href="#" role="button" draggable="false" class="govuk-button govuk-button--secondary" data-module="register-security-key">
         Register a key
@@ -78,9 +79,9 @@ describe('Register security key', () => {
       done()
     })
 
-    // this will make the test fail if the alert is called
+    // this will make the test fail if the error banner is displayed
-    jest.spyOn(window, 'alert').mockImplementation((msg) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      done(msg)
+      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) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      expect(msg).toEqual('Error during registration.\n\nerror')
       done()
     })
 
@@ -107,10 +107,9 @@ describe('Register security key', () => {
   })
 
   test.each([
-    ['network error'],
+    ['network'],
-    ['internal server error'],
+    ['server'],
-    ['bad request'],
+  ])('errors if sending WebAuthn credentials fails (%s)', (errorType, done) => {
-  ])('alerts 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':
+        case 'server':
-          message = Promise.resolve(window.CBOR.encode('error'))
+          return Promise.resolve({ ok: false, statusText: 'FORBIDDEN' })
-          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' })
       }
     })
 
-    jest.spyOn(window, 'alert').mockImplementation((msg) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      expect(msg).toEqual('Error during registration.\n\nerror')
       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) => {
+    jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => {
-      expect(msg).toEqual('Error during registration.\n\nerror')
       done()
     })