From c190886bfed725e088a1e5d0457d31ad7ad1079a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 12 May 2021 15:34:37 +0100 Subject: [PATCH] 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 | 7 +-- app/webauthn/rest.py | 18 ++------ app/webauthn/webauthn_schema.py | 8 ++-- tests/app/webauthn/test_rest.py | 74 +++++++++++++++++++----------- 4 files changed, 60 insertions(+), 47 deletions(-) diff --git a/app/dao/webauthn_credential_dao.py b/app/dao/webauthn_credential_dao.py index 36d79315e..ac2bc9c7a 100644 --- a/app/dao/webauthn_credential_dao.py +++ b/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): - return WebauthnCredential.query.filter_by( - id=webauthn_credential_id +def dao_get_webauthn_credential_by_user_and_id(user_id, webauthn_credential_id): + return WebauthnCredential.query.filter( + WebauthnCredential.user_id == user_id, + WebauthnCredential.id == webauthn_credential_id ).one() diff --git a/app/webauthn/rest.py b/app/webauthn/rest.py index 6ce7bb6bf..2b63d349f 100644 --- a/app/webauthn/rest.py +++ b/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) - - user = get_user_by_id(user_id) - - check_credential_belongs_to_user(webauthn_credential.user_id, user.id) + webauthn_credential = dao_get_webauthn_credential_by_user_and_id(user_id, webauthn_credential_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('/', 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) diff --git a/app/webauthn/webauthn_schema.py b/app/webauthn/webauthn_schema.py index cc2d2f05a..0bcefa1b6 100644 --- a/app/webauthn/webauthn_schema.py +++ b/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"}, - "credential_data": {"type": "string"}, - "registration_response": {"type": "string"}, + "name": {"type": "string", "minLength": 1}, + "credential_data": {"type": "string", "minLength": 1}, + "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 diff --git a/tests/app/webauthn/test_rest.py b/tests/app/webauthn/test_rest.py index 7c9404271..7f71b99d2 100644 --- a/tests/app/webauthn/test_rest.py +++ b/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={ - 'name': 'my key', - 'credential_data': 'ABC123', - # missing registration_response - }, + _data=data, _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={ - 'name': 'my key', - # you can't update credential_data - 'credential_data': 'NAUGHTY123' - }, + _data=data, _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_1b = create_webauthn_credential(user_1) # noqa - cred_2a = create_webauthn_credential(user_2) - cred_2b = create_webauthn_credential(user_2) # noqa + cred_2 = create_webauthn_credential(user_2) 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'