mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-08-08 11:53:19 +00:00
Fix: address final review findings for permission-filter unification
- Add a permanent regression test pinning TrashView's include_granted=False wiring: an explicit view_document grant on a trashed document must not leak it into /api/trash/ for a non-owner, non-superuser requester. - Drop the now-dead direct dependency djangorestframework-guardian; the last rest_framework_guardian import was removed by this branch's migration onto PermittedObjectsFilter. django-guardian is untouched. - Replace the hand-maintained, already-stale caller lists in get_objects_for_user_owner_aware/has_perms_owner_aware docstrings with a pointer to grep for remaining callers instead. - In PermittedObjectsFilter.filter_queryset, compute `model` only on the include_granted=True path that actually uses it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UFyrt7FWbRRdTUAcdqBcsc
This commit is contained in:
committed by
Trenton H
co-authored by
Claude Sonnet 5
parent
a98b5843cc
commit
3e12425196
@@ -38,7 +38,6 @@ dependencies = [
|
||||
"django-soft-delete~=1.0.18",
|
||||
"django-treenode>=0.24",
|
||||
"djangorestframework~=3.16",
|
||||
"djangorestframework-guardian~=0.4.0",
|
||||
"drf-spectacular~=0.30",
|
||||
"drf-spectacular-sidecar~=2026.7.1",
|
||||
"drf-writable-nested~=0.7.1",
|
||||
|
||||
@@ -1049,9 +1049,9 @@ class PermittedObjectsFilter(BaseFilterBackend):
|
||||
def filter_queryset(self, request, queryset, view):
|
||||
if request.user.is_superuser:
|
||||
return queryset
|
||||
model = queryset.model
|
||||
if not self.include_granted:
|
||||
return queryset.filter(Q(owner=request.user) | Q(owner__isnull=True))
|
||||
model = queryset.model
|
||||
perm = self.perm_codename or f"view_{model._meta.model_name}"
|
||||
return queryset.filter(
|
||||
id__in=permitted_object_ids(request.user, model, perm),
|
||||
|
||||
@@ -363,10 +363,9 @@ def get_objects_for_user_owner_aware(
|
||||
Legacy slow path (guardian-backed, O(n) style permission resolution).
|
||||
The Stage 4 unification migrated most call sites onto
|
||||
``PermittedObjectsFilter``/``permitted_object_ids()``, but this function
|
||||
is kept because production callers still remain, e.g.
|
||||
``documents/views.py`` (several call sites), ``documents/signals/handlers.py``,
|
||||
``paperless_ai/matching.py``, and ``paperless_ai/ai_classifier.py``.
|
||||
Do not remove until those call sites are migrated in a future task.
|
||||
is kept because production callers still remain. Several callers remain
|
||||
across ``documents/``, ``paperless_mail/``, and ``paperless_ai/`` --
|
||||
grep for this function name before removing it.
|
||||
"""
|
||||
manager = (
|
||||
Model.global_objects
|
||||
@@ -391,9 +390,9 @@ def has_perms_owner_aware(user, perms, obj):
|
||||
|
||||
Stage 4's ``PermittedObjectsFilter``/``permitted_object_ids()`` covers
|
||||
queryset-level filtering, but this single-object check still has many
|
||||
production callers, notably ``documents/views.py`` and
|
||||
``documents/serialisers.py``. Kept only for those remaining callers;
|
||||
do not remove until they are migrated in a future task.
|
||||
production callers. Several callers remain across ``documents/``,
|
||||
``paperless_mail/``, and ``paperless_ai/`` -- grep for this function
|
||||
name before removing it.
|
||||
"""
|
||||
checker = ObjectPermissionChecker(user)
|
||||
return obj.owner is None or obj.owner == user or checker.has_perm(perms, obj)
|
||||
|
||||
@@ -446,6 +446,33 @@ class TestTrashRestorePermissionBoundary:
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
class TestTrashViewExcludesExplicitlyGrantedDocuments:
|
||||
"""
|
||||
Regression test pinning TrashView's use of
|
||||
``_TrashPermittedObjectsFilter`` (``include_granted = False``). If that
|
||||
flag were ever flipped to the default ``True``, or the subclass removed
|
||||
in favor of the base ``PermittedObjectsFilter``, a trashed document
|
||||
would leak into ``/api/trash/`` results for any user holding an
|
||||
explicit guardian grant on it, even though they are neither the owner
|
||||
nor a superuser.
|
||||
"""
|
||||
|
||||
def test_explicit_grant_does_not_leak_trashed_document(self, rest_api_client):
|
||||
owner = User.objects.create_user(username="trash_owner")
|
||||
grantee = User.objects.create_user(username="trash_grantee")
|
||||
doc = DocumentFactory(owner=owner)
|
||||
doc.delete() # soft delete
|
||||
assign_perm("view_document", grantee, doc)
|
||||
|
||||
rest_api_client.force_authenticate(user=grantee)
|
||||
response = rest_api_client.get("/api/trash/")
|
||||
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
result_ids = {result["id"] for result in response.data["results"]}
|
||||
assert doc.pk not in result_ids
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
@pytest.mark.parametrize(
|
||||
("model", "factory", "perm"),
|
||||
|
||||
Reference in New Issue
Block a user