♻️(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

@@ -98,6 +98,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

@@ -234,43 +234,6 @@ def get_serializer_context(self):
         context["resource_id"] = self.kwargs["resource_id"]
         return context
 
-    def destroy(self, request, *args, **kwargs):
-        """Forbid deleting the last owner access"""
-        instance = self.get_object()
-        resource = getattr(instance, self.resource_field_name)
-
-        # Check if the access being deleted is the last owner access for the resource
-        if (
-            instance.role == "owner"
-            and resource.accesses.filter(role="owner").count() == 1
-        ):
-            return drf.response.Response(
-                {"detail": "Cannot delete the last owner access for the resource."},
-                status=drf.status.HTTP_403_FORBIDDEN,
-            )
-
-        return super().destroy(request, *args, **kwargs)
-
-    def perform_update(self, serializer):
-        """Check that we don't change the role if it leads to losing the last owner."""
-        instance = serializer.instance
-
-        # Check if the role is being updated and the new role is not "owner"
-        if (
-            "role" in self.request.data
-            and self.request.data["role"] != models.RoleChoices.OWNER
-        ):
-            resource = getattr(instance, self.resource_field_name)
-            # Check if the access being updated is the last owner access for the resource
-            if (
-                instance.role == models.RoleChoices.OWNER
-                and resource.accesses.filter(role=models.RoleChoices.OWNER).count() == 1
-            ):
-                message = "Cannot change the role to a non-owner role for the last owner access."
-                raise drf.exceptions.PermissionDenied({"detail": message})
-
-        serializer.save()
-
 
 class DocumentMetadata(drf.metadata.SimpleMetadata):
     """Custom metadata class to add information"""
@@ -649,7 +612,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,
@@ -659,12 +622,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(
@@ -674,6 +640,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
         )
@@ -720,11 +699,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

@@ -1120,9 +1120,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

@@ -224,12 +224,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
@@ -335,7 +330,12 @@ def test_api_document_accesses_retrieve_set_role_to_child():
         ],
         [
             ["owner", "reader", "reader", "reader"],
-            [[], [], [], ["reader", "editor", "administrator", "owner"]],
+            [
+                ["reader", "editor", "administrator", "owner"],
+                [],
+                [],
+                ["reader", "editor", "administrator", "owner"],
+            ],
         ],
         [
             ["owner", "reader", "reader", "owner"],
@@ -412,7 +412,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"],
@@ -425,7 +430,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"],
@@ -929,7 +939,7 @@ def test_api_document_accesses_update_owner(
 
 
 @pytest.mark.parametrize("via", VIA)
-def test_api_document_accesses_update_owner_self(
+def test_api_document_accesses_update_owner_self_root(
     via,
     mock_user_teams,
     mock_reset_connections,  # pylint: disable=redefined-outer-name
@@ -990,6 +1000,51 @@ def test_api_document_accesses_update_owner_self(
         assert access.role == new_role
 
 
+@pytest.mark.parametrize("via", VIA)
+def test_api_document_accesses_update_owner_self_child(
+    via,
+    mock_user_teams,
+    mock_reset_connections,  # pylint: disable=redefined-outer-name
+):
+    """
+    A user who is owner of a document should be allowed to update
+    their own user access even if they are the only owner in the document,
+    provided the document is not a root.
+    """
+    user = factories.UserFactory(with_owned_document=True)
+
+    client = APIClient()
+    client.force_login(user)
+
+    parent = factories.DocumentFactory()
+    document = factories.DocumentFactory(parent=parent)
+    access = None
+    if via == USER:
+        access = factories.UserDocumentAccessFactory(
+            document=document, user=user, role="owner"
+        )
+    elif via == TEAM:
+        mock_user_teams.return_value = ["lasuite", "unknown"]
+        access = factories.TeamDocumentAccessFactory(
+            document=document, team="lasuite", role="owner"
+        )
+
+    old_values = serializers.DocumentAccessSerializer(instance=access).data
+    new_role = random.choice(["administrator", "editor", "reader"])
+
+    user_id = str(access.user_id) if via == USER else None
+    with mock_reset_connections(document.id, user_id):
+        response = client.put(
+            f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
+            data={**old_values, "role": new_role},
+            format="json",
+        )
+
+    assert response.status_code == 200
+    access.refresh_from_db()
+    assert access.role == new_role
+
+
 # Delete
 
 
@@ -1170,17 +1225,16 @@ def test_api_document_accesses_delete_owners(
             f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
         )
 
-        assert response.status_code == 204
-        assert models.DocumentAccess.objects.count() == 1
+    assert response.status_code == 204
+    assert models.DocumentAccess.objects.count() == 1
 
 
 @pytest.mark.parametrize("via", VIA)
-def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams):
+def test_api_document_accesses_delete_owners_last_owner_root(via, mock_user_teams):
     """
-    It should not be possible to delete the last owner access from a document
+    It should not be possible to delete the last owner access from a root document
     """
     user = factories.UserFactory(with_owned_document=True)
-
     client = APIClient()
     client.force_login(user)
 
@@ -1203,3 +1257,63 @@ def test_api_document_accesses_delete_owners_last_owner(via, mock_user_teams):
 
     assert response.status_code == 403
     assert models.DocumentAccess.objects.count() == 2
+
+
+def test_api_document_accesses_delete_owners_last_owner_child_user(
+    mock_reset_connections,  # pylint: disable=redefined-outer-name
+):
+    """
+    It should be possible to delete the last owner access from a document that is not a root.
+    """
+    user = factories.UserFactory(with_owned_document=True)
+    client = APIClient()
+    client.force_login(user)
+
+    parent = factories.DocumentFactory()
+    document = factories.DocumentFactory(parent=parent)
+    access = None
+    access = factories.UserDocumentAccessFactory(
+        document=document, user=user, role="owner"
+    )
+
+    assert models.DocumentAccess.objects.count() == 2
+    with mock_reset_connections(document.id, str(access.user_id)):
+        response = client.delete(
+            f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
+        )
+
+    assert response.status_code == 204
+    assert models.DocumentAccess.objects.count() == 1
+
+
+@pytest.mark.skip(
+    reason="Pending fix on https://github.com/suitenumerique/docs/issues/969"
+)
+def test_api_document_accesses_delete_owners_last_owner_child_team(
+    mock_user_teams,
+    mock_reset_connections,  # pylint: disable=redefined-outer-name
+):
+    """
+    It should be possible to delete the last owner access from a document that
+    is not a root.
+    """
+    user = factories.UserFactory(with_owned_document=True)
+    client = APIClient()
+    client.force_login(user)
+
+    parent = factories.DocumentFactory()
+    document = factories.DocumentFactory(parent=parent)
+    access = None
+    mock_user_teams.return_value = ["lasuite", "unknown"]
+    access = factories.TeamDocumentAccessFactory(
+        document=document, team="lasuite", role="owner"
+    )
+
+    assert models.DocumentAccess.objects.count() == 2
+    with mock_reset_connections(document.id, str(access.user_id)):
+        response = client.delete(
+            f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/",
+        )
+
+    assert response.status_code == 204
+    assert models.DocumentAccess.objects.count() == 1

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():