From 1d7a94fe7da01c13da026568f6a8568d4baf93f8 Mon Sep 17 00:00:00 2001 From: Andreas Hasenkopf Date: Fri, 25 Nov 2022 12:10:58 +0100 Subject: [PATCH] Bugfix for permission management of entities (#106) A small bug allowed authenticated users to perform any actions on entities without permission, when the `reviewable` flag of the schema was unset. On the flip side, it prevented them from creating change requests for entities they had no permissions on. This change fixes the situation: * Any authenticated user is allowed to create change requests * Only authorized users are allowed to apply changes to entities Co-authored-by: Gabriel Niebler --- backend/dynamic_routes.py | 6 +-- backend/tests/conftest.py | 10 +++- backend/tests/test_auth_api.py | 83 ++++++++++++++++++++++++++++++---- 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/backend/dynamic_routes.py b/backend/dynamic_routes.py index 4d75efc..cc94db4 100644 --- a/backend/dynamic_routes.py +++ b/backend/dynamic_routes.py @@ -166,7 +166,7 @@ def get_entities( def route_create_entity(router: APIRouter, schema: Schema): req_permission = authenticated_user - if schema.reviewable: + if not schema.reviewable: req_permission = authorized_user(RequirePermission( permission=PermissionType.CREATE_ENTITY, target=Schema() @@ -229,7 +229,7 @@ def create_entity(data: factory(schema=schema, variant=ModelVariant.CREATE), res def route_update_entity(router: APIRouter, schema: Schema): req_permission = authenticated_user - if schema.reviewable: + if not schema.reviewable: req_permission = authorized_user(RequirePermission( permission=PermissionType.UPDATE_ENTITY, target=Entity() @@ -300,7 +300,7 @@ def update_entity(id_or_slug: Union[int, str], def route_delete_entity(router: APIRouter, schema: Schema): req_permission = authenticated_user - if schema.reviewable: + if not schema.reviewable: req_permission = authorized_user(RequirePermission( permission=PermissionType.DELETE_ENTITY, target=Entity() diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 3c1fee1..27c65d9 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -11,7 +11,7 @@ from ..auth import authenticated_user, authorized_user from ..auth.crud import get_or_create_user, get_user, grant_permission from ..auth.enum import RecipientType, PermissionType -from ..auth.models import User +from ..auth.models import User, Permission from ..database import get_db from ..models import * from ..schemas.auth import UserCreateSchema, PermissionSchema @@ -353,6 +353,14 @@ def delete(self, url, *, params=None, headers=None, cookies=None, auth=USE_CLIEN ) +@pytest.fixture +def unauthorized_testuser(dbsession, testuser) -> User: + # The testuser is a superuser. Remove permissions. + dbsession.query(Permission).filter(Permission.recipient_type == RecipientType.USER, + Permission.recipient_id == testuser.id).delete() + return testuser + + @pytest.fixture def client(dbsession): app = create_app(session=dbsession) diff --git a/backend/tests/test_auth_api.py b/backend/tests/test_auth_api.py index a88333c..8b1c3dd 100644 --- a/backend/tests/test_auth_api.py +++ b/backend/tests/test_auth_api.py @@ -12,6 +12,8 @@ from ..models import Schema, Entity from ..schemas import BaseGroupSchema, GroupSchema, UserCreateSchema, PermissionSchema, \ RequirePermission +from ..traceability.enum import ReviewResult, ChangeStatus +from ..traceability.models import ChangeRequest from .conftest import TEST_USER from .mixins import CreateMixin @@ -373,9 +375,9 @@ class TestRoutesRequiringAuth: ('delete', '/users/tester'), ('post', '/permissions'), ('delete', '/permissions'), - ('post', '/entity/unperson'), - ('put', '/entity/unperson/foo'), - ('delete', '/entity/unperson/foo'), + ('post', '/entity/person'), + ('put', '/entity/person/foo'), + ('delete', '/entity/person/foo'), ) @pytest.mark.parametrize("method, url", authenticated + authorized) @@ -385,12 +387,75 @@ def test_require_authentication(self, method, url, client: TestClient): assert response.status_code == 401 @pytest.mark.parametrize("method, url", authorized) - def test_require_authorization(self, method, url, dbsession: Session, testuser: User, - authenticated_client: TestClient): - # The testuser is a superuser. Remove permissions first. - dbsession.query(Permission).filter(Permission.recipient_type == RecipientType.USER, - Permission.recipient_id == testuser.id).delete() - + def test_require_authorization(self, method, url, dbsession: Session, + unauthorized_testuser: User, authenticated_client: TestClient): func = getattr(authenticated_client, method) response = func(url) assert response.status_code == 403 + + def test_approve_request(self, dbsession: Session, unauthorized_testuser: User, + authenticated_client: TestClient): + """ + Test that an authenticated but unauthorized user is not allowed to approve a change request. + """ + change_request_ids = dbsession.query(ChangeRequest.id)\ + .where(ChangeRequest.status == ChangeStatus.PENDING)\ + .first() + assert len(change_request_ids) == 1 + + response = authenticated_client.post(f"/changes/review/{change_request_ids[0]}", + json={"result": ReviewResult.APPROVE.value, + "comment": "Foo"}) + assert response.status_code == 403 + + def test_review_change_request_breakout__create(self, dbsession: Session, + unauthorized_testuser: User, + authenticated_client: TestClient): + """ + Test that authenticated user is allowed to create a change request without actually creating + an entity + """ + response = authenticated_client.post("/entity/unperson", + json={"name": "Foo", "slug": "foo"}) + assert response.status_code == 202 + q = dbsession.query(Entity)\ + .join(Schema)\ + .filter(Schema.slug == "unperson", Entity.slug == "foo")\ + .exists() + assert not dbsession.query(Entity.id).filter(q).scalar() + + def test_review_change_request_breakout__update(self, dbsession: Session, + unauthorized_testuser: User, + authenticated_client: TestClient): + """ + Test that authenticated user is allowed to update a change request without actually changing + an entity + """ + schema = dbsession.query(Schema).where(Schema.slug == "unperson").one() + entity = Entity(name="Foo", slug="foo", schema_id=schema.id) + dbsession.add(entity) + dbsession.commit() + response = authenticated_client.put("/entity/unperson/foo", + json={"name": "Føø", "slug": "foo"}) + + assert response.status_code == 202 + dbsession.refresh(entity) + assert entity.name == "Foo" + + def test_review_change_request_breakout__delete(self, dbsession: Session, + unauthorized_testuser: User, + authenticated_client: TestClient): + """ + Test that authenticated user is allowed to delete a change request without actually deleting + an entity + """ + schema = dbsession.query(Schema).where(Schema.slug == "unperson").one() + entity = Entity(name="Foo", slug="foo", schema_id=schema.id) + dbsession.add(entity) + dbsession.commit() + response = authenticated_client.delete("/entity/unperson/foo") + + assert response.status_code == 202 + dbsession.refresh(entity) + assert entity.name == "Foo" + assert entity.deleted is False