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'))