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