From 907a7dc36309a8569e3e84e95dc27e8af2f45f22 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 May 2021 11:20:56 +0100 Subject: [PATCH 01/15] create webauthn 2fa page if user has `webauthn_auth` as their auth type, then redirect them to an interstitial that prompts them to click on a button which right now just logs to the JS console, but in a future commit will open up the webauthn browser prompt content is unsurprisingly not final. --- .../javascripts/authenticateSecurityKey.js | 13 ++++++++ app/main/views/sign_in.py | 2 +- app/main/views/two_factor.py | 7 ++++ app/navigation.py | 1 + app/templates/views/two-factor-webauthn.html | 33 +++++++++++++++++++ gulpfile.js | 1 + tests/app/main/views/test_sign_in.py | 28 ++++++++++++++++ tests/app/main/views/test_two_factor.py | 26 ++++++++++----- tests/app/test_navigation.py | 1 + 9 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 app/assets/javascripts/authenticateSecurityKey.js create mode 100644 app/templates/views/two-factor-webauthn.html diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js new file mode 100644 index 000000000..9ac503223 --- /dev/null +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -0,0 +1,13 @@ +(function (window) { + "use strict"; + + window.GOVUK.Modules.AuthenticateSecurityKey = function () { + this.start = function (component) { + $(component) + .on('click', function (event) { + event.preventDefault(); + console.log('pretend you just logged in okay'); + }); + }; + }; +})(window); diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 6a1d9ba4b..edca1bc12 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -50,7 +50,7 @@ def sign_in(): if user.email_auth: return redirect(url_for('.two_factor_email_sent', next=redirect_url)) if user.webauthn_auth: - raise NotImplementedError('webauthn not supported yet') + return redirect(url_for('.two_factor_webauthn', next=redirect_url)) # Vague error message for login in case of user not known, locked, inactive or password not verified flash(Markup( diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 8bbbe0ba1..6757d6391 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -82,6 +82,13 @@ def two_factor(): return render_template('views/two-factor.html', form=form, redirect_url=redirect_url) +@main.route('/two-factor-webauthn', methods=['GET']) +@redirect_to_sign_in +def two_factor_webauthn(): + redirect_url = request.args.get('next') + return render_template('views/two-factor-webauthn.html', redirect_url=redirect_url) + + @main.route('/re-validate-email', methods=['GET']) def revalidate_email_sent(): title = 'Email resent' if request.args.get('email_resent') else 'Check your email' diff --git a/app/navigation.py b/app/navigation.py index 77e1f59e3..447a1b102 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -111,6 +111,7 @@ class HeaderNavigation(Navigation): 'two_factor_email', 'two_factor_email_sent', 'two_factor_email_interstitial', + 'two_factor_webauthn', 'verify', 'verify_email', }, diff --git a/app/templates/views/two-factor-webauthn.html b/app/templates/views/two-factor-webauthn.html new file mode 100644 index 000000000..a2758e76e --- /dev/null +++ b/app/templates/views/two-factor-webauthn.html @@ -0,0 +1,33 @@ +{% extends "withoutnav_template.html" %} +{% from "components/page-header.html" import page_header %} +{% from "components/button/macro.njk" import govukButton %} + +{% set page_title = 'Security keys' %} + +{% block per_page_title %} + {{ page_title }} +{% endblock %} + +{% block maincolumn_content %} + {{ page_header( + page_title, + back_link=url_for('.user_profile') + ) }} + +
+
+

Security key

+

When you are ready to authenticate, press the button below.

