From 1dbb24065d9d8f31444b37fb39213ce637c77f49 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 7 Nov 2018 13:38:09 +0000 Subject: [PATCH 1/2] Ensure that new template folder belongs to the same service as parent Since template folders are only linked by ID to their parent we need to check that the parent folder belongs to the same service as the one being created. Otherwise, admin users could modify parent ID to create a folder outside their service. Ideally, this check would be performed by a DB constraint, but since parent_id can be nullable this is only possible to express using DB triggers. Instead, we perform the check in the API endpoint code. --- app/template_folder/rest.py | 12 +++++++++++- .../test_template_folder_rest.py | 18 +++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/template_folder/rest.py b/app/template_folder/rest.py index e4b2779d5..2499ea933 100644 --- a/app/template_folder/rest.py +++ b/app/template_folder/rest.py @@ -1,5 +1,6 @@ from flask import Blueprint, jsonify, request from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm.exc import NoResultFound from app.dao.template_folder_dao import ( dao_create_template_folder, @@ -8,7 +9,7 @@ from app.dao.template_folder_dao import ( dao_delete_template_folder ) from app.dao.services_dao import dao_fetch_service_by_id -from app.errors import register_errors +from app.errors import InvalidRequest, register_errors from app.models import TemplateFolder from app.template_folder.template_folder_schema import ( post_create_template_folder_schema, @@ -46,6 +47,15 @@ def create_template_folder(service_id): validate(data, post_create_template_folder_schema) + if data.get('parent_id') is not None: + try: + parent_folder = dao_get_template_folder_by_id(data['parent_id']) + except NoResultFound: + raise InvalidRequest("parent_id not found", status_code=400) + + if parent_folder.service_id != service_id: + raise InvalidRequest("parent_id belongs to a different service", status_code=400) + template_folder = TemplateFolder( service_id=service_id, name=data['name'].strip(), diff --git a/tests/app/template_folder/test_template_folder_rest.py b/tests/app/template_folder/test_template_folder_rest.py index 726b7d135..b3dcaf431 100644 --- a/tests/app/template_folder/test_template_folder_rest.py +++ b/tests/app/template_folder/test_template_folder_rest.py @@ -74,9 +74,6 @@ def test_create_template_folder_fails_if_missing_fields(admin_request, sample_se def test_create_template_folder_fails_if_unknown_parent_id(admin_request, sample_service): - # create existing folder - create_template_folder(sample_service) - resp = admin_request.post( 'template_folder.create_template_folder', service_id=sample_service.id, @@ -88,6 +85,21 @@ def test_create_template_folder_fails_if_unknown_parent_id(admin_request, sample assert resp['message'] == 'parent_id not found' +def test_create_template_folder_fails_if_parent_id_from_different_service(admin_request, sample_service): + s1 = create_service(service_name='a') + parent_folder_id = create_template_folder(s1).id + + resp = admin_request.post( + 'template_folder.create_template_folder', + service_id=sample_service.id, + _data={'name': 'bar', 'parent_id': str(parent_folder_id)}, + _expected_status=400 + ) + + assert resp['result'] == 'error' + assert resp['message'] == 'parent_id belongs to a different service' + + def test_rename_template_folder(admin_request, sample_service): existing_folder = create_template_folder(sample_service) From 36f41c23e16d4d71e8fbf0dc21774fcdc17266e6 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 7 Nov 2018 13:48:18 +0000 Subject: [PATCH 2/2] Always use both folder and service ID when getting template folder Currently there aren't any permission checks based on folder IDs in the admin app or the API, so it's possible for a user to modify the folder ID to perform operations on folders outside their service. Our usual way to avoid this is to always use service_id filter when fetching objects from the database. --- app/dao/template_folder_dao.py | 7 +++++-- app/template_folder/rest.py | 11 ++++------- .../app/template_folder/test_template_folder_rest.py | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/dao/template_folder_dao.py b/app/dao/template_folder_dao.py index 7df47c6ff..a162f79c0 100644 --- a/app/dao/template_folder_dao.py +++ b/app/dao/template_folder_dao.py @@ -3,8 +3,11 @@ from app.dao.dao_utils import transactional from app.models import TemplateFolder -def dao_get_template_folder_by_id(template_folder_id): - return TemplateFolder.query.filter(TemplateFolder.id == template_folder_id).one() +def dao_get_template_folder_by_id_and_service_id(template_folder_id, service_id): + return TemplateFolder.query.filter( + TemplateFolder.id == template_folder_id, + TemplateFolder.service_id == service_id + ).one() @transactional diff --git a/app/template_folder/rest.py b/app/template_folder/rest.py index 2499ea933..9105ed621 100644 --- a/app/template_folder/rest.py +++ b/app/template_folder/rest.py @@ -4,7 +4,7 @@ from sqlalchemy.orm.exc import NoResultFound from app.dao.template_folder_dao import ( dao_create_template_folder, - dao_get_template_folder_by_id, + dao_get_template_folder_by_id_and_service_id, dao_update_template_folder, dao_delete_template_folder ) @@ -49,13 +49,10 @@ def create_template_folder(service_id): if data.get('parent_id') is not None: try: - parent_folder = dao_get_template_folder_by_id(data['parent_id']) + dao_get_template_folder_by_id_and_service_id(data['parent_id'], service_id) except NoResultFound: raise InvalidRequest("parent_id not found", status_code=400) - if parent_folder.service_id != service_id: - raise InvalidRequest("parent_id belongs to a different service", status_code=400) - template_folder = TemplateFolder( service_id=service_id, name=data['name'].strip(), @@ -73,7 +70,7 @@ def rename_template_folder(service_id, template_folder_id): validate(data, post_rename_template_folder_schema) - template_folder = dao_get_template_folder_by_id(template_folder_id) + template_folder = dao_get_template_folder_by_id_and_service_id(template_folder_id, service_id) template_folder.name = data['name'] dao_update_template_folder(template_folder) @@ -83,7 +80,7 @@ def rename_template_folder(service_id, template_folder_id): @template_folder_blueprint.route('/', methods=['DELETE']) def delete_template_folder(service_id, template_folder_id): - template_folder = dao_get_template_folder_by_id(template_folder_id) + template_folder = dao_get_template_folder_by_id_and_service_id(template_folder_id, service_id) # don't allow deleting if there's anything in the folder (even if it's just more empty subfolders) if template_folder.subfolders or template_folder.templates: diff --git a/tests/app/template_folder/test_template_folder_rest.py b/tests/app/template_folder/test_template_folder_rest.py index b3dcaf431..a85ea8664 100644 --- a/tests/app/template_folder/test_template_folder_rest.py +++ b/tests/app/template_folder/test_template_folder_rest.py @@ -97,7 +97,7 @@ def test_create_template_folder_fails_if_parent_id_from_different_service(admin_ ) assert resp['result'] == 'error' - assert resp['message'] == 'parent_id belongs to a different service' + assert resp['message'] == 'parent_id not found' def test_rename_template_folder(admin_request, sample_service):