Change: better handle permissions in patch requests (#9393)

diff --git a/src/documents/permissions.py b/src/documents/permissions.py
index 4380c6994a5e8e7c94c7ff2848b93459556de2f3..c5de34607f2b146ef979eb07d8472b6be230afb8 100644 (file)
--- a/src/documents/permissions.py
+++ b/src/documents/permissions.py
@@ -70,57 +70,59 @@ def set_permissions_for_object(permissions: list[str], object, *, merge: bool =
 
     for action in permissions:
         permission = f"{action}_{object.__class__.__name__.lower()}"
-        # users
-        users_to_add = User.objects.filter(id__in=permissions[action]["users"])
-        users_to_remove = (
-            get_users_with_perms(
-                object,
-                only_with_perms_in=[permission],
-                with_group_users=False,
+        if "users" in permissions[action]:
+            # users
+            users_to_add = User.objects.filter(id__in=permissions[action]["users"])
+            users_to_remove = (
+                get_users_with_perms(
+                    object,
+                    only_with_perms_in=[permission],
+                    with_group_users=False,
+                )
+                if not merge
+                else User.objects.none()
             )
-            if not merge
-            else User.objects.none()
-        )
-        if len(users_to_add) > 0 and len(users_to_remove) > 0:
-            users_to_remove = users_to_remove.exclude(id__in=users_to_add)
-        if len(users_to_remove) > 0:
-            for user in users_to_remove:
-                remove_perm(permission, user, object)
-        if len(users_to_add) > 0:
-            for user in users_to_add:
-                assign_perm(permission, user, object)
-                if action == "change":
-                    # change gives view too
-                    assign_perm(
-                        f"view_{object.__class__.__name__.lower()}",
-                        user,
-                        object,
-                    )
-        # groups
-        groups_to_add = Group.objects.filter(id__in=permissions[action]["groups"])
-        groups_to_remove = (
-            get_groups_with_only_permission(
-                object,
-                permission,
+            if len(users_to_add) > 0 and len(users_to_remove) > 0:
+                users_to_remove = users_to_remove.exclude(id__in=users_to_add)
+            if len(users_to_remove) > 0:
+                for user in users_to_remove:
+                    remove_perm(permission, user, object)
+            if len(users_to_add) > 0:
+                for user in users_to_add:
+                    assign_perm(permission, user, object)
+                    if action == "change":
+                        # change gives view too
+                        assign_perm(
+                            f"view_{object.__class__.__name__.lower()}",
+                            user,
+                            object,
+                        )
+        if "groups" in permissions[action]:
+            # groups
+            groups_to_add = Group.objects.filter(id__in=permissions[action]["groups"])
+            groups_to_remove = (
+                get_groups_with_only_permission(
+                    object,
+                    permission,
+                )
+                if not merge
+                else Group.objects.none()
             )
-            if not merge
-            else Group.objects.none()
-        )
-        if len(groups_to_add) > 0 and len(groups_to_remove) > 0:
-            groups_to_remove = groups_to_remove.exclude(id__in=groups_to_add)
-        if len(groups_to_remove) > 0:
-            for group in groups_to_remove:
-                remove_perm(permission, group, object)
-        if len(groups_to_add) > 0:
-            for group in groups_to_add:
-                assign_perm(permission, group, object)
-                if action == "change":
-                    # change gives view too
-                    assign_perm(
-                        f"view_{object.__class__.__name__.lower()}",
-                        group,
-                        object,
-                    )
+            if len(groups_to_add) > 0 and len(groups_to_remove) > 0:
+                groups_to_remove = groups_to_remove.exclude(id__in=groups_to_add)
+            if len(groups_to_remove) > 0:
+                for group in groups_to_remove:
+                    remove_perm(permission, group, object)
+            if len(groups_to_add) > 0:
+                for group in groups_to_add:
+                    assign_perm(permission, group, object)
+                    if action == "change":
+                        # change gives view too
+                        assign_perm(
+                            f"view_{object.__class__.__name__.lower()}",
+                            group,
+                            object,
+                        )
 
 
 def get_objects_for_user_owner_aware(user, perms, Model) -> QuerySet:

diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py
index c8a4a12c34aa2b3c6342a16f85d91258d78c0977..5a87092b8f2e651384f4c3a2bd81e2e5e1944af6 100644 (file)
--- a/src/documents/serialisers.py
+++ b/src/documents/serialisers.py
@@ -160,24 +160,24 @@ class SetPermissionsMixin:
 
     def validate_set_permissions(self, set_permissions=None):
         permissions_dict = {
-            "view": {
-                "users": User.objects.none(),
-                "groups": Group.objects.none(),
-            },
-            "change": {
-                "users": User.objects.none(),
-                "groups": Group.objects.none(),
-            },
+            "view": {},
+            "change": {},
         }
         if set_permissions is not None:
-            for action, _ in permissions_dict.items():
+            for action in ["view", "change"]:
                 if action in set_permissions:
-                    users = set_permissions[action]["users"]
-                    permissions_dict[action]["users"] = self._validate_user_ids(users)
-                    groups = set_permissions[action]["groups"]
-                    permissions_dict[action]["groups"] = self._validate_group_ids(
-                        groups,
-                    )
+                    if "users" in set_permissions[action]:
+                        users = set_permissions[action]["users"]
+                        permissions_dict[action]["users"] = self._validate_user_ids(
+                            users,
+                        )
+                    if "groups" in set_permissions[action]:
+                        groups = set_permissions[action]["groups"]
+                        permissions_dict[action]["groups"] = self._validate_group_ids(
+                            groups,
+                        )
+                else:
+                    del permissions_dict[action]
         return permissions_dict
 
     def _set_permissions(self, permissions, object):

diff --git a/src/documents/tests/test_api_permissions.py b/src/documents/tests/test_api_permissions.py
index 637bd1fe049b2593bcc79e31deaf647340d602f3..692c2241754178c4fa98b627d11e450f48ea94f8 100644 (file)
--- a/src/documents/tests/test_api_permissions.py
+++ b/src/documents/tests/test_api_permissions.py
@@ -395,6 +395,52 @@ class TestApiAuth(DirectoriesMixin, APITestCase):
         self.assertTrue(checker.has_perm("view_document", doc))
         self.assertIn("view_document", get_perms(group1, doc))
 
+    def test_patch_doesnt_remove_permissions(self):
+        """
+        GIVEN:
+            - existing document with permissions set
+        WHEN:
+            - PATCH API request to update doc that is not json
+        THEN:
+            - Object permissions are not removed
+        """
+        doc = Document.objects.create(
+            title="test",
+            mime_type="application/pdf",
+            content="this is a document",
+        )
+        user1 = User.objects.create_superuser(username="user1")
+        user2 = User.objects.create(username="user2")
+        group1 = Group.objects.create(name="group1")
+        doc.owner = user1
+        doc.save()
+
+        assign_perm("view_document", user2, doc)
+        assign_perm("change_document", user2, doc)
+        assign_perm("view_document", group1, doc)
+        assign_perm("change_document", group1, doc)
+
+        self.client.force_authenticate(user1)
+
+        response = self.client.patch(
+            f"/api/documents/{doc.id}/",
+            {
+                "archive_serial_number": "123",
+            },
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        doc = Document.objects.get(pk=doc.id)
+
+        self.assertEqual(doc.owner, user1)
+        from guardian.core import ObjectPermissionChecker
+
+        checker = ObjectPermissionChecker(user2)
+        self.assertTrue(checker.has_perm("view_document", doc))
+        self.assertIn("view_document", get_perms(group1, doc))
+        self.assertTrue(checker.has_perm("change_document", doc))
+        self.assertIn("change_document", get_perms(group1, doc))
+
     def test_dynamic_permissions_fields(self):
         user1 = User.objects.create_user(username="user1")
         user1.user_permissions.add(*Permission.objects.filter(codename="view_document"))