🛂(backend) exclude readers from mentions and access list

A reader cannot see comments, so a mention would lead them nowhere.
Reject readers as mention targets. while users allowed to comment
see each other so they can pick who to mention.

Signed-off-by: BOUKERFA Mohamed El Amine <boukerfa.ma@gmail.com>

CHANGELOG.md

@@ -72,6 +72,7 @@ and this project adheres to
 
 ### Changed
 
+- 🛂(backend) let users allowed to comment list each other's accesses #2447
 - ♻️(collaboration) migrate the collaboration server from hocuspocus to yhub
 - 💥(y-provider) y-provider becomes converter-only
 - 💥(backend) move the resource server JWKS from `/api/{version}/jwks` to
@@ -266,7 +267,6 @@ and this project adheres to
 
 ### Changed
 
-- 🛂(backend) make document access list visible to all collaborators
 - 👷(CI) remove test-e2e-other-browser job #2404
 - ♿️(frontend) use heading element for pinned documents section title #2380
 - ♿️(frontend) use anchor links for table of contents entries #2390

src/backend/core/api/serializers.py

@@ -1027,14 +1027,21 @@ class MentionSerializer(serializers.ModelSerializer):
         ]
 
     def validate_mentioned_user_id(self, user):
-        """Ensure the mentioned user has access to the document."""
+        """Ensure the mentioned user is allowed to comment on the document."""
         document = models.Document.objects.annotate_user_roles(user).get(
             pk=self.context["document"].pk
         )
-        if document.get_role(user) is None:
+        role = document.get_role(user)
+        if role is None:
             raise serializers.ValidationError(
                 "This user does not have access to the document."
             )
+
+        # A reader cannot see comments, mentioning them would lead nowhere
+        if role not in choices.COMMENTING_ROLES:
+            raise serializers.ValidationError(
+                "This user is not allowed to comment on the document."
+            )
         return user
 
     def validate_thread_id(self, thread):

src/backend/core/api/viewsets.py

@@ -2640,11 +2640,16 @@ class DocumentAccessViewSet(
             | models.Document.objects.filter(pk=self.document.pk)
         ).filter(ancestors_deleted_at__isnull=True)
 
-        # All users with access see the full list of accesses (with limited
-        # user details for unprivileged roles) so that any collaborator
-        # allowed to comment can mention the others.
         queryset = self.get_queryset().filter(document__in=ancestors)
 
+        # Readers only see privileged accesses. Users allowed to comment also
+        # see the other roles allowed to comment (with limited user details)
+        # so that they can mention each other. Privileged users see everything.
+        if role not in choices.COMMENTING_ROLES:
+            queryset = queryset.filter(role__in=choices.PRIVILEGED_ROLES)
+        elif role not in choices.PRIVILEGED_ROLES:
+            queryset = queryset.filter(role__in=choices.COMMENTING_ROLES)
+
         accesses = list(queryset.order_by("document__path"))
 
         # Annotate more information on roles

src/backend/core/choices.py

@@ -48,6 +48,7 @@ class RoleChoices(PriorityTextChoices):
 
 
 PRIVILEGED_ROLES = [RoleChoices.ADMIN, RoleChoices.OWNER]
+COMMENTING_ROLES = [RoleChoices.COMMENTER, RoleChoices.EDITOR, *PRIVILEGED_ROLES]
 
 
 class LinkReachChoices(PriorityTextChoices):

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

@@ -97,9 +97,10 @@ def test_api_document_accesses_list_authenticated_related_non_privileged(
     via, role, mock_user_teams, django_assert_num_queries
 ):
     """
-    Authenticated users with no privileged role should be able to list all document
-    accesses, including from ancestors, but with limited user information, so that
-    any collaborator allowed to comment can mention the others.
+    Authenticated users with no privileged role should only be able to list document
+    accesses associated with privileged roles, including from ancestors. Users allowed
+    to comment should also see the accesses of the other roles allowed to comment, with
+    limited user information, so that they can mention each other.
     """
     user = factories.UserFactory()
     client = APIClient()
