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)