fixup! 🥅(backend) link role could be updated when restricted document

Author: AntoLC

src/backend/core/api/serializers.py

@@ -486,6 +486,10 @@ class LinkDocumentSerializer(serializers.ModelSerializer):
     We expose it separately from document in order to simplify and secure access control.
     """
 
+    link_reach = serializers.ChoiceField(
+        choices=models.LinkReachChoices.choices, required=True
+    )
+
     class Meta:
         model = models.Document
         fields = [
@@ -494,19 +498,62 @@ class Meta:
         ]
 
     def validate(self, attrs):
-        """Validate that link_role is compatible with link_reach."""
-        link_reach = attrs.get("link_reach", self.instance.link_reach)
+        """Validate that link_role and link_reach are compatible using get_select_options."""
+        link_reach = attrs.get("link_reach")
         link_role = attrs.get("link_role")
 
-        # If link_reach is restricted, link_role should not be set to anything meaningful
-        if link_reach == models.LinkReachChoices.RESTRICTED and link_role in [
-            models.LinkRoleChoices.READER,
-            models.LinkRoleChoices.EDITOR,
-        ]:
+        if not link_reach:
+            raise serializers.ValidationError(
+                {"link_reach": _("This field is required.")}
+            )
+
+        # Get available options based on ancestors' link definition
+        available_options = models.LinkReachChoices.get_select_options(
+            **self.instance.ancestors_link_definition
+        )
+
+        # Validate link_reach is allowed
+        if link_reach not in available_options:
             msg = _(
-                "Cannot set link_role when link_reach is 'restricted'. Change link_reach first."
+                "Link reach '%(link_reach)s' is not allowed based on parent document configuration."
+            )
+            raise serializers.ValidationError(
+                {"link_reach": msg % {"link_reach": link_reach}}
+            )
+
+        # Validate link_role is compatible with link_reach
+        allowed_roles = available_options[link_reach]
+
+        if link_reach == models.LinkReachChoices.RESTRICTED:
+            # For restricted reach, link_role is ignored but
+            # we shouldn't allow meaningful roles to be set
+            if link_role is not None and link_role in [
+                models.LinkRoleChoices.READER,
+                models.LinkRoleChoices.EDITOR,
+            ]:
+                msg = _(
+                    "Cannot set link_role when link_reach is 'restricted'. "
+                    "Link role must be null for restricted reach."
+                )
+                raise serializers.ValidationError({"link_role": msg})
+        # For non-restricted reach, validate link_role is in allowed roles
+        elif link_role is not None and link_role not in allowed_roles:
+            msg = _(
+                "Link role '%(link_role)s' is not allowed for link reach '%(link_reach)s'. "
+                "Allowed roles: %(allowed_roles)s"
+            )
+            raise serializers.ValidationError(
+                {
+                    "link_role": msg
+                    % {
+                        "link_role": link_role,
+                        "link_reach": link_reach,
+                        "allowed_roles": ", ".join(allowed_roles)
+                        if allowed_roles
+                        else "none",
+                    }
+                }
             )
-            raise serializers.ValidationError({"link_role": msg})
 
         return attrs
 

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

@@ -184,7 +184,11 @@ def test_api_documents_link_configuration_update_role_restricted_forbidden():
         document=document, user=user, role=models.RoleChoices.OWNER
     )
 
-    new_data = {"link_role": models.LinkRoleChoices.EDITOR}
+    # Try to set a meaningful role on a restricted document
+    new_data = {
+        "link_reach": models.LinkReachChoices.RESTRICTED,
+        "link_role": models.LinkRoleChoices.EDITOR,
+    }
 
     response = client.put(
         f"/api/v1.0/documents/{document.id!s}/link-configuration/",
@@ -198,3 +202,196 @@ def test_api_documents_link_configuration_update_role_restricted_forbidden():
         "Cannot set link_role when link_reach is 'restricted'"
         in response.json()["link_role"][0]
     )
+
+
+def test_api_documents_link_configuration_update_link_reach_required():
+    """
+    Test that link_reach is required when updating link configuration.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    factories.UserDocumentAccessFactory(
+        document=document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    # Try to update without providing link_reach
+    new_data = {"link_role": models.LinkRoleChoices.EDITOR}
+
+    response = client.put(
+        f"/api/v1.0/documents/{document.id!s}/link-configuration/",
+        new_data,
+        format="json",
+    )
+
+    assert response.status_code == 400
+    assert "link_reach" in response.json()
+    assert "This field is required" in response.json()["link_reach"][0]
+
+
+def test_api_documents_link_configuration_update_restricted_without_role_success():
+    """
+    Test that setting link_reach to restricted without specifying link_role succeeds.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    factories.UserDocumentAccessFactory(
+        document=document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    # Only specify link_reach, not link_role
+    new_data = {
+        "link_reach": models.LinkReachChoices.RESTRICTED,
+    }
+
+    response = client.put(
+        f"/api/v1.0/documents/{document.id!s}/link-configuration/",
+        new_data,
+        format="json",
+    )
+
+    assert response.status_code == 200
+    document.refresh_from_db()
+    assert document.link_reach == models.LinkReachChoices.RESTRICTED
+
+
+@pytest.mark.parametrize(
+    "reach", [models.LinkReachChoices.PUBLIC, models.LinkReachChoices.AUTHENTICATED]
+)
+@pytest.mark.parametrize("role", models.LinkRoleChoices.values)
+def test_api_documents_link_configuration_update_non_restricted_with_valid_role_success(
+    reach, role
+):
+    """
+    Test that setting non-restricted link_reach with valid link_role succeeds.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.RESTRICTED,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    factories.UserDocumentAccessFactory(
+        document=document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    new_data = {
+        "link_reach": reach,
+        "link_role": role,
+    }
+
+    response = client.put(
+        f"/api/v1.0/documents/{document.id!s}/link-configuration/",
+        new_data,
+        format="json",
+    )
+
+    assert response.status_code == 200
+    document.refresh_from_db()
+    assert document.link_reach == reach
+    assert document.link_role == role
+
+
+def test_api_documents_link_configuration_update_with_ancestor_constraints():
+    """
+    Test that link configuration respects ancestor constraints using get_select_options.
+    This test may need adjustment based on the actual get_select_options implementation.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    parent_document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    child_document = factories.DocumentFactory(
+        parent=parent_document,
+        link_reach=models.LinkReachChoices.PUBLIC,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    factories.UserDocumentAccessFactory(
+        document=child_document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    # Try to set child to PUBLIC when parent is RESTRICTED
+    new_data = {
+        "link_reach": models.LinkReachChoices.RESTRICTED,
+        "link_role": models.LinkRoleChoices.READER,
+    }
+
+    response = client.put(
+        f"/api/v1.0/documents/{child_document.id!s}/link-configuration/",
+        new_data,
+        format="json",
+    )
+
+    assert response.status_code == 400
+    assert "link_reach" in response.json()
+    assert (
+        "Link reach 'restricted' is not allowed based on parent"
+        in response.json()["link_reach"][0]
+    )
+
+
+def test_api_documents_link_configuration_update_invalid_role_for_reach_validation():
+    """
+    Test the specific validation logic that checks if link_role is allowed for link_reach.
+    This tests the code section that validates allowed_roles from get_select_options.
+    """
+    user = factories.UserFactory()
+    client = APIClient()
+    client.force_login(user)
+
+    parent_document = factories.DocumentFactory(
+        link_reach=models.LinkReachChoices.AUTHENTICATED,
+        link_role=models.LinkRoleChoices.EDITOR,
+    )
+
+    child_document = factories.DocumentFactory(
+        parent=parent_document,
+        link_reach=models.LinkReachChoices.RESTRICTED,
+        link_role=models.LinkRoleChoices.READER,
+    )
+
+    factories.UserDocumentAccessFactory(
+        document=child_document, user=user, role=models.RoleChoices.OWNER
+    )
+
+    new_data = {
+        "link_reach": models.LinkReachChoices.AUTHENTICATED,
+        "link_role": models.LinkRoleChoices.READER,  # This should be rejected
+    }
+
+    response = client.put(
+        f"/api/v1.0/documents/{child_document.id!s}/link-configuration/",
+        new_data,
+        format="json",
+    )
+
+    assert response.status_code == 400
+    assert "link_role" in response.json()
+    error_message = response.json()["link_role"][0]
+    assert (
+        "Link role 'reader' is not allowed for link reach 'authenticated'"
+        in error_message
+    )
+    assert "Allowed roles: editor" in error_message