From ef26bc1570a27bc6bd561215293156fb99913a3e Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:46:32 -0700 Subject: [PATCH] Fix: enforce set_permissions shape via a nested DRF serializer (#14119) SetPermissionsSerializer was a bare DictField, and the legacy bulk_edit set_permissions parameter bypassed even that by hand-calling validate_set_permissions() on an unchecked dict. A bool or a non-list in place of users/groups crashed with a raw TypeError instead of a 400. It is now a nested serializer (view/change, each with users/groups as lists of integers), used for owned-object create/update, the legacy bulk_edit set_permissions parameter, and bulk_edit_objects permissions. Unknown action keys are rejected: previously a typo like "veiw" was silently dropped, leaving an empty permission set that could clear existing grants. An explicit set_permissions null (an owner-only change) remains a no-op, and an empty bulk_edit_objects permissions dict is still rejected. --- src/documents/permissions.py | 18 ++-- src/documents/serialisers.py | 91 +++++++++-------- src/documents/tests/test_api_bulk_edit.py | 73 +++++++++++++- src/documents/tests/test_api_permissions.py | 104 ++++++++++++++++++++ src/documents/tests/test_bulk_edit.py | 11 +-- 5 files changed, 230 insertions(+), 67 deletions(-) diff --git a/src/documents/permissions.py b/src/documents/permissions.py index b7a1d7e0e..70c48b6cc 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -177,13 +177,10 @@ def _resolve_permissions(codenames: set[str], ctype: ContentType) -> list[Permis """ Resolves `codenames` to Permission rows, raising like the single-object assign_perm() this bulk path replaces does (via a `.get()` internally) - if any codename doesn't exist -- e.g. a client-supplied action name that - was never validated (BulkEditObjectsSerializer._validate_permissions - calls validate_set_permissions() only for its side-effecting id checks - and discards the filtered dict it returns, so an unrecognized action key - reaches this function as-is). A plain `.filter()` with no existence - check would otherwise silently build zero rows and no-op instead of - reporting the bad input. + if any codename doesn't exist. SetPermissionsSerializer rejects unknown + action names at the API, but a caller passing one directly would + otherwise get a plain `.filter()` that silently builds zero rows and + no-ops instead of reporting the bad input. """ permission_objs = list( Permission.objects.filter(content_type=ctype, codename__in=codenames), @@ -298,10 +295,9 @@ def set_permissions_for_objects( # Every action is resolved up front, before anything is written, so an # unrecognized action name (see _resolve_permissions) aborts the whole - # call instead of leaving the actions ahead of it already applied -- - # BulkEditObjectsSerializer lets unknown keys through and its view turns - # the exception into a 400, so a half-applied change would otherwise be - # reported to the client as a failure. + # call instead of leaving the actions ahead of it already applied. + # SetPermissionsSerializer rejects unknown actions at the API, so this + # guards any other caller. permissions_by_action: dict[str, list[Permission]] = {} for action, entry in permissions.items(): if "users" not in entry and "groups" not in entry: diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index c13cf23d8..908ab66e9 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -210,6 +210,9 @@ class MatchingModelSerializer(serializers.ModelSerializer[Any]): return match +PERMISSION_ACTIONS = ("view", "change") + + class SetPermissionsMixin: def _validate_user_ids(self, user_ids): users = User.objects.none() @@ -232,12 +235,9 @@ class SetPermissionsMixin: return groups def validate_set_permissions(self, set_permissions=None): - permissions_dict = { - "view": {}, - "change": {}, - } + permissions_dict = {action: {} for action in PERMISSION_ACTIONS} if set_permissions is not None: - for action in ["view", "change"]: + for action in PERMISSION_ACTIONS: if action in set_permissions: if "users" in set_permissions[action]: users = set_permissions[action]["users"] @@ -265,41 +265,31 @@ class SerializerWithPerms(serializers.Serializer[dict[str, Any]]): super().__init__(*args, **kwargs) -@extend_schema_field( - field={ - "type": "object", - "properties": { - "view": { - "type": "object", - "properties": { - "users": { - "type": "array", - "items": {"type": "integer"}, - }, - "groups": { - "type": "array", - "items": {"type": "integer"}, - }, - }, - }, - "change": { - "type": "object", - "properties": { - "users": { - "type": "array", - "items": {"type": "integer"}, - }, - "groups": { - "type": "array", - "items": {"type": "integer"}, - }, - }, - }, - }, - }, -) -class SetPermissionsSerializer(serializers.DictField): - pass +class PermissionSetSerializer(serializers.Serializer[dict[str, Any]]): + users = serializers.ListField( + child=serializers.IntegerField(), + required=False, + allow_null=True, + ) + groups = serializers.ListField( + child=serializers.IntegerField(), + required=False, + allow_null=True, + ) + + +class SetPermissionsSerializer(serializers.Serializer[dict[str, Any]]): + view = PermissionSetSerializer(required=False) + change = PermissionSetSerializer(required=False) + + def to_internal_value(self, data): + if isinstance(data, dict): + unknown_keys = set(data) - set(PERMISSION_ACTIONS) + if unknown_keys: + raise serializers.ValidationError( + {key: "Unknown permission action." for key in sorted(unknown_keys)}, + ) + return super().to_internal_value(data) class OwnedObjectSerializer( @@ -470,7 +460,6 @@ class OwnedObjectSerializer( set_permissions = SetPermissionsSerializer( label="Set permissions", - allow_empty=True, required=False, write_only=True, ) @@ -2051,8 +2040,13 @@ class BulkEditSerializer( def _validate_parameters_set_permissions(self, parameters) -> None: if "set_permissions" not in parameters: raise serializers.ValidationError("set_permissions not specified") + set_permissions = parameters["set_permissions"] + if set_permissions is not None: + set_permissions = SetPermissionsSerializer().run_validation( + set_permissions, + ) parameters["set_permissions"] = self.validate_set_permissions( - parameters["set_permissions"], + set_permissions, ) if "owner" in parameters and parameters["owner"] is not None: self._validate_owner(parameters["owner"]) @@ -3011,9 +3005,8 @@ class BulkEditObjectsSerializer(SerializerWithPerms, SetPermissionsMixin): allow_null=True, ) - permissions = serializers.DictField( + permissions = SetPermissionsSerializer( label="Set permissions", - allow_empty=False, required=False, write_only=True, ) @@ -3049,8 +3042,8 @@ class BulkEditObjectsSerializer(SerializerWithPerms, SetPermissionsMixin): ) return objects - def _validate_permissions(self, permissions) -> None: - self.validate_set_permissions( + def _validate_permissions(self, permissions) -> dict: + return self.validate_set_permissions( permissions, ) @@ -3074,7 +3067,11 @@ class BulkEditObjectsSerializer(SerializerWithPerms, SetPermissionsMixin): if operation == "set_permissions": permissions = attrs.get("permissions") if permissions is not None: - self._validate_permissions(permissions) + if not permissions: + raise serializers.ValidationError( + "permissions must not be empty", + ) + attrs["permissions"] = self._validate_permissions(permissions) return attrs diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index cdced37c6..dafbf4365 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/src/documents/tests/test_api_bulk_edit.py @@ -1165,6 +1165,77 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertIn(b"set_permissions not specified", response.content) m.assert_not_called() + @mock.patch("documents.serialisers.bulk_edit.set_permissions") + def test_set_permissions_rejects_malformed_set_permissions(self, m) -> None: + """ + GIVEN: + - A set_permissions bulk edit where set_permissions is not an + object, has a non-list users value, or has an unknown action + WHEN: + - API to bulk edit is called + THEN: + - API returns HTTP 400 describing the problem + - set_permissions is not called + """ + self.setup_mock(m, "set_permissions") + + for set_permissions, expected_message in ( + (False, b"Expected a dictionary"), + ({"view": {"users": False}}, b"Expected a list"), + ({"not_a_real_action": {"users": [1]}}, b"Unknown permission action"), + ): + with self.subTest(set_permissions=set_permissions): + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "documents": [self.doc2.id], + "method": "set_permissions", + "parameters": {"set_permissions": set_permissions}, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(expected_message, response.content) + m.assert_not_called() + + @mock.patch("documents.serialisers.bulk_edit.set_permissions") + def test_set_permissions_null_is_a_noop(self, m) -> None: + """ + GIVEN: + - A set_permissions bulk edit with set_permissions null and an + owner, i.e. a request that only changes the owner + WHEN: + - API to bulk edit is called + THEN: + - Request succeeds + - set_permissions receives no users or groups for any action + """ + self.setup_mock(m, "set_permissions") + + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "documents": [self.doc2.id], + "method": "set_permissions", + "parameters": { + "set_permissions": None, + "owner": self.user.id, + }, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + m.assert_called_once() + self.assertEqual( + m.call_args.kwargs["set_permissions"], + {"view": {}, "change": {}}, + ) + @mock.patch("documents.serialisers.bulk_edit.set_permissions") def test_set_permissions_merge(self, m) -> None: self.setup_mock(m, "set_permissions") @@ -1300,7 +1371,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.client.force_authenticate(user=user1) permissions = { - "owner": user1.id, + "view": {"users": [user1.id]}, } response = self.client.post( diff --git a/src/documents/tests/test_api_permissions.py b/src/documents/tests/test_api_permissions.py index 29eb8f496..0b2c491a4 100644 --- a/src/documents/tests/test_api_permissions.py +++ b/src/documents/tests/test_api_permissions.py @@ -1499,6 +1499,110 @@ class TestBulkEditObjectPermissions(APITestCase): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) self.assertTrue(Tag.objects.filter(pk=self.t1.id).exists()) + def test_bulk_object_set_permissions_rejects_empty_permissions(self) -> None: + """ + GIVEN: + - Existing objects + WHEN: + - bulk_edit_objects API endpoint is called with set_permissions + operation and an empty permissions dict + THEN: + - Validation fails rather than silently applying a no-op + """ + response = self.client.post( + "/api/bulk_edit_objects/", + json.dumps( + { + "objects": [self.t1.id], + "object_type": "tags", + "operation": "set_permissions", + "permissions": {}, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_bulk_object_set_permissions_rejects_non_dict_permissions(self) -> None: + """ + GIVEN: + - Existing objects + WHEN: + - bulk_edit_objects API endpoint is called with set_permissions + operation and a non-dict permissions value + THEN: + - Validation fails rather than crashing + """ + response = self.client.post( + "/api/bulk_edit_objects/", + json.dumps( + { + "objects": [self.t1.id], + "object_type": "tags", + "operation": "set_permissions", + "permissions": False, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_bulk_object_set_permissions_rejects_unknown_action(self) -> None: + """ + GIVEN: + - Existing objects + WHEN: + - bulk_edit_objects API endpoint is called with set_permissions + operation and an unrecognized permission action name + THEN: + - Validation fails rather than silently no-oping + """ + response = self.client.post( + "/api/bulk_edit_objects/", + json.dumps( + { + "objects": [self.t1.id], + "object_type": "tags", + "operation": "set_permissions", + "permissions": {"not_a_real_action": {"users": [self.user1.id]}}, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_bulk_object_set_permissions_null_users_clears_users(self) -> None: + """ + GIVEN: + - An object a user has view permission on + WHEN: + - bulk_edit_objects API endpoint is called with set_permissions + operation, merge off, and an explicit null for the view users + THEN: + - Request succeeds and null is treated as an empty user list, + so the existing view permission is removed + """ + assign_perm("view_tag", self.user1, self.t1) + + response = self.client.post( + "/api/bulk_edit_objects/", + json.dumps( + { + "objects": [self.t1.id], + "object_type": "tags", + "operation": "set_permissions", + "permissions": {"view": {"users": None}}, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertNotIn(self.user1, get_users_with_perms(self.t1)) + def test_bulk_edit_object_permissions_validation(self) -> None: """ GIVEN: diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index f9d4a5028..bfd134516 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -636,14 +636,9 @@ class TestBulkEdit(DirectoriesMixin, TestCase): THEN: - Permission.DoesNotExist is raised, not a silent no-op - Regression test: the endpoint that calls this - (BulkEditObjectPermissionsView) never actually validates action - names against the raw client-supplied permissions dict -- - BulkEditObjectsSerializer._validate_permissions calls - validate_set_permissions() only for its side-effecting user/group id - checks and discards the filtered dict it returns -- so a bogus - action key reaches this function as-is. Resolving the Permission via - a bare `.filter()` (which returns empty instead of raising) would + The API rejects unknown action names before they get here, but a + direct caller could still pass one. Resolving the Permission via a + bare `.filter()` (which returns empty instead of raising) would silently drop the grant and report success. """ with self.assertRaises(Permission.DoesNotExist):