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

This commit is contained in:
shamoon 2025-03-14 08:53:00 -07:00 committed by GitHub
parent 7146a5f4fc
commit 3b19a727b8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 112 additions and 64 deletions

View File

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

View File

@ -160,24 +160,24 @@ class SetPermissionsMixin:
def validate_set_permissions(self, set_permissions=None): def validate_set_permissions(self, set_permissions=None):
permissions_dict = { permissions_dict = {
"view": { "view": {},
"users": User.objects.none(), "change": {},
"groups": Group.objects.none(),
},
"change": {
"users": User.objects.none(),
"groups": Group.objects.none(),
},
} }
if set_permissions is not None: if set_permissions is not None:
for action, _ in permissions_dict.items(): for action in ["view", "change"]:
if action in set_permissions: if action in set_permissions:
users = set_permissions[action]["users"] if "users" in set_permissions[action]:
permissions_dict[action]["users"] = self._validate_user_ids(users) users = set_permissions[action]["users"]
groups = set_permissions[action]["groups"] permissions_dict[action]["users"] = self._validate_user_ids(
permissions_dict[action]["groups"] = self._validate_group_ids( users,
groups, )
) 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 return permissions_dict
def _set_permissions(self, permissions, object): def _set_permissions(self, permissions, object):

View File

@ -395,6 +395,52 @@ class TestApiAuth(DirectoriesMixin, APITestCase):
self.assertTrue(checker.has_perm("view_document", doc)) self.assertTrue(checker.has_perm("view_document", doc))
self.assertIn("view_document", get_perms(group1, 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): def test_dynamic_permissions_fields(self):
user1 = User.objects.create_user(username="user1") user1 = User.objects.create_user(username="user1")
user1.user_permissions.add(*Permission.objects.filter(codename="view_document")) user1.user_permissions.add(*Permission.objects.filter(codename="view_document"))