diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index f12bd8bd3..22e6a68b3 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -949,8 +949,8 @@ class DocumentViewSet( "as a child to this target document." ) elif target_document.is_root(): - owner_accesses = document.get_root().accesses.filter( - role=models.RoleChoices.OWNER + owner_accesses = list( + document.get_root().accesses.filter(role=models.RoleChoices.OWNER) ) elif not target_document.get_parent().get_abilities(user).get("move"): message = ( @@ -972,6 +972,30 @@ class DocumentViewSet( status=status.HTTP_400_BAD_REQUEST, ) + # A move changes the document's permission scope in any of these cases: + # - it is currently a root (it carries its own scope), + # - it is moving into a different tree (different current root than target's), + # - it is being promoted to root as a sibling of its own current root. + # In all these cases, direct accesses and pending invitations must be wiped so + # the document inherits the new scope. Deletions and the move share the same + # atomic transaction, so a failure rolls everything back. + becomes_sibling_root = ( + position + not in [ + enums.MoveNodePositionChoices.FIRST_CHILD, + enums.MoveNodePositionChoices.LAST_CHILD, + ] + and target_document.is_root() + ) + scope_changes = ( + document.is_root() + or becomes_sibling_root + or document.get_root() != target_document.get_root() + ) + if scope_changes: + document.accesses.all().delete() + document.invitations.all().delete() + # Make sure we have at least one owner if ( owner_accesses diff --git a/src/backend/core/tests/documents/test_api_documents_move.py b/src/backend/core/tests/documents/test_api_documents_move.py index 4cd3727b0..ebab88c20 100644 --- a/src/backend/core/tests/documents/test_api_documents_move.py +++ b/src/backend/core/tests/documents/test_api_documents_move.py @@ -237,10 +237,10 @@ def test_api_documents_move_authenticated_no_owner_user_and_team(): document.refresh_from_db() assert list(document.get_children()) == [child] - assert document.accesses.count() == 3 + + assert document.accesses.count() == 2 assert document.accesses.get(user__isnull=False, role="owner").user == parent_owner assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" - assert document.accesses.get(role="administrator").user == user def test_api_documents_move_authenticated_no_owner_same_user(): @@ -304,8 +304,10 @@ def test_api_documents_move_authenticated_no_owner_same_team(): document.refresh_from_db() assert list(document.get_children()) == [child] - assert document.accesses.count() == 2 - assert document.accesses.get(user__isnull=False, role="administrator").user == user + + # The user's direct administrator access was wiped during the cross-tree move; + # only the "lasuite" team remains, re-added as owner from the previous root. + assert document.accesses.count() == 1 assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" @@ -527,3 +529,269 @@ def test_api_documents_move_to_self(position): # Ensure document has not moved document.refresh_from_db() assert document.is_root() is True + + +def test_api_documents_move_root_deletes_accesses_and_invitations(): + """ + Moving a root document should automatically delete all its direct accesses and + invitations so it inherits the target's permissions. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + factories.InvitationFactory(document=document) + + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 2 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 0 + assert document.invitations.count() == 0 + + +def test_api_documents_move_cross_tree_deletes_accesses_and_invitations(): + """ + Moving a non-root document to a different tree should delete its direct accesses + and invitations because it is leaving its current permission scope. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + source_root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=source_root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + target_root = factories.DocumentFactory(users=[(user, "owner")]) + target = factories.DocumentFactory(parent=target_root, users=[(user, "owner")]) + + assert not document.is_root() + assert document.get_root() != target.get_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 0 + assert document.invitations.count() == 0 + + +def test_api_documents_move_same_tree_keeps_accesses_and_invitations(): + """ + Moving a non-root document within the same tree should preserve its direct + accesses and invitations since it stays in the same permission scope. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + target = factories.DocumentFactory(parent=root, users=[(user, "owner")]) + + assert not document.is_root() + assert document.get_root() == target.get_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_sub_document_to_root_deletes_accesses_and_invitations( + position, +): + """ + Moving a sub-document to root level (as a sibling of a root document) changes its + permission scope. Its direct accesses and invitations must be deleted. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + parent = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=parent, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + # target is a root document; moving as its sibling promotes document to root level + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert not document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + 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 == 200 + document.refresh_from_db() + assert document.is_root() + # Direct accesses and invitations are wiped; the backend then ensures at least one + # owner exists by inheriting the owner(s) from the previous root. + assert document.accesses.count() == 1 + assert document.accesses.filter(role="owner").exists() + assert document.invitations.count() == 0 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_sub_document_as_sibling_of_its_own_root_deletes_accesses( + position, +): + """ + Moving a sub-document as a sibling of its current root promotes it to a + new root. Even though before the move both share the same root, the document is + leaving its permission scope and its direct accesses/invitations must be deleted. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + assert not document.is_root() + assert document.get_root() == root + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(root.id), "position": position}, + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.is_root() + # Original accesses wiped; owner re-inherited from previous root ensures non-orphaned. + assert document.accesses.count() == 1 + assert document.accesses.filter(role="owner").exists() + assert document.invitations.count() == 0 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_root_as_sibling_of_root_preserves_owner(position): + """ + Moving a root document as sibling of another root, the owners + collected from the previous root (which is the document itself) must survive the + pre-move access deletion, so the document keeps at least one owner afterwards. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert target.is_root() + + 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 == 200 + document.refresh_from_db() + assert document.is_root() + # The original editor access and invitation are wiped; the previous root owner + # is re-added so the document still has at least one owner. + assert document.accesses.count() == 1 + assert document.accesses.get(role="owner").user == user + assert document.invitations.count() == 0 + + +def test_api_documents_move_scope_change_deletion_is_atomic(monkeypatch): + """ + When accesses/invitations are to be deleted (root or cross-tree move), both + deletions and the tree move are atomic: if the move fails, deletions roll back. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + # Force Document.move to fail *after* the deletion block has already run, + # so we actually exercise the rollback path rather than bailing out earlier. + def failing_move(self, *args, **kwargs): + raise RuntimeError("Simulated move failure for atomicity test") + + monkeypatch.setattr(models.Document, "move", failing_move) + + with pytest.raises(RuntimeError, match="Simulated move failure"): + client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-sibling"}, + ) + + # Accesses and invitations must still exist due to transaction rollback + document.refresh_from_db() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1