@@ -151,9 +152,17 @@ def test_api_document_accesses_list_authenticated_related_non_privileged(
     assert response.status_code == 200
     content = response.json()
 
-    # All accesses on the document and its ancestors are returned
-    all_accesses = [*accesses, user_access]
-    assert len(content) == len(all_accesses)
+    # Readers only see privileged accesses, users allowed to comment
+    # see the accesses of every role allowed to comment
+    visible_roles = (
+        choices.COMMENTING_ROLES
+        if role in choices.COMMENTING_ROLES
+        else choices.PRIVILEGED_ROLES
+    )
+    visible_accesses = [
+        access for access in [*accesses, user_access] if access.role in visible_roles
+    ]
+    assert len(content) == len(visible_accesses)
 
     assert sorted(content, key=lambda x: x["id"]) == sorted(
         [
@@ -185,7 +194,7 @@ def test_api_document_accesses_list_authenticated_related_non_privileged(
                 "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"),
                 "created_at": access.created_at.isoformat().replace("+00:00", "Z"),
             }
-            for access in all_accesses
+            for access in visible_accesses
         ],
         key=lambda x: x["id"],
     )

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

@@ -13,7 +13,7 @@ from django.utils import timezone
 import pytest
 from rest_framework.test import APIClient
 
-from core import factories, models
+from core import choices, factories, models
 
 pytestmark = pytest.mark.django_db
 
