From 73a444b33a09fc41b4af8cb2efe1adffaa27721e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Jun 2021 11:25:02 +0100 Subject: [PATCH] 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'))