diff --git a/CHANGELOG.md b/CHANGELOG.md index bb68e4fd4b..b4b53667a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ and this project adheres to - 🔒️(backend) bind comment thread to the path document on creation #2838 - 🐛(frontend) fix search highlight hidden inside code blocks #2681 +- 🔒️(backend) require present document access for thread and comment + writes #2840 ## [v6.0.1] - 2026-10-15 diff --git a/src/backend/core/api/permissions.py b/src/backend/core/api/permissions.py index da410ca220..a7ed02ed95 100644 --- a/src/backend/core/api/permissions.py +++ b/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.""" diff --git a/src/backend/core/models.py b/src/backend/core/models.py index eaa2caf5b3..fe01838885 100644 --- a/src/backend/core/models.py +++ b/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, } diff --git a/src/backend/core/tests/documents/test_api_documents_comments.py b/src/backend/core/tests/documents/test_api_documents_comments.py index efbe5ab2d8..a453eefd97 100644 --- a/src/backend/core/tests/documents/test_api_documents_comments.py +++ b/src/backend/core/tests/documents/test_api_documents_comments.py @@ -284,6 +284,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 @@ -619,6 +638,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): """ diff --git a/src/backend/core/tests/documents/test_api_documents_threads.py b/src/backend/core/tests/documents/test_api_documents_threads.py index b10ec2b2ba..691db44fad 100644 --- a/src/backend/core/tests/documents/test_api_documents_threads.py +++ b/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. diff --git a/src/backend/core/tests/test_models_comment.py b/src/backend/core/tests/test_models_comment.py index 567fcd7d1e..2d314c7ca8 100644 --- a/src/backend/core/tests/test_models_comment.py +++ b/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, + }