From bb7343d846fd0e165ff3a0a0f1b9c8e45ae4cd52 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 May 2021 12:07:11 +0100 Subject: [PATCH 1/6] 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' } From 26ad20719fed0ebc35a77d13ed1240013893c823 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Jun 2021 12:43:53 +0100 Subject: [PATCH 2/6] send people to /two-factor-sms instead of /two-factor both routes are already valid, however, the link from sign-in sends to the old link. it fetches whichever URL is second in the route decorator list when you call `url_for`. Swapping the order around keeps the routes valid but starts pointing users to the new url. --- app/main/views/two_factor.py | 2 +- tests/app/main/views/test_sign_in.py | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) 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/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') From 0993792137d940e472da945c5f1a461ede03c9fa Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Jun 2021 12:46:31 +0100 Subject: [PATCH 3/6] rename verify to complete in api endpoint it was changed in this PR: https://github.com/alphagov/notifications-api/pull/3260 --- app/notify_client/user_api_client.py | 3 +-- tests/app/notify_client/test_user_client.py | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) 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/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') From 9fe8666733beaa544749a72b5f6de0c5a10f7957 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Jun 2021 16:58:59 +0100 Subject: [PATCH 4/6] add some docstrings for the webauthn endpoints --- app/main/views/webauthn_credentials.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 918dd3bb6..1c726ee9b 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -66,6 +66,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']) @@ -86,6 +95,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 +169,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): From e9636119ef131ffd00e402a38ebb132584c730ba Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Jun 2021 17:01:04 +0100 Subject: [PATCH 5/6] set user_verification to discouraged this is in line with our settings during registration. user verification involves the browser popping up a PIN prompt. Since the user has already entered their password correctly to get to this stage, we don't need any more proof of Something They Know, so there's no need for this. --- app/main/views/webauthn_credentials.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 1c726ee9b..d20b93ed0 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -86,7 +86,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) From 4ad93a0ea9a0600d77ac8f1f6fdd66414b1ea14d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Jun 2021 17:31:27 +0100 Subject: [PATCH 6/6] add logging when webauthn cred registration fails --- app/main/views/webauthn_credentials.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index d20b93ed0..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(