From 9ee01a2567a89ca66e26bb602c447fbea017174b Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Wed, 12 May 2021 17:16:03 +0100
Subject: [PATCH] Check for response.ok in fetch calls

It's possible for a call to fetch to trigger then "then" callback
even thought the response is an error [1]. We should test for both
scenarios, since they are handled differently. To avoid duplicating
the tests, I've used Jest's parameterisation feature [2].

[1]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
[2]: https://jestjs.io/docs/api#testeachtablename-fn-timeout
---
 app/assets/javascripts/registerSecurityKey.js | 10 +++++++-
 tests/javascripts/registerSecurityKey.test.js | 24 +++++++++++++++----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js
index 69c2c0383..bd6826b4c 100644
--- a/app/assets/javascripts/registerSecurityKey.js
+++ b/app/assets/javascripts/registerSecurityKey.js
@@ -9,6 +9,10 @@
 
           fetch('/webauthn/register')
             .then((response) => {
+              if (!response.ok) {
+                throw Error(response.statusText);
+              }
+
               return response.arrayBuffer();
             })
             .then((data) => {
@@ -21,7 +25,11 @@
                 credential.response, component.data('csrfToken')
               );
             })
-            .then(() => {
+            .then((response) => {
+              if (!response.ok) {
+                throw Error(response.statusText);
+              }
+
               window.location.reload();
             })
             .catch((error) => {
diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js
index 03b675e1d..48a93dd12 100644
--- a/tests/javascripts/registerSecurityKey.test.js
+++ b/tests/javascripts/registerSecurityKey.test.js
@@ -77,16 +77,23 @@ describe('Register security key', () => {
         expect(decodedData.clientDataJSON).toEqual(new Uint8Array([4,5,6]))
         expect(decodedData.attestationObject).toEqual(new Uint8Array([1,2,3]))
         expect(options.headers['X-CSRFToken']).toBe()
-        return Promise.resolve()
+        return Promise.resolve({ ok: true })
       }
     })
 
     button.click()
   })
 
-  test('alerts if fetching WebAuthn options fails', (done) => {
+  test.each([
+    ['network'],
+    ['server'],
+  ])('alerts if fetching WebAuthn options fails (%s error)', (errorType, done) => {
     jest.spyOn(window, 'fetch').mockImplementation((_url, options = {}) => {
-      return Promise.reject('error')
+      if (errorType == 'network') {
+        return Promise.reject('error')
+      } else {
+        return Promise.resolve({ ok: false, statusText: 'error' })
+      }
     })
 
     jest.spyOn(window, 'alert').mockImplementation((msg) => {
@@ -97,7 +104,10 @@ describe('Register security key', () => {
     button.click()
   })
 
-  test('alerts if sending WebAuthn credentials fails', (done) => {
+  test.each([
+    ['network'],
+    ['server'],
+  ])('alerts if sending WebAuthn credentials fails (%s error)', ({errorType}, done) => {
     Object.defineProperty(window.navigator, 'credentials', {
       value: {
         // fake PublicKeyCredential response from WebAuthn API
@@ -120,7 +130,11 @@ describe('Register security key', () => {
 
       // subsequent POST of credential data to server
       } else {
-        return Promise.reject('error')
+        if (errorType == 'network') {
+          return Promise.reject('error')
+        } else {
+          return Promise.resolve({ ok: false, statusText: 'error' })
+        }
       }
     })