mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-08-08 20:03:18 +00:00
Performance: unify permission-filtering backends, fixes Correspondent/Tag list slowness (#13601)
* feat: add unified PermittedObjectsFilter backed by permitted_object_ids * refactor: migrate all ViewSets to unified PermittedObjectsFilter Replace the deprecated ObjectOwnedOrGrantedPermissionsFilter, DocumentPermissionsFilter, and ObjectOwnedPermissionsFilter aliases with PermittedObjectsFilter directly across documents/views.py (8 sites, including TrashView's include_granted=False subclass) and paperless_mail/views.py (3 sites), then delete the now-unreferenced alias classes from documents/filters.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UFyrt7FWbRRdTUAcdqBcsc * docs: document legacy status of get_objects_for_user_owner_aware/has_perms_owner_aware Stage 4's PermittedObjectsFilter/permitted_object_ids() covers the queryset-filtering use case, but both functions still have production callers outside this plan's scope (documents/views.py, documents/serialisers.py, documents/signals/handlers.py, paperless_ai/matching.py, paperless_ai/ai_classifier.py). Per Task 20 Step 2, they are kept in place rather than partially deleted, with docstrings updated to note their legacy status and remaining callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UFyrt7FWbRRdTUAcdqBcsc * 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 * perf: check bulk-edit-objects apply_to_all permissions via DB-side exclude/exists Materialized the full permitted_object_ids() set into a Python set() just to check membership for the request's objs queryset -- the same pattern already fixed at four other sites for Document. This one is used by apply_to_all, where objs can be an unbounded filtered selection (e.g. all tags matching a filter) rather than a small request-supplied ID list, making the wasted materialization worse here than at the sites already fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Cleans up the comment about why this is still here for now --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
b192a419fd
commit
fc242bb570
@@ -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",
|
||||
|
||||
+24
-49
@@ -39,7 +39,6 @@ from guardian.utils import get_user_obj_perms_model
|
||||
from rest_framework import serializers
|
||||
from rest_framework.filters import BaseFilterBackend
|
||||
from rest_framework.filters import OrderingFilter
|
||||
from rest_framework_guardian.filters import ObjectPermissionsFilter
|
||||
|
||||
from documents.models import Correspondent
|
||||
from documents.models import CustomField
|
||||
@@ -51,7 +50,7 @@ from documents.models import ShareLink
|
||||
from documents.models import ShareLinkBundle
|
||||
from documents.models import StoragePath
|
||||
from documents.models import Tag
|
||||
from documents.permissions import permitted_document_ids
|
||||
from documents.permissions import permitted_object_ids
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import Callable
|
||||
@@ -1028,59 +1027,35 @@ class PaperlessTaskFilterSet(FilterSet):
|
||||
return queryset.exclude(status__in=PaperlessTask.COMPLETE_STATUSES)
|
||||
|
||||
|
||||
class ObjectOwnedOrGrantedPermissionsFilter(ObjectPermissionsFilter):
|
||||
class PermittedObjectsFilter(BaseFilterBackend):
|
||||
"""
|
||||
A filter backend that limits results to those where the requesting user
|
||||
has read object level permissions, owns the objects, or objects without
|
||||
an owner (for backwards compat)
|
||||
Filters a queryset down to objects the requesting user owns, are
|
||||
unowned, or (when ``include_granted`` is True) has an explicit
|
||||
user/group guardian permission on. Backed by ``permitted_object_ids``
|
||||
-- a single ``id__in`` subquery, not a join -- so it can't produce
|
||||
duplicate rows even when the base queryset already carries independent
|
||||
joins (e.g. multi-value ``tags__id__all`` filtering), and stays
|
||||
index-friendly at scale instead of falling back to guardian's
|
||||
varchar-cast join.
|
||||
|
||||
Set ``include_granted = False`` on a subclass for endpoints that
|
||||
intentionally only show owned/unowned objects regardless of explicit
|
||||
shares (e.g. ``TrashView``).
|
||||
"""
|
||||
|
||||
include_granted: bool = True
|
||||
perm_codename: str | None = None
|
||||
|
||||
def filter_queryset(self, request, queryset, view):
|
||||
if request.user.is_superuser:
|
||||
return queryset
|
||||
objects_with_perms = super().filter_queryset(request, queryset, view)
|
||||
objects_owned = queryset.filter(owner=request.user)
|
||||
objects_unowned = queryset.filter(owner__isnull=True)
|
||||
return objects_with_perms | objects_owned | objects_unowned
|
||||
|
||||
|
||||
class DocumentPermissionsFilter(BaseFilterBackend):
|
||||
"""
|
||||
A filter backend limiting Document results to those the requesting user
|
||||
owns, are unowned, or has explicit (user- or group-level) view
|
||||
permission on.
|
||||
|
||||
Unlike ``ObjectOwnedOrGrantedPermissionsFilter``, this does not build an
|
||||
``objects_with_perms | objects_owned | objects_unowned`` union of
|
||||
querysets derived from the same base queryset. When that base queryset
|
||||
already carries independent joins on a multi-valued relation (e.g. two
|
||||
separate joins from ``tags__id__all`` filtering on two tags), each
|
||||
OR-ed branch can end up pairing those joins' aliases differently,
|
||||
letting more than one row out of the join's cross product satisfy the
|
||||
combined WHERE -- returning the same document more than once. Filtering
|
||||
via a single ``id__in`` against ``permitted_document_ids`` (a plain
|
||||
subquery, not a join) sidesteps that entirely and is also cheaper than
|
||||
guardian's join-based permission check.
|
||||
"""
|
||||
|
||||
def filter_queryset(self, request, queryset, view):
|
||||
if request.user.is_superuser:
|
||||
return queryset
|
||||
return queryset.filter(id__in=permitted_document_ids(request.user))
|
||||
|
||||
|
||||
class ObjectOwnedPermissionsFilter(ObjectPermissionsFilter):
|
||||
"""
|
||||
A filter backend that limits results to those where the requesting user
|
||||
owns the objects or objects without an owner (for backwards compat)
|
||||
"""
|
||||
|
||||
def filter_queryset(self, request, queryset, view):
|
||||
if request.user.is_superuser:
|
||||
return queryset
|
||||
objects_owned = queryset.filter(owner=request.user)
|
||||
objects_unowned = queryset.filter(owner__isnull=True)
|
||||
return objects_owned | objects_unowned
|
||||
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),
|
||||
)
|
||||
|
||||
|
||||
class DocumentsOrderingFilter(OrderingFilter):
|
||||
|
||||
@@ -359,6 +359,13 @@ def get_objects_for_user_owner_aware(
|
||||
"""
|
||||
Returns objects the user owns, are unowned, or has explicit perms.
|
||||
When include_deleted is True, soft-deleted items are also included.
|
||||
|
||||
Legacy slow path (guardian-backed, O(n) style permission resolution).
|
||||
Most queryset-filtering call sites have migrated onto
|
||||
``PermittedObjectsFilter``/``permitted_object_ids()``, but this function
|
||||
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
|
||||
@@ -378,6 +385,15 @@ def get_objects_for_user_owner_aware(
|
||||
|
||||
|
||||
def has_perms_owner_aware(user, perms, obj):
|
||||
"""
|
||||
Legacy slow path (guardian-backed) single-object permission check.
|
||||
|
||||
The queryset-filtering side of this migrated onto
|
||||
``PermittedObjectsFilter``/``permitted_object_ids()``, but this
|
||||
single-object check still has many 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"),
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
import pytest
|
||||
from django.contrib.auth.models import User
|
||||
from guardian.shortcuts import assign_perm
|
||||
from rest_framework.test import APIRequestFactory
|
||||
|
||||
from documents.filters import PermittedObjectsFilter
|
||||
from documents.models import Tag
|
||||
from documents.tests.factories import TagFactory
|
||||
|
||||
|
||||
class _DummyView:
|
||||
queryset = Tag.objects.all()
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
class TestPermittedObjectsFilter:
|
||||
def test_superuser_bypasses_filtering_entirely(self):
|
||||
superuser = User.objects.create_superuser(username="root")
|
||||
owner = User.objects.create_user(username="owner")
|
||||
TagFactory(owner=owner)
|
||||
request = APIRequestFactory().get("/")
|
||||
request.user = superuser
|
||||
|
||||
result = PermittedObjectsFilter().filter_queryset(
|
||||
request,
|
||||
Tag.objects.all(),
|
||||
_DummyView(),
|
||||
)
|
||||
assert result.count() == Tag.objects.count()
|
||||
|
||||
def test_non_superuser_sees_only_owned_unowned_and_granted(self):
|
||||
owner = User.objects.create_user(username="owner")
|
||||
grantee = User.objects.create_user(username="grantee")
|
||||
owned = TagFactory(owner=grantee)
|
||||
unowned = TagFactory(owner=None)
|
||||
granted = TagFactory(owner=owner)
|
||||
hidden = TagFactory(owner=owner)
|
||||
assign_perm("view_tag", grantee, granted)
|
||||
request = APIRequestFactory().get("/")
|
||||
request.user = grantee
|
||||
|
||||
result = PermittedObjectsFilter().filter_queryset(
|
||||
request,
|
||||
Tag.objects.all(),
|
||||
_DummyView(),
|
||||
)
|
||||
visible_ids = set(result.values_list("id", flat=True))
|
||||
assert visible_ids == {owned.pk, unowned.pk, granted.pk}
|
||||
assert hidden.pk not in visible_ids
|
||||
|
||||
def test_include_granted_false_excludes_explicitly_shared_objects(self):
|
||||
owner = User.objects.create_user(username="owner2")
|
||||
grantee = User.objects.create_user(username="grantee2")
|
||||
owned = TagFactory(owner=grantee)
|
||||
granted = TagFactory(owner=owner)
|
||||
assign_perm("view_tag", grantee, granted)
|
||||
request = APIRequestFactory().get("/")
|
||||
request.user = grantee
|
||||
|
||||
class _OwnerOnlyFilter(PermittedObjectsFilter):
|
||||
include_granted = False
|
||||
|
||||
result = _OwnerOnlyFilter().filter_queryset(
|
||||
request,
|
||||
Tag.objects.all(),
|
||||
_DummyView(),
|
||||
)
|
||||
visible_ids = set(result.values_list("id", flat=True))
|
||||
assert visible_ids == {owned.pk}
|
||||
assert granted.pk not in visible_ids
|
||||
+19
-15
@@ -133,12 +133,10 @@ from documents.file_handling import format_filename
|
||||
from documents.filters import CorrespondentFilterSet
|
||||
from documents.filters import CustomFieldFilterSet
|
||||
from documents.filters import DocumentFilterSet
|
||||
from documents.filters import DocumentPermissionsFilter
|
||||
from documents.filters import DocumentsOrderingFilter
|
||||
from documents.filters import DocumentTypeFilterSet
|
||||
from documents.filters import ObjectOwnedOrGrantedPermissionsFilter
|
||||
from documents.filters import ObjectOwnedPermissionsFilter
|
||||
from documents.filters import PaperlessTaskFilterSet
|
||||
from documents.filters import PermittedObjectsFilter
|
||||
from documents.filters import ShareLinkBundleFilterSet
|
||||
from documents.filters import ShareLinkFilterSet
|
||||
from documents.filters import StoragePathFilterSet
|
||||
@@ -551,7 +549,7 @@ class CorrespondentViewSet(
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = CorrespondentFilterSet
|
||||
ordering_fields = (
|
||||
@@ -592,7 +590,7 @@ class TagViewSet(PermissionsAwareDocumentCountMixin, ModelViewSet[Tag]):
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = TagFilterSet
|
||||
ordering_fields = ("color", "name", "matching_algorithm", "match", "document_count")
|
||||
@@ -684,7 +682,7 @@ class DocumentTypeViewSet(
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = DocumentTypeFilterSet
|
||||
ordering_fields = ("name", "matching_algorithm", "match", "document_count")
|
||||
@@ -988,7 +986,7 @@ class DocumentViewSet(
|
||||
DjangoFilterBackend,
|
||||
SearchFilter,
|
||||
DocumentsOrderingFilter,
|
||||
DocumentPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = DocumentFilterSet
|
||||
search_fields = ("title", "correspondent__name", "effective_content")
|
||||
@@ -2674,7 +2672,7 @@ class SavedViewViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[SavedVie
|
||||
permission_classes = (IsAuthenticated, PaperlessObjectPermissions)
|
||||
filter_backends = (
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
ordering_fields = ("name",)
|
||||
|
||||
@@ -3921,7 +3919,7 @@ class StoragePathViewSet(PermissionsAwareDocumentCountMixin, ModelViewSet[Storag
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = StoragePathFilterSet
|
||||
ordering_fields = ("name", "path", "matching_algorithm", "match", "document_count")
|
||||
@@ -4452,7 +4450,7 @@ class ShareLinkViewSet(
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = ShareLinkFilterSet
|
||||
ordering_fields = ("created", "expiration", "document")
|
||||
@@ -4482,7 +4480,7 @@ class ShareLinkBundleViewSet(PassUserMixin, ModelViewSet[ShareLinkBundle]):
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = ShareLinkBundleFilterSet
|
||||
ordering_fields = ("created", "expiration", "status")
|
||||
@@ -4791,9 +4789,11 @@ class BulkEditObjectsView(PassUserMixin):
|
||||
|
||||
if not user.is_superuser:
|
||||
perm = f"documents.{perm_codename}"
|
||||
permitted_ids = set(permitted_object_ids(user, object_class, perm_codename))
|
||||
has_perms = user.has_perm(perm) and all(
|
||||
obj.pk in permitted_ids for obj in objs
|
||||
has_perms = (
|
||||
user.has_perm(perm)
|
||||
and not objs.exclude(
|
||||
pk__in=permitted_object_ids(user, object_class, perm_codename),
|
||||
).exists()
|
||||
)
|
||||
|
||||
if not has_perms:
|
||||
@@ -5294,7 +5294,11 @@ class SystemStatusView(PassUserMixin):
|
||||
class TrashView(ListModelMixin, PassUserMixin):
|
||||
permission_classes = (IsAuthenticated,)
|
||||
serializer_class = TrashSerializer
|
||||
filter_backends = (ObjectOwnedPermissionsFilter,)
|
||||
|
||||
class _TrashPermittedObjectsFilter(PermittedObjectsFilter):
|
||||
include_granted = False
|
||||
|
||||
filter_backends = (_TrashPermittedObjectsFilter,)
|
||||
pagination_class = StandardPagination
|
||||
|
||||
model = Document
|
||||
|
||||
@@ -23,7 +23,7 @@ from rest_framework.response import Response
|
||||
from rest_framework.viewsets import ModelViewSet
|
||||
from rest_framework.viewsets import ReadOnlyModelViewSet
|
||||
|
||||
from documents.filters import ObjectOwnedOrGrantedPermissionsFilter
|
||||
from documents.filters import PermittedObjectsFilter
|
||||
from documents.models import PaperlessTask
|
||||
from documents.permissions import PaperlessObjectPermissions
|
||||
from documents.permissions import has_perms_owner_aware
|
||||
@@ -75,7 +75,7 @@ class MailAccountViewSet(PassUserMixin, ModelViewSet[MailAccount]):
|
||||
serializer_class = MailAccountSerializer
|
||||
pagination_class = StandardPagination
|
||||
permission_classes = (IsAuthenticated, PaperlessObjectPermissions)
|
||||
filter_backends = (ObjectOwnedOrGrantedPermissionsFilter,)
|
||||
filter_backends = (PermittedObjectsFilter,)
|
||||
|
||||
def get_permissions(self):
|
||||
if self.action == "test":
|
||||
@@ -197,7 +197,7 @@ class ProcessedMailViewSet(PassUserMixin, ReadOnlyModelViewSet[ProcessedMail]):
|
||||
filter_backends = (
|
||||
DjangoFilterBackend,
|
||||
OrderingFilter,
|
||||
ObjectOwnedOrGrantedPermissionsFilter,
|
||||
PermittedObjectsFilter,
|
||||
)
|
||||
filterset_class = ProcessedMailFilterSet
|
||||
|
||||
@@ -225,7 +225,7 @@ class MailRuleViewSet(PassUserMixin, ModelViewSet[MailRule]):
|
||||
serializer_class = MailRuleSerializer
|
||||
pagination_class = StandardPagination
|
||||
permission_classes = (IsAuthenticated, PaperlessObjectPermissions)
|
||||
filter_backends = (ObjectOwnedOrGrantedPermissionsFilter,)
|
||||
filter_backends = (PermittedObjectsFilter,)
|
||||
|
||||
|
||||
@extend_schema_view(
|
||||
|
||||
Reference in New Issue
Block a user