From 4de03d292a9d5318a31247515802229c2364ede9 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Thu, 2 Jan 2025 23:15:03 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20API=20endpoint=20to?= =?UTF-8?q?=20move=20a=20document=20in=20the=20document=20tree?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only administrators or owners of a document can move it to a target document for which they are also administrator or owner. We allow different moving modes: - first-child: move the document as the first child of the target - last-child: move the document as the last child of the target - first-sibling: move the document as the first sibling of the target - last-sibling: move the document as the last sibling of the target - left: move the document as sibling ordered just before the target - right: move the document as sibling ordered just after the target The whole subtree below the document that is being moved, moves as well and remains below the document after it is moved. --- src/backend/core/api/serializers.py | 34 ++ src/backend/core/api/viewsets.py | 60 ++++ src/backend/core/enums.py | 12 + src/backend/core/models.py | 1 + .../documents/test_api_documents_move.py | 339 ++++++++++++++++++ .../documents/test_api_documents_retrieve.py | 8 +- .../core/tests/test_models_documents.py | 8 + .../demo/tests/test_commands_create_demo.py | 8 +- 8 files changed, 466 insertions(+), 4 deletions(-) create mode 100644 src/backend/core/tests/documents/test_api_documents_move.py diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 556f209e..c16ecd4d 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -568,3 +568,37 @@ class AITranslateSerializer(serializers.Serializer): if len(value.strip()) == 0: raise serializers.ValidationError("Text field cannot be empty.") return value + + +class MoveDocumentSerializer(serializers.Serializer): + """ + Serializer for validating input data to move a document within the tree structure. + + Fields: + - target_document_id (UUIDField): The ID of the target parent document where the + document should be moved. This field is required and must be a valid UUID. + - position (ChoiceField): Specifies the position of the document in relation to + the target parent's children. + Choices: + - "first-child": Place the document as the first child of the target parent. + - "last-child": Place the document as the last child of the target parent (default). + - "left": Place the document as the left sibling of the target parent. + - "right": Place the document as the right sibling of the target parent. + + Example: + Input payload for moving a document: + { + "target_document_id": "123e4567-e89b-12d3-a456-426614174000", + "position": "first-child" + } + + Notes: + - The `target_document_id` is mandatory. + - The `position` defaults to "last-child" if not provided. + """ + + target_document_id = serializers.UUIDField(required=True) + position = serializers.ChoiceField( + choices=enums.MoveNodePositionChoices.choices, + default=enums.MoveNodePositionChoices.LAST_CHILD, + ) diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 9087c95d..e34b4695 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -311,6 +311,7 @@ class DocumentMetadata(drf.metadata.SimpleMetadata): return simple_metadata +# pylint: disable=too-many-public-methods class DocumentViewSet( drf.mixins.CreateModelMixin, drf.mixins.DestroyModelMixin, @@ -490,6 +491,65 @@ class DocumentViewSet( {"id": str(document.id)}, status=status.HTTP_201_CREATED ) + @drf.decorators.action(detail=True, methods=["post"]) + @transaction.atomic + def move(self, request, *args, **kwargs): + """ + Move a document to another location within the document tree. + + The user must be an administrator or owner of both the document being moved + and the target parent document. + """ + user = request.user + document = self.get_object() # including permission checks + + # Validate the input payload + serializer = serializers.MoveDocumentSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + validated_data = serializer.validated_data + + target_document_id = validated_data["target_document_id"] + try: + target_document = models.Document.objects.get( + id=target_document_id, ancestors_deleted_at__isnull=True + ) + except models.Document.DoesNotExist: + return drf.response.Response( + {"target_document_id": "Target parent document does not exist."}, + status=status.HTTP_400_BAD_REQUEST, + ) + + position = validated_data["position"] + message = None + + if position in [ + enums.MoveNodePositionChoices.FIRST_CHILD, + enums.MoveNodePositionChoices.LAST_CHILD, + ]: + if not target_document.get_abilities(user).get("move"): + message = ( + "You do not have permission to move documents " + "as a child to this target document." + ) + elif not target_document.is_root(): + if not target_document.get_parent().get_abilities(user).get("move"): + message = ( + "You do not have permission to move documents " + "as a sibling of this target document." + ) + + if message: + return drf.response.Response( + {"target_document_id": message}, + status=status.HTTP_400_BAD_REQUEST, + ) + + document.move(target_document, pos=position) + + return drf.response.Response( + {"message": "Document moved successfully."}, status=status.HTTP_200_OK + ) + @drf.decorators.action( detail=True, methods=["get", "post"], diff --git a/src/backend/core/enums.py b/src/backend/core/enums.py index 8f7e70cf..592a84a4 100644 --- a/src/backend/core/enums.py +++ b/src/backend/core/enums.py @@ -3,6 +3,7 @@ Core application enums declaration """ from django.conf import global_settings +from django.db import models from django.utils.translation import gettext_lazy as _ # In Django's code base, `LANGUAGES` is set by default with all supported languages. @@ -10,3 +11,14 @@ from django.utils.translation import gettext_lazy as _ # active in the app. # pylint: disable=no-member ALL_LANGUAGES = {language: _(name) for language, name in global_settings.LANGUAGES} + + +class MoveNodePositionChoices(models.TextChoices): + """Defines the possible positions when moving a django-treebeard node.""" + + FIRST_CHILD = "first-child", _("First child") + LAST_CHILD = "last-child", _("Last child") + FIRST_SIBLING = "first-sibling", _("First sibling") + LAST_SIBLING = "last-sibling", _("Last sibling") + LEFT = "left", _("Left") + RIGHT = "right", _("Right") diff --git a/src/backend/core/models.py b/src/backend/core/models.py index ec75922b..265b3314 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -592,6 +592,7 @@ class Document(MP_Node, BaseModel): "favorite": can_get and user.is_authenticated, "link_configuration": is_owner_or_admin, "invite_owner": RoleChoices.OWNER in roles, + "move": is_owner_or_admin, "partial_update": can_update, "retrieve": can_get, "media_auth": can_get, diff --git a/src/backend/core/tests/documents/test_api_documents_move.py b/src/backend/core/tests/documents/test_api_documents_move.py new file mode 100644 index 00000000..d093e491 --- /dev/null +++ b/src/backend/core/tests/documents/test_api_documents_move.py @@ -0,0 +1,339 @@ +""" +Test moving documents within the document tree via an detail action API endpoint. +""" + +import random +from uuid import uuid4 + +from django.utils import timezone + +import pytest +from rest_framework.test import APIClient + +from core import enums, factories, models + +pytestmark = pytest.mark.django_db + + +def test_api_documents_move_anonymous_user(): + """Anonymous users should not be able to move documents.""" + document = factories.DocumentFactory() + target = factories.DocumentFactory() + + response = APIClient().post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id)}, + ) + + assert response.status_code == 401 + assert response.json() == { + "detail": "Authentication credentials were not provided." + } + + +@pytest.mark.parametrize("role", [None, "reader", "editor"]) +def test_api_documents_move_authenticated_document_no_permission(role): + """ + Authenticated users should not be able to move documents with insufficient + permissions on the origin document. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + target = factories.UserDocumentAccessFactory(user=user, role="owner").document + + if role: + factories.UserDocumentAccessFactory(document=document, user=user, role=role) + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id)}, + ) + + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + +def test_api_documents_move_invalid_target_string(): + """Test for moving a document to an invalid target as a random string.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.UserDocumentAccessFactory(user=user, role="owner").document + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": "non-existent-id"}, + ) + + assert response.status_code == 400 + assert response.json() == {"target_document_id": ["Must be a valid UUID."]} + + +def test_api_documents_move_invalid_target_uuid(): + """Test for moving a document to an invalid target that looks like a UUID.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.UserDocumentAccessFactory(user=user, role="owner").document + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(uuid4())}, + ) + + assert response.status_code == 400 + assert response.json() == { + "target_document_id": "Target parent document does not exist." + } + + +def test_api_documents_move_invalid_position(): + """Test moving a document to an invalid position.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.UserDocumentAccessFactory(user=user, role="owner").document + target = factories.UserDocumentAccessFactory(user=user, role="owner").document + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={ + "target_document_id": str(target.id), + "position": "invalid-position", + }, + ) + + assert response.status_code == 400 + assert response.json() == { + "position": ['"invalid-position" is not a valid choice.'] + } + + +@pytest.mark.parametrize("position", enums.MoveNodePositionChoices.values) +@pytest.mark.parametrize("target_parent_role", models.RoleChoices.values) +@pytest.mark.parametrize("target_role", models.RoleChoices.values) +def test_api_documents_move_authenticated_target_roles_mocked( + target_role, target_parent_role, position +): + """ + Authenticated users with insufficient permissions on the target document (or its + parent depending on the position chosen), should not be allowed to move documents. + """ + + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + power_roles = ["administrator", "owner"] + + document = factories.DocumentFactory(users=[(user, random.choice(power_roles))]) + children = factories.DocumentFactory.create_batch(3, parent=document) + + target_parent = factories.DocumentFactory(users=[(user, target_parent_role)]) + sibling1, target, sibling2 = factories.DocumentFactory.create_batch( + 3, parent=target_parent + ) + models.DocumentAccess.objects.create(document=target, user=user, role=target_role) + target_children = factories.DocumentFactory.create_batch(2, parent=target) + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": position}, + ) + + document.refresh_from_db() + + if ( + position in ["first-child", "last-child"] + and (target_role in power_roles or target_parent_role in power_roles) + ) or ( + position in ["first-sibling", "last-sibling", "left", "right"] + and target_parent_role in power_roles + ): + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + match position: + case "first-child": + assert list(target.get_children()) == [document, *target_children] + case "last-child": + assert list(target.get_children()) == [*target_children, document] + case "first-sibling": + assert list(target.get_siblings()) == [ + document, + sibling1, + target, + sibling2, + ] + case "last-sibling": + assert list(target.get_siblings()) == [ + sibling1, + target, + sibling2, + document, + ] + case "left": + assert list(target.get_siblings()) == [ + sibling1, + document, + target, + sibling2, + ] + case "right": + assert list(target.get_siblings()) == [ + sibling1, + target, + document, + sibling2, + ] + case _: + raise ValueError(f"Invalid position: {position}") + + # Verify that the document's children have also been moved + assert list(document.get_children()) == children + else: + assert response.status_code == 400 + assert ( + "You do not have permission to move documents" + in response.json()["target_document_id"] + ) + assert document.is_root() is True + + +def test_api_documents_move_authenticated_deleted_document(): + """ + It should not be possible to move a deleted document or its descendants, even + for an owner. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + users=[(user, "owner")], deleted_at=timezone.now() + ) + child = factories.DocumentFactory(parent=document, users=[(user, "owner")]) + + target = factories.DocumentFactory(users=[(user, "owner")]) + + # Try moving the deleted document + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id)}, + ) + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + # Verify that the document has not moved + document.refresh_from_db() + assert document.is_root() is True + + # Try moving the child of the deleted document + response = client.post( + f"/api/v1.0/documents/{child.id!s}/move/", + data={"target_document_id": str(target.id)}, + ) + assert response.status_code == 403 + assert response.json() == { + "detail": "You do not have permission to perform this action." + } + + # Verify that the child has not moved + child.refresh_from_db() + assert child.is_child_of(document) is True + + +@pytest.mark.parametrize( + "position", + enums.MoveNodePositionChoices.values, +) +def test_api_documents_move_authenticated_deleted_target_as_child(position): + """ + It should not be possible to move a document as a child of a deleted target + even for a owner. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(users=[(user, "owner")]) + + target = factories.DocumentFactory( + users=[(user, "owner")], deleted_at=timezone.now() + ) + child = factories.DocumentFactory(parent=target, users=[(user, "owner")]) + + # Try moving the document to the deleted target + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": position}, + ) + + assert response.status_code == 400 + assert response.json() == { + "target_document_id": "Target parent document does not exist." + } + + # Verify that the document has not moved + document.refresh_from_db() + assert document.is_root() is True + + # Try moving the document to the child of the deleted target + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(child.id), "position": position}, + ) + assert response.status_code == 400 + assert response.json() == { + "target_document_id": "Target parent document does not exist." + } + + # Verify that the document has not moved + document.refresh_from_db() + assert document.is_root() is True + + +@pytest.mark.parametrize( + "position", + ["first-sibling", "last-sibling", "left", "right"], +) +def test_api_documents_move_authenticated_deleted_target_as_sibling(position): + """ + It should not be possible to move a document as a sibling of a deleted target document + if the user has no rigths on its parent. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(users=[(user, "owner")]) + + target_parent = factories.DocumentFactory( + users=[(user, "owner")], deleted_at=timezone.now() + ) + target = factories.DocumentFactory(users=[(user, "owner")], parent=target_parent) + + # Try moving the document as a sibling of the target + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": position}, + ) + + assert response.status_code == 400 + assert response.json() == { + "target_document_id": "Target parent document does not exist." + } + + # Verify that the document has not moved + document.refresh_from_db() + assert document.is_root() is True 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 7c530e4f..140e0042 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -36,6 +36,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": document.link_role == "editor", "retrieve": True, "update": document.link_role == "editor", @@ -90,6 +91,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": grand_parent.link_role == "editor", "retrieve": True, "update": grand_parent.link_role == "editor", @@ -175,8 +177,9 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "destroy": False, "favorite": True, "invite_owner": False, - "media_auth": True, "link_configuration": False, + "media_auth": True, + "move": False, "partial_update": document.link_role == "editor", "retrieve": True, "update": document.link_role == "editor", @@ -237,6 +240,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "favorite": True, "invite_owner": False, "link_configuration": False, + "move": False, "media_auth": True, "partial_update": grand_parent.link_role == "editor", "retrieve": True, @@ -408,6 +412,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "invite_owner": access.role == "owner", "link_configuration": access.role in ["administrator", "owner"], "media_auth": True, + "move": access.role in ["administrator", "owner"], "partial_update": access.role != "reader", "retrieve": True, "update": access.role != "reader", @@ -753,4 +758,3 @@ def test_api_documents_retrieve_numqueries_with_link_trace(django_assert_num_que assert response.status_code == 200 assert response.json()["id"] == str(document.id) - diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index b961499a..ac1fb00b 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -116,6 +116,7 @@ def test_models_documents_get_abilities_forbidden(is_authenticated, reach, role) "favorite": False, "invite_owner": False, "media_auth": False, + "move": False, "link_configuration": False, "partial_update": False, "retrieve": False, @@ -156,6 +157,7 @@ def test_models_documents_get_abilities_reader(is_authenticated, reach): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": False, "retrieve": True, "update": False, @@ -195,6 +197,7 @@ def test_models_documents_get_abilities_editor(is_authenticated, reach): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": True, "retrieve": True, "update": True, @@ -223,6 +226,7 @@ def test_models_documents_get_abilities_owner(): "invite_owner": True, "link_configuration": True, "media_auth": True, + "move": True, "partial_update": True, "retrieve": True, "update": True, @@ -250,6 +254,7 @@ def test_models_documents_get_abilities_administrator(): "invite_owner": False, "link_configuration": True, "media_auth": True, + "move": True, "partial_update": True, "retrieve": True, "update": True, @@ -280,6 +285,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": True, "retrieve": True, "update": True, @@ -312,6 +318,7 @@ def test_models_documents_get_abilities_reader_user(django_assert_num_queries): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": False, "retrieve": True, "update": False, @@ -345,6 +352,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): "invite_owner": False, "link_configuration": False, "media_auth": True, + "move": False, "partial_update": False, "retrieve": True, "update": False, diff --git a/src/backend/demo/tests/test_commands_create_demo.py b/src/backend/demo/tests/test_commands_create_demo.py index 3cd52e17..fef8a488 100644 --- a/src/backend/demo/tests/test_commands_create_demo.py +++ b/src/backend/demo/tests/test_commands_create_demo.py @@ -1,4 +1,5 @@ """Test the `create_demo` management command""" + from unittest import mock from django.core.management import call_command @@ -11,11 +12,14 @@ from core import models pytestmark = pytest.mark.django_db -@mock.patch("demo.defaults.NB_OBJECTS", { +@mock.patch( + "demo.defaults.NB_OBJECTS", + { "users": 10, "docs": 10, "max_users_per_document": 5, - } ) + }, +) @override_settings(DEBUG=True) def test_commands_create_demo(): """The create_demo management command should create objects as expected."""