♻️(backend) stop requiring owner for non-root documents

If root documents are guaranteed to have a owner, non-root documents
will automatically have them as owner by inheritance. We should not
require non-root documents to have their own direct owner because
this will make it difficult to manage access rights when we move
documents around or when we want to remove access rights for someone
on a document subtree... There should be as few overrides as possible.

Author: sampaccoud

CHANGELOG.md

@@ -25,6 +25,7 @@ and this project adheres to
 
 ## Changed
 
+- ♻️(backend) stop requiring owner for non-root documents #846
 - ♻️(backend) simplify roles by ranking them and return only the max role #846
 - ⚡️(frontend) reduce unblocking time for config #867
 - ♻️(frontend) bind UI with ability access #900

src/backend/core/api/viewsets.py

@@ -645,7 +645,7 @@ def move(self, request, *args, **kwargs):
 
         position = validated_data["position"]
         message = None
-
+        owner_accesses = []
         if position in [
             enums.MoveNodePositionChoices.FIRST_CHILD,
             enums.MoveNodePositionChoices.LAST_CHILD,
@@ -655,12 +655,15 @@ def move(self, request, *args, **kwargs):
                     "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."
-                )
+        elif target_document.is_root():
+            owner_accesses = document.get_root().accesses.filter(
+                role=models.RoleChoices.OWNER
+            )
+        elif 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(
@@ -670,6 +673,19 @@ def move(self, request, *args, **kwargs):
 
         document.move(target_document, pos=position)
 
+        # Make sure we have at least one owner
+        if (
+            owner_accesses
+            and not document.accesses.filter(role=models.RoleChoices.OWNER).exists()
+        ):
+            for owner_access in owner_accesses:
+                models.DocumentAccess.objects.update_or_create(
+                    document=document,
+                    user=owner_access.user,
+                    team=owner_access.team,
+                    defaults={"role": models.RoleChoices.OWNER},
+                )
+
         return drf.response.Response(
             {"message": "Document moved successfully."}, status=status.HTTP_200_OK
         )
@@ -716,11 +732,7 @@ def children(self, request, *args, **kwargs):
                     creator=request.user,
                     **serializer.validated_data,
                 )
-                models.DocumentAccess.objects.create(
-                    document=child_document,
-                    user=request.user,
-                    role=models.RoleChoices.OWNER,
-                )
+
             # Set the created instance to the serializer
             serializer.instance = child_document
 

src/backend/core/models.py

@@ -1093,9 +1093,12 @@ def get_abilities(self, user):
         is_owner_or_admin = role in PRIVILEGED_ROLES
 
         if self.role == RoleChoices.OWNER:
