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.
This commit is contained in:
Leo Hemsted
2021-06-02 11:25:02 +01:00
parent 0ec92e8c2f
commit 73a444b33a
4 changed files with 30 additions and 20 deletions

View File

@@ -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

View File

@@ -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):

View File

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

View File

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