Merge pull request #3920 from alphagov/refactor-webauthn-model

Refactor User.webauthn_credentials into a ModelList

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/<uuid:key_id>/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)

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

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(

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)

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],
     )
 

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