@@ -22,7 +22,9 @@ def test_api_documents_mention_anonymous():
     """Anonymous users should not be allowed to mention users on a document."""
     document = factories.DocumentFactory()
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     response = APIClient().post(
         f"/api/v1.0/documents/{document.id!s}/mention/",
@@ -41,7 +43,9 @@ def test_api_documents_mention_anonymous_public_document():
     """
     document = factories.DocumentFactory(link_reach="public", link_role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     response = APIClient().post(
         f"/api/v1.0/documents/{document.id!s}/mention/",
@@ -60,7 +64,9 @@ def test_api_documents_mention_authenticated_no_access():
     user = factories.UserFactory()
     document = factories.DocumentFactory(link_reach="restricted")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -79,7 +85,9 @@ def test_api_documents_mention_authenticated_reader():
     document = factories.DocumentFactory(link_reach="restricted")
     factories.UserDocumentAccessFactory(document=document, user=user, role="reader")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -102,7 +110,9 @@ def test_api_documents_mention_authenticated_success(role):
     document = factories.DocumentFactory(link_reach="restricted", title="My doc")
     factories.UserDocumentAccessFactory(document=document, user=user, role=role)
     mentioned_user = factories.UserFactory(language="en-us")
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     anchor_id = str(uuid4())
 
     client = APIClient()
@@ -149,13 +159,16 @@ def test_api_documents_mention_authenticated_success(role):
 
 def test_api_documents_mention_via_link_role():
     """
-    Authenticated users allowed to comment via the document link role should be
-    allowed to mention collaborators.
+    Authenticated users allowed to comment only via the document link role
+    should not be allowed to mention: they cannot list the accesses, and it
+    would let any link holder email the collaborators.
     """
     user = factories.UserFactory()
     document = factories.DocumentFactory(link_reach="public", link_role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -164,8 +177,8 @@ def test_api_documents_mention_via_link_role():
         {"anchor_id": str(uuid4()), "mentioned_user_id": str(mentioned_user.id)},
     )
 
-    assert response.status_code == 201
-    assert len(mail.outbox) == 1
+    assert response.status_code == 403
+    assert models.Mention.objects.exists() is False
 
 
 def test_api_documents_mention_missing_anchor_id():
@@ -174,7 +187,9 @@ def test_api_documents_mention_missing_anchor_id():
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -195,7 +210,9 @@ def test_api_documents_mention_invalid_anchor_id(anchor_id):
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -253,6 +270,52 @@ def test_api_documents_mention_user_without_access():
     assert len(mail.outbox) == 0
 
 
+@pytest.mark.parametrize("role", choices.COMMENTING_ROLES)
+def test_api_documents_mention_user_allowed_to_comment(role):
+    """Users with a role allowing them to comment on the document can be mentioned."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory()
+    factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
+    mentioned_user = factories.UserFactory()
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role=role
+    )
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/mention/",
+        {"anchor_id": str(uuid4()), "mentioned_user_id": str(mentioned_user.id)},
+    )
+
+    assert response.status_code == 201
+    assert len(mail.outbox) == 1
+
+
+def test_api_documents_mention_user_with_reader_role():
+    """Readers cannot see comments on the document and should not be mentionable."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory()
+    factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
+    mentioned_user = factories.UserFactory()
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="reader"
+    )
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/mention/",
+        {"anchor_id": str(uuid4()), "mentioned_user_id": str(mentioned_user.id)},
+    )
+
+    assert response.status_code == 400
+    assert response.json() == {
+        "mentioned_user_id": ["This user is not allowed to comment on the document."]
+    }
+    assert models.Mention.objects.exists() is False
+
+
 def test_api_documents_mention_user_with_access_on_ancestor():
     """Users with access inherited from an ancestor document can be mentioned."""
     user = factories.UserFactory()
@@ -260,7 +323,9 @@ def test_api_documents_mention_user_with_access_on_ancestor():
     document = factories.DocumentFactory(parent=parent)
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=parent, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=parent, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -281,7 +346,9 @@ def test_api_documents_mention_user_with_access_via_team(mock_user_teams):
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.TeamDocumentAccessFactory(document=document, team="lasuite")
+    factories.TeamDocumentAccessFactory(
+        document=document, team="lasuite", role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -303,7 +370,9 @@ def test_api_documents_mention_thread():
     document = factories.DocumentFactory(title="My doc")
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     thread = factories.ThreadFactory(document=document)
     anchor_id = str(uuid4())
 
@@ -344,7 +413,9 @@ def test_api_documents_mention_thread_other_document():
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     other_thread = factories.ThreadFactory()
 
     client = APIClient()
@@ -374,7 +445,9 @@ def test_api_documents_mention_cooldown_same_context():
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     anchor_id1, anchor_id2 = str(uuid4()), str(uuid4())
 
     client = APIClient()
@@ -406,7 +479,9 @@ def test_api_documents_mention_cooldown_distinct_contexts():
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     thread1, thread2 = factories.ThreadFactory.create_batch(2, document=document)
 
     client = APIClient()
@@ -442,7 +517,9 @@ def test_api_documents_mention_cooldown_distinct_contexts():
 
     # The cooldown should apply per mentioned user
     other_mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=other_mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=other_mentioned_user, role="commenter"
+    )
     anchor_id = str(uuid4())
     response = client.post(
         f"/api/v1.0/documents/{document.id!s}/mention/",
@@ -464,7 +541,9 @@ def test_api_documents_mention_cooldown_expired(settings):
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -504,7 +583,9 @@ def test_api_documents_mention_cooldown_only_considers_notified_mentions():
     document = factories.DocumentFactory()
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     existing_mention = factories.MentionFactory(
         document=document,
         mentioned_user=mentioned_user,
@@ -531,7 +612,9 @@ def test_api_documents_mention_soft_deleted_document():
     document = factories.DocumentFactory(link_reach="restricted")
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
     document.soft_delete()
 
     client = APIClient()
@@ -554,7 +637,9 @@ def test_api_documents_mention_throttling(settings):
     document = factories.DocumentFactory(link_reach="restricted")
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)
@@ -586,7 +671,9 @@ def test_api_documents_mention_throttling_y_provider_exempted(settings):
     document = factories.DocumentFactory(link_reach="restricted")
     factories.UserDocumentAccessFactory(document=document, user=user, role="commenter")
     mentioned_user = factories.UserFactory()
-    factories.UserDocumentAccessFactory(document=document, user=mentioned_user)
+    factories.UserDocumentAccessFactory(
+        document=document, user=mentioned_user, role="commenter"
+    )
 
     client = APIClient()
     client.force_login(user)