diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad4dcd4b..2c21c8e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,12 +44,12 @@ and this project adheres to moment they were given access to it. The collaboration server's `activity` and `changeset` routes are opened to the browser, bounded per user to the earliest access they hold on the document or on one of its ancestors — the same cut-off - the version endpoints have always applied, now computed once in the backend - (`user_access_since`) and applied by the collaboration server as well. The - bound is enforced server-side and silently: a client asks for whatever range it - likes and receives only its own share. A reader who reaches a document through - its link alone holds no access and so has no date to bound a history with, and - gets none — as they never did + the version endpoints have always applied, read from the backend + (`GET /documents/{id}/accesses/me/`) and applied by the collaboration + server. The bound is enforced server-side and silently: a client asks for + whatever range it likes and receives only its own share. A reader who reaches + a document through its link alone holds no access and so has no date to bound + a history with, and gets none — as they never did - ✨(collaboration) grant the browser only the routes it uses. The collaboration server now answers what a caller may do with a document as a permission object, facet by facet, and enforces every facet itself. A browser @@ -119,6 +119,17 @@ and this project adheres to ### Removed +- 🔥(backend) remove the document version endpoints. `GET + /documents/{id}/versions/` and `GET, DELETE /documents/{id}/versions/{id}/` + listed S3 object versions of the legacy `{id}/file` key. Nothing has written + that key since the migration to the collaboration server, so the list was + frozen at each document's migration date, and nothing has called the + endpoints since the history panel started reading the collaboration server's + `activity` and `changeset` routes instead. Gone with them: + `Document.get_versions_slice`, `Document.delete_version`, the `version_id` + argument of + `get_content_response`, the `DOCUMENT_VERSIONS_PAGE_SIZE` setting, and the + `versions_retrieve` and `versions_destroy` abilities - 🔥(backend) remove the unused `CollaborationService` - 💥(backend) remove the `documents/{id}/can-edit/` endpointt - 💥(backend) remove the `documents/{id}/content/` endpoint diff --git a/src/backend/core/api/permissions.py b/src/backend/core/api/permissions.py index affd9b070..da410ca22 100644 --- a/src/backend/core/api/permissions.py +++ b/src/backend/core/api/permissions.py @@ -10,7 +10,6 @@ from core import choices from core.models import DocumentAccess, RoleChoices, get_trashbin_cutoff ACTION_FOR_METHOD_TO_PERMISSION = { - "versions_detail": {"DELETE": "versions_destroy", "GET": "versions_retrieve"}, "children": {"GET": "children_list", "POST": "children_create"}, } diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 8c291cbbc..ff48656db 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -183,16 +183,6 @@ class DocumentSerializer(ListDocumentSerializer): file = serializers.FileField( required=False, write_only=True, allow_null=True, max_length=255 ) - # When the current user gained access to this document — the earliest access they hold on - # it or on one of its ancestors, `null` when they reach it through its link reach alone. - # It bounds the history they may read: the collaboration server fetches this endpoint to - # authorize a connection and turns this into `history.from`. - # - # Read from the `user_access_since` annotation, which `filter_queryset` applies — so it is - # answered on the retrieve endpoint, the one that is read for it. It falls back to `null` - # on the write responses (create/update), which serialize an instance that never came from - # that queryset; nothing consumes it there. - user_access_since = serializers.DateTimeField(read_only=True, default=None) class Meta: model = models.Document @@ -218,7 +208,6 @@ class DocumentSerializer(ListDocumentSerializer): "path", "title", "updated_at", - "user_access_since", "user_role", ] read_only_fields = [ @@ -240,7 +229,6 @@ class DocumentSerializer(ListDocumentSerializer): "numchild", "path", "updated_at", - "user_access_since", "user_role", ] @@ -348,6 +336,8 @@ class DocumentAccessSerializer(serializers.ModelSerializer): "abilities", "max_ancestors_role", "max_role", + "updated_at", + "created_at", ] read_only_fields = [ "id", @@ -355,6 +345,8 @@ class DocumentAccessSerializer(serializers.ModelSerializer): "abilities", "max_ancestors_role", "max_role", + "updated_at", + "created_at", ] def get_abilities(self, instance) -> dict: @@ -399,6 +391,8 @@ class DocumentAccessLightSerializer(DocumentAccessSerializer): "abilities", "max_ancestors_role", "max_role", + "updated_at", + "created_at", ] read_only_fields = [ "id", @@ -408,6 +402,8 @@ class DocumentAccessLightSerializer(DocumentAccessSerializer): "abilities", "max_ancestors_role", "max_role", + "updated_at", + "created_at", ] @@ -801,15 +797,6 @@ class DocumentAskForAccessSerializer(serializers.ModelSerializer): return {} -class VersionFilterSerializer(serializers.Serializer): - """Validate version filters applied to the list endpoint.""" - - version_id = serializers.CharField(required=False, allow_blank=True) - page_size = serializers.IntegerField( - required=False, min_value=1, max_value=50, default=20 - ) - - class AITransformSerializer(serializers.Serializer): """Serializer for AI transform requests.""" diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 792c4caeb..9113b6901 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -21,7 +21,7 @@ from django.core.validators import URLValidator from django.db import DatabaseError, transaction from django.db import models as db from django.db.models.expressions import RawSQL -from django.db.models.functions import Greatest +from django.db.models.functions import Greatest, Left, Length from django.http import Http404, StreamingHttpResponse from django.urls import reverse from django.utils import timezone @@ -75,7 +75,6 @@ from core.tasks.mail import send_ask_for_access_mail from core.tasks.search import trigger_batch_document_indexer from core.utils.analytics import PosthogEventName, posthog_capture from core.utils.dicts import lowercase_keys -from core.utils.paths import filter_descendants from core.utils.s3 import get_s3_client from core.utils.treebeard import create_tree_node_with_retry from core.utils.users import users_sharing_documents_with @@ -479,31 +478,25 @@ class DocumentViewSet( 5. **Children**: List or create child documents. Example: GET, POST /documents/{id}/children/ - 6. **Versions List**: Retrieve version history of a document. - Example: GET /documents/{id}/versions/ - - 7. **Version Detail**: Get or delete a specific document version. - Example: GET, DELETE /documents/{id}/versions/{version_id}/ - - 8. **Favorite**: Get list of favorite documents for a user. Mark or unmark + 6. **Favorite**: Get list of favorite documents for a user. Mark or unmark a document as favorite. Examples: - GET /documents/favorites/ - POST, DELETE /documents/{id}/favorite/ - 9. **Create for Owner**: Create a document via server-to-server on behalf of a user. + 7. **Create for Owner**: Create a document via server-to-server on behalf of a user. Example: POST /documents/create-for-owner/ - 10. **Link Configuration**: Update document link configuration. + 8. **Link Configuration**: Update document link configuration. Example: PUT /documents/{id}/link-configuration/ - 11. **Attachment Upload**: Upload a file attachment for the document. + 9. **Attachment Upload**: Upload a file attachment for the document. Example: POST /documents/{id}/attachment-upload/ - 12. **Media Auth**: Authorize access to document media. + 10. **Media Auth**: Authorize access to document media. Example: GET /documents/media-auth/ - 13. **AI Transform**: Apply a transformation action on a piece of text with AI. + 11. **AI Transform**: Apply a transformation action on a piece of text with AI. Example: POST /documents/{id}/ai-transform/ Expected data: - text (str): The input text. @@ -511,7 +504,7 @@ class DocumentViewSet( Returns: JSON response with the processed text. Throttled by: AIDocumentRateThrottle, AIUserRateThrottle. - 14. **AI Translate**: Translate a piece of text with AI. + 12. **AI Translate**: Translate a piece of text with AI. Example: POST /documents/{id}/ai-translate/ Expected data: - text (str): The input text. @@ -519,7 +512,7 @@ class DocumentViewSet( Returns: JSON response with the translated text. Throttled by: AIDocumentRateThrottle, AIUserRateThrottle. - 15. **AI Proxy**: Proxy an AI request to an external AI service. + 13. **AI Proxy**: Proxy an AI request to an external AI service. Example: POST /api/v1.0/documents//ai-proxy ### Ordering: created_at, updated_at, is_favorite, title @@ -613,9 +606,6 @@ class DocumentViewSet( queryset = queryset.annotate_is_favorite(user) queryset = queryset.annotate_user_roles(user) queryset = queryset.annotate_user_has_link_trace(user) - # Detail views only — `list` builds its own annotation chain below and does not need - # this one, so the list endpoint keeps its current query cost - queryset = queryset.annotate_user_access_since(user) return queryset @@ -1809,109 +1799,6 @@ class DocumentViewSet( lambda paths: {parent.path: parent}, ) - @drf.decorators.action(detail=True, methods=["get"], url_path="versions") - def versions_list(self, request, *args, **kwargs): - """ - Return the document's versions but only those created after the user got access - to the document - - DEPRECATED — nothing calls this any more, and it can be removed once the - migration to the collaboration server is finished. - - The collaboration server is the source of truth for document history and - keeps it itself; the version history in the frontend is built from its - `activity` and `changeset` routes, bounded by the same date this method - applies (`user_access_since`, which the collaboration server is handed to - bound what it serves). What this endpoint lists is S3 object versions of - the legacy `{pk}/file` key, and nothing writes that key any more — the - content endpoint that used to went away with the migration — so the list - is frozen at each document's migration date and gains no further entries. - - Removing it is safe once every document has had its real history replayed - into the collaboration server by `manage.py migrate_documents`; until - then these versions are the only record of what a soft-migrated document - looked like before it moved. `versions_detail` and - `Document.get_versions_slice` are kept for the same reason and go at the - same time. - - The `versions_list` ability still gates the history menu item in the - frontend, and correctly: it is `has_access_role`, which is exactly the - condition under which `user_access_since` is not None and the - collaboration server grants a history — so the gate and the grant cannot - disagree. - """ - user = request.user - if not user.is_authenticated: - raise drf.exceptions.PermissionDenied("Authentication required.") - - # Validate query parameters using dedicated serializer - serializer = serializers.VersionFilterSerializer(data=request.query_params) - serializer.is_valid(raise_exception=True) - - document = self.get_object() - - # Users should not see version history dating from before they gained access to the - # document. `user_access_since` is annotated onto the queryset (see - # `DocumentQuerySet.annotate_user_access_since`) and is the one definition of that - # date — the collaboration server is handed the same value to bound the history it - # serves. It is None for a user who reaches the document through its link reach - # alone: no access, no date, and so no history. - min_datetime = document.user_access_since - if min_datetime is None: - raise drf.exceptions.PermissionDenied( - "Only users with specific access can see version history" - ) - - versions_data = document.get_versions_slice( - from_version_id=serializer.validated_data.get("version_id"), - min_datetime=min_datetime, - page_size=serializer.validated_data.get("page_size"), - ) - - return drf.response.Response(versions_data) - - @drf.decorators.action( - detail=True, - methods=["get", "delete"], - url_path=r"versions/(?P[A-Za-z0-9._+\-=~]{1,1024})", - ) - # pylint: disable=unused-argument - def versions_detail(self, request, pk, version_id, *args, **kwargs): - """Custom action to retrieve a specific version of a document""" - document = self.get_object() - - # Don't let users access versions that were created before they were given access to - # the document — the same cut-off as `versions_list`, from the same annotation. - # Checked before the object is fetched: a caller who may see no version at all should - # not learn from a 404 whether this one exists. - min_datetime = document.user_access_since - if min_datetime is None: - raise drf.exceptions.PermissionDenied( - "Only users with specific access can see version history" - ) - - try: - response = document.get_content_response(version_id=version_id) - except (FileNotFoundError, ClientError) as err: - raise Http404 from err - - if response["LastModified"] < min_datetime: - raise Http404 - - if request.method == "DELETE": - response = document.delete_version(version_id) - return drf.response.Response( - status=response["ResponseMetadata"]["HTTPStatusCode"] - ) - - return drf.response.Response( - { - "content": response["Body"].read().decode("utf-8"), - "last_modified": response["LastModified"], - "id": version_id, - } - ) - @drf.decorators.action(detail=True, methods=["put"], url_path="link-configuration") def link_configuration(self, request, *args, **kwargs): """Update link configuration with specific rights (cf get_abilities).""" @@ -2654,6 +2541,7 @@ class DocumentAccessViewSet( "created_at", "role", "team", + "updated_at", "user__id", "user__short_name", "user__full_name", @@ -2835,6 +2723,36 @@ class DocumentAccessViewSet( # Notify collaboration server about the access removed reset_service_connections_in_cascade.delay(document_id, user_id) + @drf.decorators.action( + detail=False, + methods=["get"], + url_name="me", + url_path="me", + ) + def get_user_access(self, request, *args, **kwargs): + """Retrieve the access related to the current user and return it.""" + + document = self.document + user = request.user + access = ( + self.get_queryset() + .filter( + db.Q(user=user) | db.Q(team__in=user.teams), + document__path=Left(db.Value(document.path), Length("document__path")), + document__ancestors_deleted_at__isnull=True, + ) + .order_by("created_at") + .first() + ) + + if not access: + raise drf.exceptions.PermissionDenied() + + serializer = serializers.DocumentAccessLightSerializer( + access, context=self.get_serializer_context() + ) + return drf.response.Response(serializer.data) + class InvitationViewset( drf.mixins.CreateModelMixin, diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 33ac80cd4..9703135cd 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -925,44 +925,6 @@ class DocumentQuerySet(MP_NodeQuerySet): user_roles=models.Value([], output_field=output_field), ) - def annotate_user_access_since(self, user): - """ - Annotate document queryset with the moment the current user gained access to the - document — the earliest access they hold on it or on one of its ancestors. - - This is the point from which they may see the document's history: the collaboration - server turns it into a `history.from` permission, and the version endpoints filter on - it. `None` when the user reaches the document through its link reach alone, which is - not an access and carries no date — those users get no history at all, deliberately - (see the comment in `get_abilities`). - """ - if user.is_authenticated: - # the same ancestor-aware subquery as `annotate_user_roles`: the access's document - # path is a prefix of this one's, so it matches the document and every ancestor - user_access_since_subquery = ( - DocumentAccess.objects.filter( - models.Q(user=user) | models.Q(team__in=user.teams), - document__path=Left( - models.OuterRef("path"), Length("document__path") - ), - ) - .order_by() - .values("user") - .annotate(min_created_at=models.Min("created_at")) - .values("min_created_at") - ) - - return self.annotate( - user_access_since=models.Subquery( - user_access_since_subquery, - output_field=models.DateTimeField(), - ) - ) - - return self.annotate( - user_access_since=models.Value(None, output_field=models.DateTimeField()), - ) - def annotate_user_has_link_trace(self, user): """ Annotate document queryset with a boolean to know if the current user @@ -1144,78 +1106,10 @@ class Document(MP_Node, BaseModel): self._content = content - def get_content_response(self, version_id=""): - """Get the content in a specific version of the document""" - params = { - "Bucket": default_storage.bucket_name, - "Key": self.file_key, - } - if version_id: - params["VersionId"] = version_id - return default_storage.connection.meta.client.get_object(**params) - - def get_versions_slice(self, from_version_id="", min_datetime=None, page_size=None): - """Get document versions from object storage with pagination and starting conditions""" - # /!\ Trick here /!\ - # The "KeyMarker" and "VersionIdMarker" fields must either be both set or both not set. - # The error we get otherwise is not helpful at all. - markers = {} - if from_version_id: - markers.update( - {"KeyMarker": self.file_key, "VersionIdMarker": from_version_id} - ) - - real_page_size = ( - min(page_size, settings.DOCUMENT_VERSIONS_PAGE_SIZE) - if page_size - else settings.DOCUMENT_VERSIONS_PAGE_SIZE - ) - - response = default_storage.connection.meta.client.list_object_versions( - Bucket=default_storage.bucket_name, - Prefix=self.file_key, - # compensate the latest version that we exclude below and get one more to - # know if there are more pages - MaxKeys=real_page_size + 2, - **markers, - ) - - min_last_modified = min_datetime or self.created_at - versions = [ - { - key_snake: version[key_camel] - for key_snake, key_camel in [ - ("etag", "ETag"), - ("is_latest", "IsLatest"), - ("last_modified", "LastModified"), - ("version_id", "VersionId"), - ] - } - for version in response.get("Versions", []) - if version["LastModified"] >= min_last_modified - and version["IsLatest"] is False - ] - results = versions[:real_page_size] - - count = len(results) - if count == len(versions): - is_truncated = False - next_version_id_marker = "" - else: - is_truncated = True - next_version_id_marker = versions[count - 1]["version_id"] - - return { - "next_version_id_marker": next_version_id_marker, - "is_truncated": is_truncated, - "versions": results, - "count": count, - } - - def delete_version(self, version_id): - """Delete a version from object storage given its version id""" - return default_storage.connection.meta.client.delete_object( - Bucket=default_storage.bucket_name, Key=self.file_key, VersionId=version_id + def get_content_response(self): + """Get the content of the document from object storage""" + return default_storage.connection.meta.client.get_object( + Bucket=default_storage.bucket_name, Key=self.file_key ) def get_self_and_ancestors_paths(self): @@ -1400,6 +1294,13 @@ class Document(MP_Node, BaseModel): # want anonymous users to access versions (we wouldn't know from # which date to allow them anyway) # Anonymous users should also not see document accesses + # + # This is what `versions_list` reports. The history itself lives in the + # collaboration server, which reads the ability to decide whether to ask + # this backend for the caller's access — the `created_at` it answers with + # bounds what it serves. So the ability and `accesses/me/` have to agree, + # and a test holds them to it + # (test_api_document_accesses_me_agrees_with_the_versions_list_ability). has_access_role = bool(role) and not is_deleted can_update_from_access = ( is_owner_or_admin or role == RoleChoices.EDITOR @@ -1487,9 +1388,7 @@ class Document(MP_Node, BaseModel): "link_select_options": link_select_options, "tree": retrieve, "update": can_update, - "versions_destroy": is_owner_or_admin, "versions_list": has_access_role, - "versions_retrieve": has_access_role, "search": can_get, } diff --git a/src/backend/core/tests/documents/test_api_document_accesses.py b/src/backend/core/tests/documents/test_api_document_accesses.py index 83c4b392f..2f4393881 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -180,6 +180,8 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( "set_role_to": [], "update": False, }, + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": access.created_at.isoformat().replace("+00:00", "Z"), } for access in privileged_accesses ], @@ -280,6 +282,8 @@ def test_api_document_accesses_list_authenticated_related_privileged( "team": access.team, "role": access.role, "abilities": access.get_abilities(user), + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": access.created_at.isoformat().replace("+00:00", "Z"), } for access in ancestors_accesses + document_accesses ], @@ -646,6 +650,8 @@ def test_api_document_accesses_retrieve_authenticated_related( "max_ancestors_role": None, "max_role": access.role, "abilities": access.get_abilities(user), + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": access.created_at.isoformat().replace("+00:00", "Z"), } @@ -808,9 +814,13 @@ def test_api_document_accesses_update_administrator_except_owner( **old_values, "role": new_values["role"], "max_role": new_values["role"], + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), } else: - assert updated_values == old_values + assert updated_values == { + **old_values, + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + } @pytest.mark.parametrize("via", VIA) @@ -857,7 +867,10 @@ def test_api_document_accesses_update_administrator_from_owner(via, mock_user_te assert response.status_code == 403 access.refresh_from_db() updated_values = serializers.DocumentAccessSerializer(instance=access).data - assert updated_values == old_values + assert updated_values == { + **old_values, + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + } @pytest.mark.parametrize("via", VIA) @@ -922,7 +935,10 @@ def test_api_document_accesses_update_administrator_to_owner( access.refresh_from_db() updated_values = serializers.DocumentAccessSerializer(instance=access).data - assert updated_values == old_values + assert updated_values == { + **old_values, + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + } @pytest.mark.parametrize("via", VIA) @@ -985,9 +1001,13 @@ def test_api_document_accesses_update_owner( **old_values, "role": new_values["role"], "max_role": new_values["role"], + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), } else: - assert updated_values == old_values + assert updated_values == { + **old_values, + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + } @pytest.mark.parametrize("via", VIA) diff --git a/src/backend/core/tests/documents/test_api_document_accesses_create.py b/src/backend/core/tests/documents/test_api_document_accesses_create.py index 95192b4a1..6ed96129c 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses_create.py +++ b/src/backend/core/tests/documents/test_api_document_accesses_create.py @@ -199,6 +199,8 @@ def test_api_document_accesses_create_authenticated_administrator_share_to_user( "role": role, "team": "", "user": other_user, + "updated_at": new_document_access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": new_document_access.created_at.isoformat().replace("+00:00", "Z"), } assert len(mail.outbox) == 1 email = mail.outbox[0] @@ -306,6 +308,8 @@ def test_api_document_accesses_create_authenticated_administrator_share_to_team( "role": role, "team": "new-team", "user": None, + "updated_at": new_document_access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": new_document_access.created_at.isoformat().replace("+00:00", "Z"), } assert len(mail.outbox) == 0 @@ -387,6 +391,8 @@ def test_api_document_accesses_create_authenticated_owner_share_to_user( "role": role, "team": "", "user": other_user, + "updated_at": new_document_access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": new_document_access.created_at.isoformat().replace("+00:00", "Z"), } assert len(mail.outbox) == 1 email = mail.outbox[0] @@ -477,6 +483,8 @@ def test_api_document_accesses_create_authenticated_owner_share_to_team( "role": role, "team": "new-team", "user": None, + "updated_at": new_document_access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": new_document_access.created_at.isoformat().replace("+00:00", "Z"), } assert len(mail.outbox) == 0 @@ -555,6 +563,12 @@ def test_api_document_accesses_create_email_in_receivers_language(via, mock_user "role": role, "team": "", "user": other_user_data, + "updated_at": new_document_access.updated_at.isoformat().replace( + "+00:00", "Z" + ), + "created_at": new_document_access.created_at.isoformat().replace( + "+00:00", "Z" + ), } assert len(mail.outbox) == index + 1 email = mail.outbox[index] diff --git a/src/backend/core/tests/documents/test_api_document_accesses_me.py b/src/backend/core/tests/documents/test_api_document_accesses_me.py new file mode 100644 index 000000000..71ac47ce2 --- /dev/null +++ b/src/backend/core/tests/documents/test_api_document_accesses_me.py @@ -0,0 +1,387 @@ +""" +Test documents accesses /me API. +""" + +from datetime import timedelta +from unittest import mock +from uuid import uuid4 + +from django.utils import timezone + +import pytest +from rest_framework.test import APIClient + +from core import choices, factories +from core.tests.conftest import TEAM, USER, VIA + +pytestmark = pytest.mark.django_db + + +def grant_access(via, document, user, mock_user_teams, role="reader"): + """ + Give `user` an access on `document`, personally or through one of their teams. + + The endpoint reaches an access by `user` or by `team` in one query, so every case + below is worth running both ways: a filter that works for one is not evidence about + the other. The teams are set on the mock only for `TEAM` — a `PropertyMock` left + alone iterates empty, which is what an authenticated user with no team looks like. + """ + if via == USER: + return factories.UserDocumentAccessFactory( + document=document, user=user, role=role + ) + + mock_user_teams.return_value = ["lasuite", "unknown"] + return factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role=role + ) + + +def expected_access(access, user, via): + """The payload the endpoint serves for an access held by the calling user.""" + return { + "id": str(access.id), + "document": { + "id": str(access.document_id), + "path": access.document.path, + "depth": access.document.depth, + }, + "user": None + if via == TEAM + else {"full_name": user.full_name, "short_name": user.short_name}, + "team": "lasuite" if via == TEAM else "", + "role": access.role, + "max_ancestors_role": None, + "max_role": access.role, + "abilities": access.get_abilities(user), + "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), + "created_at": access.created_at.isoformat().replace("+00:00", "Z"), + } + + +@pytest.mark.parametrize("link_reach", choices.LinkReachChoices) +def test_api_document_accesses_me_anonymous(link_reach): + """Anonymous users should not be allowed to fetch document accesses /me.""" + document = factories.DocumentFactory(link_reach=link_reach) + factories.UserDocumentAccessFactory.create_batch(2, document=document) + + response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + assert response.status_code == 401 + assert response.json() == { + "detail": "Authentication credentials were not provided." + } + + +@pytest.mark.parametrize("link_reach", choices.LinkReachChoices) +def test_api_document_accesses_me_not_existing_access(link_reach): + """Connected user should not be allowed to fetch an access if not existing.""" + document = factories.DocumentFactory(link_reach=link_reach) + factories.UserDocumentAccessFactory.create_batch(2, document=document) + + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + +def test_api_document_accesses_me_team_the_user_does_not_belong_to(mock_user_teams): + """ + A team access is only the caller's if they are in that team. The negative case of the + `team__in=user.teams` half of the lookup: being in *a* team is not being in this one. + """ + mock_user_teams.return_value = ["another-team"] + + document = factories.DocumentFactory(link_reach="public", link_role="editor") + factories.TeamDocumentAccessFactory(document=document, team="lasuite") + + client = APIClient() + client.force_login(factories.UserFactory()) + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + +@pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize("role", choices.RoleChoices) +def test_api_document_accesses_me_direct_access(via, role, mock_user_teams): + """Connected user with direct access should retrieve their own access.""" + document = factories.DocumentFactory() + factories.UserDocumentAccessFactory.create_batch(2, document=document) + + user = factories.UserFactory() + access = grant_access(via, document, user, mock_user_teams, role=role) + + client = APIClient() + client.force_login(user) + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + assert response.status_code == 200 + assert response.json() == expected_access(access, user, via) + + +@pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize("role", choices.RoleChoices) +def test_api_document_accesses_me_in_depth(via, role, mock_user_teams): + """Connected user with access to a root should retrieve it from children documents""" + + document = factories.DocumentFactory() + factories.UserDocumentAccessFactory.create_batch(2, document=document) + + user = factories.UserFactory() + access = grant_access(via, document, user, mock_user_teams, role=role) + + client = APIClient() + client.force_login(user) + + child1 = factories.DocumentFactory(parent=document) + child2 = factories.DocumentFactory(parent=child1) + + response = client.get(f"/api/v1.0/documents/{child2.id!s}/accesses/me/") + assert response.status_code == 200 + assert response.json() == expected_access(access, user, via) + + +@pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize("role", choices.RoleChoices) +def test_api_document_accesses_me_multiple_accesses_in_depth( + via, role, mock_user_teams +): + """Connected user with multiple accesses in the same tree should retrieve the older created.""" + + document = factories.DocumentFactory() + factories.UserDocumentAccessFactory.create_batch(2, document=document) + + user = factories.UserFactory() + access = grant_access(via, document, user, mock_user_teams, role=role) + + client = APIClient() + client.force_login(user) + + child1 = factories.DocumentFactory(parent=document) + child2 = factories.DocumentFactory(parent=child1) + grant_access(via, child2, user, mock_user_teams, role=role) + + response = client.get(f"/api/v1.0/documents/{child2.id!s}/accesses/me/") + assert response.status_code == 200 + assert response.json() == expected_access(access, user, via) + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_me_ancestor_access_wins_over_a_recent_stronger_one( + via, mock_user_teams +): + """ + The access returned is the earliest one, whatever its role and whatever the document + it is held on. This is what bounds the history the collaboration server serves — it + turns `created_at` into `history.from` — so a later access on the document itself must + not shorten what an earlier one on a parent already gave. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory(link_reach="restricted") + child = factories.DocumentFactory(parent=parent, link_reach="restricted") + + ten_days_ago = timezone.now() - timedelta(days=10) + with mock.patch("django.utils.timezone.now", return_value=ten_days_ago): + parent_access = grant_access(via, parent, user, mock_user_teams, role="reader") + # granted later, and deliberately the stronger role: recency must not win + grant_access(via, child, user, mock_user_teams, role="editor") + + response = client.get(f"/api/v1.0/documents/{child.id!s}/accesses/me/") + + assert response.status_code == 200 + assert response.json()["id"] == str(parent_access.id) + assert response.json()["created_at"] == ten_days_ago.isoformat().replace( + "+00:00", "Z" + ) + + +def test_api_document_accesses_me_earliest_of_team_and_user_accesses(mock_user_teams): + """ + A user holding both a team access and a personal one gets the earliest of the two, + whichever kind it is — the one axis the `via` parametrization above cannot express, + because the two halves of the lookup have to be compared against each other. + """ + mock_user_teams.return_value = ["lasuite"] + + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + child = factories.DocumentFactory(parent=parent) + + ten_days_ago = timezone.now() - timedelta(days=10) + with mock.patch("django.utils.timezone.now", return_value=ten_days_ago): + team_access = factories.TeamDocumentAccessFactory( + document=parent, team="lasuite", role="reader" + ) + factories.UserDocumentAccessFactory(document=child, user=user, role="owner") + + response = client.get(f"/api/v1.0/documents/{child.id!s}/accesses/me/") + + assert response.status_code == 200 + assert response.json()["id"] == str(team_access.id) + assert response.json()["team"] == "lasuite" + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_me_ignores_a_descendant_access(via, mock_user_teams): + """ + An access on a child grants nothing on its parent: the lookup walks up the tree, never + down. Otherwise a user invited to one page would be handed the history of the whole + space above it. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory(link_reach="public", link_role="editor") + child = factories.DocumentFactory(parent=parent) + grant_access(via, child, user, mock_user_teams, role="owner") + + response = client.get(f"/api/v1.0/documents/{parent.id!s}/accesses/me/") + + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_me_ignores_a_sibling_access(via, mock_user_teams): + """An access on a sibling — a path of the same length — is not an access on this one.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory(link_reach="public", link_role="editor") + sibling = factories.DocumentFactory(parent=parent) + document = factories.DocumentFactory(parent=parent) + grant_access(via, sibling, user, mock_user_teams, role="owner") + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + + assert response.status_code == 403 + + +def test_api_document_accesses_me_unknown_document(): + """An unknown document is a 404, not a 403: there is no access question to answer.""" + client = APIClient() + client.force_login(factories.UserFactory()) + + response = client.get(f"/api/v1.0/documents/{uuid4()!s}/accesses/me/") + + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_me_soft_deleted_document(via, mock_user_teams): + """ + A soft deleted document has no history to bound — `versions_list` is withheld on it + for the same reason — so its accesses are not served either. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + grant_access(via, document, user, mock_user_teams, role="owner") + document.soft_delete() + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + + assert response.status_code == 403 + + +@pytest.mark.parametrize("via", VIA) +def test_api_document_accesses_me_deleted_ancestor(via, mock_user_teams): + """An access held on a deleted ancestor is not an access any more.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + grant_access(via, parent, user, mock_user_teams, role="owner") + child = factories.DocumentFactory(parent=parent) + parent.soft_delete() + + response = client.get(f"/api/v1.0/documents/{child.id!s}/accesses/me/") + + assert response.status_code == 403 + + +@pytest.mark.parametrize("method", ["post", "put", "patch", "delete"]) +def test_api_document_accesses_me_method_not_allowed(method): + """The endpoint only reads: "me" is not a document access id one can write to.""" + user = factories.UserFactory() + document = factories.DocumentFactory(users=[(user, "owner")]) + + client = APIClient() + client.force_login(user) + + response = getattr(client, method)( + f"/api/v1.0/documents/{document.id!s}/accesses/me/", {}, format="json" + ) + + assert response.status_code == 405 + + +@pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize( + "scenario", ["direct", "ancestor", "none", "deleted", "deleted_ancestor"] +) +def test_api_document_accesses_me_agrees_with_the_versions_list_ability( + via, scenario, mock_user_teams +): + """ + `versions_list` and this endpoint must answer the same question. + + The collaboration server reads the ability to decide whether to ask for the access at + all, and turns the access's `created_at` into the start of the history it serves. The + two are computed from different queries — `has_access_role` from the `user_roles` + annotation, this one from its own ancestor-aware lookup — so nothing but a test keeps + them in step. Were they to disagree, a user would be offered a history menu that opens + on nothing (or, worse, the other way round). + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory(link_reach="public", link_role="editor") + document = factories.DocumentFactory(parent=parent) + + if scenario == "direct": + grant_access(via, document, user, mock_user_teams) + elif scenario == "ancestor": + grant_access(via, parent, user, mock_user_teams) + elif scenario == "deleted": + grant_access(via, document, user, mock_user_teams, role="owner") + document.soft_delete() + elif scenario == "deleted_ancestor": + grant_access(via, parent, user, mock_user_teams, role="owner") + parent.soft_delete() + + retrieve = client.get(f"/api/v1.0/documents/{document.id!s}/") + assert retrieve.status_code == 200 + can_list_versions = retrieve.json()["abilities"]["versions_list"] + + response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/me/") + + assert can_list_versions is (response.status_code == 200), ( + f"{scenario} via {via}: versions_list={can_list_versions} but /accesses/me/ " + f"answered {response.status_code}" + ) diff --git a/src/backend/core/tests/documents/test_api_document_versions.py b/src/backend/core/tests/documents/test_api_document_versions.py deleted file mode 100644 index 7e5105bb5..000000000 --- a/src/backend/core/tests/documents/test_api_document_versions.py +++ /dev/null @@ -1,749 +0,0 @@ -""" -Test document versions API endpoints for users in impress's core app. -""" - -import random -import time - -import pytest -from rest_framework.test import APIClient - -from core import factories, models -from core.factories import YDOC_HELLO_WORLD_BASE64 -from core.tests.conftest import TEAM, USER, VIA - -pytestmark = pytest.mark.django_db - - -def create_document(**kwargs): - """ - Create a document holding content in the legacy object storage. - - Versions are the versions of that object, so these tests are the ones still - about it: the factories give a document no content anymore, the - collaboration server holds it. - """ - return factories.DocumentFactory(content=YDOC_HELLO_WORLD_BASE64, **kwargs) - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -@pytest.mark.parametrize("role", models.LinkRoleChoices.values) -def test_api_document_versions_list_anonymous(role, reach): - """ - Anonymous users should not be allowed to list document versions for a document - whatever the reach and role. - """ - document = create_document(link_role=role, link_reach=reach) - - # Accesses and traces for other users should not interfere - factories.UserDocumentAccessFactory(document=document) - models.LinkTrace.objects.create(document=document, user=factories.UserFactory()) - - response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/versions/") - - assert response.status_code == 403 - assert response.json() == {"detail": "Authentication required."} - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -def test_api_document_versions_list_authenticated_unrelated(reach): - """ - Authenticated users should not be allowed to list document versions for a document - to which they are not related. - """ - user = factories.UserFactory(with_owned_document=True) - - client = APIClient() - client.force_login(user) - - document = create_document(link_reach=reach) - factories.UserDocumentAccessFactory.create_batch(3, document=document) - - # The versions of another document to which the user is related should not be listed either - factories.UserDocumentAccessFactory(user=user) - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/", - ) - assert response.status_code == 403 - assert response.json() == { - "detail": "You do not have permission to perform this action." - } - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_list_authenticated_related_success(via, mock_user_teams): - """ - Authenticated users should be able to list document versions for a document - to which they are directly related, whatever their role in the document. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - if via == USER: - models.DocumentAccess.objects.create( - document=document, - user=user, - role=random.choice(models.RoleChoices.values), - ) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - models.DocumentAccess.objects.create( - document=document, - team="lasuite", - role=random.choice(models.RoleChoices.values), - ) - - # Other versions of documents to which the user has access should not be listed - factories.UserDocumentAccessFactory(user=user) - - # A version created before the user got access should be hidden - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/", - ) - - assert response.status_code == 200 - content = response.json() - assert content["count"] == 0 - - # Add a new version to the document - for i in range(3): - document.content = f"new content {i:d}" - document.save() - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/", - ) - - assert response.status_code == 200 - content = response.json() - # The current version is not listed - assert content["count"] == 2 - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_list_authenticated_related_pagination( - via, mock_user_teams -): - """ - The list of versions should be paginated and exclude versions that were created prior to the - user gaining access to the document. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - for i in range(3): - document.content = f"before {i:d}" - document.save() - - if via == USER: - models.DocumentAccess.objects.create( - document=document, - user=user, - role=random.choice(models.RoleChoices.values), - ) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - models.DocumentAccess.objects.create( - document=document, - team="lasuite", - role=random.choice(models.RoleChoices.values), - ) - - for i in range(4): - document.content = f"after {i:d}" - document.save() - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/", - ) - - content = response.json() - assert content["is_truncated"] is False - # The current version is not listed - assert content["count"] == 3 - assert content["next_version_id_marker"] == "" - all_version_ids = [version["version_id"] for version in content["versions"]] - - # - set page size - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/?page_size=2", - ) - - content = response.json() - assert content["count"] == 2 - assert content["is_truncated"] is True - marker = content["next_version_id_marker"] - assert marker == all_version_ids[1] - assert [ - version["version_id"] for version in content["versions"] - ] == all_version_ids[:2] - - # - get page 2 - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/?page_size=2&version_id={marker:s}", - ) - - content = response.json() - assert content["count"] == 1 - assert content["is_truncated"] is False - assert content["next_version_id_marker"] == "" - assert content["versions"][0]["version_id"] == all_version_ids[2] - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_list_authenticated_related_pagination_parent( - via, mock_user_teams -): - """ - When a user gains access to a document's versions via an ancestor, the date of access - to the parent should be used to filter versions that were created prior to the - user gaining access to the document. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - grand_parent = create_document() - parent = create_document(parent=grand_parent) - document = create_document(parent=parent) - for i in range(3): - document.content = f"before {i:d}" - document.save() - - if via == USER: - models.DocumentAccess.objects.create( - document=grand_parent, - user=user, - role=random.choice(models.RoleChoices.values), - ) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - models.DocumentAccess.objects.create( - document=grand_parent, - team="lasuite", - role=random.choice(models.RoleChoices.values), - ) - - for i in range(4): - document.content = f"after {i:d}" - document.save() - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/", - ) - - content = response.json() - - assert response.status_code == 200 - assert content["is_truncated"] is False - # The current version is not listed - assert content["count"] == 3 - assert content["next_version_id_marker"] == "" - all_version_ids = [version["version_id"] for version in content["versions"]] - - # - set page size - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/?page_size=2", - ) - - content = response.json() - assert content["count"] == 2 - assert content["is_truncated"] is True - marker = content["next_version_id_marker"] - assert marker == all_version_ids[1] - assert [ - version["version_id"] for version in content["versions"] - ] == all_version_ids[:2] - - # - get page 2 - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/?page_size=2&version_id={marker:s}", - ) - - content = response.json() - assert content["count"] == 1 - assert content["is_truncated"] is False - assert content["next_version_id_marker"] == "" - assert content["versions"][0]["version_id"] == all_version_ids[2] - - -def test_api_document_versions_list_exceeds_max_page_size(): - """Page size should not exceed the limit set on the serializer""" - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document(users=[user]) - document.content = "version 2" - document.save() - - response = client.get(f"/api/v1.0/documents/{document.id!s}/versions/?page_size=51") - - assert response.status_code == 400 - assert response.json() == { - "page_size": ["Ensure this value is less than or equal to 50."] - } - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -def test_api_document_versions_retrieve_anonymous(reach): - """ - Anonymous users should not be allowed to find specific versions for a document with - restricted or authenticated link reach. - """ - document = create_document(link_reach=reach) - document.content = "new content" - document.save() - - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - url = f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/" - response = APIClient().get(url) - - assert response.status_code == 401 - assert response.json() == { - "detail": "Authentication credentials were not provided." - } - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -def test_api_document_versions_retrieve_authenticated_unrelated(reach): - """ - Authenticated users should not be allowed to retrieve specific versions for a - document to which they are not related. - """ - user = factories.UserFactory(with_owned_document=True) - - client = APIClient() - client.force_login(user) - - document = create_document(link_reach=reach) - document.content = "new content" - document.save() - - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - assert response.status_code == 403 - assert response.json() == { - "detail": "You do not have permission to perform this action." - } - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_retrieve_authenticated_related(via, mock_user_teams): - """ - A user who is related to a document should be allowed to retrieve the - associated document versions. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - document.content = "new content" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 1 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory(document=document, team="lasuite") - - time.sleep(1) # minio stores datetimes with the precision of a second - - # Versions created before the document was shared should not be seen by the user - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 404 - - # Create a new version should not make it available to the user because - # only the current version is available to the user but it is excluded - # from the list - document.content = "new content 1" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 2 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 404 - - # Adding one more version should make the previous version available to the user - document.content = "new content 2" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 3 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 200 - assert response.json()["content"] == "new content 1" - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_retrieve_authenticated_related_parent( - via, mock_user_teams -): - """ - A user who gains access to a document's versions via one of its ancestors, should be able to - retrieve the document versions. The date of access to the parent should be used to filter - versions that were created prior to the user gaining access to the document. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - grand_parent = create_document() - parent = create_document(parent=grand_parent) - document = create_document(parent=parent) - document.content = "new content" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 1 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - if via == USER: - factories.UserDocumentAccessFactory(document=grand_parent, user=user) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory(document=grand_parent, team="lasuite") - - time.sleep(1) # minio stores datetimes with the precision of a second - - # Versions created before the document was shared should not be seen by the user - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 404 - - # Create a new version should not make it available to the user because - # only the current version is available to the user but it is excluded - # from the list - document.content = "new content 1" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 2 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 404 - - # Adding one more version should make the previous version available to the user - document.content = "new content 2" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 3 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.get( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 200 - assert response.json()["content"] == "new content 1" - - -def test_api_document_versions_create_anonymous(): - """Anonymous users should not be allowed to create document versions.""" - document = create_document() - - response = APIClient().post( - f"/api/v1.0/documents/{document.id!s}/versions/", - {"foo": "bar"}, - format="json", - ) - - assert response.status_code == 405 - assert response.json() == {"detail": 'Method "POST" not allowed.'} - - -def test_api_document_versions_create_authenticated_unrelated(): - """ - Authenticated users should not be allowed to create document versions for a document to - which they are not related. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - - response = client.post( - f"/api/v1.0/documents/{document.id!s}/versions/", - {"foo": "bar"}, - format="json", - ) - - assert response.status_code == 405 - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_create_authenticated_related(via, mock_user_teams): - """ - Authenticated users related to a document should not be allowed to create document versions - whatever their role. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory(document=document, team="lasuite") - - response = client.post( - f"/api/v1.0/documents/{document.id!s}/versions/", - {"foo": "bar"}, - format="json", - ) - - assert response.status_code == 405 - - -def test_api_document_versions_update_anonymous(): - """Anonymous users should not be allowed to update a document version.""" - document = create_document() - factories.UserDocumentAccessFactory(document=document) - # a second version of the object: the first one is the latest, which the - # listing excludes - document.content = "new content" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 1 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = APIClient().put( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - {"foo": "bar"}, - format="json", - ) - assert response.status_code == 405 - - -def test_api_document_versions_update_authenticated_unrelated(): - """ - Authenticated users should not be allowed to update a document version for a document to which - they are not related. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - factories.UserDocumentAccessFactory(document=document) - # a second version of the object: the first one is the latest, which the - # listing excludes - document.content = "new content" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 1 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.put( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - {"foo": "bar"}, - format="json", - ) - assert response.status_code == 405 - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_update_authenticated_related(via, mock_user_teams): - """ - Authenticated users with access to a document should not be able to update its versions - whatever their role. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - - if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory(document=document, team="lasuite") - - time.sleep(1) # minio stores datetimes with the precision of a second - - document.content = "new content" - document.save() - - assert len(document.get_versions_slice()["versions"]) == 1 - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.put( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id!s}/", - {"foo": "bar"}, - format="json", - ) - assert response.status_code == 405 - - -# Delete - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -def test_api_document_versions_delete_anonymous(reach): - """Anonymous users should not be allowed to destroy a document version.""" - access = factories.UserDocumentAccessFactory(document__link_reach=reach) - - response = APIClient().delete( - f"/api/v1.0/documents/{access.document_id!s}/versions/{access.id!s}/", - ) - - assert response.status_code == 401 - assert response.json() == { - "detail": "Authentication credentials were not provided." - } - - -@pytest.mark.parametrize("reach", models.LinkReachChoices.values) -def test_api_document_versions_delete_authenticated(reach): - """ - Authenticated users should not be allowed to delete a document version for a - public document to which they are not related. - """ - user = factories.UserFactory(with_owned_document=True) - - client = APIClient() - client.force_login(user) - - document = create_document(link_reach=reach) - document.content = "new content" - document.save() - - version_id = document.get_versions_slice()["versions"][0]["version_id"] - - response = client.delete( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - - assert response.status_code == 403 - - -@pytest.mark.parametrize("role", ["reader", "editor"]) -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_delete_reader_or_editor(via, role, mock_user_teams): - """ - Authenticated users should not be allowed to delete a document version for a - document in which they are a simple reader or editor. - """ - user = factories.UserFactory(with_owned_document=True) - - client = APIClient() - client.force_login(user) - - document = create_document() - if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user, role=role) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory( - document=document, team="lasuite", role=role - ) - - # Create a new version should make it available to the user - time.sleep(1) # minio stores datetimes with the precision of a second - document.content = "new content" - document.save() - - versions = document.get_versions_slice()["versions"] - assert len(versions) == 1 - - version_id = versions[0]["version_id"] - response = client.delete( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - assert response.status_code == 403 - - versions = document.get_versions_slice()["versions"] - assert len(versions) == 1 - - -@pytest.mark.parametrize("via", VIA) -def test_api_document_versions_delete_administrator_or_owner(via, mock_user_teams): - """ - Users who are administrator or owner of a document should be allowed to delete a version. - """ - user = factories.UserFactory() - - client = APIClient() - client.force_login(user) - - document = create_document() - role = random.choice(["administrator", "owner"]) - if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user, role=role) - elif via == TEAM: - mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory( - document=document, team="lasuite", role=role - ) - - # Create a new version should make it available to the user - time.sleep(1) # minio stores datetimes with the precision of a second - document.content = "new content 1" - document.save() - - versions = document.get_versions_slice()["versions"] - assert len(versions) == 1 - - version_id = versions[0]["version_id"] - response = client.delete( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - # 404 because the version was created before the user was given access to the document - assert response.status_code == 404 - - document.content = "new content 2" - document.save() - - versions = document.get_versions_slice()["versions"] - assert len(versions) == 2 - - version_id = versions[0]["version_id"] - response = client.delete( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - assert response.status_code == 204 - - versions = document.get_versions_slice()["versions"] - assert len(versions) == 1 diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index 00a3504a7..bd6d0f697 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -61,9 +61,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "search": True, "tree": True, "update": document.link_role == "editor", - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": None, "ancestors_link_role": None, @@ -83,7 +81,6 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": None, "user_role": None, } @@ -138,9 +135,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "search": True, "tree": True, "update": grand_parent.link_role == "editor", - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": "public", "ancestors_link_role": grand_parent.link_role, @@ -160,7 +155,6 @@ def test_api_documents_retrieve_anonymous_public_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": None, "user_role": None, } @@ -248,9 +242,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "search": True, "tree": True, "update": document.link_role == "editor", - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": None, "ancestors_link_role": None, @@ -270,7 +262,6 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": None, "user_role": None, } assert ( @@ -332,9 +323,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "search": True, "tree": True, "update": grand_parent.link_role == "editor", - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": reach, "ancestors_link_role": grand_parent.link_role, @@ -354,7 +343,6 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": None, "user_role": None, } @@ -469,7 +457,6 @@ def test_api_documents_retrieve_authenticated_related_direct(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": access.created_at.isoformat().replace("+00:00", "Z"), "user_role": access.role, } @@ -531,9 +518,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "search": True, "tree": True, "update": access.role not in ["reader", "commenter"], - "versions_destroy": access.role in ["administrator", "owner"], "versions_list": True, - "versions_retrieve": True, }, "ancestors_link_reach": "restricted", "ancestors_link_role": None, @@ -553,7 +538,6 @@ def test_api_documents_retrieve_authenticated_related_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": access.created_at.isoformat().replace("+00:00", "Z"), "user_role": access.role, } @@ -684,14 +668,6 @@ def test_api_documents_retrieve_authenticated_related_team_members( factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() - # the history this user may read starts at the earliest access they hold — - # here through one of their teams - expected_access_since = ( - models.DocumentAccess.objects.filter(document=document, team__in=teams) - .earliest("created_at") - .created_at.isoformat() - .replace("+00:00", "Z") - ) response = client.get(f"/api/v1.0/documents/{document.id!s}/") @@ -718,7 +694,6 @@ def test_api_documents_retrieve_authenticated_related_team_members( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": expected_access_since, "user_role": role, } @@ -759,14 +734,6 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() - # the history this user may read starts at the earliest access they hold — - # here through one of their teams - expected_access_since = ( - models.DocumentAccess.objects.filter(document=document, team__in=teams) - .earliest("created_at") - .created_at.isoformat() - .replace("+00:00", "Z") - ) response = client.get(f"/api/v1.0/documents/{document.id!s}/") @@ -793,7 +760,6 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": expected_access_since, "user_role": role, } @@ -834,14 +800,6 @@ def test_api_documents_retrieve_authenticated_related_team_owners( factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() - # the history this user may read starts at the earliest access they hold — - # here through one of their teams - expected_access_since = ( - models.DocumentAccess.objects.filter(document=document, team__in=teams) - .earliest("created_at") - .created_at.isoformat() - .replace("+00:00", "Z") - ) response = client.get(f"/api/v1.0/documents/{document.id!s}/") @@ -868,7 +826,6 @@ def test_api_documents_retrieve_authenticated_related_team_owners( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_access_since": expected_access_since, "user_role": role, } @@ -1086,58 +1043,3 @@ def test_api_documents_retrieve_permanently_deleted_related(role, depth): assert response.status_code == 404 assert response.json() == {"detail": "Not found."} - - -def test_api_documents_retrieve_user_access_since_is_ancestor_aware(): - """ - `user_access_since` is the earliest access the user holds on the document or on any of - its ancestors. It is what bounds the history they may read — the collaboration server - turns it into `history.from` — so a later access on the document itself must not shorten - what an earlier one on a parent already gave them. - """ - user = factories.UserFactory() - client = APIClient() - client.force_login(user) - - parent = factories.DocumentFactory(link_reach="restricted") - child = factories.DocumentFactory(parent=parent, link_reach="restricted") - - ten_days_ago = timezone.now() - timedelta(days=10) - with mock.patch("django.utils.timezone.now", return_value=ten_days_ago): - parent_access = factories.UserDocumentAccessFactory( - document=parent, user=user, role="reader" - ) - # granted later, and deliberately the stronger role: recency must not win - factories.UserDocumentAccessFactory(document=child, user=user, role="editor") - - response = client.get(f"/api/v1.0/documents/{child.id!s}/") - - assert response.status_code == 200 - expected = parent_access.created_at.isoformat().replace("+00:00", "Z") - assert response.json()["user_access_since"] == expected - - -def test_api_documents_retrieve_user_access_since_is_null_without_an_access(): - """ - A user who reaches a document through its link reach alone holds no access, so there is - no date to bound their history with — and they get none at all. This is the reason the - version endpoints have always refused them, and the collaboration server withholds the - `history` facet on the same grounds. - """ - document = factories.DocumentFactory(link_reach="public", link_role="editor") - - # anonymous - assert ( - APIClient() - .get(f"/api/v1.0/documents/{document.id!s}/") - .json()["user_access_since"] - is None - ) - - # signed in, but still reaching it only through the link - client = APIClient() - client.force_login(factories.UserFactory()) - response = client.get(f"/api/v1.0/documents/{document.id!s}/") - - assert response.status_code == 200 - assert response.json()["user_access_since"] is None diff --git a/src/backend/core/tests/documents/test_api_documents_trashbin.py b/src/backend/core/tests/documents/test_api_documents_trashbin.py index 3a9caafec..5e87a738c 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -110,9 +110,7 @@ def test_api_documents_trashbin_format(): "search": False, "tree": True, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": None, "ancestors_link_role": None, @@ -175,9 +173,7 @@ def test_api_documents_trashbin_format(): "search": False, "tree": True, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, }, "ancestors_link_reach": None, "ancestors_link_role": None, diff --git a/src/backend/core/tests/external_api/test_external_api_documents_versions.py b/src/backend/core/tests/external_api/test_external_api_documents_versions.py deleted file mode 100644 index 5c460bd3f..000000000 --- a/src/backend/core/tests/external_api/test_external_api_documents_versions.py +++ /dev/null @@ -1,163 +0,0 @@ -""" -Tests for the Resource Server API for document versions. - -Not testing external API endpoints that are already tested in the /api -because the resource server viewsets inherit from the api viewsets. - -""" - -import time - -from django.test import override_settings - -import pytest -from rest_framework.test import APIClient - -from core import factories, models - -pytestmark = pytest.mark.django_db - -# pylint: disable=unused-argument - - -def test_external_api_documents_versions_list_not_allowed( - user_token, resource_server_backend, user_specific_sub -): - """ - Connected users SHOULD NOT be allowed to list the versions of a document - from a resource server by default. - """ - - client = APIClient() - client.credentials(HTTP_AUTHORIZATION=f"Bearer {user_token}") - - document = factories.DocumentFactory( - link_reach=models.LinkReachChoices.RESTRICTED, - creator=user_specific_sub, - ) - factories.UserDocumentAccessFactory( - document=document, - user=user_specific_sub, - role=models.RoleChoices.OWNER, - ) - - response = client.get(f"/external_api/v1.0/documents/{document.id!s}/versions/") - - assert response.status_code == 403 - - -def test_external_api_documents_versions_detail_not_allowed( - user_token, resource_server_backend, user_specific_sub -): - """ - Connected users SHOULD NOT be allowed to retrieve a specific version of a document - from a resource server by default. - """ - client = APIClient() - client.credentials(HTTP_AUTHORIZATION=f"Bearer {user_token}") - - document = factories.DocumentFactory(link_reach=models.LinkReachChoices.RESTRICTED) - factories.UserDocumentAccessFactory( - document=document, user=user_specific_sub, role=models.RoleChoices.OWNER - ) - - response = client.get( - f"/external_api/v1.0/documents/{document.id!s}/versions/1234/" - ) - - assert response.status_code == 403 - - -# Overrides - - -@override_settings( - EXTERNAL_API={ - "documents": { - "enabled": True, - "actions": ["list", "retrieve", "children", "versions_list"], - }, - } -) -def test_external_api_documents_versions_list_can_be_allowed( - user_token, resource_server_backend, user_specific_sub -): - """ - Connected users SHOULD be allowed to list version of a document from a resource server - when the versions action is enabled in EXTERNAL_API settings. - """ - client = APIClient() - client.credentials(HTTP_AUTHORIZATION=f"Bearer {user_token}") - - document = factories.DocumentFactory(link_reach=models.LinkReachChoices.RESTRICTED) - factories.UserDocumentAccessFactory( - document=document, user=user_specific_sub, role=models.RoleChoices.OWNER - ) - - # Add new versions to the document - for i in range(3): - document.content = f"new content {i:d}" - document.save() - - response = client.get(f"/external_api/v1.0/documents/{document.id!s}/versions/") - - assert response.status_code == 200 - - content = response.json() - assert content["count"] == 2 - - -@override_settings( - EXTERNAL_API={ - "documents": { - "enabled": True, - "actions": [ - "list", - "retrieve", - "children", - "versions_list", - "versions_detail", - ], - }, - } -) -def test_external_api_documents_versions_detail_can_be_allowed( - user_token, resource_server_backend, user_specific_sub -): - """ - Connected users SHOULD be allowed to retrieve a specific version of a document - from a resource server when the versions_detail action is enabled. - """ - client = APIClient() - client.credentials(HTTP_AUTHORIZATION=f"Bearer {user_token}") - - document = factories.DocumentFactory(link_reach=models.LinkReachChoices.RESTRICTED) - factories.UserDocumentAccessFactory( - document=document, user=user_specific_sub, role=models.RoleChoices.OWNER - ) - - # ensure access datetime is earlier than versions (minio precision is one second) - time.sleep(1) - - # create several versions, spacing them out to get distinct LastModified values - for i in range(3): - document.content = f"new content {i:d}" - document.save() - time.sleep(1) - - # call the list endpoint and verify basic structure - response = client.get(f"/external_api/v1.0/documents/{document.id!s}/versions/") - assert response.status_code == 200 - - content = response.json() - # count should reflect two saved versions beyond the original - assert content.get("count") == 2 - - # pick the first version returned by the list (should be accessible) - version_id = content.get("versions")[0]["version_id"] - - detailed_response = client.get( - f"/external_api/v1.0/documents/{document.id!s}/versions/{version_id}/" - ) - assert detailed_response.status_code == 200 - assert detailed_response.json()["content"] == "new content 1" diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index ba3269926..b75304b91 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -185,9 +185,7 @@ def test_models_documents_get_abilities_forbidden( "retrieve": False, "tree": False, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, "search": False, } nb_queries = 2 if is_authenticated else 0 @@ -251,9 +249,7 @@ def test_models_documents_get_abilities_reader( "retrieve": True, "tree": True, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, "search": True, } nb_queries = 2 if is_authenticated else 0 @@ -322,9 +318,7 @@ def test_models_documents_get_abilities_commenter( "retrieve": True, "tree": True, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, "search": True, } nb_queries = 2 if is_authenticated else 0 @@ -390,9 +384,7 @@ def test_models_documents_get_abilities_editor( "retrieve": True, "tree": True, "update": True, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, "search": True, } nb_queries = 2 if is_authenticated else 0 @@ -447,9 +439,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): "retrieve": True, "tree": True, "update": True, - "versions_destroy": True, "versions_list": True, - "versions_retrieve": True, "search": True, } with django_assert_num_queries(1): @@ -490,9 +480,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): "retrieve": True, "tree": True, "update": False, - "versions_destroy": False, "versions_list": False, - "versions_retrieve": False, "search": False, } @@ -537,9 +525,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) "retrieve": True, "tree": True, "update": True, - "versions_destroy": True, "versions_list": True, - "versions_retrieve": True, "search": True, } with django_assert_num_queries(1): @@ -594,9 +580,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): "retrieve": True, "tree": True, "update": True, - "versions_destroy": False, "versions_list": True, - "versions_retrieve": True, "search": True, } with django_assert_num_queries(1): @@ -659,9 +643,7 @@ def test_models_documents_get_abilities_reader_user( "retrieve": True, "tree": True, "update": access_from_link, - "versions_destroy": False, "versions_list": True, - "versions_retrieve": True, "search": True, } @@ -725,9 +707,7 @@ def test_models_documents_get_abilities_commenter_user( "retrieve": True, "tree": True, "update": access_from_link, - "versions_destroy": False, "versions_list": True, - "versions_retrieve": True, "search": True, } @@ -787,9 +767,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): "retrieve": True, "tree": True, "update": False, - "versions_destroy": False, "versions_list": True, - "versions_retrieve": True, "search": True, } @@ -930,71 +908,6 @@ def test_models_document_get_abilities_ai_access_public(is_authenticated, reach) assert abilities["ai_translate"] == is_authenticated -def test_models_documents_get_versions_slice_pagination(settings): - """ - The "get_versions_slice" method should allow navigating all versions of - the document with pagination. - """ - settings.DOCUMENT_VERSIONS_PAGE_SIZE = 4 - - # Create a document with 7 versions - document = factories.DocumentFactory(content=factories.YDOC_HELLO_WORLD_BASE64) - for i in range(6): - document.content = f"bar{i:d}" - document.save() - - # Add a document version not related to the first document - factories.DocumentFactory() - - # - Get default max versions - response = document.get_versions_slice() - assert response["is_truncated"] is True - assert len(response["versions"]) == 4 - assert response["next_version_id_marker"] != "" - - expected_keys = ["etag", "is_latest", "last_modified", "version_id"] - for i in range(4): - assert list(response["versions"][i].keys()) == expected_keys - - # - Get page 2 - response = document.get_versions_slice( - from_version_id=response["next_version_id_marker"] - ) - assert response["is_truncated"] is False - assert len(response["versions"]) == 2 - assert response["next_version_id_marker"] == "" - - # - Get custom max versions - response = document.get_versions_slice(page_size=2) - assert response["is_truncated"] is True - assert len(response["versions"]) == 2 - assert response["next_version_id_marker"] != "" - - -def test_models_documents_get_versions_slice_min_datetime(): - """ - The "get_versions_slice" method should filter out versions anterior to - the from_datetime passed in argument and the current version. - """ - document = factories.DocumentFactory() - from_dt = [] - for i in range(6): - from_dt.append(timezone.now()) - document.content = f"bar{i:d}" - document.save() - - response = document.get_versions_slice(min_datetime=from_dt[2]) - - assert len(response["versions"]) == 3 - for version in response["versions"]: - assert version["last_modified"] > from_dt[2] - - response = document.get_versions_slice(min_datetime=from_dt[4]) - - assert len(response["versions"]) == 1 - assert response["versions"][0]["last_modified"] > from_dt[4] - - def test_models_documents_version_duplicate(): """A new version should be created in object storage only if the content has changed.""" document = factories.DocumentFactory(content=factories.YDOC_HELLO_WORLD_BASE64) diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index 72653e086..9e102450e 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -287,9 +287,6 @@ class Base(Configuration): environ_name="DOCUMENT_ATTACHMENT_CHECK_UNSAFE_MIME_TYPES_ENABLED", environ_prefix=None, ) - # Document versions - DOCUMENT_VERSIONS_PAGE_SIZE = 50 - # Document /all endpoint DOCUMENT_ALL_ENDPOINT_ENABLED = values.BooleanValue( default=True, diff --git a/src/backend/pyproject.toml b/src/backend/pyproject.toml index 1d2e4013a..bfe6abafe 100644 --- a/src/backend/pyproject.toml +++ b/src/backend/pyproject.toml @@ -100,6 +100,9 @@ dev = [ "types-requests==2.33.0.20260712", ] +[tool.basedpyright] +typeCheckingMode = "standard" + [tool.uv.build-backend] module-name = [ "core", diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts index 8cb102d10..2eb3bcd60 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts @@ -461,9 +461,7 @@ const data = [ }, tree: true, update: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, }, created_at: '2025-03-14T14:45:22.527221Z', creator: 'bc6895e0-8f6d-4b00-827d-c143aa6b2ecb', @@ -510,9 +508,7 @@ const data = [ }, tree: true, update: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, }, created_at: '2025-03-14T14:45:22.527221Z', creator: 'bc6895e0-8f6d-4b00-827d-c143aa6b2ecb', @@ -558,9 +554,7 @@ const data = [ }, tree: true, update: false, - versions_destroy: false, versions_list: true, - versions_retrieve: true, }, created_at: '2025-03-14T14:44:16.032773Z', creator: '9264f420-f018-4bd6-96ae-4788f41af56d', diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-grid.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-grid.spec.ts index ba627e475..0118076fa 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-grid.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-grid.spec.ts @@ -59,9 +59,7 @@ test.describe('Documents Grid mobile', () => { partial_update: true, retrieve: true, update: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, }, link_role: 'reader', link_reach: 'public', diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts index 467fd9ff4..16c99fe10 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts @@ -288,9 +288,7 @@ test.describe('Doc Header', () => { accesses_view: true, destroy: false, // Means not owner link_configuration: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, update: true, partial_update: true, retrieve: true, @@ -358,9 +356,7 @@ test.describe('Doc Header', () => { accesses_view: true, destroy: false, // Means not owner link_configuration: false, - versions_destroy: true, versions_list: true, - versions_retrieve: true, update: true, partial_update: true, // Means editor retrieve: true, @@ -428,9 +424,7 @@ test.describe('Doc Header', () => { accesses_view: true, destroy: false, // Means not owner link_configuration: false, - versions_destroy: false, versions_list: true, - versions_retrieve: true, update: false, partial_update: false, // Means not editor retrieve: true, @@ -499,9 +493,7 @@ test.describe('Doc Header', () => { abilities: { destroy: false, // Means owner link_configuration: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, accesses_manage: false, accesses_view: false, update: true, diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts index c99ae5f37..ef566777b 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts @@ -63,9 +63,7 @@ test.describe('Doc Tree', () => { }, tree: true, update: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, search: true, }, ancestors_link_reach: 'restricted', diff --git a/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts index 0644acbb0..b9245c95e 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts @@ -669,9 +669,7 @@ test.describe('Presenter Mode mobile', () => { abilities: { destroy: true, link_configuration: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, accesses_manage: true, accesses_view: true, update: true, diff --git a/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts b/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts index 2f0562a2f..69ef41296 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts @@ -315,9 +315,7 @@ export const mockedDocument = async (page: Page, data: object) => { abilities: { destroy: false, // Means not owner link_configuration: false, - versions_destroy: false, versions_list: true, - versions_retrieve: true, accesses_manage: false, // Means not admin update: false, partial_update: false, // Means not editor diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx index ec1309978..0a6431a90 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx @@ -98,9 +98,7 @@ export interface Doc { retrieve: boolean; search: boolean; update: boolean; - versions_destroy: boolean; versions_list: boolean; - versions_retrieve: boolean; }; } diff --git a/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts b/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts index 935a21526..31f5453a9 100644 --- a/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts +++ b/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts @@ -229,9 +229,7 @@ export class ApiPlugin implements WorkboxPlugin { retrieve: true, search: true, update: true, - versions_destroy: true, versions_list: true, - versions_retrieve: true, link_select_options: { public: [LinkRole.READER, LinkRole.EDITOR], authenticated: [LinkRole.READER, LinkRole.EDITOR], diff --git a/src/yhub-server/permissions.js b/src/yhub-server/permissions.js index 995c980fc..3ab013150 100644 --- a/src/yhub-server/permissions.js +++ b/src/yhub-server/permissions.js @@ -30,14 +30,15 @@ * first write. * * `historyFrom` is the moment this user gained access to the document, in unix - * milliseconds — the backend's `user_access_since`, which is the earliest access - * they hold on the document or on one of its ancestors. It becomes the start of - * the history they may read, which is the rule Docs has always had rather than a - * new one: the version endpoints have always shown "only those created after the - * user got access to the document". yhub clamps `from` up to this on every - * changeset/activity read, so a client asks for whatever range it likes and gets - * back only its own share — it never has to know the bound, and a stale or - * modified one cannot widen it. + * milliseconds — the `created_at` of the earliest access they hold on the + * document or on one of its ancestors, which the backend serves as + * `GET /accesses/me/` and `resolveHistoryFrom` below turns into this number. It + * becomes the start of the history they may read, which is the rule Docs has + * always had rather than a new one: the version endpoints have always shown + * "only those created after the user got access to the document". yhub clamps + * `from` up to this on every changeset/activity read, so a client asks for + * whatever range it likes and gets back only its own share — it never has to + * know the bound, and a stale or modified one cannot widen it. * * `null` for a reader who reaches the document by link alone. There is no access * row and so no date, and the backend has always refused those users their @@ -133,3 +134,49 @@ export const publicGlobalPermissions = { type: 'permissions:global:v1', endpoint: { ping: '-r--', ready: '-r--', jwks: '-r--' }, }; + +/** + * Where the history this caller may read starts, in unix milliseconds, or `null` + * when they get none — the `historyFrom` argument of `browserDocumentPermissions` + * above. + * + * Only a user holding an access has a history at all, and `abilities.versions_list` + * is exactly that condition (`has_access_role` on the backend, which is also false + * on a deleted document). So the access is fetched only when it is granted, and a + * link-reach reader — who holds no access and so has no date — costs no extra + * request on their way in. + * + * `fetchAccess` is called only in that case and must resolve the backend's + * `GET /api/v1.0/documents/{id}/accesses/me/` payload; it is passed in rather than + * built here so this stays testable without a backend. + * + * Two ways this ends with no history rather than with a date. The backend refusing + * (401/403/404) means the abilities and the access disagree — a race with a + * revocation, most likely — and the safe reading of that is no history. And + * anything unparseable is *no* history rather than full history, zero refused with + * it: `from: 0` is the one value that also unlocks a `gc=false` websocket, and no + * real access date is ever zero, so a zero here could only ever be a bug upstream. + * + * A backend that did not answer at all is not a permission decision and is not + * turned into one: the error is rethrown, and the caller reports it as retryable. + * Silently dropping the history there would cost the connection its version panel + * for as long as it lives, on a blip. + */ +export const resolveHistoryFrom = async (abilities, fetchAccess) => { + if (abilities?.versions_list !== true) { + return null; + } + + let access; + try { + access = await fetchAccess(); + } catch (err) { + if (err?.status === 401 || err?.status === 403 || err?.status === 404) { + return null; + } + throw err; + } + + const from = Date.parse(access?.created_at ?? ''); + return Number.isFinite(from) && from > 0 ? from : null; +}; diff --git a/src/yhub-server/permissions.test.js b/src/yhub-server/permissions.test.js index 1a505ceac..3d23ebe18 100644 --- a/src/yhub-server/permissions.test.js +++ b/src/yhub-server/permissions.test.js @@ -17,6 +17,7 @@ import { adminDocumentPermissions, browserDocumentPermissions, publicGlobalPermissions, + resolveHistoryFrom, } from './permissions.js'; /** @@ -317,3 +318,118 @@ describe('the public global routes', () => { assert.equal(globalGrants({ endpoint: { anything: '-r--' } }), false); }); }); + +/** + * Where a caller's history starts, from the backend's answers about them. This is + * the one input `browserDocumentPermissions` cannot check for itself: every + * assertion above takes `historyFrom` as given, and this is what decides it. + * + * The `status`-carrying errors are `backendFetch`'s, which is the only thing that + * ever rejects the callback in server.js. + */ +describe('resolveHistoryFrom', () => { + const httpError = (status) => Object.assign(new Error(`HTTP ${status}`), { + status, + }); + const never = () => { + throw new Error('the access must not be fetched'); + }; + + it('does not ask for an access the caller has no history to bound', async () => { + // a link-reach reader: no access, no date, and no extra request on their way in + assert.equal(await resolveHistoryFrom({ versions_list: false }, never), null); + assert.equal(await resolveHistoryFrom({}, never), null); + assert.equal(await resolveHistoryFrom(undefined, never), null); + }); + + it('turns the access date into unix milliseconds', async () => { + const from = await resolveHistoryFrom({ versions_list: true }, async () => ({ + created_at: '2023-11-14T22:13:20Z', + })); + + assert.equal(from, Date.parse('2023-11-14T22:13:20Z')); + assert.equal(from, ACCESS_SINCE); + }); + + it('grants no history when the backend refuses the access', async () => { + // abilities and access disagree — a revocation racing this connection + for (const status of [401, 403, 404]) { + assert.equal( + await resolveHistoryFrom({ versions_list: true }, async () => { + throw httpError(status); + }), + null, + ); + } + }); + + it('rethrows when the backend does not answer at all', async () => { + // not a permission decision: the caller turns this into a retryable 503 + for (const status of [500, 502, 503]) { + await assert.rejects( + resolveHistoryFrom({ versions_list: true }, async () => { + throw httpError(status); + }), + { status }, + ); + } + + await assert.rejects( + resolveHistoryFrom({ versions_list: true }, async () => { + throw new TypeError('fetch failed'); + }), + TypeError, + ); + }); + + it('grants no history rather than all of it on an unusable date', async () => { + for (const created_at of [undefined, null, '', 'not a date']) { + assert.equal( + await resolveHistoryFrom({ versions_list: true }, async () => ({ + created_at, + })), + null, + ); + } + + // and on no payload at all + assert.equal( + await resolveHistoryFrom({ versions_list: true }, async () => undefined), + null, + ); + }); + + it('refuses the epoch, which would unlock a gc=false socket', async () => { + // `from: 0` is the one value that also opens the ungarbage-collected + // connection; no real access date is ever zero + assert.equal( + await resolveHistoryFrom({ versions_list: true }, async () => ({ + created_at: '1970-01-01T00:00:00Z', + })), + null, + ); + }); + + it('answers with a bound a reader is actually held to', async () => { + // the round trip: what this returns is what the tables above are given + const from = await resolveHistoryFrom({ versions_list: true }, async () => ({ + created_at: new Date(ACCESS_SINCE).toISOString(), + })); + const perms = normalizePermissions(browserDocumentPermissions(false, from)); + + assert.equal( + hasPermissions(perms, { + type: 'permissions:document:v1', + history: { from: ACCESS_SINCE }, + }), + true, + ); + assert.equal( + hasPermissions(perms, { + type: 'permissions:document:v1', + history: { from: ACCESS_SINCE - 1 }, + }), + false, + ); + }); +}); diff --git a/src/yhub-server/server.js b/src/yhub-server/server.js index 1d27d3af0..adeb25e5a 100644 --- a/src/yhub-server/server.js +++ b/src/yhub-server/server.js @@ -26,6 +26,7 @@ import { adminDocumentPermissions, browserDocumentPermissions, publicGlobalPermissions, + resolveHistoryFrom, } from './permissions.js'; // legacy Django/S3 document store — see migration.js and README.md import { @@ -454,19 +455,20 @@ const auth = createAuthPlugin({ if (SOFT_MIGRATION) { await seedFromLegacyStore({ org, docid, branch }); } + // When this caller was given access, which is where the history they may - // read starts. The backend sends ISO-8601 (null for a link-reach reader, - // who holds no access and so has no date); `history.from` is unix ms. - // - // Anything unparseable is *no* history rather than full history, and zero - // is refused with it: `from: 0` is the one value that also unlocks a - // `gc=false` websocket, and no real access date is ever zero, so a zero - // here could only ever be a bug upstream. - const accessSince = Date.parse(doc.user_access_since ?? ''); - return browserDocumentPermissions( - doc.abilities.update === true, - Number.isFinite(accessSince) && accessSince > 0 ? accessSince : null, - ); + // read starts. `resolveHistoryFrom` decides whether to ask for it at all + // and what an unusable answer means — see permissions.js. + let accessSince; + try { + accessSince = await resolveHistoryFrom(doc.abilities, () => + backendFetch(`/api/v1.0/documents/${docid}/accesses/me/`, user), + ); + } catch { + // the backend did not answer; same treatment as the document fetch above + throw apiError(503, 'Document authorization backend is unavailable'); + } + return browserDocumentPermissions(doc.abilities.update === true, accessSince); }, async global() { return publicGlobalPermissions;