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 <[email protected]>

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()

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)

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