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