From 3798a3bd1d894f2d20a84824eccf174c85225667 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 7 May 2021 16:08:34 +0100 Subject: [PATCH 1/6] Add webauthn_credential table This is to store data for registered webauthn credentials, so platform admins can later use them to log in. --- app/models.py | 18 +++++++++ .../versions/0355_add_webauthn_table.py | 37 +++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 migrations/versions/0355_add_webauthn_table.py diff --git a/app/models.py b/app/models.py index 84b2abdff..7abeaf848 100644 --- a/app/models.py +++ b/app/models.py @@ -2592,3 +2592,21 @@ class ServiceBroadcastProviderRestriction(db.Model): provider = db.Column(db.String, nullable=False) created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + + +class WebauthnCredential(db.Model): + """ + A table that stores data for registered webauthn credentials. + """ + __tablename__ = "webauthn_credential" + + credential_id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + aaguid = db.Column(UUID(as_uuid=True), default=uuid.uuid4, nullable=False) + public_key = db.Column(db.String, nullable=False) + + user_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), primary_key=True, nullable=False) + + registration_response = db.Column(JSONB(none_as_null=True), nullable=False, default={}) + + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, nullable=True, default=datetime.datetime.utcnow) diff --git a/migrations/versions/0355_add_webauthn_table.py b/migrations/versions/0355_add_webauthn_table.py new file mode 100644 index 000000000..6e25ede16 --- /dev/null +++ b/migrations/versions/0355_add_webauthn_table.py @@ -0,0 +1,37 @@ +""" + +Revision ID: 0355_add_webauthn_table +Revises: 0354_government_channel +Create Date: 2021-05-07 17:04:22.017137 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0355_add_webauthn_table' +down_revision = '0354_government_channel' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + 'webauthn_credential', + sa.Column('credential_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('aaguid', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('public_key', sa.String(), nullable=False), + sa.Column('user_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('registration_response', postgresql.JSONB(none_as_null=True, astext_type=sa.Text()), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['user_id'], ['users.id'], ), + sa.PrimaryKeyConstraint('credential_id', 'user_id') + ) + # ### end Alembic commands ### + + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('webauthn_credential') + # ### end Alembic commands ### From 500feba50d0071424b5a40d6ec2ca8a5552671fc Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 10 May 2021 16:36:00 +0100 Subject: [PATCH 2/6] add name/id and consolidate webauthn types in model/table so we can be in line with what the admin handles, and keep it simple on the api side and do as little manipulation of binary data as possible. ### Minor changes * id is a UUID we can use for referencing within notify. No relation to the key itself. * name is a user viewable name that can be set/edited * fix updated_at to have onupdate, not default ### Simplify the webauthn data credential_data is the data we store about an authenticator that we'll use to identify the key when logging in. includes the credential_id, the public_key, and the aaguid (which identifies the authenticator make/model) registration_response is the data containing audit information - in the future we can use this to ensure that the authenticators used are of high quality. both of these fields are CBOR (a kind of binary json), encoded in base64 so that they can be embedded within our regular JSON api endpoints. we don't anticipate the api ever needing to interact with this data directly. --- app/models.py | 17 ++++++++++------ .../versions/0355_add_webauthn_table.py | 20 +++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/app/models.py b/app/models.py index 7abeaf848..9c471b5f2 100644 --- a/app/models.py +++ b/app/models.py @@ -2600,13 +2600,18 @@ class WebauthnCredential(db.Model): """ __tablename__ = "webauthn_credential" - credential_id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - aaguid = db.Column(UUID(as_uuid=True), default=uuid.uuid4, nullable=False) - public_key = db.Column(db.String, nullable=False) + id = db.Column(UUID(as_uuid=True), primary_key=True, nullable=False, default=uuid.uuid4) - user_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), primary_key=True, nullable=False) + user_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=False) + user = db.relationship(User, backref=db.backref("webauthn_credentials")) - registration_response = db.Column(JSONB(none_as_null=True), nullable=False, default={}) + name = db.Column(db.String, nullable=False) + + # base64 encoded CBOR. used for logging in. https://w3c.github.io/webauthn/#sctn-attested-credential-data + credential_data = db.Column(db.String, nullable=False) + + # base64 encoded CBOR. used for auditing. https://www.w3.org/TR/webauthn-2/#authenticatorattestationresponse + registration_response = db.Column(db.String, nullable=False) created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) - updated_at = db.Column(db.DateTime, nullable=True, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) diff --git a/migrations/versions/0355_add_webauthn_table.py b/migrations/versions/0355_add_webauthn_table.py index 6e25ede16..ed0e23967 100644 --- a/migrations/versions/0355_add_webauthn_table.py +++ b/migrations/versions/0355_add_webauthn_table.py @@ -14,24 +14,22 @@ down_revision = '0354_government_channel' def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.create_table( 'webauthn_credential', - sa.Column('credential_id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('aaguid', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('public_key', sa.String(), nullable=False), + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('user_id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('registration_response', postgresql.JSONB(none_as_null=True, astext_type=sa.Text()), nullable=False), + sa.Column('name', sa.String(), nullable=False), + + sa.Column('credential_data', sa.String(), nullable=False), + sa.Column('registration_response', sa.String(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), sa.Column('updated_at', sa.DateTime(), nullable=True), - sa.ForeignKeyConstraint(['user_id'], ['users.id'], ), - sa.PrimaryKeyConstraint('credential_id', 'user_id') - ) - # ### end Alembic commands ### + sa.ForeignKeyConstraint(['user_id'], ['users.id'], ), + sa.PrimaryKeyConstraint('id') + ) def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_table('webauthn_credential') - # ### end Alembic commands ### From e62e0509631a977b53bb22e56810a26579e9b0d1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 10 May 2021 22:09:07 +0100 Subject: [PATCH 3/6] add webauthn crud endpoints added some simple validation to the delete endpoint for sanity, but generally my assumption is that more validation will happen on the admin side. noteably im not checking whether the credentials are duplicated, nor is there a uniqueness constraint in the database - I'm not sure if the credential blob will always reliably be equivalent, and I believe the browser should hopefully take care of dupes. --- app/__init__.py | 4 + app/dao/webauthn_credential_dao.py | 39 +++++++ app/models.py | 11 ++ app/webauthn/__init__.py | 0 app/webauthn/rest.py | 66 +++++++++++ app/webauthn/webauthn_schema.py | 23 ++++ tests/app/db.py | 20 ++++ tests/app/webauthn/test_rest.py | 173 +++++++++++++++++++++++++++++ 8 files changed, 336 insertions(+) create mode 100644 app/dao/webauthn_credential_dao.py create mode 100644 app/webauthn/__init__.py create mode 100644 app/webauthn/rest.py create mode 100644 app/webauthn/webauthn_schema.py create mode 100644 tests/app/webauthn/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index 57cd1c2ef..c5a63bcf1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -178,6 +178,7 @@ def register_blueprint(application): ) from app.upload.rest import upload_blueprint from app.user.rest import user_blueprint + from app.webauthn.rest import webauthn_blueprint service_blueprint.before_request(requires_admin_auth) application.register_blueprint(service_blueprint, url_prefix='/service') @@ -185,6 +186,9 @@ def register_blueprint(application): user_blueprint.before_request(requires_admin_auth) application.register_blueprint(user_blueprint, url_prefix='/user') + webauthn_blueprint.before_request(requires_admin_auth) + application.register_blueprint(webauthn_blueprint) + template_blueprint.before_request(requires_admin_auth) application.register_blueprint(template_blueprint) diff --git a/app/dao/webauthn_credential_dao.py b/app/dao/webauthn_credential_dao.py new file mode 100644 index 000000000..36d79315e --- /dev/null +++ b/app/dao/webauthn_credential_dao.py @@ -0,0 +1,39 @@ +from app import db +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 + ).one() + + +@autocommit +def dao_create_webauthn_credential( + *, + user_id, + name, + credential_data, + registration_response, +): + webauthn_credential = WebauthnCredential( + user_id=user_id, + name=name, + credential_data=credential_data, + registration_response=registration_response + ) + db.session.add(webauthn_credential) + return webauthn_credential + + +@autocommit +def dao_update_webauthn_credential_name(webauthn_credential, new_name): + webauthn_credential.name = new_name + db.session.add(webauthn_credential) + return webauthn_credential + + +@autocommit +def dao_delete_webauthn_credential(webauthn_credential): + db.session.delete(webauthn_credential) diff --git a/app/models.py b/app/models.py index 9c471b5f2..23928451c 100644 --- a/app/models.py +++ b/app/models.py @@ -2615,3 +2615,14 @@ class WebauthnCredential(db.Model): created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + def serialize(self): + return { + 'id': str(self.id), + 'user_id': str(self.user_id), + 'name': self.name, + 'credential_data': self.credential_data, + 'registration_response': self.registration_response, + 'created_at': self.created_at.strftime(DATETIME_FORMAT), + 'updated_at': get_dt_string_or_none(self.updated_at), + } diff --git a/app/webauthn/__init__.py b/app/webauthn/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/webauthn/rest.py b/app/webauthn/rest.py new file mode 100644 index 000000000..0342a1523 --- /dev/null +++ b/app/webauthn/rest.py @@ -0,0 +1,66 @@ +from flask import Blueprint, jsonify, request + +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_update_webauthn_credential_name, +) +from app.errors import InvalidRequest, register_errors +from app.schema_validation import validate +from app.webauthn.webauthn_schema import ( + post_create_webauthn_credential_schema, + post_update_webauthn_credential_schema, +) + +webauthn_blueprint = Blueprint('webauthn', __name__, url_prefix='/user//webauthn') +register_errors(webauthn_blueprint) + + +@webauthn_blueprint.route('', methods=['GET']) +def get_webauthn_credentials(user_id): + user = get_user_by_id(user_id) + return jsonify(data=[cred.serialize() for cred in user.webauthn_credentials]), 200 + + +@webauthn_blueprint.route('', methods=['POST']) +def create_webauthn_credential(user_id): + data = request.get_json() + validate(data, post_create_webauthn_credential_schema) + webauthn_credential = dao_create_webauthn_credential( + user_id=user_id, + name=data['name'], + credential_data=data['credential_data'], + registration_response=data['registration_response'] + ) + + return jsonify(data=webauthn_credential.serialize()), 201 + + +@webauthn_blueprint.route('/', methods=['POST']) +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) + + dao_update_webauthn_credential_name(webauthn_credential, data['name']) + + return jsonify(data=webauthn_credential.serialize()), 200 + + +@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) + user = get_user_by_id(user_id) + + if webauthn_credential.user_id != user.id: + raise InvalidRequest('Webauthn credential does not belong to this user', status_code=400) + + if len(user.webauthn_credentials) == 1: + raise InvalidRequest('Cannot delete last remaining webauthn credential for user', status_code=400) + + dao_delete_webauthn_credential(webauthn_credential) + + return '', 204 diff --git a/app/webauthn/webauthn_schema.py b/app/webauthn/webauthn_schema.py new file mode 100644 index 000000000..cc2d2f05a --- /dev/null +++ b/app/webauthn/webauthn_schema.py @@ -0,0 +1,23 @@ +post_create_webauthn_credential_schema = { + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "POST webauthn_credential schema", + "type": "object", + "properties": { + "name": {"type": "string"}, + "credential_data": {"type": "string"}, + "registration_response": {"type": "string"}, + }, + "required": ["name", "credential_data", "registration_response"], + "additionalProperties": False +} + +post_update_webauthn_credential_schema = { + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "POST update webauthn_credential schema", + "type": "object", + "properties": { + "name": {"type": "string"}, + }, + "required": ["name"], + "additionalProperties": False +} diff --git a/tests/app/db.py b/tests/app/db.py index 2338a62ec..eed02e534 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -74,6 +74,7 @@ from app.models import ( Template, TemplateFolder, User, + WebauthnCredential, ) @@ -1202,3 +1203,22 @@ def create_broadcast_provider_message( db.session.add(provider_message_number) db.session.commit() return provider_message + + +def create_webauthn_credential( + user, + name='my key', + *, + credential_data='ABC123', + registration_response='DEF456', +): + webauthn_credential = WebauthnCredential( + user=user, + name=name, + credential_data=credential_data, + registration_response=registration_response + ) + + db.session.add(webauthn_credential) + db.session.commit() + return webauthn_credential diff --git a/tests/app/webauthn/test_rest.py b/tests/app/webauthn/test_rest.py new file mode 100644 index 000000000..8256ffb2d --- /dev/null +++ b/tests/app/webauthn/test_rest.py @@ -0,0 +1,173 @@ +import uuid +from unittest.mock import ANY + +from tests.app.db import create_user, create_webauthn_credential + + +def test_get_webauthn_credentials_returns_all_credentials_for_user(admin_request, notify_db_session): + me = create_user(email='a') + other = create_user(email='b') + first = create_webauthn_credential(me, '1') + create_webauthn_credential(me, '2') + create_webauthn_credential(other, '3') + + response = admin_request.get( + 'webauthn.get_webauthn_credentials', + user_id=me.id, + ) + + creds = sorted(response['data'], key=lambda x: x['name']) + assert len(creds) == 2 + + assert creds[0] == { + 'id': str(first.id), + 'user_id': str(me.id), + 'name': '1', + 'credential_data': 'ABC123', + 'registration_response': 'DEF456', + 'created_at': ANY, + 'updated_at': None + } + + assert creds[1]['name'] == '2' + + +def test_get_webauthn_credentials_returns_empty_list_if_no_creds(admin_request, sample_user): + response = admin_request.get('webauthn.get_webauthn_credentials', user_id=sample_user.id) + assert response == {'data': []} + + +def test_get_webauthn_credentials_errors_if_user_doesnt_exist(admin_request, sample_user): + create_webauthn_credential(sample_user, '1') + + admin_request.get( + 'webauthn.get_webauthn_credentials', + user_id=uuid.uuid4(), + _expected_status=404 + ) + + +def test_create_webauthn_credential_returns_201(admin_request, sample_user): + response = admin_request.post( + 'webauthn.create_webauthn_credential', + user_id=sample_user.id, + _data={ + 'name': 'my key', + 'credential_data': 'ABC123', + 'registration_response': 'DEF456', + }, + _expected_status=201 + ) + assert len(sample_user.webauthn_credentials) == 1 + + new_cred = sample_user.webauthn_credentials[0] + + assert new_cred.name == 'my key' + assert new_cred.credential_data == 'ABC123' + assert new_cred.registration_response == 'DEF456' + assert response['data']['id'] == str(new_cred.id) + + +def test_create_webauthn_credential_errors_if_schema_violation(admin_request, sample_user): + response = admin_request.post( + 'webauthn.create_webauthn_credential', + user_id=sample_user.id, + _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' + } + + +def test_update_webauthn_credential_returns_200(admin_request, sample_user): + cred = create_webauthn_credential(sample_user) + assert cred.name != 'new name' + + response = admin_request.post( + 'webauthn.update_webauthn_credential', + user_id=sample_user.id, + webauthn_credential_id=cred.id, + _data={ + 'name': 'new name', + }, + ) + assert response['data']['id'] == str(cred.id) + assert response['data']['name'] == 'new name' + + +def test_update_webauthn_credential_errors_if_schema_violation(admin_request, sample_user): + 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' + }, + _expected_status=400 + ) + assert response['errors'][0] == { + 'error': 'ValidationError', + 'message': 'Additional properties are not allowed (credential_data was unexpected)' + } + + +def test_update_webauthn_credential_errors_if_webauthn_credential_doesnt_exist(admin_request, sample_user): + admin_request.post( + 'webauthn.update_webauthn_credential', + user_id=sample_user.id, + webauthn_credential_id=uuid.uuid4(), + _data={ + 'name': 'my key', + }, + _expected_status=404 + ) + + +def test_delete_webauthn_credential_returns_204(admin_request, sample_user): + cred1 = create_webauthn_credential(sample_user) + cred2 = create_webauthn_credential(sample_user) + admin_request.delete( + 'webauthn.update_webauthn_credential', + user_id=sample_user.id, + webauthn_credential_id=cred1.id, + _expected_status=204 + ) + assert sample_user.webauthn_credentials == [cred2] + + +def test_delete_webauthn_credential_errors_if_last_key(admin_request, sample_user): + cred = create_webauthn_credential(sample_user) + response = admin_request.delete( + 'webauthn.delete_webauthn_credential', + user_id=sample_user.id, + webauthn_credential_id=cred.id, + _expected_status=400 + ) + assert response['message'] == 'Cannot delete last remaining webauthn credential for user' + + +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 + ) + + assert response['message'] == 'Webauthn credential does not belong to this user' From e6291187ba1beef9e7785a9097c9957cba368e3d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 11 May 2021 12:19:33 +0100 Subject: [PATCH 4/6] Remove registration_response from webauthn serialize - not needed in admin app Also fix tests: First add init file so the tests are found correctly, then update the tests after we stopped serialising webauthn registration_response. --- app/models.py | 1 - tests/app/webauthn/__init__.py | 0 tests/app/webauthn/test_rest.py | 1 - 3 files changed, 2 deletions(-) create mode 100644 tests/app/webauthn/__init__.py diff --git a/app/models.py b/app/models.py index 23928451c..1c36c3a01 100644 --- a/app/models.py +++ b/app/models.py @@ -2622,7 +2622,6 @@ class WebauthnCredential(db.Model): 'user_id': str(self.user_id), 'name': self.name, 'credential_data': self.credential_data, - 'registration_response': self.registration_response, 'created_at': self.created_at.strftime(DATETIME_FORMAT), 'updated_at': get_dt_string_or_none(self.updated_at), } diff --git a/tests/app/webauthn/__init__.py b/tests/app/webauthn/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/webauthn/test_rest.py b/tests/app/webauthn/test_rest.py index 8256ffb2d..0838e52e2 100644 --- a/tests/app/webauthn/test_rest.py +++ b/tests/app/webauthn/test_rest.py @@ -24,7 +24,6 @@ def test_get_webauthn_credentials_returns_all_credentials_for_user(admin_request 'user_id': str(me.id), 'name': '1', 'credential_data': 'ABC123', - 'registration_response': 'DEF456', 'created_at': ANY, 'updated_at': None } From d6fead7c0483b84ad9771cb76f74b58055023e66 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 11 May 2021 16:04:39 +0100 Subject: [PATCH 5/6] On update, check that webauthn credential belongs to user --- app/webauthn/rest.py | 12 ++++++++++-- tests/app/webauthn/test_rest.py | 21 +++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/app/webauthn/rest.py b/app/webauthn/rest.py index 0342a1523..6ce7bb6bf 100644 --- a/app/webauthn/rest.py +++ b/app/webauthn/rest.py @@ -45,6 +45,10 @@ def update_webauthn_credential(user_id, webauthn_credential_id): 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) + dao_update_webauthn_credential_name(webauthn_credential, data['name']) return jsonify(data=webauthn_credential.serialize()), 200 @@ -55,8 +59,7 @@ def delete_webauthn_credential(user_id, webauthn_credential_id): webauthn_credential = dao_get_webauthn_credential_by_id(webauthn_credential_id) user = get_user_by_id(user_id) - if webauthn_credential.user_id != user.id: - raise InvalidRequest('Webauthn credential does not belong to this user', status_code=400) + check_credential_belongs_to_user(webauthn_credential.user_id, user.id) if len(user.webauthn_credentials) == 1: raise InvalidRequest('Cannot delete last remaining webauthn credential for user', status_code=400) @@ -64,3 +67,8 @@ def delete_webauthn_credential(user_id, webauthn_credential_id): 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/tests/app/webauthn/test_rest.py b/tests/app/webauthn/test_rest.py index 0838e52e2..7c9404271 100644 --- a/tests/app/webauthn/test_rest.py +++ b/tests/app/webauthn/test_rest.py @@ -131,6 +131,27 @@ 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 + + response = admin_request.post( + 'webauthn.update_webauthn_credential', + user_id=user_1.id, + webauthn_credential_id=cred_2a.id, + _data={ + 'name': 'new key name', + }, + _expected_status=400 + ) + + assert response['message'] == 'Webauthn credential does not belong to this user' + + def test_delete_webauthn_credential_returns_204(admin_request, sample_user): cred1 = create_webauthn_credential(sample_user) cred2 = create_webauthn_credential(sample_user) From c190886bfed725e088a1e5d0457d31ad7ad1079a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 12 May 2021 15:34:37 +0100 Subject: [PATCH 6/6] 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'