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/app/main/views/two_factor.py b/app/main/views/two_factor.py index 9ed4b45fd..12cdb7103 100644 --- a/app/main/views/two_factor.py +++ b/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'] diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 918dd3bb6..d1b458da6 100644 --- a/app/main/views/webauthn_credentials.py +++ b/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): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 4ab0305a8..b65836878 100644 --- a/app/notify_client/user_api_client.py +++ b/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, '' diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 4dfeaf7fe..a96fa0ad5 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/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') 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/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 97dfce3cd..c0fc4aaaf 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/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') 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' }