From 3264e29941fd58c16dbc2f7589bbc1a927247ba8 Mon Sep 17 00:00:00 2001 From: Amine BOUKERFA Date: Tue, 26 May 2026 09:26:31 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92(backend)=20prevent=20admins=20from?= =?UTF-8?q?=20rewriting=20other=20users=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Admins/owners could overwrite another user's comment text and silently take ownership of it. Split write permissions: destroy stays broad (author + admin/owner) for moderation. Signed-off-by: Mohamed El Amine BOUKERFA --- CHANGELOG.md | 1 + src/backend/core/api/serializers.py | 5 ---- src/backend/core/api/viewsets.py | 5 ++++ src/backend/core/models.py | 9 +++---- .../documents/test_api_documents_comments.py | 13 ++++++---- .../documents/test_api_documents_threads.py | 4 ++-- src/backend/core/tests/test_models_comment.py | 24 +++++++++---------- 7 files changed, 33 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe9125bac..af92ae62c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to ### Fixed - 🐛(docs) run migration 0027 without superuser role +- 🐛(backend) prevent admins/owners from overwriting other users comments ## [v5.1.0] - 2026-05-11 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 7ed06ebe7..74e168b9b 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -911,12 +911,7 @@ class CommentSerializer(serializers.ModelSerializer): def validate(self, attrs): """Validate comment data.""" - - request = self.context.get("request") - user = getattr(request, "user", None) - attrs["thread_id"] = self.context["thread_id"] - attrs["user_id"] = user.id if user else None return attrs def get_abilities(self, obj): diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 22e6a68b3..0c3659392 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -3004,6 +3004,11 @@ class CommentViewSet( context["thread_id"] = self.kwargs["thread_id"] return context + def perform_create(self, serializer): + """Attach the request user as the comment author.""" + user = self.request.user if self.request.user.is_authenticated else None + serializer.save(user=user) + @drf.decorators.action( detail=True, methods=["post", "delete"], diff --git a/src/backend/core/models.py b/src/backend/core/models.py index c23b0cfd7..1599accd0 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1895,14 +1895,15 @@ class Comment(BaseModel): doc_abilities = self.thread.document.get_abilities(user) read_access = doc_abilities.get("comment", False) can_react = read_access and user.is_authenticated - write_access = self.user == user or role in [ + is_author = self.user == user + can_moderate = is_author or role in [ RoleChoices.OWNER, RoleChoices.ADMIN, ] return { - "destroy": write_access, - "update": write_access, - "partial_update": write_access, + "destroy": can_moderate, + "update": is_author, + "partial_update": 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 85c90aa84..6ae2c72f8 100644 --- a/src/backend/core/tests/documents/test_api_documents_comments.py +++ b/src/backend/core/tests/documents/test_api_documents_comments.py @@ -438,6 +438,7 @@ def test_update_comment_authenticated_user_own_comment(): comment.refresh_from_db() assert comment.body == "other content" + assert comment.user == user def test_update_comment_authenticated_user_not_enough_access(): @@ -479,14 +480,15 @@ def test_update_comment_authenticated_no_access(): @pytest.mark.parametrize("role", [models.RoleChoices.ADMIN, models.RoleChoices.OWNER]) -def test_update_comment_authenticated_admin_or_owner_can_update_any_comment(role): +def test_update_comment_authenticated_admin_or_owner_cannot_update_other_comment(role): """ - Authenticated users should be able to update comments on a document they don't have access to. + Admins and owners can moderate (delete) but must not edit other users' comments. """ user = factories.UserFactory() document = factories.DocumentFactory(users=[(user, role)]) thread = factories.ThreadFactory(document=document) - comment = factories.CommentFactory(thread=thread, body="test") + original_author = factories.UserFactory() + comment = factories.CommentFactory(thread=thread, body="test", user=original_author) client = APIClient() client.force_login(user) @@ -494,10 +496,11 @@ def test_update_comment_authenticated_admin_or_owner_can_update_any_comment(role f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/comments/{comment.id!s}/", {"body": "other content"}, ) - assert response.status_code == 200 + assert response.status_code == 403 comment.refresh_from_db() - assert comment.body == "other content" + assert comment.body == "test" + assert comment.user == original_author @pytest.mark.parametrize("role", [models.RoleChoices.ADMIN, models.RoleChoices.OWNER]) 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 b7a53f265..a950bf76e 100644 --- a/src/backend/core/tests/documents/test_api_documents_threads.py +++ b/src/backend/core/tests/documents/test_api_documents_threads.py @@ -796,8 +796,8 @@ def test_api_documents_threads_retrieve_restricted_document_privileged_roles(rol "reactions": [], "abilities": { "destroy": True, - "update": True, - "partial_update": True, + "update": False, + "partial_update": False, "reactions": True, "retrieve": True, }, diff --git a/src/backend/core/tests/test_models_comment.py b/src/backend/core/tests/test_models_comment.py index 7ff8cc876..567fcd7d1 100644 --- a/src/backend/core/tests/test_models_comment.py +++ b/src/backend/core/tests/test_models_comment.py @@ -249,13 +249,12 @@ def test_comment_get_abilities_user_editor_own_comment(link_role, link_reach): } -def test_comment_get_abilities_user_admin(): - """Admins have all accesses to a comment.""" +@pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER]) +def test_comment_get_abilities_user_admin_or_owner_own_comment(role): + """Admins/owners have full write access on their own comment.""" user = factories.UserFactory() - document = factories.DocumentFactory(users=[(user, RoleChoices.ADMIN)]) - comment = factories.CommentFactory( - thread__document=document, user=random.choice([user, None]) - ) + document = factories.DocumentFactory(users=[(user, role)]) + comment = factories.CommentFactory(thread__document=document, user=user) assert comment.get_abilities(user) == { "destroy": True, @@ -266,18 +265,19 @@ def test_comment_get_abilities_user_admin(): } -def test_comment_get_abilities_user_owner(): - """Owners have all accesses to a comment.""" +@pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER]) +def test_comment_get_abilities_user_admin_or_owner_other_comment(role): + """Admins/owners can moderate others' comments (destroy) but cannot edit them.""" user = factories.UserFactory() - document = factories.DocumentFactory(users=[(user, RoleChoices.OWNER)]) + document = factories.DocumentFactory(users=[(user, role)]) comment = factories.CommentFactory( - thread__document=document, user=random.choice([user, None]) + thread__document=document, user=random.choice([factories.UserFactory(), None]) ) assert comment.get_abilities(user) == { "destroy": True, - "update": True, - "partial_update": True, + "update": False, + "partial_update": False, "reactions": True, "retrieve": True, }