+ + {{ govukButton({ + "element": "button", + "text": "Webauthn authenticate click me click me", + "classes": "govuk-button--secondary", + "attributes": { + "data-module": "authenticate-security-key", + "data-csrf-token": csrf_token(), + } + }) }} +
+
+{% endblock %} diff --git a/gulpfile.js b/gulpfile.js index 88e55fc77..9dbd998ee 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -180,6 +180,7 @@ const javascripts = () => { paths.src + 'javascripts/collapsibleCheckboxes.js', paths.src + 'javascripts/radioSlider.js', paths.src + 'javascripts/registerSecurityKey.js', + paths.src + 'javascripts/authenticateSecurityKey.js', paths.src + 'javascripts/updateStatus.js', paths.src + 'javascripts/homepage.js', paths.src + 'javascripts/main.js', diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 9d4aafbcf..d803e26ec 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -160,6 +160,34 @@ def test_process_email_auth_sign_in_return_2fa_template( mock_verify_password.assert_called_with(api_user_active_email_auth['id'], 'val1dPassw0rd!') +@pytest.mark.parametrize('redirect_url', [ + None, + f'/services/{SERVICE_ONE_ID}/templates', +]) +def test_process_webauthn_auth_sign_in_redirects_to_webauthn_with_next_redirect( + client, + api_user_active, + mocker, + mock_verify_password, + redirect_url +): + api_user_active['auth_type'] = 'webauthn_auth' + mock_get_user_by_email = mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_active) + + response = client.post( + url_for( + 'main.sign_in', next=redirect_url + ), + data={ + 'email_address': 'valid@example.gov.uk', + 'password': 'val1dPassw0rd!' + } + ) + mock_get_user_by_email.assert_called_once_with('valid@example.gov.uk') + assert response.status_code == 302 + assert response.location == url_for('.two_factor_webauthn', _external=True, next=redirect_url) + + def test_should_return_locked_out_true_when_user_is_locked( client, mock_get_user_by_email_locked, diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 07ffabe17..7a0120bda 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -258,15 +258,25 @@ def test_two_factor_returns_error_when_user_is_locked( assert 'Code not found' in response.get_data(as_text=True) -def test_two_factor_should_redirect_to_sign_in_if_user_not_in_session( - client, - api_user_active, - mock_get_user, +def test_two_factor_post_should_redirect_to_sign_in_if_user_not_in_session( + client_request, ): - response = client.post(url_for('main.two_factor'), - data={'sms_code': '12345'}) - assert response.status_code == 302 - assert response.location == url_for('main.sign_in', _external=True) + client_request.post( + 'main.two_factor', + _data={'sms_code': '12345'}, + _expected_redirect=url_for('main.sign_in', _external=True) + ) + + +@pytest.mark.parametrize('endpoint', ['main.two_factor_webauthn', 'main.two_factor']) +def test_two_factor_get_should_redirect_to_sign_in_if_user_not_in_session( + client_request, + endpoint, +): + client_request.get( + endpoint, + _expected_redirect=url_for('main.sign_in', _external=True) + ) @freeze_time('2020-01-27T12:00:00') diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 9871491c2..da168c240 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -293,6 +293,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'two_factor_email', 'two_factor_email_interstitial', 'two_factor_email_sent', + 'two_factor_webauthn', 'update_email_branding', 'update_letter_branding', 'upload_a_letter', From c203f624cab70d58ef2c4078dc7bdcb0d32882f4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 May 2021 19:15:12 +0100 Subject: [PATCH 02/15] rename two_factor to two_factor_sms it's a bit confusing now that there are three endpoints. the other two are already renamed two_factor_email and two_factor_webauthn --- app/main/views/code_not_received.py | 2 +- app/main/views/new_password.py | 2 +- app/main/views/sign_in.py | 2 +- app/main/views/two_factor.py | 5 ++-- app/main/views/verify.py | 2 +- app/navigation.py | 2 +- .../{two-factor.html => two-factor-sms.html} | 0 .../app/main/views/test_code_not_received.py | 2 +- tests/app/main/views/test_new_password.py | 2 +- tests/app/main/views/test_sign_in.py | 4 +++- tests/app/main/views/test_two_factor.py | 24 +++++++++---------- tests/app/test_navigation.py | 2 +- 12 files changed, 26 insertions(+), 23 deletions(-) rename app/templates/views/{two-factor.html => two-factor-sms.html} (100%) diff --git a/app/main/views/code_not_received.py b/app/main/views/code_not_received.py index 25e2f060a..37269a7d2 100644 --- a/app/main/views/code_not_received.py +++ b/app/main/views/code_not_received.py @@ -43,7 +43,7 @@ def check_and_resend_verification_code(): if user.state == 'pending': return redirect(url_for('main.verify', next=redirect_url)) else: - return redirect(url_for('main.two_factor', next=redirect_url)) + return redirect(url_for('main.two_factor_sms', next=redirect_url)) @main.route('/email-not-received', methods=['GET']) diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index 22f2a9719..e060736a1 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -49,6 +49,6 @@ def new_password(token): else: # send user a 2fa sms code user.send_verify_code() - return redirect(url_for('main.two_factor', next=request.args.get('next'))) + return redirect(url_for('main.two_factor_sms', next=request.args.get('next'))) else: return render_template('views/new-password.html', token=token, form=form, user=user) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index edca1bc12..a119faada 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -46,7 +46,7 @@ def sign_in(): invited_user.accept_invite() if user and user.sign_in(): if user.sms_auth: - return redirect(url_for('.two_factor', next=redirect_url)) + return redirect(url_for('.two_factor_sms', next=redirect_url)) if user.email_auth: return redirect(url_for('.two_factor_email_sent', next=redirect_url)) if user.webauthn_auth: diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 6757d6391..163c71b05 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -60,9 +60,10 @@ 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']) @redirect_to_sign_in -def two_factor(): +def two_factor_sms(): user_id = session['user_details']['id'] user = User.from_id(user_id) @@ -79,7 +80,7 @@ def two_factor(): user_api_client.send_verify_code(user.id, 'email', None, redirect_url) return redirect(url_for('.revalidate_email_sent', next=redirect_url)) - return render_template('views/two-factor.html', form=form, redirect_url=redirect_url) + return render_template('views/two-factor-sms.html', form=form, redirect_url=redirect_url) @main.route('/two-factor-webauthn', methods=['GET']) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index b1d27ea10..87c8fa95d 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -36,7 +36,7 @@ def verify(): finally: session.pop('user_details', None) - return render_template('views/two-factor.html', form=form) + return render_template('views/two-factor-sms.html', form=form) @main.route('/verify-email/') diff --git a/app/navigation.py b/app/navigation.py index 447a1b102..2ceda0b15 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -107,7 +107,7 @@ class HeaderNavigation(Navigation): 'sign-in': { 'revalidate_email_sent', 'sign_in', - 'two_factor', + 'two_factor_sms', 'two_factor_email', 'two_factor_email_sent', 'two_factor_email_interstitial', diff --git a/app/templates/views/two-factor.html b/app/templates/views/two-factor-sms.html similarity index 100% rename from app/templates/views/two-factor.html rename to app/templates/views/two-factor-sms.html diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 11405790f..6343dd271 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -138,7 +138,7 @@ def test_check_and_redirect_to_two_factor_if_user_active( 'email': api_user_active['email_address']} response = client.get(url_for('main.check_and_resend_verification_code', next=redirect_url)) assert response.status_code == 302 - assert response.location == url_for('main.two_factor', _external=True, next=redirect_url) + assert response.location == url_for('main.two_factor_sms', _external=True, next=redirect_url) @pytest.mark.parametrize('redirect_url', [ diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index 53cd87754..c7102771e 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -56,7 +56,7 @@ def test_should_redirect_to_two_factor_when_password_reset_is_successful( response = client.post(url_for_endpoint_with_token('.new_password', token=token, next=redirect_url), data={'new_password': 'a-new_password'}) assert response.status_code == 302 - assert response.location == url_for('.two_factor', _external=True, next=redirect_url) + assert response.location == url_for('.two_factor_sms', _external=True, next=redirect_url) mock_get_user_by_email_request_password_reset.assert_called_once_with(user['email_address']) diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index d803e26ec..4dfeaf7fe 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -130,7 +130,9 @@ def test_process_sms_auth_sign_in_return_2fa_template( 'email_address': email_address, 'password': password}) assert response.status_code == 302 - assert response.location == url_for('.two_factor', next=redirect_url, _external=True) + # 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_two_factor.py b/tests/app/main/views/test_two_factor.py index 7a0120bda..f431a75fd 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -54,7 +54,7 @@ def test_should_render_two_factor_page( 'id': api_user_active['id'], 'email': api_user_active['email_address']} mocker.patch('app.user_api_client.get_user', return_value=api_user_active) - response = client.get(url_for('main.two_factor', next=redirect_url)) + response = client.get(url_for('main.two_factor_sms', next=redirect_url)) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.select_one('main p').text.strip() == ( @@ -86,7 +86,7 @@ def test_should_login_user_and_should_redirect_to_next_url( 'email': api_user_active['email_address']} api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' - response = client.post(url_for('main.two_factor', next='/services/{}'.format(SERVICE_ONE_ID)), + response = client.post(url_for('main.two_factor_sms', next='/services/{}'.format(SERVICE_ONE_ID)), data={'sms_code': '12345'}) assert response.status_code == 302 assert response.location == url_for( @@ -112,7 +112,7 @@ def test_should_send_email_and_redirect_to_info_page_if_user_needs_to_revalidate session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - response = client.post(url_for('main.two_factor', next=f'/services/{SERVICE_ONE_ID}'), + response = client.post(url_for('main.two_factor_sms', next=f'/services/{SERVICE_ONE_ID}'), data={'sms_code': '12345'}) assert response.status_code == 302 @@ -140,7 +140,7 @@ def test_should_login_user_and_not_redirect_to_external_url( 'email': api_user_active['email_address']} api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' - response = client.post(url_for('main.two_factor', next='http://www.google.com'), + response = client.post(url_for('main.two_factor_sms', next='http://www.google.com'), data={'sms_code': '12345'}) assert response.status_code == 302 assert response.location == url_for('main.show_accounts_or_dashboard', _external=True) @@ -166,7 +166,7 @@ def test_should_login_user_and_redirect_to_show_accounts( api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' api_user_active['platform_admin'] = platform_admin - response = client.post(url_for('main.two_factor'), + response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '12345'}) assert response.status_code == 302 @@ -186,7 +186,7 @@ def test_should_return_200_with_sms_code_error_when_sms_code_is_wrong( 'email': api_user_active['email_address']} mocker.patch('app.user_api_client.get_user', return_value=api_user_active) - response = client.post(url_for('main.two_factor'), + response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '23456'}) assert response.status_code == 200 assert 'Code not found' in response.get_data(as_text=True) @@ -208,7 +208,7 @@ def test_should_login_user_when_multiple_valid_codes_exist( 'email': api_user_active['email_address']} api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' - response = client.post(url_for('main.two_factor'), + response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '23456'}) assert response.status_code == 302 @@ -230,7 +230,7 @@ def test_two_factor_should_set_password_when_new_password_exists_in_session( 'password': 'changedpassword'} api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' - response = client.post(url_for('main.two_factor'), + response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '12345'}) assert response.status_code == 302 assert response.location == url_for('main.show_accounts_or_dashboard', _external=True) @@ -252,7 +252,7 @@ def test_two_factor_returns_error_when_user_is_locked( 'id': api_user_locked['id'], 'email': api_user_locked['email_address'], } - response = client.post(url_for('main.two_factor'), + response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '12345'}) assert response.status_code == 200 assert 'Code not found' in response.get_data(as_text=True) @@ -262,13 +262,13 @@ def test_two_factor_post_should_redirect_to_sign_in_if_user_not_in_session( client_request, ): client_request.post( - 'main.two_factor', + 'main.two_factor_sms', _data={'sms_code': '12345'}, _expected_redirect=url_for('main.sign_in', _external=True) ) -@pytest.mark.parametrize('endpoint', ['main.two_factor_webauthn', 'main.two_factor']) +@pytest.mark.parametrize('endpoint', ['main.two_factor_webauthn', 'main.two_factor_sms']) def test_two_factor_get_should_redirect_to_sign_in_if_user_not_in_session( client_request, endpoint, @@ -296,7 +296,7 @@ def test_two_factor_should_activate_pending_user( 'id': api_user_pending['id'], 'email_address': api_user_pending['email_address'] } - client.post(url_for('main.two_factor'), data={'sms_code': '12345'}) + client.post(url_for('main.two_factor_sms'), data={'sms_code': '12345'}) assert mock_activate_user.called diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index da168c240..3e947de47 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -289,7 +289,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'trial_mode', 'trial_mode_new', 'trial_services', - 'two_factor', + 'two_factor_sms', 'two_factor_email', 'two_factor_email_interstitial', 'two_factor_email_sent', From c26a596839e638fb7afe6a7bbe7272e49685c1d3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 May 2021 17:37:57 +0100 Subject: [PATCH 03/15] allow sign in via webauthn credentials The flow of the code is roughly as follows: user clicks button on webauthn page js sends GET request python reads GET request, sets up login challenge python returns login challenge in response js reads GET response, passes login challenge to browser browser asks user to touch yubikey browser returns yubikey challenge response data to js js sends POST request with yubikey challenge response data python reads yubikey challenge and compares with users creds from db if its a match, python signs user in The login challenge is a PublicKeyCredentialRequestOptions: [1] The browser function we call is navigator.credentials.get(): [2] The response to the challenge from the browser is a PublicKeyCredential: [3] The python server does all the work setting those up and tearing them back down again (and checking them against the values we have stored in the database), but we need to do work to convert them to-and-from CBOR. [1] https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialRequestOptions [2] https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/get [3] https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential --- .../javascripts/authenticateSecurityKey.js | 58 +++++++++++++++---- app/main/views/webauthn_credentials.py | 53 ++++++++++++++++- .../main/views/test_webauthn_credentials.py | 24 ++++++++ tests/app/test_navigation.py | 2 + 4 files changed, 125 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 9ac503223..f5f4c50ce 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -1,13 +1,51 @@ (function (window) { - "use strict"; + "use strict"; - window.GOVUK.Modules.AuthenticateSecurityKey = function () { - this.start = function (component) { - $(component) - .on('click', function (event) { - event.preventDefault(); - console.log('pretend you just logged in okay'); - }); - }; + window.GOVUK.Modules.AuthenticateSecurityKey = function () { + this.start = function (component) { + $(component) + .on('click', function (event) { + event.preventDefault(); + + fetch('/webauthn/authenticate') + .then(response => { + if (!response.ok) { + throw Error(response.statusText); + } + + return response.arrayBuffer(); + }) + .then(data => { + var options = window.CBOR.decode(data); + // triggers browser dialogue to login with authenticator + return window.navigator.credentials.get(options); + }) + .then(credential => { + return fetch('/webauthn/authenticate', { + method: 'POST', + headers: { 'X-CSRFToken': component.data('csrfToken') }, + body: window.CBOR.encode({ + credentialId: new Uint8Array(credential.rawId), + authenticatorData: new Uint8Array(credential.response.authenticatorData), + signature: new Uint8Array(credential.response.signature), + clientDataJSON: new Uint8Array(credential.response.clientDataJSON), + }) + }); + }) + .then(response => { + if (!response.ok) { + throw Error(response.statusText); + } + // TODO: redirect + }) + .catch(error => { + console.error(error); + // some browsers will show an error dialogue for some + // errors; to be safe we always pop up an alert + var message = error.message || error; + alert('Error during authentication.\n\n' + message); + }); + }); }; -})(window); + }; +}) (window); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index e212374d2..1f5cd273f 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -1,11 +1,14 @@ from fido2 import cbor -from flask import current_app, request, session +from fido2.client import ClientData +from fido2.ctap2 import AuthenticatorData +from flask import abort, current_app, request, session from flask_login import current_user from app.main import main +from app.models.user import User from app.models.webauthn_credential import RegistrationError, WebAuthnCredential from app.notify_client.user_api_client import user_api_client -from app.utils import user_is_platform_admin +from app.utils import redirect_to_sign_in, user_is_platform_admin @main.route('/webauthn/register') @@ -50,3 +53,49 @@ def webauthn_complete_register(): ) return cbor.encode('') + + +@main.route('/webauthn/authenticate', methods=['GET']) +@redirect_to_sign_in +def webauthn_begin_authentication(): + # get user from session + user_to_login = User.from_id(session['user_details']['id']) + + authentication_data, state = current_app.webauthn_server.authenticate_begin( + credentials=[ + credential.to_credential_data() + for credential in user_to_login.webauthn_credentials + ], + user_verification=None, # required, preferred, discouraged. sets whether to ask for PIN + ) + session["webauthn_authentication_state"] = state + return cbor.encode(authentication_data) + + +@main.route('/webauthn/authenticate', methods=['POST']) +@redirect_to_sign_in +def webauthn_complete_authentication(): + state = session.pop("webauthn_authentication_state") + request_data = cbor.decode(request.get_data()) + + user_id = session['user_details']['id'] + user_to_login = User.from_id(user_id) + + try: + current_app.webauthn_server.authenticate_complete( + state=state, + credentials=[ + credential.to_credential_data() + for credential in user_to_login.webauthn_credentials + ], + credential_id=request_data['credentialId'], + client_data=ClientData(request_data['clientDataJSON']), + auth_data=AuthenticatorData(request_data['authenticatorData']), + signature=request_data['signature'] + ) + except ValueError as exc: + current_app.logger.info(f'User {user_id} could not sign in using their webauthn token - {exc}') + abort(403) + + from app.main.views.two_factor import log_in_user + return log_in_user(user_id) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index c74b1685e..4bfbfa904 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -157,3 +157,27 @@ def test_complete_register_handles_missing_state( assert response.status_code == 400 assert cbor.decode(response.data) == 'No registration in progress' + + +def test_begin_authentication_returns_encoded_options(client): + pass + + +def test_begin_authentication_includes_existing_credentials(client): + pass + + +def test_begin_authentication_stores_state_in_session(client): + pass + + +def test_complete_authentication_logs_user_in(client): + pass + + +def test_complete_authentication_403s_if_key_isnt_in_users_credentials(client): + pass + + +def test_complete_authentication_clears_session(client): + pass diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 3e947de47..f29ea0a0d 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -341,6 +341,8 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'view_template_versions', 'webauthn_begin_register', 'webauthn_complete_register', + 'webauthn_begin_authentication', + 'webauthn_complete_authentication', 'who_can_use_notify', 'who_its_for', 'write_new_broadcast', From a753e32c8de5e3b7968b0d18df57ac22111e2239 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 May 2021 18:14:13 +0100 Subject: [PATCH 04/15] only let platform admins with webauthn access the sign in pages --- app/main/views/webauthn_credentials.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 1f5cd273f..b80c37705 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -61,6 +61,12 @@ def webauthn_begin_authentication(): # get user from session user_to_login = User.from_id(session['user_details']['id']) + if not user_to_login.webauthn_auth: + abort(403) + + if not user_to_login.platform_admin: + abort(403) + authentication_data, state = current_app.webauthn_server.authenticate_begin( credentials=[ credential.to_credential_data() @@ -75,12 +81,18 @@ def webauthn_begin_authentication(): @main.route('/webauthn/authenticate', methods=['POST']) @redirect_to_sign_in def webauthn_complete_authentication(): - state = session.pop("webauthn_authentication_state") - request_data = cbor.decode(request.get_data()) - user_id = session['user_details']['id'] user_to_login = User.from_id(user_id) + if not user_to_login.webauthn_auth: + abort(403) + + if not user_to_login.platform_admin: + abort(403) + + state = session.pop("webauthn_authentication_state") + request_data = cbor.decode(request.get_data()) + try: current_app.webauthn_server.authenticate_complete( state=state, From 28ee2a1f9af28c65716ad68120e757768345bf53 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 17 May 2021 12:35:43 +0100 Subject: [PATCH 05/15] Add tests for GET webauthn_begin_authentication --- .../main/views/test_webauthn_credentials.py | 59 ++++++++++++++++--- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 4bfbfa904..9c958a404 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -159,20 +159,63 @@ def test_complete_register_handles_missing_state( assert cbor.decode(response.data) == 'No registration in progress' -def test_begin_authentication_returns_encoded_options(client): - pass +def test_begin_authentication_forbidden_for_non_platform_admins(client, api_user_active, mock_get_user): + # mock_get_user returns api_user_active so changes to the api user will reflect + api_user_active['auth_type'] = 'webauthn_auth' + + with client.session_transaction() as session: + session['user_details'] = {'id': '1'} + + response = client.get(url_for('main.webauthn_begin_authentication')) + assert response.status_code == 403 -def test_begin_authentication_includes_existing_credentials(client): - pass +def test_begin_authentication_forbidden_for_users_without_webauthn(client, mocker, platform_admin_user): + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + + with client.session_transaction() as session: + session['user_details'] = {'id': '1'} + + response = client.get(url_for('main.webauthn_begin_authentication')) + assert response.status_code == 403 -def test_begin_authentication_stores_state_in_session(client): - pass +def test_begin_authentication_returns_encoded_options(client, mocker, webauthn_credential, platform_admin_user): + platform_admin_user['auth_type'] = 'webauthn_auth' + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + + with client.session_transaction() as session: + session['user_details'] = {'id': platform_admin_user['id']} + + get_creds_mock = mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential] + ) + response = client.get(url_for('main.webauthn_begin_authentication')) + + decoded_data = cbor.decode(response.data) + allowed_credentials = decoded_data['publicKey']['allowCredentials'] + + assert len(allowed_credentials) == 1 + assert decoded_data['publicKey']['timeout'] == 30000 + get_creds_mock.assert_called_once_with(platform_admin_user['id']) -def test_complete_authentication_logs_user_in(client): - pass +def test_begin_authentication_stores_state_in_session(client, mocker, webauthn_credential, platform_admin_user): + platform_admin_user['auth_type'] = 'webauthn_auth' + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + + with client.session_transaction() as session: + session['user_details'] = {'id': platform_admin_user['id']} + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential] + ) + client.get(url_for('main.webauthn_begin_authentication')) + + with client.session_transaction() as session: + assert 'challenge' in session['webauthn_authentication_state'] def test_complete_authentication_403s_if_key_isnt_in_users_credentials(client): From d9fd37a4856c3c3f0026289a950868b79485fde0 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 17 May 2021 12:37:04 +0100 Subject: [PATCH 06/15] add test for succesfully logging in with security key this is a bit complex, but essentially we're using the test variables defined in the duolabs py_webauthn library [1]. We're already using their test variables in tests/app/models/test_webauthn_credential.py and in the webauthn_credential fixture in conftest.py. By using sample signature, authenticatordata and clientdatajson from the same key we can test that the library correctly verifies the signed challenge matches the original. We needed to transform some of this data as the yubico/fido2 library we use has a slightly different way of formatting the fields for the request body, which is why we're doing things like base64 decoding and converting from hex to bytes in the post data. The pytest fixture has changed - before it was incomplete/corrupted and would error when trying to verify the signature. We took the credential_data from the pytest fixture, converted it to an AttestedCredentialData using WebauthnCredential.to_credential_data, modified the public_key private dictionary to add `public_key[-1]: 1`, and then called `AttestedCredentialData.create` to re-CBOR-encode the blob. The `-1: 1` is the numeric ID of the "SECP256R1" elliptic curve algorithm. The py_webauthn library forces this particular algorithm, which differs from the sample creds we took from the fido2 lib tests, which is why we've had to update our data. [1] https://github.com/duo-labs/py_webauthn/blob/master/tests/test_webauthn.py#L13-L32 --- .../main/views/test_webauthn_credentials.py | 91 ++++++++++++++++++- tests/conftest.py | 6 +- 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 9c958a404..9d37517c6 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -1,8 +1,35 @@ +import base64 + import pytest from fido2 import cbor from flask import url_for -from app.models.webauthn_credential import RegistrationError +from app.models.webauthn_credential import RegistrationError, WebAuthnCredential + + +@pytest.fixture +def webauthn_authentication_post_data(fake_uuid, webauthn_credential, client): + """ + Sets up session, challenge, etc as if a user with uuid `fake_uuid` has logged in and touched the webauthn token + as found in the `webauthn_credential` fixture. Sets up the session as if `begin_authentication` had been called + so that the challenge matches and the credential will validate (provided that the key belongs to the user referenced + in the session). + """ + with client.session_transaction() as session: + session['user_details'] = {'id': fake_uuid} + session['webauthn_authentication_state'] = { + "challenge": "e-g-nXaRxMagEiqTJSyD82RsEc5if_6jyfJDy8bNKlw", + "user_verification": None + } + + credential_id = WebAuthnCredential(webauthn_credential).to_credential_data().credential_id + + return cbor.encode({ + 'credentialId': credential_id, + 'authenticatorData': base64.b64decode(b'dKbqkhPJnC90siSSsyDPQCYqlMGpUKA5fyklC2CEHvABAAACfQ=='), + 'clientDataJSON': b'{"challenge":"e-g-nXaRxMagEiqTJSyD82RsEc5if_6jyfJDy8bNKlw","origin":"https://webauthn.io","type":"webauthn.get"}', # noqa + 'signature': bytes.fromhex('304502204a76f05cd52a778cdd4df1565e0004e5cc1ead360419d0f5c3a0143bf37e7f15022100932b5c308a560cfe4f244214843075b904b3eda64e85d64662a81198c386cdde'), # noqa + }) @pytest.mark.parametrize('endpoint', [ @@ -218,9 +245,63 @@ def test_begin_authentication_stores_state_in_session(client, mocker, webauthn_c assert 'challenge' in session['webauthn_authentication_state'] -def test_complete_authentication_403s_if_key_isnt_in_users_credentials(client): - pass +def test_complete_authentication_checks_credentials( + client, + mocker, + webauthn_credential, + webauthn_dev_server, + mock_create_event, + webauthn_authentication_post_data, + platform_admin_user +): + platform_admin_user['auth_type'] = 'webauthn_auth' + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) + + response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) + assert response.status_code == 302 -def test_complete_authentication_clears_session(client): - pass +def test_complete_authentication_403s_if_key_isnt_in_users_credentials( + client, + mocker, + webauthn_credential, + webauthn_dev_server, + webauthn_authentication_post_data, + platform_admin_user +): + platform_admin_user['auth_type'] = 'webauthn_auth' + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + # user has no keys in the database + mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) + + response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) + assert response.status_code == 403 + + with client.session_transaction() as session: + assert session['user_details']['id'] == platform_admin_user['id'] + # user not logged in + assert 'user_id' not in session + # webauthn state reset so can't replay + assert 'webauthn_authentication_state' not in session + + +def test_complete_authentication_clears_session( + client, + mocker, + webauthn_credential, + webauthn_dev_server, + webauthn_authentication_post_data, + mock_create_event, + platform_admin_user +): + platform_admin_user['auth_type'] = 'webauthn_auth' + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) + + response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) + assert response.status_code == 302 + + with client.session_transaction() as session: + # it's important that we clear the session to ensure that we don't re-use old login artifacts in future + assert 'webauthn_authentication_state' not in session diff --git a/tests/conftest.py b/tests/conftest.py index ad3e3d02f..ab5f83e56 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3963,7 +3963,7 @@ def create_active_user_manage_template_permissions(with_unique_id=False): } -def create_platform_admin_user(with_unique_id=False, permissions=None): +def create_platform_admin_user(with_unique_id=False, auth_type='sms_auth', permissions=None): return { 'id': str(uuid4()) if with_unique_id else sample_uuid(), 'name': 'Platform admin user', @@ -3974,7 +3974,7 @@ def create_platform_admin_user(with_unique_id=False, permissions=None): 'failed_login_count': 0, 'permissions': permissions or {}, 'platform_admin': True, - 'auth_type': 'sms_auth', + 'auth_type': auth_type, 'password_changed_at': str(datetime.utcnow()), 'services': [], 'organisations': [], @@ -4481,7 +4481,7 @@ def webauthn_credential(): return { 'id': str(uuid4()), 'name': 'Test credential', - 'credential_data': 'WJ0AAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6', # noqa + 'credential_data': 'WJ8AAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpQECAyYgASFYIDGeoB8RJc5iMpRzZYAK5dndyHQkfFXRUWutPKPKMgdcIlggWfHwfzsvhsClHgz6E9xX58d6EQ55b4oLJ3Qf5YZjyzo=', # noqa 'registration_response': 'anything', 'created_at': '2017-10-18T16:57:14.154185Z', } From 92f78b14fecb3a398f25b4c326ee22af4f7042f8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 17 May 2021 15:56:15 +0100 Subject: [PATCH 07/15] redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function --- .../javascripts/authenticateSecurityKey.js | 13 ++- app/main/views/webauthn_credentials.py | 46 +++++++-- app/models/user.py | 3 + app/notify_client/user_api_client.py | 12 +++ .../main/views/test_webauthn_credentials.py | 96 ++++++++++++++++++- tests/app/notify_client/test_user_client.py | 45 ++++++++- 6 files changed, 204 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index f5f4c50ce..5f604962b 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -33,10 +33,21 @@ }); }) .then(response => { + if (response.status === 403){ + // flask will have `flash`ed an error message up + window.location.reload(); + return; + } + if (!response.ok) { + // probably an internal server error throw Error(response.statusText); } - // TODO: redirect + + // fetch will already have done the login redirect dance and will at this point be + // referring to the final 200 - hopefully to the `/accounts` url or similar. Set the location + // to trigger a browser navigate to that URL. + window.location.href = response.url; }) .catch(error => { console.error(error); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index b80c37705..ee38b65c8 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -1,14 +1,19 @@ from fido2 import cbor from fido2.client import ClientData from fido2.ctap2 import AuthenticatorData -from flask import abort, current_app, request, session +from flask import abort, current_app, flash, redirect, request, session, url_for from flask_login import current_user from app.main import main +from app.main.views.two_factor import log_in_user from app.models.user import User from app.models.webauthn_credential import RegistrationError, WebAuthnCredential from app.notify_client.user_api_client import user_api_client -from app.utils import redirect_to_sign_in, user_is_platform_admin +from app.utils import ( + is_less_than_days_ago, + redirect_to_sign_in, + user_is_platform_admin, +) @main.route('/webauthn/register') @@ -90,6 +95,12 @@ def webauthn_complete_authentication(): if not user_to_login.platform_admin: abort(403) + _complete_webauthn_authentication(user_to_login) + + return _verify_webauthn_login(user_to_login) + + +def _complete_webauthn_authentication(user): state = session.pop("webauthn_authentication_state") request_data = cbor.decode(request.get_data()) @@ -98,7 +109,7 @@ def webauthn_complete_authentication(): state=state, credentials=[ credential.to_credential_data() - for credential in user_to_login.webauthn_credentials + for credential in user.webauthn_credentials ], credential_id=request_data['credentialId'], client_data=ClientData(request_data['clientDataJSON']), @@ -106,8 +117,31 @@ def webauthn_complete_authentication(): signature=request_data['signature'] ) except ValueError as exc: - current_app.logger.info(f'User {user_id} could not sign in using their webauthn token - {exc}') + current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') + flash('Security key not recognised') + # TODO: increment failed login count abort(403) - from app.main.views.two_factor import log_in_user - return log_in_user(user_id) + +def _verify_webauthn_login(user): + """ + * check the user hasn't gone over their max logins + * check that the user's email is validated + * if succesful, update current_session_id, log in date, and then redirect + + """ + redirect_url = request.args.get('next') + + # normally API handles this when verifying an sms or email code but since the webauthn logic happens in the + # admin we need a separate call that just finalises the login in the database + logged_in, _ = user.verify_webauthn_login() + if not logged_in: + # user account is locked as too many failed logins + flash('Security key not recognised') + abort(403) + + if not is_less_than_days_ago(user.email_access_validated_at, 90): + user_api_client.send_verify_code(user.id, 'email', None, redirect_url) + return redirect(url_for('.revalidate_email_sent', next=redirect_url)) + + return log_in_user(user.id) diff --git a/app/models/user.py b/app/models/user.py index 4e9dde722..0a635cb93 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -430,6 +430,9 @@ class User(JSONModel, UserMixin): self.id, ) + def verify_webauthn_login(self, is_successful=True): + return user_api_client.verify_webauthn_login(self.id, is_successful) + class InvitedUser(JSONModel): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 915b87a2f..560fd094c 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -125,6 +125,18 @@ class UserApiClient(NotifyAdminAPIClient): return False, e.message raise e + @cache.delete('user-{user_id}') + def verify_webauthn_login(self, user_id, is_successful): + data = {'successful': is_successful} + endpoint = f'/user/{user_id}/verify/webauthn-login' + try: + self.post(endpoint, data=data) + return True, '' + except HTTPError as e: + if e.status_code == 403: + return False, e.message + raise e + def get_users_for_service(self, service_id): endpoint = '/service/{}/users'.format(service_id) return self.get(endpoint)['data'] diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 9d37517c6..573945267 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -1,8 +1,10 @@ import base64 +from unittest.mock import ANY import pytest from fido2 import cbor from flask import url_for +from freezegun.api import freeze_time from app.models.webauthn_credential import RegistrationError, WebAuthnCredential @@ -49,6 +51,7 @@ def test_begin_register_returns_encoded_options( webauthn_dev_server, ): mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) + response = platform_admin_client.get(url_for('main.webauthn_begin_register')) assert response.status_code == 200 @@ -257,9 +260,13 @@ def test_complete_authentication_checks_credentials( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) + # fake returning a 200 just to keep flask happy, normally this'll redirect + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) - assert response.status_code == 302 + + # matches response of verify_webauthn_login + assert response.data == b'ok' def test_complete_authentication_403s_if_key_isnt_in_users_credentials( @@ -274,6 +281,7 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) # user has no keys in the database mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) + mock_verify_webauthn_login = mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login') response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) assert response.status_code == 403 @@ -285,6 +293,8 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( # webauthn state reset so can't replay assert 'webauthn_authentication_state' not in session + assert mock_verify_webauthn_login.called is False + def test_complete_authentication_clears_session( client, @@ -298,10 +308,90 @@ def test_complete_authentication_clears_session( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) + # fake returning a 200 just to keep flask happy, normally this'll redirect + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) - response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) - assert response.status_code == 302 + client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) with client.session_transaction() as session: # it's important that we clear the session to ensure that we don't re-use old login artifacts in future assert 'webauthn_authentication_state' not in 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): + platform_admin_user['auth_type'] = 'webauthn_auth' + platform_admin_user['email_access_validated_at'] = '2020-01-25T00:00:00.000000Z' + + with client.session_transaction() as session: + session['user_details'] = { + 'id': platform_admin_user['id'], + 'email': platform_admin_user['email_address'] + } + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') + mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(True, None)) + + resp = client.post(url_for('main.webauthn_complete_authentication')) + + assert resp.status_code == 302 + assert resp.location == url_for('main.show_accounts_or_dashboard', _external=True) + # removes stuff from session + with client.session_transaction() as session: + assert 'user_details' not in session + + mock_create_event.assert_called_once_with('sucessful_login', ANY) + + +def test_verify_webauthn_login_signs_user_in_doesnt_sign_user_in_if_api_rejects( + client, + mocker, + platform_admin_user, +): + platform_admin_user['auth_type'] = 'webauthn_auth' + + with client.session_transaction() as session: + session['user_details'] = { + 'id': platform_admin_user['id'], + 'email': platform_admin_user['email_address'] + } + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') + mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(False, None)) + + resp = client.post(url_for('main.webauthn_complete_authentication')) + + assert resp.status_code == 403 + + +@freeze_time('2020-04-30') +def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( + client, + mocker, + mock_send_verify_code, + platform_admin_user, +): + platform_admin_user['auth_type'] = 'webauthn_auth' + platform_admin_user['email_access_validated_at'] = '2020-01-25T00:00:00.000000Z' + user_details = { + 'id': platform_admin_user['id'], + 'email': platform_admin_user['email_address'] + } + + with client.session_transaction() as session: + session['user_details'] = user_details + + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') + mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(True, None)) + + resp = client.post(url_for('main.webauthn_complete_authentication')) + + assert resp.status_code == 302 + assert resp.location == url_for('main.revalidate_email_sent', _external=True) + + with client.session_transaction() as session: + # stuff stays in session so we can log them in later when they validate their email + assert session['user_details'] == user_details + + mock_send_verify_code.assert_called_once_with(platform_admin_user['id'], 'email', ANY, ANY) diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 5331de1a2..97dfce3cd 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -1,7 +1,8 @@ import uuid -from unittest.mock import call +from unittest.mock import Mock, call import pytest +from notifications_python_client.errors import HTTPError from app import invite_api_client, service_api_client, user_api_client from app.models.webauthn_credential import WebAuthnCredential @@ -191,6 +192,7 @@ def test_returns_value_from_cache( (user_api_client, 'update_password', [user_id, 'hunter2'], {}), (user_api_client, 'verify_password', [user_id, 'hunter2'], {}), (user_api_client, 'check_verify_code', [user_id, '', ''], {}), + (user_api_client, 'complete_webauthn_login_attempt', [user_id], {'is_successful': True}), (user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, user_id, [], []], {}), (user_api_client, 'add_user_to_organisation', [sample_uuid(), user_id], {}), (user_api_client, 'set_user_permissions', [user_id, SERVICE_ONE_ID, []], {}), @@ -263,3 +265,44 @@ def test_create_webauthn_credential_for_user(mocker, webauthn_credential, fake_u user_api_client.create_webauthn_credential_for_user(fake_uuid, credential) mock_post.assert_called_once_with(expected_url, data=credential.serialize()) + + +def test_complete_webauthn_login_attempt_returns_true_and_no_message_normally(fake_uuid, mocker): + mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post') + + 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) + assert resp == (True, '') + + +def test_complete_webauthn_login_attempt_returns_false_and_message_on_403(fake_uuid, mocker): + mock_post = mocker.patch( + 'app.notify_client.user_api_client.UserApiClient.post', + side_effect=HTTPError( + response=Mock( + status_code=403, + json=Mock( + return_value={'message': 'forbidden'} + ) + ) + ) + ) + + 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) + + assert resp == (False, 'forbidden') + + +def test_complete_webauthn_login_attempt_raises_on_api_error(fake_uuid, mocker): + mocker.patch( + 'app.notify_client.user_api_client.UserApiClient.post', + side_effect=HTTPError(response=Mock(status_code=503, message='error')) + ) + + with pytest.raises(HTTPError): + user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True) From c29f87f55d05ec08fb7285d68acba211fbbabf1e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 19 May 2021 18:18:09 +0100 Subject: [PATCH 08/15] increment failed login count on unsuccesful webauthn login this doesn't include timeouts or other errors on the browser side - the main thing this catches is if the token doesn't belong to the user. However I'm not entirely clear if that's something that will be caught at this point, or if the browser would reject that key as it's not in the credentials passed in to the begin_authentication process. --- app/main/views/webauthn_credentials.py | 2 +- tests/app/main/views/test_webauthn_credentials.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index ee38b65c8..5fe4c5435 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -119,7 +119,7 @@ def _complete_webauthn_authentication(user): except ValueError as exc: current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') flash('Security key not recognised') - # TODO: increment failed login count + user.verify_webauthn_login(is_successful=False) abort(403) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 573945267..da8def9eb 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -282,6 +282,7 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( # user has no keys in the database mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) mock_verify_webauthn_login = mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login') + mock_unsuccesful_login_api_call = mocker.patch('app.user_api_client.verify_webauthn_login') response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) assert response.status_code == 403 @@ -294,6 +295,8 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( assert 'webauthn_authentication_state' not in session assert mock_verify_webauthn_login.called is False + # make sure we incremented the failed login count + mock_unsuccesful_login_api_call.assert_called_once_with(platform_admin_user['id'], False) def test_complete_authentication_clears_session( From d05f127e415b2b6eeed2c08d9ecfa3971962674e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 May 2021 15:50:14 +0100 Subject: [PATCH 09/15] return 200 to js instead of 302 when logging in the js fetch function is really not designed to work with 302s. when it receives a 302, it automatically follows it and fetches the next page. This is awkward because I don't want js to do all this in ajax, I want the browser to get the new URL so it can load the page. A better approach is to view the admin endpoint as a more pure API: the js sends a request for authentication to the admin app, and the admin app responds with a 200 indicating success, and then a payload of relevant data with that. The relevant data in this case is "Which URL should I redirect to", it might be the user's list of services page, or it might be a page telling them that their email needs revalidating. --- .../javascripts/authenticateSecurityKey.js | 21 ++++++++++--------- app/main/views/webauthn_credentials.py | 3 ++- .../main/views/test_webauthn_credentials.py | 20 ++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 5f604962b..d9f396580 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -33,21 +33,22 @@ }); }) .then(response => { - if (response.status === 403){ + if (response.status === 403) { // flask will have `flash`ed an error message up window.location.reload(); return; } - if (!response.ok) { - // probably an internal server error - throw Error(response.statusText); - } - - // fetch will already have done the login redirect dance and will at this point be - // referring to the final 200 - hopefully to the `/accounts` url or similar. Set the location - // to trigger a browser navigate to that URL. - window.location.href = response.url; + return response.arrayBuffer() + .then(cbor => { + return Promise.resolve(window.CBOR.decode(cbor)); + }) + .catch(() => { + throw Error(response.statusText); + }) + .then(data => { + window.location = data.redirect_url; + }); }) .catch(error => { console.error(error); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 5fe4c5435..2bfdf98ac 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -97,7 +97,8 @@ def webauthn_complete_authentication(): _complete_webauthn_authentication(user_to_login) - return _verify_webauthn_login(user_to_login) + redirect = _verify_webauthn_login(user_to_login) + return cbor.encode({'redirect_url': redirect.location}), 200 def _complete_webauthn_authentication(user): diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index da8def9eb..54e1254cf 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -1,5 +1,5 @@ import base64 -from unittest.mock import ANY +from unittest.mock import ANY, Mock import pytest from fido2 import cbor @@ -260,13 +260,12 @@ def test_complete_authentication_checks_credentials( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - # fake returning a 200 just to keep flask happy, normally this'll redirect - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) - # matches response of verify_webauthn_login - assert response.data == b'ok' + assert response.status_code == 200 + assert cbor.decode(response.data) == {'redirect_url': '/foo'} def test_complete_authentication_403s_if_key_isnt_in_users_credentials( @@ -311,8 +310,7 @@ def test_complete_authentication_clears_session( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - # fake returning a 200 just to keep flask happy, normally this'll redirect - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) @@ -337,8 +335,8 @@ def test_verify_webauthn_login_signs_user_in_signs_user_in(client, mocker, mock_ resp = client.post(url_for('main.webauthn_complete_authentication')) - assert resp.status_code == 302 - assert resp.location == url_for('main.show_accounts_or_dashboard', _external=True) + assert resp.status_code == 200 + assert cbor.decode(resp.data)['redirect_url'] == url_for('main.show_accounts_or_dashboard') # removes stuff from session with client.session_transaction() as session: assert 'user_details' not in session @@ -390,8 +388,8 @@ def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( resp = client.post(url_for('main.webauthn_complete_authentication')) - assert resp.status_code == 302 - assert resp.location == url_for('main.revalidate_email_sent', _external=True) + assert resp.status_code == 200 + assert cbor.decode(resp.data)['redirect_url'] == url_for('main.revalidate_email_sent') with client.session_transaction() as session: # stuff stays in session so we can log them in later when they validate their email From e765f98a0250a03e9e236d23ce051429bc80336a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 May 2021 18:43:01 +0100 Subject: [PATCH 10/15] use mockImplementationOnce to define separate return vals for fetch rather than having a gross if/else, we can define separately. This means we can separate the asserts and test setups for the first fetch (get) and the second fetch (post), which means we can arrange all the mocks in the order they're called in the function, significantly enhancing legibility of the tests --- tests/javascripts/registerSecurityKey.test.js | 87 ++++++++++--------- 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 53e13e9d3..14fea8487 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -36,6 +36,17 @@ describe('Register security key', () => { }) test('creates a new credential and reloads', (done) => { + + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { + // initial fetch of options from the server + // options from the server are CBOR-encoded + const webauthnOptions = window.CBOR.encode('options') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }); + jest.spyOn(window.navigator.credentials, 'create').mockImplementation((options) => { expect(options).toEqual('options') @@ -49,29 +60,23 @@ describe('Register security key', () => { }) }) + jest.spyOn(window, 'fetch').mockImplementationOnce((_url, options) => { + // subsequent POST of credential data to server + const decodedData = window.CBOR.decode(options.body) + 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({ ok: true }) + }); + jest.spyOn(window.location, 'reload').mockImplementation(() => { // signal that the async promise chain was called done() }) - jest.spyOn(window, 'fetch').mockImplementation((_url, options = {}) => { - // initial fetch of options from the server - if (!options.method) { - // options from the server are CBOR-encoded - webauthnOptions = window.CBOR.encode('options') - - return Promise.resolve({ - ok: true, arrayBuffer: () => webauthnOptions - }) - - // subsequent POST of credential data to server - } else { - decodedData = window.CBOR.decode(options.body) - 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({ ok: true }) - } + // this will make the test fail if the alert is called + jest.spyOn(window, 'alert').mockImplementation((msg) => { + done(msg) }) button.click() @@ -81,7 +86,7 @@ describe('Register security key', () => { ['network'], ['server'], ])('alerts if fetching WebAuthn options fails (%s error)', (errorType, done) => { - jest.spyOn(window, 'fetch').mockImplementation((_url, options = {}) => { + jest.spyOn(window, 'fetch').mockImplementation((_url) => { if (errorType == 'network') { return Promise.reject('error') } else { @@ -102,32 +107,32 @@ describe('Register security key', () => { ['internal server error'], ['bad request'], ])('alerts if sending WebAuthn credentials fails (%s)', (errorType, done) => { + + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { + // initial fetch of options from the server + const webauthnOptions = window.CBOR.encode('options') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }) + jest.spyOn(window.navigator.credentials, 'create').mockImplementation(() => { // fake PublicKeyCredential response from WebAuthn API return Promise.resolve({ response: {} }) }) - jest.spyOn(window, 'fetch').mockImplementation((_url, options = {}) => { - // initial fetch of options from the server - if (!options.method) { - webauthnOptions = window.CBOR.encode('options') - - return Promise.resolve({ - ok: true, arrayBuffer: () => webauthnOptions - }) - + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { // subsequent POST of credential data to server - } else { - switch (errorType) { - case 'network error': - return Promise.reject('error') - case 'bad request': - message = Promise.resolve(window.CBOR.encode('error')) - return Promise.resolve({ ok: false, arrayBuffer: () => message }) - case 'internal server error': - message = Promise.reject('encoding error') - return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) - } + switch (errorType) { + case 'network error': + return Promise.reject('error') + case 'bad request': + message = Promise.resolve(window.CBOR.encode('error')) + return Promise.resolve({ ok: false, arrayBuffer: () => message }) + case 'internal server error': + message = Promise.reject('encoding error') + return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) } }) @@ -144,9 +149,9 @@ describe('Register security key', () => { return Promise.reject(new DOMException('error')) }) - jest.spyOn(window, 'fetch').mockImplementation((_url, options) => { + jest.spyOn(window, 'fetch').mockImplementation((_url) => { // initial fetch of options from the server - webauthnOptions = window.CBOR.encode('options') + const webauthnOptions = window.CBOR.encode('options') return Promise.resolve({ ok: true, arrayBuffer: () => webauthnOptions From 6a21915ceee653b6bb81515b238c86207ef28d1f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 May 2021 18:02:38 +0100 Subject: [PATCH 11/15] add webauthn authentication js tests notably i had to change `window.location = foo` to `window.location.assign` so that i could have something to spy on with jest. mocking sucks. Otherwise this is pretty similar to the registerSecurityKey.test.js file. --- .../javascripts/authenticateSecurityKey.js | 2 +- .../authenticateSecurityKey.test.js | 232 ++++++++++++++++++ 2 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 tests/javascripts/authenticateSecurityKey.test.js diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index d9f396580..1326c923b 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -47,7 +47,7 @@ throw Error(response.statusText); }) .then(data => { - window.location = data.redirect_url; + window.location.assign(data.redirect_url); }); }) .catch(error => { diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js new file mode 100644 index 000000000..90bbf89f3 --- /dev/null +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -0,0 +1,232 @@ +beforeAll(() => { + window.CBOR = require('../../node_modules/cbor-js/cbor.js') + require('../../app/assets/javascripts/authenticateSecurityKey.js') + + // disable console.error() so we don't see it in test output + // you might need to comment this out to debug some failures + jest.spyOn(console, 'error').mockImplementation(() => { }) + + // 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', () => { + let button + + beforeEach(() => { + document.body.innerHTML = ` + + ` + button = document.querySelector('[data-module="authenticate-security-key"]') + window.GOVUK.modules.start() + }) + + test('authenticates a credential and redirects', (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 + const webauthnOptions = window.CBOR.encode('someArbitraryOptions') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }) + + jest.spyOn(window.navigator.credentials, 'get').mockImplementation((options) => { + expect(options).toEqual('someArbitraryOptions') + + // fake PublicKeyCredential response from WebAuthn API + // all of the array properties represent Array(Buffer) objects + const credentialsGetResponse = { + response: { + authenticatorData: [2, 2, 2], + signature: [3, 3, 3], + clientDataJSON: [4, 4, 4] + }, + rawId: [1, 1, 1], + type: "public-key", + } + return Promise.resolve(credentialsGetResponse) + }) + + jest.spyOn(window, 'fetch') + .mockImplementationOnce((_url, options = {}) => { + // subsequent POST of credential data to server + const decodedData = window.CBOR.decode(options.body) + expect(decodedData.credentialId).toEqual(new Uint8Array([1, 1, 1])) + expect(decodedData.authenticatorData).toEqual(new Uint8Array([2, 2, 2])) + expect(decodedData.signature).toEqual(new Uint8Array([3, 3, 3])) + expect(decodedData.clientDataJSON).toEqual(new Uint8Array([4, 4, 4])) + expect(options.headers['X-CSRFToken']).toBe('abc123') + const loginResponse = window.CBOR.encode({ redirect_url: '/foo' }) + + return Promise.resolve({ + ok: true, arrayBuffer: () => Promise.resolve(loginResponse) + }) + }) + + jest.spyOn(window.location, 'assign').mockImplementation((href) => { + expect(href).toEqual("/foo") + done(); + }) + + // this will make the test fail if the alert is called + jest.spyOn(window, 'alert').mockImplementation((msg) => { + done(msg) + }) + + button.click() + }); + + test.each([ + ['network'], + ['server'], + ])('alerts if fetching WebAuthn fails (%s error)', (errorType, done) => { + jest.spyOn(window, 'fetch').mockImplementation((_url) => { + if (errorType == 'network') { + return Promise.reject('error') + } else { + return Promise.resolve({ ok: false, statusText: 'error' }) + } + }) + + jest.spyOn(window, 'alert').mockImplementation((msg) => { + expect(msg).toEqual('Error during authentication.\n\nerror') + done() + }) + + button.click() + }) + + test('alerts if comms with the authenticator fails', (done) => { + jest.spyOn(window, 'fetch') + .mockImplementationOnce((_url) => { + const webauthnOptions = window.CBOR.encode('someArbitraryOptions') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }) + + jest.spyOn(window.navigator.credentials, 'get').mockImplementation(() => { + return Promise.reject(new DOMException('error')) + }) + + jest.spyOn(window, 'alert').mockImplementation((msg) => { + expect(msg).toEqual('Error during authentication.\n\nerror') + done() + }) + + button.click() + }); + + test.each([ + ['network error'], + ['internal server error'], + ])('alerts if POSTing WebAuthn credentials fails (%s)', (errorType, done) => { + jest.spyOn(window, 'fetch') + .mockImplementationOnce((_url) => { + const webauthnOptions = window.CBOR.encode('someArbitraryOptions') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }) + + jest.spyOn(window.navigator.credentials, 'get').mockImplementation((options) => { + expect(options).toEqual('someArbitraryOptions') + const credentialsGetResponse = { + response: { + authenticatorData: [2, 2, 2], + signature: [3, 3, 3], + clientDataJSON: [4, 4, 4] + }, + rawId: [1, 1, 1], + type: "public-key", + } + return Promise.resolve(credentialsGetResponse) + }) + + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { + // subsequent POST of credential data to server + switch (errorType) { + case 'network error': + return Promise.reject('error') + case 'internal server error': + // dont need this becuase we dont cbor return errors + const message = Promise.reject('encoding error') + return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) + } + }) + + jest.spyOn(window, 'alert').mockImplementation((msg) => { + expect(msg).toEqual('Error during authentication.\n\nerror') + done() + }) + + button.click() + }); + + + test('reloads page if POSTing WebAuthn credentials returns 403', (done) => { + jest.spyOn(window, 'fetch') + .mockImplementationOnce((_url) => { + const webauthnOptions = window.CBOR.encode('someArbitraryOptions') + + return Promise.resolve({ + ok: true, arrayBuffer: () => webauthnOptions + }) + }) + + jest.spyOn(window.navigator.credentials, 'get').mockImplementation((options) => { + expect(options).toEqual('someArbitraryOptions') + const credentialsGetResponse = { + response: { + authenticatorData: [2, 2, 2], + signature: [3, 3, 3], + clientDataJSON: [4, 4, 4] + }, + rawId: [1, 1, 1], + type: "public-key", + } + return Promise.resolve(credentialsGetResponse) + }) + + jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { + return Promise.resolve( + { + ok: false, + status: 403, + }) + }) + + // assert that reload is called and the page is refreshed + jest.spyOn(window.location, 'reload').mockImplementation(() => { + done(); + }) + + // this will make the test fail if the alert is called + jest.spyOn(window, 'alert').mockImplementation((msg) => { + done(msg) + }) + + button.click() + }); + + +}); From a3870af87d73ad5a05c6112e7c97436321509d4a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 May 2021 12:58:42 +0100 Subject: [PATCH 12/15] allow password reset with webauthn login flow --- app/main/views/new_password.py | 2 +- tests/app/main/views/test_new_password.py | 26 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index e060736a1..5167d7ae2 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -45,7 +45,7 @@ def new_password(token): # they've just clicked an email link, so have done an email auth journey anyway. Just log them in. return log_in_user(user.id) elif user.webauthn_auth: - raise NotImplementedError('webauthn not supported yet') + return redirect(url_for('main.two_factor_webauthn', next=request.args.get('next'))) else: # send user a 2fa sms code user.send_verify_code() diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index c7102771e..b0c690d1f 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -60,6 +60,32 @@ def test_should_redirect_to_two_factor_when_password_reset_is_successful( mock_get_user_by_email_request_password_reset.assert_called_once_with(user['email_address']) +@pytest.mark.parametrize('redirect_url', [ + None, + f'/services/{SERVICE_ONE_ID}/templates', +]) +def test_should_redirect_to_two_factor_webauthn_when_password_reset_is_successful( + notify_admin, + client, + mock_get_user_by_email_request_password_reset, + mock_send_verify_code, + mock_reset_failed_login_count, + redirect_url +): + user = mock_get_user_by_email_request_password_reset.return_value + user['auth_type'] = 'webauthn_auth' + data = json.dumps({'email': user['email_address'], 'created_at': str(datetime.utcnow())}) + token = generate_token(data, notify_admin.config['SECRET_KEY'], notify_admin.config['DANGEROUS_SALT']) + response = client.post(url_for_endpoint_with_token('.new_password', token=token, next=redirect_url), + data={'new_password': 'a-new_password'}) + assert response.status_code == 302 + assert response.location == url_for('.two_factor_webauthn', _external=True, next=redirect_url) + mock_get_user_by_email_request_password_reset.assert_called_once_with(user['email_address']) + + assert not mock_send_verify_code.called + assert mock_reset_failed_login_count.called + + def test_should_redirect_index_if_user_has_already_changed_password( notify_admin, client, From e864100be7aa2493f8643791ba8990c628d59902 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 27 May 2021 14:30:11 +0100 Subject: [PATCH 13/15] make sure error message flashes work properly flashes are consumed by the jinja template calling get_flashed_messages in flash_messages.html. When you call `abort(403)` the 403 error page is rendered, with the flashed message on it. However, the webauthn endpoints just return that page to the ajax `fetch`, which ignores the response and just reloads the page. Instead of calling abort, we can just return an empty response body and the 403 error code, so that the flashed messages stay in the session and will be rendered when the `GET /two-factor-webauthn` request happens after the js reloads the page. --- app/main/views/two_factor.py | 1 + app/main/views/webauthn_credentials.py | 24 +++++++++++++++---- .../main/views/test_webauthn_credentials.py | 6 +++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 163c71b05..9ed4b45fd 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -86,6 +86,7 @@ def two_factor_sms(): @main.route('/two-factor-webauthn', methods=['GET']) @redirect_to_sign_in def two_factor_webauthn(): + # TODO: Return a sensible error page if the user isn't platform admin or doesn't have webauthn redirect_url = request.args.get('next') return render_template('views/two-factor-webauthn.html', redirect_url=redirect_url) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 2bfdf98ac..d0f2eb8d6 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -3,6 +3,7 @@ from fido2.client import ClientData from fido2.ctap2 import AuthenticatorData from flask import abort, current_app, flash, redirect, request, session, url_for from flask_login import current_user +from werkzeug.exceptions import Forbidden from app.main import main from app.main.views.two_factor import log_in_user @@ -95,9 +96,25 @@ def webauthn_complete_authentication(): if not user_to_login.platform_admin: abort(403) - _complete_webauthn_authentication(user_to_login) + try: + _complete_webauthn_authentication(user_to_login) + redirect = _verify_webauthn_login(user_to_login) + except Forbidden: + # We don't expect to reach this case in normal situations - normally errors (such as using the wrong + # security key) will be caught in the browser inside `window.navigator.credentials.get`, and the js will + # error first meaning it doesn't send the POST request to this method. If this method is called but the key + # couldn't be authenticated, something went wrong along the way, probably: + # * The browser didn't implement the webauthn standard correctly, and let something through it shouldn't have + # * The key itself is in some way corrupted, or of lower security standard + flash('Security key not recognised') + + # flash sets the error message in the user's session cookie, and flask renders it next time `render_template` + # is called. In authenticateSecurityKey.js we refresh the page if this POST returns a 403. + # we can't use `abort(403)` here, and just return an empty body instead as our 403 error handler would return + # an error page response containing the flash, but our javascript ignores the body of the error response and + # just looks at the error code + return '', 403 - redirect = _verify_webauthn_login(user_to_login) return cbor.encode({'redirect_url': redirect.location}), 200 @@ -119,7 +136,6 @@ def _complete_webauthn_authentication(user): ) except ValueError as exc: current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') - flash('Security key not recognised') user.verify_webauthn_login(is_successful=False) abort(403) @@ -138,7 +154,7 @@ def _verify_webauthn_login(user): logged_in, _ = user.verify_webauthn_login() if not logged_in: # user account is locked as too many failed logins - flash('Security key not recognised') + abort(403) if not is_less_than_days_ago(user.email_access_validated_at, 90): diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 54e1254cf..3c951b7dc 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -292,6 +292,8 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( assert 'user_id' not in session # webauthn state reset so can't replay assert 'webauthn_authentication_state' not in session + # make sure there's an error message to show when the page reloads + assert '_flashes' in session assert mock_verify_webauthn_login.called is False # make sure we incremented the failed login count @@ -362,6 +364,10 @@ def test_verify_webauthn_login_signs_user_in_doesnt_sign_user_in_if_api_rejects( resp = client.post(url_for('main.webauthn_complete_authentication')) + with client.session_transaction() as session: + # make sure there's an error message to show when the page reloads + assert '_flashes' in session + assert resp.status_code == 403 From 0ec92e8c2f54a5867582d929c0f7e52de6c5a520 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 1 Jun 2021 18:29:02 +0100 Subject: [PATCH 14/15] DRY attested webauthn creds data --- app/main/views/webauthn_credentials.py | 15 +++------------ app/models/user.py | 7 +++++++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index d0f2eb8d6..3bf59149a 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -28,10 +28,7 @@ def webauthn_begin_register(): "name": current_user.email_address, "displayName": current_user.name, }, - credentials=[ - credential.to_credential_data() - for credential in current_user.webauthn_credentials - ], + credentials=current_user.webauthn_credentials_as_cbor, user_verification="discouraged", # don't ask for PIN authenticator_attachment="cross-platform", ) @@ -74,10 +71,7 @@ def webauthn_begin_authentication(): abort(403) authentication_data, state = current_app.webauthn_server.authenticate_begin( - credentials=[ - credential.to_credential_data() - for credential in user_to_login.webauthn_credentials - ], + credentials=user_to_login.webauthn_credentials_as_cbor, user_verification=None, # required, preferred, discouraged. sets whether to ask for PIN ) session["webauthn_authentication_state"] = state @@ -125,10 +119,7 @@ def _complete_webauthn_authentication(user): try: current_app.webauthn_server.authenticate_complete( state=state, - credentials=[ - credential.to_credential_data() - for credential in user.webauthn_credentials - ], + credentials=user.webauthn_credentials_as_cbor, credential_id=request_data['credentialId'], client_data=ClientData(request_data['clientDataJSON']), auth_data=AuthenticatorData(request_data['authenticatorData']), diff --git a/app/models/user.py b/app/models/user.py index 0a635cb93..5de35723d 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -353,6 +353,13 @@ class User(JSONModel, UserMixin): return [WebAuthnCredential(json) for json in user_api_client.get_webauthn_credentials_for_user(self.id)] + @property + def webauthn_credentials_as_cbor(self): + return [ + credential.to_credential_data() + for credential in self.webauthn_credentials + ] + def serialize(self): dct = { "id": self.id, From 73a444b33a09fc41b4af8cb2efe1adffaa27721e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Jun 2021 11:25:02 +0100 Subject: [PATCH 15/15] rename webauthn auth functions _complete_webauthn_authentication -> _verify_webauthn_authentication This function just does verification of the actual auth process - checking the challenge is correct, the signature matches the public key we have stored in our database, etc. verify_webauthn_login -> _complete_webauthn_login_attempt This function doesn't do any actual verification, we've already verified the user is who they say they are (or not), it's about marking the attempt, either unsuccessful (we bump the failed_login_count in the db) or successful (we set the logged_in_at and current_session_id in the db). This change also informs changes to the names of methods on the user model and in user_api_client. --- app/main/views/webauthn_credentials.py | 17 +++++++----- app/models/user.py | 4 +-- app/notify_client/user_api_client.py | 3 ++- .../main/views/test_webauthn_credentials.py | 26 ++++++++++++------- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 3bf59149a..b4f12e63f 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -91,8 +91,8 @@ def webauthn_complete_authentication(): abort(403) try: - _complete_webauthn_authentication(user_to_login) - redirect = _verify_webauthn_login(user_to_login) + _verify_webauthn_authentication(user_to_login) + redirect = _complete_webauthn_login_attempt(user_to_login) except Forbidden: # We don't expect to reach this case in normal situations - normally errors (such as using the wrong # security key) will be caught in the browser inside `window.navigator.credentials.get`, and the js will @@ -112,7 +112,11 @@ def webauthn_complete_authentication(): return cbor.encode({'redirect_url': redirect.location}), 200 -def _complete_webauthn_authentication(user): +def _verify_webauthn_authentication(user): + """ + Check that the presented security key is valid, has signed the right challenge, and belongs to the user + we're trying to log in. + """ state = session.pop("webauthn_authentication_state") request_data = cbor.decode(request.get_data()) @@ -127,22 +131,21 @@ def _complete_webauthn_authentication(user): ) except ValueError as exc: current_app.logger.info(f'User {user.id} could not sign in using their webauthn token - {exc}') - user.verify_webauthn_login(is_successful=False) + user.complete_webauthn_login_attempt(is_successful=False) abort(403) -def _verify_webauthn_login(user): +def _complete_webauthn_login_attempt(user): """ * check the user hasn't gone over their max logins * check that the user's email is validated * if succesful, update current_session_id, log in date, and then redirect - """ redirect_url = request.args.get('next') # normally API handles this when verifying an sms or email code but since the webauthn logic happens in the # admin we need a separate call that just finalises the login in the database - logged_in, _ = user.verify_webauthn_login() + logged_in, _ = user.complete_webauthn_login_attempt() if not logged_in: # user account is locked as too many failed logins diff --git a/app/models/user.py b/app/models/user.py index 5de35723d..54fbb85d2 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -437,8 +437,8 @@ class User(JSONModel, UserMixin): self.id, ) - def verify_webauthn_login(self, is_successful=True): - return user_api_client.verify_webauthn_login(self.id, is_successful) + def complete_webauthn_login_attempt(self, is_successful=True): + return user_api_client.complete_webauthn_login_attempt(self.id, is_successful) class InvitedUser(JSONModel): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 560fd094c..4ab0305a8 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -126,8 +126,9 @@ class UserApiClient(NotifyAdminAPIClient): raise e @cache.delete('user-{user_id}') - def verify_webauthn_login(self, user_id, is_successful): + 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' try: self.post(endpoint, data=data) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 3c951b7dc..ccf6913d4 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -260,7 +260,10 @@ def test_complete_authentication_checks_credentials( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) + mocker.patch( + 'app.main.views.webauthn_credentials._complete_webauthn_login_attempt', + return_value=Mock(location='/foo') + ) response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) @@ -280,8 +283,8 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) # user has no keys in the database mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) - mock_verify_webauthn_login = mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login') - mock_unsuccesful_login_api_call = mocker.patch('app.user_api_client.verify_webauthn_login') + mock_verify_webauthn_login = mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_login_attempt') + mock_unsuccesful_login_api_call = mocker.patch('app.user_api_client.complete_webauthn_login_attempt') response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) assert response.status_code == 403 @@ -312,7 +315,10 @@ def test_complete_authentication_clears_session( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) + mocker.patch( + 'app.main.views.webauthn_credentials._complete_webauthn_login_attempt', + return_value=Mock(location='/foo') + ) client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) @@ -332,8 +338,8 @@ def test_verify_webauthn_login_signs_user_in_signs_user_in(client, mocker, mock_ 'email': platform_admin_user['email_address'] } mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) - mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') - mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(True, None)) + 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')) @@ -359,8 +365,8 @@ def test_verify_webauthn_login_signs_user_in_doesnt_sign_user_in_if_api_rejects( 'email': platform_admin_user['email_address'] } mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) - mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') - mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(False, None)) + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_authentication') + mocker.patch('app.user_api_client.complete_webauthn_login_attempt', return_value=(False, None)) resp = client.post(url_for('main.webauthn_complete_authentication')) @@ -389,8 +395,8 @@ def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( session['user_details'] = user_details mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) - mocker.patch('app.main.views.webauthn_credentials._complete_webauthn_authentication') - mocker.patch('app.user_api_client.verify_webauthn_login', return_value=(True, None)) + 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'))