🔒️(backend) require present document access for thread/comment writes

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to
 
 ### Fixed
 
+- 🔒️(backend) require present document access for thread and comment writes
 - 🐛(frontend) stop the http fallback when the collaboration server refuses it
 - 🐛(backend) bypass throttling on access/me action from yhub
 - 🐛(frontend) switch language when the current one is untranslated #2790

src/backend/core/api/permissions.py

@@ -173,15 +173,12 @@ def has_object_permission(self, request, view, obj):
 
 
 class CommentPermission(permissions.BasePermission):
-    """Permission class for comments."""
+    """Permission class for threads and comments."""
 
     def has_permission(self, request, view):
-        """Check permission for a given object."""
-        if view.action in ["create", "list"]:
-            document_abilities = view.get_document_or_404().get_abilities(request.user)
-            return document_abilities["comment"]
-
-        return True
+        """Check permission for a given action on the document."""
+        document_abilities = view.get_document_or_404().get_abilities(request.user)
+        return document_abilities["comment"]
 
     def has_object_permission(self, request, view, obj):
         """Check permission for a given object."""

src/backend/core/models.py

@@ -1942,10 +1942,9 @@ def get_abilities(self, user):
         role = self.document.get_role(user)
         doc_abilities = self.document.get_abilities(user)
         read_access = doc_abilities.get("comment", False)
-        write_access = self.creator == user or role in [
-            RoleChoices.OWNER,
-            RoleChoices.ADMIN,
-        ]
+        write_access = read_access and (
+            self.creator == user or role in [RoleChoices.OWNER, RoleChoices.ADMIN]
+        )
         return {
             "destroy": write_access,
             "update": write_access,
@@ -1997,14 +1996,13 @@ def get_abilities(self, user):
         read_access = doc_abilities.get("comment", False)
         can_react = read_access and user.is_authenticated
         is_author = self.user == user
-        can_moderate = is_author or role in [
-            RoleChoices.OWNER,
-            RoleChoices.ADMIN,
-        ]
+        can_moderate = read_access and (
+            is_author or role in [RoleChoices.OWNER, RoleChoices.ADMIN]
+        )
         return {
             "destroy": can_moderate,
-            "update": is_author,
-            "partial_update": is_author,
+            "update": read_access and is_author,
+            "partial_update": read_access and is_author,
             "reactions": can_react,
             "retrieve": read_access,
         }

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

@@ -283,6 +283,25 @@ def test_create_comment_authenticated_user_accessible_document():
     }
 
 
+def test_create_comment_revoked_collaborator():
+    """A collaborator whose access was revoked must not comment on the document."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach="restricted")
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+
+    # Revoke the collaborator's access only, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/comments/",
+        {"body": "test"},
+    )
+    assert response.status_code == 403
+
+
 def test_create_comment_authenticated_user_not_enough_access():
     """
     Authenticated users should not be allowed to create comments on a document they don't have
@@ -506,6 +525,55 @@ def test_update_comment_authenticated_no_access():
     assert response.status_code == 403
 
 
