Merge pull request #3912 from alphagov/next-url

Get webauthn login to honour next queryparam url

app/assets/javascripts/authenticateSecurityKey.js

@@ -21,7 +21,21 @@
               return window.navigator.credentials.get(options);
             })
             .then(credential => {
-              return fetch('/webauthn/authenticate', {
+              const currentURL = new URL(window.location.href);
+
+              // create authenticateURL from admin hostname plus /webauthn/authenticate path
+              const authenticateURL = new URL('/webauthn/authenticate', window.location.href);
+
+              const nextUrl = currentURL.searchParams.get('next');
+              if (nextUrl) {
+                // takes nextUrl from the query string on the current browser URL
+                // (which should be /two-factor-webauthn) and pass it through to
+                // the POST. put it in a query string so it's consistent with how
+                // the other login flows manage it
+                authenticateURL.searchParams.set('next', nextUrl);
+              }
+
+              return fetch(authenticateURL, {
                 method: 'POST',
                 headers: { 'X-CSRFToken': component.data('csrfToken') },
                 body: window.CBOR.encode({

app/main/views/two_factor.py

@@ -60,8 +60,8 @@ def two_factor_email(token):
     return log_in_user(user_id)
 
 
-@main.route('/two-factor-sms', methods=['GET', 'POST'])
 @main.route('/two-factor', methods=['GET', 'POST'])
+@main.route('/two-factor-sms', methods=['GET', 'POST'])
 @redirect_to_sign_in
 def two_factor_sms():
     user_id = session['user_details']['id']

app/main/views/webauthn_credentials.py

@@ -49,6 +49,7 @@ def webauthn_complete_register():
             cbor.decode(request.get_data()),
         )
     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
 
     user_api_client.create_webauthn_credential_for_user(
@@ -66,6 +67,15 @@ def webauthn_complete_register():
 @main.route('/webauthn/authenticate', methods=['GET'])
 @redirect_to_sign_in
 def webauthn_begin_authentication():
+    """
+    Initiate the authentication flow. This is called after the user clicks the "Check security key" button.
+
+    1. Get the user's credentials out of the database to present to the browser. The browser will only let you use a
+    credential in that list.
+    2. Call webauthn_server.authenticate_begin. This returns the authentication data, which includes the challenge and
+    the origin domain to authenticate with. This also returns the state, which we store in the cookie so we can ensure
+    the challenge is correct in webauthn_complete_authentication
+    """
     # get user from session
     user_to_login = User.from_id(session['user_details']['id'])
 
@@ -77,7 +87,7 @@ def webauthn_begin_authentication():
 
     authentication_data, state = current_app.webauthn_server.authenticate_begin(
         credentials=user_to_login.webauthn_credentials_as_cbor,
-        user_verification=None,  # required, preferred, discouraged. sets whether to ask for PIN
+        user_verification="discouraged",  # don't ask for PIN
     )
     session["webauthn_authentication_state"] = state
     return cbor.encode(authentication_data)
@@ -86,6 +96,13 @@ def webauthn_begin_authentication():
 @main.route('/webauthn/authenticate', methods=['POST'])
 @redirect_to_sign_in
 def webauthn_complete_authentication():
+    """
+    Complete the authentication flow. This is called after the user taps on their security key.
+
+    1. Try verifying the signed challenge returned from the browser with each public key we have in the database for
+    that user.
+    2. If succesful, log the user in, setting up the session etc. Then return the URL they should be redirected to.
+    """
     user_id = session['user_details']['id']
     user_to_login = User.from_id(user_id)
 
@@ -153,7 +170,6 @@ def _complete_webauthn_login_attempt(user):
     logged_in, _ = user.complete_webauthn_login_attempt()
     if not logged_in:
         # user account is locked as too many failed logins
-
         abort(403)
 
     if not is_less_than_days_ago(user.email_access_validated_at, 90):

app/notify_client/user_api_client.py

@@ -128,8 +128,7 @@ class UserApiClient(NotifyAdminAPIClient):
     @cache.delete('user-{user_id}')
     def complete_webauthn_login_attempt(self, user_id, is_successful):
         data = {'successful': is_successful}
-        # TODO: Change this to `/complete/webauthn-login`
-        endpoint = f'/user/{user_id}/verify/webauthn-login'
+        endpoint = f'/user/{user_id}/complete/webauthn-login'
         try:
             self.post(endpoint, data=data)
             return True, ''

tests/app/main/views/test_sign_in.py

@@ -130,8 +130,6 @@ def test_process_sms_auth_sign_in_return_2fa_template(
             'email_address': email_address,
             'password': password})
     assert response.status_code == 302
-    # TODO: remove this assert once we start defaulting to returning two_factor_sms first
-    assert '/two-factor-sms' not in response.location
     assert response.location == url_for('.two_factor_sms', next=redirect_url, _external=True)
     mock_verify_password.assert_called_with(api_user_active['id'], password)
     mock_get_user_by_email.assert_called_with('valid@example.gov.uk')

tests/app/main/views/test_webauthn_credentials.py

@@ -332,7 +332,18 @@ def test_complete_authentication_clears_session(
 
 
 @freeze_time('2020-01-30')
-def test_verify_webauthn_login_signs_user_in_signs_user_in(client, mocker, mock_create_event, platform_admin_user):
+@pytest.mark.parametrize('url_kwargs, expected_redirect', [
+    ({}, '/accounts-or-dashboard'),
+    ({'next': '/bar'}, '/bar'),
+])
+def test_verify_webauthn_login_signs_user_in(
+    client,
+    mocker,
+    mock_create_event,
+    platform_admin_user,
+    url_kwargs,
+    expected_redirect,
+):
     platform_admin_user['auth_type'] = 'webauthn_auth'
     platform_admin_user['email_access_validated_at'] = '2020-01-25T00:00:00.000000Z'
 
@@ -345,10 +356,10 @@ def test_verify_webauthn_login_signs_user_in_signs_user_in(client, mocker, mock_
     mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_authentication')
     mocker.patch('app.user_api_client.complete_webauthn_login_attempt', return_value=(True, None))
 
-    resp = client.post(url_for('main.webauthn_complete_authentication'))
+    resp = client.post(url_for('main.webauthn_complete_authentication', **url_kwargs))
 
     assert resp.status_code == 200
-    assert cbor.decode(resp.data)['redirect_url'] == url_for('main.show_accounts_or_dashboard')
+    assert cbor.decode(resp.data)['redirect_url'] == expected_redirect
     # removes stuff from session
     with client.session_transaction() as session:
         assert 'user_details' not in session

tests/app/notify_client/test_user_client.py

@@ -273,7 +273,7 @@ def test_complete_webauthn_login_attempt_returns_true_and_no_message_normally(fa
     resp = user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True)
 
     expected_data = {'successful': True}
-    mock_post.assert_called_once_with(f'/user/{fake_uuid}/verify/webauthn-login', data=expected_data)
+    mock_post.assert_called_once_with(f'/user/{fake_uuid}/complete/webauthn-login', data=expected_data)
     assert resp == (True, '')
 
 
@@ -293,7 +293,7 @@ def test_complete_webauthn_login_attempt_returns_false_and_message_on_403(fake_u
     resp = user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True)
 
     expected_data = {'successful': True}
-    mock_post.assert_called_once_with(f'/user/{fake_uuid}/verify/webauthn-login', data=expected_data)
+    mock_post.assert_called_once_with(f'/user/{fake_uuid}/complete/webauthn-login', data=expected_data)
 
     assert resp == (False, 'forbidden')
 

tests/javascripts/authenticateSecurityKey.test.js

@@ -8,18 +8,10 @@ beforeAll(() => {
 
   // ensure window.alert() is implemented to simplify errors
   jest.spyOn(window, 'alert').mockImplementation(() => { })
-
-  // populate missing values to allow consistent jest.spyOn()
-  window.fetch = () => { }
-  window.navigator.credentials = { get: () => { } }
 })
 
 afterAll(() => {
   require('./support/teardown.js')
-
-  // restore window attributes to their original undefined state
-  delete window.fetch
-  delete window.navigator.credentials
 })
 
 describe('Authenticate with security key', () => {
@@ -30,10 +22,23 @@ describe('Authenticate with security key', () => {
     <button type="submit" data-module="authenticate-security-key" data-csrf-token="abc123"></button>
     `
     button = document.querySelector('[data-module="authenticate-security-key"]')
+
+    // populate missing values to allow consistent jest.spyOn()
+    window.fetch = () => { }
+    window.navigator.credentials = { get: () => { } }
+    window.alert = () => { }
+
     window.GOVUK.modules.start()
   })
 
-  test('authenticates a credential and redirects', (done) => {
+  afterEach(() => {
+    // restore window attributes to their original undefined state
+    delete window.fetch
+    delete window.navigator.credentials
+    delete window.alert
+  })
+
+  test('authenticates a credential and redirects based on the admin app response', (done) => {
 
     jest.spyOn(window, 'fetch')
       .mockImplementationOnce((_url) => {
@@ -93,6 +98,57 @@ describe('Authenticate with security key', () => {
     button.click()
   });
 
+  test('authenticates and passes a redirect url through to the authenticate admin endpoint', (done) => {
+    jest.spyOn(window, 'fetch')
+      .mockImplementationOnce((_url) => {
+        // initial fetch of options from the server
+        // fetch defaults to GET
+        // options from the server are CBOR-encoded
+        let webauthnOptions = window.CBOR.encode('someArbitraryOptions')
+
+        return Promise.resolve({
+          ok: true, arrayBuffer: () => webauthnOptions
+        })
+      })
+
+    jest.spyOn(window.navigator.credentials, 'get').mockImplementation((options) => {
+      let credentialsGetResponse = {
+        response: {
+          authenticatorData: [],
+          signature: [],
+          clientDataJSON: []
+        },
+        rawId: [],
+        type: "public-key",
+      }
+      return Promise.resolve(credentialsGetResponse)
+    })
+
+    jest.spyOn(window, 'fetch')
+      .mockImplementationOnce((url, options = {}) => {
+        // subsequent POST of credential data to server
+        expect(url.toString()).toEqual(
+          'https://www.notifications.service.gov.uk/webauthn/authenticate?next=%2Ffoo%3Fbar%3Dbaz'
+          );
+
+        // mark the test as done here as we've finished all our asserts - if something goes wrong later and
+        // we end up in the alert mock, that `done(msg)` will override this and mark the test as failed
+        done();
+
+        const loginResponse = window.CBOR.encode({ redirect_url: '/foo' })
+        return Promise.resolve({
+          ok: true, arrayBuffer: () => Promise.resolve(loginResponse)
+        })
+      })
+
+    // make sure we error out if alert is called
+    jest.spyOn(window, 'alert').mockImplementation((msg) => {
+      done(msg)
+    })
+
+    button.click()
+  });
+
   test.each([
     ['network'],
     ['server'],

tests/javascripts/jest.config.js

@@ -1,4 +1,4 @@
 module.exports = {
   setupFiles: ['./support/setup.js'],
-  testURL: 'https://www.notifications.service.gov.uk'
+  testURL: 'https://www.notifications.service.gov.uk/?next=%2Ffoo%3Fbar%3Dbaz'
 }