From 7372c4610f77f1a2d27e04b26f8a0edeaa3c3012 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Thu, 20 Aug 2026 16:28:31 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(backend)=20optimize=20media?= =?UTF-8?q?=5Fauth=20sql=20queries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On the media_auth endpoint the first bottleneck we have is with postgresql. We are looking for too much data and no index is used on the attachments colum. When the lookup filter on the attachement columns, a full scan is made on all the document table looking for each element in the array, this operation is really expensive. To fix this we created a GIN index on the attachments column. Also the readable_per_se lookup was selecting too much data combined with the filter_descendants function. We remove the usage of the filter_descendants, we choose to first fetch all the paths where the attachment is found, this operation is fast thanks to the new index, split all the paths in candidate paths and then filter readable_per_se queryset with these paths. All these modifications make the endpoint faster. --- src/backend/core/api/viewsets.py | 49 +++++++---- .../0033_document_document_attachments_gin.py | 19 ++++ src/backend/core/models.py | 6 ++ .../test_api_documents_media_auth.py | 87 +++++++++++++++++++ 4 files changed, 144 insertions(+), 17 deletions(-) create mode 100644 src/backend/core/migrations/0033_document_document_attachments_gin.py diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index e63fb726d..28b2463d9 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -2004,27 +2004,42 @@ class DocumentViewSet( user = request.user key = f"{url_params['pk']:s}/{url_params['attachment']:s}" - # Look for a document to which the user has access and that includes this attachment - # We must look into all descendants of any document to which the user has access per se - readable_per_se_paths = ( - self.queryset.readable_per_se(user) - .order_by("path") + # Look for a document to which the user has access and that includes this + # attachment. Access is granted when the document holding the attachment, + # or any of its ancestors, is readable per se by the user. + # + # We answer this without materialising the user's whole readable set: + # 1. find the document(s) that hold this key (indexed by the GIN index + # on `attachments`); + # 2. expand each to its own path plus every ancestor prefix -- pure + # string slicing, no query, bounded by tree depth + # (<= len(path) / steplen); + # 3. ask a single indexed EXISTS whether any of those candidate paths + # is readable per se by this user, right now. + # "descendant-or-self of a readable node" and "ancestor-or-self is + # readable" are converses over the same fixed-width prefix relation, so + # this yields the exact same decision as scanning every readable path. + # NOTE: like the previous implementation, `self.queryset` here does not + # filter out soft-deleted (ancestors_deleted_at) documents, so a + # soft-deleted ancestor still grants access. Behaviour preserved on + # purpose; revisit separately if that is not intended. + attachment_paths = list( + self.queryset.select_related(None) + .filter(attachments__contains=[key]) .values_list("path", flat=True) ) - attachments_documents = ( - self.queryset.select_related(None) - .filter(attachments__contains=[key]) - .only("path") - .order_by("path") - ) - readable_attachments_paths = filter_descendants( - [doc.path for doc in attachments_documents], - readable_per_se_paths, - skip_sorting=True, - ) + candidate_paths = { + path[:pos] + for path in attachment_paths + for pos in range(len(path), 0, -models.Document.steplen) + } - if not readable_attachments_paths: + if not candidate_paths or not ( + self.queryset.readable_per_se(user) + .filter(path__in=candidate_paths) + .exists() + ): logger.debug("User '%s' lacks permission for attachment", user) raise drf.exceptions.PermissionDenied() diff --git a/src/backend/core/migrations/0033_document_document_attachments_gin.py b/src/backend/core/migrations/0033_document_document_attachments_gin.py new file mode 100644 index 000000000..7a8a50713 --- /dev/null +++ b/src/backend/core/migrations/0033_document_document_attachments_gin.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.14 on 2026-08-19 14:36 + +import django.contrib.postgres.indexes +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("core", "0032_remove_linktrace_is_masked"), + ] + + operations = [ + migrations.AddIndex( + model_name="document", + index=django.contrib.postgres.indexes.GinIndex( + fields=["attachments"], name="document_attachments_gin" + ), + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 158ec2c44..96ba11598 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -14,6 +14,7 @@ from django.conf import settings from django.contrib.auth import models as auth_models from django.contrib.auth.base_user import AbstractBaseUser from django.contrib.postgres.fields import ArrayField +from django.contrib.postgres.indexes import GinIndex from django.contrib.sites.models import Site from django.core.cache import cache from django.core.files.base import ContentFile @@ -986,6 +987,11 @@ class Document(MP_Node, BaseModel): ordering = ("path",) verbose_name = _("Document") verbose_name_plural = _("Documents") + indexes = [ + # Used by media-auth to find the document(s) holding an attachment + # key without scanning the table (attachments @> [key]). + GinIndex(fields=["attachments"], name="document_attachments_gin"), + ] constraints = [ models.CheckConstraint( condition=( diff --git a/src/backend/core/tests/documents/test_api_documents_media_auth.py b/src/backend/core/tests/documents/test_api_documents_media_auth.py index 40514dad2..d3cccfcb6 100644 --- a/src/backend/core/tests/documents/test_api_documents_media_auth.py +++ b/src/backend/core/tests/documents/test_api_documents_media_auth.py @@ -490,3 +490,90 @@ def test_api_documents_media_auth_anonymous_public_custom_origin_header(settings timeout=1, ) assert response.content.decode("utf-8") == "my prose" + + +def _media_auth_ready(user, key): + """ + Call media-auth for `key` as `user` (None = anonymous), with the object + storage HEAD mocked as a READY attachment so the test exercises only the + authorization logic and does not depend on a live object store. + """ + client = APIClient() + if user is not None: + client.force_login(user) + + real_make_api_call = BaseClient._make_api_call # pylint: disable=protected-access + + def fake_make_api_call(self, operation_name, api_params): + if operation_name == "HeadObject": + return {"Metadata": {"status": DocumentAttachmentStatus.READY}} + return real_make_api_call(self, operation_name, api_params) + + with patch.object(BaseClient, "_make_api_call", new=fake_make_api_call): + return client.get( + "/api/v1.0/documents/media-auth/", + HTTP_X_ORIGINAL_URL=f"http://localhost/media/{key:s}", + ) + + +def test_api_documents_media_auth_grant_via_deep_ancestor(): + """ + Access to an attachment is granted when a *distant* ancestor of the + document holding it is readable, even though the document and every + intermediate ancestor are restricted and the user has no direct access to + them. This mirrors the production scenario where the attachment lived on a + deeply nested document. + """ + user = factories.UserFactory() + + # User has access only at the root; everything below is restricted. + root = factories.DocumentFactory(users=[user], link_reach="restricted") + level1 = factories.DocumentFactory(parent=root, link_reach="restricted") + level2 = factories.DocumentFactory(parent=level1, link_reach="restricted") + + filename = f"{uuid4()!s}.jpg" + key = f"{level2.id!s}/attachments/{filename:s}" + factories.DocumentFactory(parent=level2, link_reach="restricted", attachments=[key]) + + response = _media_auth_ready(user, key) + + assert response.status_code == 200 + assert "AWS4-HMAC-SHA256 Credential=" in response["Authorization"] + + # A user with no access anywhere in the tree is denied. + other = factories.UserFactory() + assert _media_auth_ready(other, key).status_code == 403 + + +# Number of DB queries a single media-auth authorization performs, end to end +# (session + user resolution, the attachment lookup and the readable EXISTS). +# It is a small constant and, crucially, independent of how many documents the +# instance holds -- that invariance is the regression guard for the thundering +# herd, where the check used to materialise the user's entire readable set. +MEDIA_AUTH_QUERY_COUNT = 5 + + +def test_api_documents_media_auth_authorization_cost_is_bounded( + django_assert_num_queries, +): + """ + The authorization decision must not get more expensive as the instance + grows: adding many unrelated readable documents leaves the query count + unchanged. + """ + user = factories.UserFactory() + filename = f"{uuid4()!s}.jpg" + document = factories.DocumentFactory(users=[user], link_reach="restricted") + key = f"{document.id!s}/attachments/{filename:s}" + document.attachments = [key] + document.save() + + with django_assert_num_queries(MEDIA_AUTH_QUERY_COUNT): + assert _media_auth_ready(user, key).status_code == 200 + + # Flood the instance with unrelated readable (public) documents: the query + # count must not change. + factories.DocumentFactory.create_batch(30, link_reach="public") + + with django_assert_num_queries(MEDIA_AUTH_QUERY_COUNT): + assert _media_auth_ready(user, key).status_code == 200