+def test_update_comment_revoked_collaborator():
+    """
+    A collaborator whose access was revoked must not edit their own comment, even though
+    they are its author.
+    """
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach="restricted")
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    comment = factories.CommentFactory(thread=thread, body="test", user=user)
+
+    # Revoke the collaborator's access, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.patch(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/comments/{comment.id!s}/",
+        {"body": "edited after revocation"},
+    )
+    assert response.status_code == 403
+
+    comment.refresh_from_db()
+    assert comment.body == "test"
+
+
+def test_delete_comment_revoked_collaborator():
+    """
+    A collaborator whose access was revoked must not delete their own comment, even
+    though they are its author.
+    """
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach="restricted")
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    comment = factories.CommentFactory(thread=thread, body="test", user=user)
+
+    # Revoke the collaborator's access, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.delete(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/comments/{comment.id!s}/",
+    )
+    assert response.status_code == 403
+    assert models.Comment.objects.filter(pk=comment.pk).exists()
+
+
 @pytest.mark.parametrize("role", [models.RoleChoices.ADMIN, models.RoleChoices.OWNER])
 def test_update_comment_authenticated_admin_or_owner_cannot_update_other_comment(role):
     """

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

@@ -921,9 +921,9 @@ def test_api_documents_threads_retrieve_number_of_queries(
             )
 
     client = APIClient()
-    # 1 query for the thread and 1 per prefetched relation: comments, reactions
-    # and reaction users.
-    with django_assert_num_queries(4):
+    # 1 query for the document from the permission check, 1 for the thread and
+    # 1 per prefetched relation: comments, reactions and reaction users.
+    with django_assert_num_queries(5):
         response = client.get(
             f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/",
         )
@@ -1041,6 +1041,89 @@ def test_api_documents_threads_destroy_authenticated_document(link_role):
     assert response.status_code == 403
 
 
+@pytest.mark.parametrize("method", ["patch", "delete"])
+def test_api_documents_threads_write_revoked_collaborator(method):
+    """A collaborator whose access was revoked must not write to their own thread."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach="restricted")
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    factories.CommentFactory(thread=thread, user=user)
+
+    # Revoke the collaborator's access only, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = getattr(client, method)(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/",
+    )
+    assert response.status_code == 403
+
+
+def test_api_documents_threads_destroy_revoked_collaborator_cascade_guard():
+    """
+    A collaborator whose access was revoked must not delete their own thread:
+    deleting it would cascade-destroy every comment in it, including the owner's.
+    """
+    owner = factories.UserFactory()
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(
+        link_reach="restricted", users=[(owner, models.RoleChoices.OWNER)]
+    )
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    factories.CommentFactory(thread=thread, user=user)
+    factories.CommentFactory(thread=thread, user=owner)
+
+    # Revoke the collaborator's access only, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.delete(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/",
+    )
+    assert response.status_code == 403
+    assert models.Comment.objects.filter(thread=thread).count() == 2
+
+
+# Note: the sixth route named in the report (DELETE of a child document created by the
+# revoked collaborator) is deliberately NOT covered by this fix. Deleting a non-root
+# document a user created is an intentional product rule (`Document.get_abilities`
+# `can_destroy`, see test_models_documents.py) and the deletion is a soft delete the
+# owner can undo. Tightening it would be a separate product decision, not part of this
+# security fix.
+@pytest.mark.parametrize("action", ["resolve", "unresolve"])
+def test_api_documents_threads_resolve_revoked_collaborator(action):
+    """A collaborator whose access was revoked must not resolve/unresolve their thread."""
+    owner = factories.UserFactory()
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach="restricted")
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    factories.CommentFactory(thread=thread, user=user)
+    if action == "resolve":
+        # The owner resolved the thread before the collaborator's access was revoked
+        thread.resolved = True
+        thread.resolved_by = owner
+        thread.save(update_fields=["resolved", "resolved_by"])
+
+    # Revoke the collaborator's access only, as an owner would
+    document.accesses.filter(user=user).delete()
+
+    client = APIClient()
+    client.force_login(user)
+    response = client.post(
+        f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/{action}/",
+    )
+    assert response.status_code == 403
+
+    # The thread state must be unchanged by the denied request
+    thread.refresh_from_db()
+    assert thread.resolved == (action == "resolve")
+
+
 def test_api_documents_threads_destroy_restricted_document_anonymous_user():
     """
     Anonymous users should not be allowed to destroy threads on restricted documents.

src/backend/core/tests/test_models_comment.py

@@ -281,3 +281,75 @@ def test_comment_get_abilities_user_admin_or_owner_other_comment(role):
         "reactions": True,
         "retrieve": True,
     }
+
+
+def test_comment_get_abilities_revoked_author():
+    """An author whose document access was revoked loses every ability on their comment."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach=LinkReachChoices.RESTRICTED)
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    comment = factories.CommentFactory(thread__document=document, user=user)
+
+    document.accesses.all().delete()
+
+    assert comment.get_abilities(user) == {
+        "destroy": False,
+        "update": False,
+        "partial_update": False,
+        "reactions": False,
+        "retrieve": False,
+    }
+
+
+def test_comment_get_abilities_author_with_present_access():
+    """An author who still holds access keeps their write abilities on their comment."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach=LinkReachChoices.RESTRICTED)
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    comment = factories.CommentFactory(thread__document=document, user=user)
+
+    assert comment.get_abilities(user) == {
+        "destroy": True,
+        "update": True,
+        "partial_update": True,
+        "reactions": True,
+        "retrieve": True,
+    }
+
+
+def test_thread_get_abilities_revoked_creator():
+    """A creator whose document access was revoked loses every ability on their thread."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach=LinkReachChoices.RESTRICTED)
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    factories.CommentFactory(thread=thread, user=user)
+
+    document.accesses.all().delete()
+
+    assert thread.get_abilities(user) == {
+        "destroy": False,
+        "update": False,
+        "partial_update": False,
+        "resolve": False,
+        "unresolve": False,
+        "retrieve": False,
+    }
+
+
+def test_thread_get_abilities_creator_with_present_access():
+    """A creator who still holds access keeps their write abilities on their thread."""
+    user = factories.UserFactory()
+    document = factories.DocumentFactory(link_reach=LinkReachChoices.RESTRICTED)
+    factories.UserDocumentAccessFactory(document=document, user=user, role="editor")
+    thread = factories.ThreadFactory(document=document, creator=user)
+    factories.CommentFactory(thread=thread, user=user)
+
+    assert thread.get_abilities(user) == {
+        "destroy": True,
+        "update": True,
+        "partial_update": True,
+        "resolve": True,
+        "unresolve": True,
+        "retrieve": True,
+    }