tweak webauthn rest errors

simplify logic by changing the dao function to require a user id and a
webauthn cred id. Note that this changes the response from a 400 to a
404 if the cred is for a different user than the supplied id.

give a minimum length to the text fields in POSTS to create/update a
credential to avoid surprising unexpected edge cases involving empty
string names etc.

app/dao/webauthn_credential_dao.py

@@ -3,9 +3,10 @@ from app.dao.dao_utils import autocommit
 from app.models import WebauthnCredential
 
 
-def dao_get_webauthn_credential_by_id(webauthn_credential_id):
+def dao_get_webauthn_credential_by_user_and_id(user_id, webauthn_credential_id):
-    return WebauthnCredential.query.filter_by(
+    return WebauthnCredential.query.filter(
-        id=webauthn_credential_id
+        WebauthnCredential.user_id == user_id,
+        WebauthnCredential.id == webauthn_credential_id
     ).one()
 
 

app/webauthn/rest.py

@@ -4,7 +4,7 @@ from app.dao.users_dao import get_user_by_id
 from app.dao.webauthn_credential_dao import (
     dao_create_webauthn_credential,
     dao_delete_webauthn_credential,
-    dao_get_webauthn_credential_by_id,
+    dao_get_webauthn_credential_by_user_and_id,
     dao_update_webauthn_credential_name,
 )
 from app.errors import InvalidRequest, register_errors
@@ -43,11 +43,7 @@ def update_webauthn_credential(user_id, webauthn_credential_id):
     data = request.get_json()
     validate(data, post_update_webauthn_credential_schema)
 
-    webauthn_credential = dao_get_webauthn_credential_by_id(webauthn_credential_id)
+    webauthn_credential = dao_get_webauthn_credential_by_user_and_id(user_id, webauthn_credential_id)
-
-    user = get_user_by_id(user_id)
-
-    check_credential_belongs_to_user(webauthn_credential.user_id, user.id)
 
     dao_update_webauthn_credential_name(webauthn_credential, data['name'])
 
@@ -56,19 +52,13 @@ def update_webauthn_credential(user_id, webauthn_credential_id):
 
 @webauthn_blueprint.route('/<uuid:webauthn_credential_id>', methods=['DELETE'])
 def delete_webauthn_credential(user_id, webauthn_credential_id):
-    webauthn_credential = dao_get_webauthn_credential_by_id(webauthn_credential_id)
+    webauthn_credential = dao_get_webauthn_credential_by_user_and_id(user_id, webauthn_credential_id)
     user = get_user_by_id(user_id)
 
-    check_credential_belongs_to_user(webauthn_credential.user_id, user.id)
-
     if len(user.webauthn_credentials) == 1:
+        # TODO: Only raise an error if user has auth type webauthn_auth
         raise InvalidRequest('Cannot delete last remaining webauthn credential for user', status_code=400)
 
     dao_delete_webauthn_credential(webauthn_credential)
 
     return '', 204
-
-
-def check_credential_belongs_to_user(credential_user_id, user_id):
-    if credential_user_id != user_id:
-        raise InvalidRequest('Webauthn credential does not belong to this user', status_code=400)

app/webauthn/webauthn_schema.py

@@ -3,9 +3,9 @@ post_create_webauthn_credential_schema = {
     "description": "POST webauthn_credential schema",
     "type": "object",
     "properties": {
-        "name": {"type": "string"},
+        "name": {"type": "string", "minLength": 1},
-        "credential_data": {"type": "string"},
+        "credential_data": {"type": "string", "minLength": 1},
-        "registration_response": {"type": "string"},
+        "registration_response": {"type": "string", "minLength": 1},
     },
     "required": ["name", "credential_data", "registration_response"],
     "additionalProperties": False
@@ -16,7 +16,7 @@ post_update_webauthn_credential_schema = {
     "description": "POST update webauthn_credential schema",
     "type": "object",
     "properties": {
-        "name": {"type": "string"},
+        "name": {"type": "string", "minLength": 1},
     },
     "required": ["name"],
     "additionalProperties": False

tests/app/webauthn/test_rest.py

@@ -1,6 +1,8 @@
 import uuid
 from unittest.mock import ANY
 
+import pytest
+
 from tests.app.db import create_user, create_webauthn_credential
 
 
@@ -67,20 +69,33 @@ def test_create_webauthn_credential_returns_201(admin_request, sample_user):
     assert response['data']['id'] == str(new_cred.id)
 
 
-def test_create_webauthn_credential_errors_if_schema_violation(admin_request, sample_user):
+@pytest.mark.parametrize('data, err_msg', [
+    # missing registration_response
+    (
+        {'name': 'my key', 'credential_data': 'ABC123'},
+        'registration_response is a required property'
+    ),
+    # name is null
+    (
+        {'name': None, 'credential_data': 'ABC123'},
+        'name None is not of type string'
+    ),
+    # name is empty
+    (
+        {'name': '', 'credential_data': 'ABC123'},
+        'name  is too short'
+    ),
+])
+def test_create_webauthn_credential_errors_if_schema_violation(admin_request, sample_user, data, err_msg):
     response = admin_request.post(
         'webauthn.create_webauthn_credential',
         user_id=sample_user.id,
-        _data={
+        _data=data,
-            'name': 'my key',
-            'credential_data': 'ABC123',
-            # missing registration_response
-        },
         _expected_status=400
     )
     assert response['errors'][0] == {
         'error': 'ValidationError',
-        'message': 'registration_response is a required property'
+        'message': err_msg
     }
 
 
@@ -100,22 +115,35 @@ def test_update_webauthn_credential_returns_200(admin_request, sample_user):
     assert response['data']['name'] == 'new name'
 
 
-def test_update_webauthn_credential_errors_if_schema_violation(admin_request, sample_user):
+@pytest.mark.parametrize('data, err_msg', [
+    # you can't update credential_data
+    (
+        {'name': 'my key', 'credential_data': 'NAUGHTY123'},
+        'Additional properties are not allowed (credential_data was unexpected)'
+    ),
+    # name is null
+    (
+        {'name': None},
+        'name None is not of type string'
+    ),
+    # name is empty
+    (
+        {'name': ''},
+        'name  is too short'
+    ),
+])
+def test_update_webauthn_credential_errors_if_schema_violation(admin_request, sample_user, data, err_msg):
     cred = create_webauthn_credential(sample_user)
     response = admin_request.post(
         'webauthn.update_webauthn_credential',
         user_id=sample_user.id,
         webauthn_credential_id=cred.id,
-        _data={
+        _data=data,
-            'name': 'my key',
-            # you can't update credential_data
-            'credential_data': 'NAUGHTY123'
-        },
         _expected_status=400
     )
     assert response['errors'][0] == {
         'error': 'ValidationError',
-        'message': 'Additional properties are not allowed (credential_data was unexpected)'
+        'message': err_msg
     }
 
 
@@ -134,22 +162,19 @@ def test_update_webauthn_credential_errors_if_webauthn_credential_doesnt_exist(a
 def test_update_webauthn_credential_errors_if_user_id_doesnt_match(admin_request, notify_db_session):
     user_1 = create_user(email='1')
     user_2 = create_user(email='2')
-    cred_1a = create_webauthn_credential(user_1)  # noqa
+    cred_2 = create_webauthn_credential(user_2)
-    cred_1b = create_webauthn_credential(user_1)  # noqa
-    cred_2a = create_webauthn_credential(user_2)
-    cred_2b = create_webauthn_credential(user_2)  # noqa
 
     response = admin_request.post(
         'webauthn.update_webauthn_credential',
         user_id=user_1.id,
-        webauthn_credential_id=cred_2a.id,
+        webauthn_credential_id=cred_2.id,
         _data={
             'name': 'new key name',
         },
-        _expected_status=400
+        _expected_status=404
     )
 
-    assert response['message'] == 'Webauthn credential does not belong to this user'
+    assert response['message'] == 'No result found'
 
 
 def test_delete_webauthn_credential_returns_204(admin_request, sample_user):
@@ -178,16 +203,13 @@ def test_delete_webauthn_credential_errors_if_last_key(admin_request, sample_use
 def test_delete_webauthn_credential_errors_if_user_id_doesnt_match(admin_request, notify_db_session):
     user_1 = create_user(email='1')
     user_2 = create_user(email='2')
-    cred_1a = create_webauthn_credential(user_1)  # noqa
-    cred_1b = create_webauthn_credential(user_1)  # noqa
     cred_2a = create_webauthn_credential(user_2)
-    cred_2b = create_webauthn_credential(user_2)  # noqa
 
     response = admin_request.delete(
         'webauthn.delete_webauthn_credential',
         user_id=user_1.id,
         webauthn_credential_id=cred_2a.id,
-        _expected_status=400
+        _expected_status=404
     )
 
-    assert response['message'] == 'Webauthn credential does not belong to this user'
+    assert response['message'] == 'No result found'