From f6aa5bdfb85f40e071792db301ae2a5aa88de1ec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 Jun 2021 09:41:39 +0100 Subject: [PATCH] Refactor User.webauthn_credentials into a ModelList This saves a bit of repetition, and lets us attach other methods to the collection, rather than having multiple methods on the user object prefixed with the same name, or random functions floating about. --- app/main/views/user_profile.py | 7 +-- app/main/views/webauthn_credentials.py | 6 +-- app/models/user.py | 12 +----- app/models/webauthn_credential.py | 16 ++++++- tests/app/main/views/test_user_profile.py | 43 ++++++++++++------- .../main/views/test_webauthn_credentials.py | 14 +++--- 6 files changed, 56 insertions(+), 42 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index f6c701874..c96f306ef 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -243,10 +243,6 @@ def user_profile_security_keys(): ) -def get_key_from_list_of_keys(key_id, list_of_keys): - return next((key for key in list_of_keys if key.id == key_id), None) - - @main.route( "/user-profile/security-keys//manage", methods=['GET', 'POST'], @@ -259,8 +255,7 @@ def get_key_from_list_of_keys(key_id, list_of_keys): ) @user_is_platform_admin def user_profile_manage_security_key(key_id): - security_keys = current_user.webauthn_credentials - security_key = get_key_from_list_of_keys(key_id, security_keys) + security_key = current_user.webauthn_credentials.by_id(key_id) if not security_key: abort(404) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index a0e9ddb53..75ef6bb8d 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -28,7 +28,7 @@ def webauthn_begin_register(): "name": current_user.email_address, "displayName": current_user.name, }, - credentials=current_user.webauthn_credentials_as_cbor, + credentials=current_user.webauthn_credentials.as_cbor, user_verification="discouraged", # don't ask for PIN authenticator_attachment="cross-platform", ) @@ -85,7 +85,7 @@ def webauthn_begin_authentication(): abort(403) authentication_data, state = current_app.webauthn_server.authenticate_begin( - credentials=user_to_login.webauthn_credentials_as_cbor, + credentials=user_to_login.webauthn_credentials.as_cbor, user_verification="discouraged", # don't ask for PIN ) session["webauthn_authentication_state"] = state @@ -144,7 +144,7 @@ def _verify_webauthn_authentication(user): try: current_app.webauthn_server.authenticate_complete( state=state, - credentials=user.webauthn_credentials_as_cbor, + 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 584938513..2f534fe1a 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -10,7 +10,7 @@ from app.models.roles_and_permissions import ( all_permissions, translate_permissions_from_db_to_admin_roles, ) -from app.models.webauthn_credential import WebAuthnCredential +from app.models.webauthn_credential import WebAuthnCredentials from app.notify_client import InviteTokenError from app.notify_client.invite_api_client import invite_api_client from app.notify_client.org_invite_api_client import org_invite_api_client @@ -350,15 +350,7 @@ class User(JSONModel, UserMixin): @property def webauthn_credentials(self): - 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 - ] + return WebAuthnCredentials(self.id) def create_webauthn_credential(self, credential): user_api_client.create_webauthn_credential_for_user( diff --git a/app/models/webauthn_credential.py b/app/models/webauthn_credential.py index d9cfdfe67..c5bee7a71 100644 --- a/app/models/webauthn_credential.py +++ b/app/models/webauthn_credential.py @@ -6,7 +6,8 @@ from fido2.cose import UnsupportedKey from fido2.ctap2 import AttestationObject, AttestedCredentialData from flask import current_app -from app.models import JSONModel +from app.models import JSONModel, ModelList +from app.notify_client.user_api_client import user_api_client class RegistrationError(Exception): @@ -60,3 +61,16 @@ class WebAuthnCredential(JSONModel): 'credential_data': self.credential_data, 'registration_response': self.registration_response, } + + +class WebAuthnCredentials(ModelList): + + model = WebAuthnCredential + client_method = user_api_client.get_webauthn_credentials_for_user + + @property + def as_cbor(self): + return [credential.to_credential_data() for credential in self] + + def by_id(self, key_id): + return next((key for key in self if key.id == key_id), None) diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index e2cac7438..7e03f6a5a 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -6,8 +6,10 @@ from flask import url_for from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_token -from app.main.views.user_profile import get_key_from_list_of_keys -from app.models.user import WebAuthnCredential +from app.models.webauthn_credential import ( + WebAuthnCredential, + WebAuthnCredentials, +) from tests.conftest import ( create_api_user_active, normalize_spaces, @@ -29,7 +31,7 @@ def test_overview_page_shows_disable_for_platform_admin( platform_admin_user, mocker ): - mocker.patch('app.user_api_client.get_webauthn_credentials_for_user') + mocker.patch('app.models.webauthn_credential.WebAuthnCredentials.client_method') client_request.login(platform_admin_user) page = client_request.get('main.user_profile') assert page.select_one('h1').text.strip() == 'Your profile' @@ -52,7 +54,10 @@ def test_overview_page_shows_security_keys_for_platform_admin( ): client_request.login(platform_admin_user) credentials = [webauthn_credential for _ in range(key_count)] - mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=credentials) + mocker.patch( + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', + return_value=credentials, + ) page = client_request.get('main.user_profile') security_keys_row = page.select_one('#security-keys') assert ' '.join(security_keys_row.text.split()) == expected_row_text @@ -369,7 +374,7 @@ def test_should_show_security_keys_page( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -388,9 +393,17 @@ def test_should_show_security_keys_page( assert register_button.text.strip() == 'Register a key' -def test_get_key_from_list_of_keys(webauthn_credential, webauthn_credential_2): - list_of_keys = [WebAuthnCredential(json) for json in [webauthn_credential, webauthn_credential_2]] - assert get_key_from_list_of_keys(webauthn_credential["id"], list_of_keys) == WebAuthnCredential(webauthn_credential) +def test_get_key_from_list_of_keys( + mocker, + webauthn_credential, + webauthn_credential_2, + fake_uuid, +): + mocker.patch( + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', + return_value=[webauthn_credential, webauthn_credential_2], + ) + assert WebAuthnCredentials(fake_uuid).by_id(webauthn_credential["id"]) == WebAuthnCredential(webauthn_credential) def test_should_show_manage_security_key_page( @@ -402,7 +415,7 @@ def test_should_show_manage_security_key_page( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -425,7 +438,7 @@ def test_manage_security_key_page_404s_when_key_not_found( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential_2], ) client_request.get( @@ -469,7 +482,7 @@ def test_should_redirect_after_change_of_security_key_name( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -502,7 +515,7 @@ def test_user_profile_manage_security_key_should_not_call_api_if_key_name_stays_ client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -531,7 +544,7 @@ def test_shows_delete_link_for_security_key( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -552,7 +565,7 @@ def test_confirm_delete_security_key( client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) @@ -601,7 +614,7 @@ def test_delete_security_key_handles_last_credential_error( ): client_request.login(platform_admin_user) mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential], ) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 19386097b..f7afdddf2 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -50,7 +50,7 @@ def test_begin_register_returns_encoded_options( platform_admin_client, webauthn_dev_server, ): - mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) + mocker.patch('app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[]) response = platform_admin_client.get(url_for('main.webauthn_begin_register')) @@ -79,7 +79,7 @@ def test_begin_register_includes_existing_credentials( mocker, ): mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential, webauthn_credential] ) @@ -96,7 +96,7 @@ def test_begin_register_stores_state_in_session( mocker, ): mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[]) response = platform_admin_client.get( @@ -227,7 +227,7 @@ def test_begin_authentication_returns_encoded_options(client, mocker, webauthn_c session['user_details'] = {'id': platform_admin_user['id']} get_creds_mock = mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential] ) response = client.get(url_for('main.webauthn_begin_authentication')) @@ -248,7 +248,7 @@ def test_begin_authentication_stores_state_in_session(client, mocker, webauthn_c session['user_details'] = {'id': platform_admin_user['id']} mocker.patch( - 'app.user_api_client.get_webauthn_credentials_for_user', + 'app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential] ) client.get(url_for('main.webauthn_begin_authentication')) @@ -268,7 +268,7 @@ 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.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[webauthn_credential]) mocker.patch( 'app.main.views.webauthn_credentials._complete_webauthn_login_attempt', return_value=Mock(location='/foo') @@ -291,7 +291,7 @@ def test_complete_authentication_403s_if_key_isnt_in_users_credentials( 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=[]) + mocker.patch('app.models.webauthn_credential.WebAuthnCredentials.client_method', return_value=[]) 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')