From 5e8eeb92d9ca30673337f19c98b06908d772abfb Mon Sep 17 00:00:00 2001 From: hjlarry Date: Sun, 12 Apr 2026 18:42:08 +0800 Subject: [PATCH] fix: scope workflow comment reply mutations to tenant/app/comment --- .../console/app/workflow_comment.py | 11 +++- api/services/workflow_comment_service.py | 58 +++++++++++++++--- .../services/test_workflow_comment_service.py | 61 +++++++++++++++---- 3 files changed, 110 insertions(+), 20 deletions(-) diff --git a/api/controllers/console/app/workflow_comment.py b/api/controllers/console/app/workflow_comment.py index 2879278d82..b82323286a 100644 --- a/api/controllers/console/app/workflow_comment.py +++ b/api/controllers/console/app/workflow_comment.py @@ -266,6 +266,9 @@ class WorkflowCommentReplyDetailApi(Resource): payload = WorkflowCommentReplyPayload.model_validate(console_ns.payload or {}) reply = WorkflowCommentService.update_reply( + tenant_id=current_user.current_tenant_id, + app_id=app_model.id, + comment_id=comment_id, reply_id=reply_id, user_id=current_user.id, content=payload.content, @@ -289,7 +292,13 @@ class WorkflowCommentReplyDetailApi(Resource): comment_id=comment_id, tenant_id=current_user.current_tenant_id, app_id=app_model.id ) - WorkflowCommentService.delete_reply(reply_id=reply_id, user_id=current_user.id) + WorkflowCommentService.delete_reply( + tenant_id=current_user.current_tenant_id, + app_id=app_model.id, + comment_id=comment_id, + reply_id=reply_id, + user_id=current_user.id, + ) return {"result": "success"}, 204 diff --git a/api/services/workflow_comment_service.py b/api/services/workflow_comment_service.py index 92135cb9ce..3a59456f9f 100644 --- a/api/services/workflow_comment_service.py +++ b/api/services/workflow_comment_service.py @@ -428,14 +428,52 @@ class WorkflowCommentService: return {"id": reply.id, "created_at": reply.created_at} @staticmethod - def update_reply(reply_id: str, user_id: str, content: str, mentioned_user_ids: list[str] | None = None) -> dict: + def _get_reply_in_comment_scope( + *, + session: Session, + tenant_id: str, + app_id: str, + comment_id: str, + reply_id: str, + ) -> WorkflowCommentReply: + """Get a reply scoped to tenant/app/comment to prevent cross-thread mutations.""" + stmt = ( + select(WorkflowCommentReply) + .join(WorkflowComment, WorkflowComment.id == WorkflowCommentReply.comment_id) + .where( + WorkflowCommentReply.id == reply_id, + WorkflowCommentReply.comment_id == comment_id, + WorkflowComment.tenant_id == tenant_id, + WorkflowComment.app_id == app_id, + ) + .limit(1) + ) + reply = session.scalar(stmt) + if not reply: + raise NotFound("Reply not found") + return reply + + @staticmethod + def update_reply( + tenant_id: str, + app_id: str, + comment_id: str, + reply_id: str, + user_id: str, + content: str, + mentioned_user_ids: list[str] | None = None, + ) -> dict: """Update a comment reply and notify newly mentioned users.""" WorkflowCommentService._validate_content(content) with Session(db.engine, expire_on_commit=False) as session: - reply = session.get(WorkflowCommentReply, reply_id) - if not reply: - raise NotFound("Reply not found") + reply = WorkflowCommentService._get_reply_in_comment_scope( + session=session, + tenant_id=tenant_id, + app_id=app_id, + comment_id=comment_id, + reply_id=reply_id, + ) # Only the creator can update the reply if reply.created_by != user_id: @@ -488,12 +526,16 @@ class WorkflowCommentService: return {"id": reply.id, "updated_at": reply.updated_at} @staticmethod - def delete_reply(reply_id: str, user_id: str) -> None: + def delete_reply(tenant_id: str, app_id: str, comment_id: str, reply_id: str, user_id: str) -> None: """Delete a comment reply.""" with Session(db.engine, expire_on_commit=False) as session: - reply = session.get(WorkflowCommentReply, reply_id) - if not reply: - raise NotFound("Reply not found") + reply = WorkflowCommentService._get_reply_in_comment_scope( + session=session, + tenant_id=tenant_id, + app_id=app_id, + comment_id=comment_id, + reply_id=reply_id, + ) # Only the creator can delete the reply if reply.created_by != user_id: diff --git a/api/tests/unit_tests/services/test_workflow_comment_service.py b/api/tests/unit_tests/services/test_workflow_comment_service.py index 0120a9b007..1352ed840c 100644 --- a/api/tests/unit_tests/services/test_workflow_comment_service.py +++ b/api/tests/unit_tests/services/test_workflow_comment_service.py @@ -395,18 +395,32 @@ class TestWorkflowCommentService: mock_session.commit.assert_called_once() def test_update_reply_raises_not_found(self, mock_session: Mock) -> None: - mock_session.get.return_value = None + mock_session.scalar.return_value = None with pytest.raises(NotFound): - WorkflowCommentService.update_reply("reply-1", "user-1", "hello") + WorkflowCommentService.update_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", + reply_id="reply-1", + user_id="user-1", + content="hello", + ) def test_update_reply_raises_forbidden(self, mock_session: Mock) -> None: reply = Mock() reply.created_by = "owner" - mock_session.get.return_value = reply + mock_session.scalar.return_value = reply with pytest.raises(Forbidden): - WorkflowCommentService.update_reply("reply-1", "intruder", "hello") + WorkflowCommentService.update_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", + reply_id="reply-1", + user_id="intruder", + content="hello", + ) def test_update_reply_replaces_mentions(self, mock_session: Mock) -> None: reply = Mock() @@ -414,11 +428,18 @@ class TestWorkflowCommentService: reply.comment_id = "comment-1" reply.created_by = "owner" reply.updated_at = "updated" - mock_session.get.return_value = reply + mock_session.scalar.return_value = reply mock_session.scalars.return_value = _mock_scalars([Mock()]) + comment = Mock() + comment.tenant_id = "tenant-1" + comment.app_id = "app-1" + mock_session.get.return_value = comment with patch.object(WorkflowCommentService, "_filter_valid_mentioned_user_ids", return_value=["user-2"]): result = WorkflowCommentService.update_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", reply_id="reply-1", user_id="owner", content="new", @@ -457,24 +478,42 @@ class TestWorkflowCommentService: def test_delete_reply_raises_forbidden(self, mock_session: Mock) -> None: reply = Mock() reply.created_by = "owner" - mock_session.get.return_value = reply + mock_session.scalar.return_value = reply with pytest.raises(Forbidden): - WorkflowCommentService.delete_reply("reply-1", "intruder") + WorkflowCommentService.delete_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", + reply_id="reply-1", + user_id="intruder", + ) def test_delete_reply_raises_not_found(self, mock_session: Mock) -> None: - mock_session.get.return_value = None + mock_session.scalar.return_value = None with pytest.raises(NotFound): - WorkflowCommentService.delete_reply("reply-1", "owner") + WorkflowCommentService.delete_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", + reply_id="reply-1", + user_id="owner", + ) def test_delete_reply_removes_mentions(self, mock_session: Mock) -> None: reply = Mock() reply.created_by = "owner" - mock_session.get.return_value = reply + mock_session.scalar.return_value = reply mock_session.scalars.return_value = _mock_scalars([Mock(), Mock()]) - WorkflowCommentService.delete_reply("reply-1", "owner") + WorkflowCommentService.delete_reply( + tenant_id="tenant-1", + app_id="app-1", + comment_id="comment-1", + reply_id="reply-1", + user_id="owner", + ) assert mock_session.delete.call_count == 3 mock_session.commit.assert_called_once()