-            can_delete = (
-                role == RoleChoices.OWNER
-                and DocumentAccess.objects.filter(
+            can_delete = role == RoleChoices.OWNER and (
+                # check if document is not root trying to avoid an extra query
+                # "document_path" is annotated by the viewset's list method
+                len(getattr(self, "document_path", "")) > Document.steplen
+                or not self.document.is_root()
+                or DocumentAccess.objects.filter(
                     document_id=self.document_id, role=RoleChoices.OWNER
                 ).count()
                 > 1

src/backend/core/tests/documents/test_api_document_accesses.py

@@ -226,12 +226,7 @@ def test_api_document_accesses_list_authenticated_related_privileged(
     other_access = factories.UserDocumentAccessFactory(user=user)
     factories.UserDocumentAccessFactory(document=other_access.document)
 
-    nb_queries = 3
-    if role == "owner":
-        # Queries that secure the owner status
-        nb_queries += sum(acc.role == "owner" for acc in document_accesses)
-
-    with django_assert_num_queries(nb_queries):
+    with django_assert_num_queries(3):
         response = client.get(f"/api/v1.0/documents/{document.id!s}/accesses/")
 
     assert response.status_code == 200
@@ -278,7 +273,12 @@ def test_api_document_accesses_list_authenticated_related_privileged(
         ],
         [
             ["owner", "reader", "reader", "reader"],
-            [[], [], [], ["reader", "editor", "administrator", "owner"]],
+            [
+                ["reader", "editor", "administrator", "owner"],
+                [],
+                [],
+                ["reader", "editor", "administrator", "owner"],
+            ],
         ],
         [
             ["owner", "reader", "reader", "owner"],
@@ -355,7 +355,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul
         ],
         [
             ["owner", "reader", "reader", "reader"],
-            [[], [], [], ["reader", "editor", "administrator", "owner"]],
+            [
+                ["reader", "editor", "administrator", "owner"],
+                [],
+                [],
+                ["reader", "editor", "administrator", "owner"],
+            ],
         ],
         [
             ["owner", "reader", "reader", "owner"],
@@ -368,7 +373,12 @@ def test_api_document_accesses_list_authenticated_related_same_user(roles, resul
         ],
         [
             ["reader", "reader", "reader", "owner"],
-            [["reader", "editor", "administrator", "owner"], [], [], []],
+            [
+                ["reader", "editor", "administrator", "owner"],
+                [],
+                [],
+                ["reader", "editor", "administrator", "owner"],
+            ],
         ],
         [
             ["reader", "administrator", "reader", "editor"],

src/backend/core/tests/documents/test_api_documents_children_create.py

@@ -114,7 +114,8 @@ def test_api_documents_children_create_authenticated_success(reach, role, depth)
     child = Document.objects.get(id=response.json()["id"])
     assert child.title == "my child"
     assert child.link_reach == "restricted"
-    assert child.accesses.filter(role="owner", user=user).exists()
+    # Access objects on the child are not necessary
+    assert child.accesses.exists() is False
 
 
 @pytest.mark.parametrize("depth", [1, 2, 3])
@@ -182,7 +183,8 @@ def test_api_documents_children_create_related_success(role, depth):
     child = Document.objects.get(id=response.json()["id"])
     assert child.title == "my child"
     assert child.link_reach == "restricted"
-    assert child.accesses.filter(role="owner", user=user).exists()
+    # Access objects on the child are not necessary
+    assert child.accesses.exists() is False
 
 
 def test_api_documents_children_create_authenticated_title_null():

src/backend/core/tests/documents/test_api_documents_move.py

@@ -124,8 +124,8 @@ 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.
+    Only authenticated users with sufficient permissions on the target document (or its
+    parent depending on the position chosen), should be allowed to move documents.
     """
 
     user = factories.UserFactory()
@@ -208,6 +208,107 @@ def test_api_documents_move_authenticated_target_roles_mocked(
         assert document.is_root() is True
 
 
+def test_api_documents_move_authenticated_no_owner_user_and_team():
+    """
+    Moving a document with no owner to the root of the tree should automatically declare
+    the owner of the prevous root of the document as owner of the document itself.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    parent_owner = factories.UserFactory()
+    parent = factories.DocumentFactory(
+        users=[(parent_owner, "owner")], teams=[("lasuite", "owner")]
+    )
+    # A document with no owner
+    document = factories.DocumentFactory(parent=parent, users=[(user, "administrator")])
+    child = factories.DocumentFactory(parent=document)
+    target = factories.DocumentFactory()
+
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/move/",
+        data={"target_document_id": str(target.id), "position": "first-sibling"},
+    )
+
+    assert response.status_code == 200
+    assert response.json() == {"message": "Document moved successfully."}
+    assert list(target.get_siblings()) == [document, parent, target]
+
+    document.refresh_from_db()
+    assert list(document.get_children()) == [child]
+    assert document.accesses.count() == 3
+    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():
+    """
+    Moving a document should not fail if the user moving a document with no owner was
+    at the same time owner of the previous root and has a role on the document being moved.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    parent = factories.DocumentFactory(
+        users=[(user, "owner")], teams=[("lasuite", "owner")]
+    )
+    # A document with no owner
+    document = factories.DocumentFactory(parent=parent, users=[(user, "reader")])
+    child = factories.DocumentFactory(parent=document)
+    target = factories.DocumentFactory()
+
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/move/",
+        data={"target_document_id": str(target.id), "position": "first-sibling"},
+    )
+
+    assert response.status_code == 200
+    assert response.json() == {"message": "Document moved successfully."}
+    assert list(target.get_siblings()) == [document, parent, target]
+
+    document.refresh_from_db()
+    assert list(document.get_children()) == [child]
+    assert document.accesses.count() == 2
+    assert document.accesses.get(user__isnull=False, role="owner").user == user
+    assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite"
+
+
+def test_api_documents_move_authenticated_no_owner_same_team():
+    """
+    Moving a document should not fail if the team that is owner of the document root was
+    already declared on the document with a different role.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    parent = factories.DocumentFactory(teams=[("lasuite", "owner")])
+    # A document with no owner but same team
+    document = factories.DocumentFactory(
+        parent=parent, users=[(user, "administrator")], teams=[("lasuite", "reader")]
+    )
+    child = factories.DocumentFactory(parent=document)
+    target = factories.DocumentFactory()
+
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/move/",
+        data={"target_document_id": str(target.id), "position": "first-sibling"},
+    )
+
+    assert response.status_code == 200
+    assert response.json() == {"message": "Document moved successfully."}
+    assert list(target.get_siblings()) == [document, parent, target]
+
+    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
+    assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite"
+
+
 def test_api_documents_move_authenticated_deleted_document():
     """
     It should not be possible to move a deleted document or its descendants, even

src/backend/core/tests/test_models_document_accesses.py

@@ -127,12 +127,18 @@ def test_models_document_access_get_abilities_for_owner_of_self_allowed():
     }
 
 
-def test_models_document_access_get_abilities_for_owner_of_self_last():
+def test_models_document_access_get_abilities_for_owner_of_self_last_on_root(
+    django_assert_num_queries,
+):
     """
-    Check abilities of self access for the owner of a document when there is only one owner left.
+    Check abilities of self access for the owner of a root document when there
+    is only one owner left.
     """
     access = factories.UserDocumentAccessFactory(role="owner")
-    abilities = access.get_abilities(access.user)
+
+    with django_assert_num_queries(2):
+        abilities = access.get_abilities(access.user)
+
     assert abilities == {
         "destroy": False,
         "retrieve": True,
@@ -142,6 +148,28 @@ def test_models_document_access_get_abilities_for_owner_of_self_last():
     }
 
 
+def test_models_document_access_get_abilities_for_owner_of_self_last_on_child(
+    django_assert_num_queries,
+):
+    """
+    Check abilities of self access for the owner of a child document when there
+    is only one owner left.
+    """
+    parent = factories.DocumentFactory()
+    access = factories.UserDocumentAccessFactory(document__parent=parent, role="owner")
+
+    with django_assert_num_queries(1):
+        abilities = access.get_abilities(access.user)
+
+    assert abilities == {
+        "destroy": True,
+        "retrieve": True,
+        "update": True,
+        "partial_update": True,
+        "set_role_to": ["reader", "editor", "administrator", "owner"],
+    }
+
+
 def test_models_document_access_get_abilities_for_owner_of_owner():
     """Check abilities of owner access for the owner of a document."""
     access = factories.UserDocumentAccessFactory(role="owner")