From 3b19a727b852c24b97a010ecb57b4a421f87062b Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Fri, 14 Mar 2025 08:53:00 -0700 Subject: [PATCH] Change: better handle permissions in patch requests (#9393) --- src/documents/permissions.py | 100 ++++++++++---------- src/documents/serialisers.py | 30 +++--- src/documents/tests/test_api_permissions.py | 46 +++++++++ 3 files changed, 112 insertions(+), 64 deletions(-) diff --git a/src/documents/permissions.py b/src/documents/permissions.py index 4380c6994..c5de34607 100644 --- 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 c8a4a12c3..5a87092b8 100644 --- 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 637bd1fe0..692c22417 100644 --- 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"))