From bb7343d846fd0e165ff3a0a0f1b9c8e45ae4cd52 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 May 2021 12:07:11 +0100 Subject: [PATCH] pass nextUrl through yubikey flow the next url comes from sign in via a query param, and needs to go to the POST /webauthn/authenticate endpoint. That endpoint logs the user in and returns the redirect to the browser, and will take the next from the request query params to get there. also moving the window mocks to beforeEach/afterEach ensures that promise callbacks from previous tests aren't still associated in future tests to ensure good test isolation. unfortunately i couldn't get mocking location for a single js test to work, but by changing the global config i was able to add some query params that i can expect to be passed through. Don't love this at all but not quite sure of a good way round this. I think we're not practicing very good hygiene and best practices with our mocking and it's really confounding me here. --- .../javascripts/authenticateSecurityKey.js | 16 +++- .../main/views/test_webauthn_credentials.py | 17 ++++- .../authenticateSecurityKey.test.js | 74 ++++++++++++++++--- tests/javascripts/jest.config.js | 2 +- 4 files changed, 95 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 1326c923b..9a39236e3 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/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({ diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 32b26dc32..011200eaa 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/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 diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 90bbf89f3..c2e778334 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/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 = 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'], diff --git a/tests/javascripts/jest.config.js b/tests/javascripts/jest.config.js index adf68d50a..f686f6b69 100644 --- a/tests/javascripts/jest.config.js +++ b/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' }