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."""