From f0bea6235dc6bc17cd0ac3e00469bd1de73681ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= Date: Mon, 30 Mar 2026 13:34:59 +0200 Subject: [PATCH] fix(web): reuse project combobox in timeline popovers --- .../src/components/comments/CommentThread.tsx | 35 +-- .../estimates/EstimateWorkspaceClient.tsx | 4 +- .../timeline/BatchAssignPopover.tsx | 68 +---- .../timeline/NewAllocationPopover.tsx | 56 +--- docs/audience-scoping-backlog.md | 21 +- docs/comment-visibility-architecture.md | 74 +++++ docs/route-access-matrix.md | 11 + .../src/__tests__/assistant-router.test.ts | 11 + .../assistant-tools-import-export.test.ts | 40 ++- .../src/__tests__/comment-router-auth.test.ts | 261 ++++++++++++++++++ packages/api/src/router/assistant-tools.ts | 14 +- packages/api/src/router/assistant.ts | 3 + packages/api/src/router/comment.ts | 130 ++++++--- 13 files changed, 525 insertions(+), 203 deletions(-) create mode 100644 docs/comment-visibility-architecture.md create mode 100644 packages/api/src/__tests__/comment-router-auth.test.ts diff --git a/apps/web/src/components/comments/CommentThread.tsx b/apps/web/src/components/comments/CommentThread.tsx index ff2fe5f..71037a1 100644 --- a/apps/web/src/components/comments/CommentThread.tsx +++ b/apps/web/src/components/comments/CommentThread.tsx @@ -31,10 +31,7 @@ interface CommentItem { replies: CommentReply[]; } -type CommentEntityType = "estimate"; - interface CommentThreadProps { - entityType: CommentEntityType; entityId: string; } @@ -113,37 +110,36 @@ function CommentBody({ body }: { body: string }) { function SingleComment({ comment, - entityType, entityId, isReply = false, }: { comment: CommentItem | CommentReply; - entityType: CommentEntityType; entityId: string; isReply?: boolean; }) { const [showReplyInput, setShowReplyInput] = useState(false); const [confirmDelete, setConfirmDelete] = useState(false); const utils = trpc.useUtils(); + const commentTarget = { entityType: "estimate" as const, entityId }; const createMutation = trpc.comment.create.useMutation({ onSuccess: () => { setShowReplyInput(false); - void utils.comment.list.invalidate({ entityType, entityId }); - void utils.comment.count.invalidate({ entityType, entityId }); + void utils.comment.list.invalidate(commentTarget); + void utils.comment.count.invalidate(commentTarget); }, }); const resolveMutation = trpc.comment.resolve.useMutation({ onSuccess: () => { - void utils.comment.list.invalidate({ entityType, entityId }); + void utils.comment.list.invalidate(commentTarget); }, }); const deleteMutation = trpc.comment.delete.useMutation({ onSuccess: () => { - void utils.comment.list.invalidate({ entityType, entityId }); - void utils.comment.count.invalidate({ entityType, entityId }); + void utils.comment.list.invalidate(commentTarget); + void utils.comment.count.invalidate(commentTarget); }, }); @@ -217,12 +213,12 @@ function SingleComment({ {showReplyInput && (
{ createMutation.mutate({ - entityType, + entityType: commentTarget.entityType, entityId, parentId: comment.id, body: replyBody, @@ -259,7 +255,6 @@ function SingleComment({ @@ -270,18 +265,19 @@ function SingleComment({ ); } -export function CommentThread({ entityType, entityId }: CommentThreadProps) { +export function CommentThread({ entityId }: CommentThreadProps) { const utils = trpc.useUtils(); + const commentTarget = { entityType: "estimate" as const, entityId }; const commentsQuery = trpc.comment.list.useQuery( - { entityType, entityId }, + commentTarget, { staleTime: 10_000 }, ); const createMutation = trpc.comment.create.useMutation({ onSuccess: () => { - void utils.comment.list.invalidate({ entityType, entityId }); - void utils.comment.count.invalidate({ entityType, entityId }); + void utils.comment.list.invalidate(commentTarget); + void utils.comment.count.invalidate(commentTarget); }, }); @@ -312,7 +308,6 @@ export function CommentThread({ entityType, entityId }: CommentThreadProps) { ))} @@ -322,11 +317,11 @@ export function CommentThread({ entityType, entityId }: CommentThreadProps) { {/* New comment input */}
{ createMutation.mutate({ - entityType, + entityType: commentTarget.entityType, entityId, body, }); diff --git a/apps/web/src/components/estimates/EstimateWorkspaceClient.tsx b/apps/web/src/components/estimates/EstimateWorkspaceClient.tsx index 98f1a81..7f4672d 100644 --- a/apps/web/src/components/estimates/EstimateWorkspaceClient.tsx +++ b/apps/web/src/components/estimates/EstimateWorkspaceClient.tsx @@ -135,7 +135,7 @@ export function EstimateWorkspaceClient({ estimateId }: { estimateId: string }) const commentCountQuery = trpc.comment.count.useQuery( { entityType: "estimate", entityId: estimateId }, - { staleTime: 30_000 }, + { enabled: canViewCosts, staleTime: 30_000 }, ); const commentCount = commentCountQuery.data ?? 0; @@ -364,7 +364,7 @@ export function EstimateWorkspaceClient({ estimateId }: { estimateId: string })

Comments

- +
)} diff --git a/apps/web/src/components/timeline/BatchAssignPopover.tsx b/apps/web/src/components/timeline/BatchAssignPopover.tsx index 5dd8f84..956767f 100644 --- a/apps/web/src/components/timeline/BatchAssignPopover.tsx +++ b/apps/web/src/components/timeline/BatchAssignPopover.tsx @@ -6,6 +6,7 @@ import { createPortal } from "react-dom"; import { AllocationStatus } from "@capakraken/shared"; import { trpc } from "~/lib/trpc/client.js"; import { useInvalidateTimeline } from "~/hooks/useInvalidatePlanningViews.js"; +import { ProjectCombobox } from "~/components/ui/ProjectCombobox.js"; interface BatchAssignPopoverProps { resourceIds: string[]; @@ -33,23 +34,10 @@ export function BatchAssignPopover({ const ref = useRef(null); const invalidateTimeline = useInvalidateTimeline(); - const [search, setSearch] = useState(""); const [selectedProjectId, setSelectedProjectId] = useState( null, ); const [hoursPerDay, setHoursPerDay] = useState(8); - const [dropdownOpen, setDropdownOpen] = useState(true); - - const { data: projectsData } = trpc.project.list.useQuery( - { search, limit: 20 }, - { staleTime: 30_000 }, - ); - - const projects = (projectsData?.projects ?? []) as Array<{ - id: string; - name: string; - }>; - const selectedProject = projects.find((p) => p.id === selectedProjectId); const batchMutation = trpc.timeline.batchQuickAssign.useMutation({ onSuccess: () => { @@ -136,54 +124,12 @@ export function BatchAssignPopover({ - {selectedProject && !dropdownOpen ? ( -
{ - setDropdownOpen(true); - setSearch(""); - }} - > - - {selectedProject.name} - - - ▾ - -
- ) : ( -
- setSearch(e.target.value)} - onFocus={() => setDropdownOpen(true)} - className="w-full border border-gray-200 dark:border-gray-600 dark:bg-gray-700 dark:text-gray-200 dark:placeholder-gray-400 rounded-lg px-3 py-2 text-sm focus:outline-none focus:ring-2 focus:ring-sky-400 dark:focus:ring-sky-500" - /> - {dropdownOpen && projects.length > 0 && ( -
- {projects.map((p) => ( - - ))} -
- )} -
- )} +
{/* Hours per day */} diff --git a/apps/web/src/components/timeline/NewAllocationPopover.tsx b/apps/web/src/components/timeline/NewAllocationPopover.tsx index 1e1f27c..26cd253 100644 --- a/apps/web/src/components/timeline/NewAllocationPopover.tsx +++ b/apps/web/src/components/timeline/NewAllocationPopover.tsx @@ -1,13 +1,14 @@ "use client"; import { clsx } from "clsx"; -import { useEffect, useRef, useState } from "react"; +import { useState } from "react"; import { createPortal } from "react-dom"; import { AllocationStatus } from "@capakraken/shared"; import { trpc } from "~/lib/trpc/client.js"; import { useInvalidateTimeline } from "~/hooks/useInvalidatePlanningViews.js"; import { useViewportPopover } from "~/hooks/useViewportPopover.js"; import { DateInput } from "~/components/ui/DateInput.js"; +import { ProjectCombobox } from "~/components/ui/ProjectCombobox.js"; interface NewAllocationPopoverProps { resourceId: string; @@ -46,7 +47,6 @@ export function NewAllocationPopover({ }); const invalidateTimeline = useInvalidateTimeline(); - const [search, setSearch] = useState(""); const [selectedProjectId, setSelectedProjectId] = useState( suggestedProjectId ?? null, ); @@ -54,17 +54,6 @@ export function NewAllocationPopover({ const [hoursPerDay, setHoursPerDay] = useState(8); const [start, setStart] = useState(toDateInput(startDate)); const [end, setEnd] = useState(toDateInput(endDate)); - const [dropdownOpen, setDropdownOpen] = useState(!suggestedProjectId); - - const { data: projectsData } = trpc.project.list.useQuery( - { search, limit: 20 }, - { staleTime: 30_000 }, - ); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const projects = (projectsData?.projects ?? []) as Array<{ id: string; name: string; orderType?: string }>; - const selectedProject = projects.find((p) => p.id === selectedProjectId) - ?? (suggestedProjectId ? projects.find((p) => p.id === suggestedProjectId) : null); const createMutation = trpc.timeline.quickAssign.useMutation({ onSuccess: () => { @@ -126,41 +115,12 @@ export function NewAllocationPopover({ {/* Project picker */}
- {selectedProject && !dropdownOpen ? ( -
{ setDropdownOpen(true); setSearch(""); }} - > - {selectedProject.name} - ▾ -
- ) : ( -
- setSearch(e.target.value)} - onFocus={() => setDropdownOpen(true)} - className="w-full border border-gray-200 dark:border-gray-600 dark:bg-gray-700 dark:text-gray-200 dark:placeholder-gray-400 rounded-lg px-3 py-2 text-sm focus:outline-none focus:ring-2 focus:ring-brand-400" - /> - {dropdownOpen && projects.length > 0 && ( -
- {projects.map((p) => ( - - ))} -
- )} -
- )} +
{/* Role */} diff --git a/docs/audience-scoping-backlog.md b/docs/audience-scoping-backlog.md index c6387a1..188fd7d 100644 --- a/docs/audience-scoping-backlog.md +++ b/docs/audience-scoping-backlog.md @@ -19,6 +19,7 @@ - `project.isImageGenConfigured`, `project.isDalleConfigured`: covered as authenticated low-risk configuration checks - `notification` self-service and manager boundaries: auth-covered across list, unread counts, reminders, deletes, broadcasts, task creation, and assignment boundaries - `assistant-tools` parity metadata: descriptions and parity assertions now match narrowed router audiences for resource overview, controller-only, self-service, and manager broadcast/task tools +- `comment` architecture phase 1: generic free-form entity comments replaced by an explicit supported-entity registry, currently limited to `estimate` with controller/manager/admin access plus entity-aware checks on list/count/create/resolve/delete ### Dirty Files To Avoid Mixing Into This Batch @@ -45,27 +46,9 @@ These files already have unrelated local edits. Audience parity work that would - the previously identified small hardening and tests/docs candidates have been completed, including the notification auth follow-up and assistant tool parity metadata cleanup - the remaining audience work is now architectural (`comment.ts`) or depends on broader policy decisions rather than another ready-made auth slice -### Needs Architecture Or Policy Design - -These routes should not be batch-edited as “small safe slices” until a visibility model exists. - -#### `packages/api/src/router/comment.ts` - -- Current state: all core routes are `protectedProcedure` -- Why this is not a small slice: - - reads and writes are keyed by generic `entityType` and `entityId` - - visibility depends on the backing entity, not on the comment record alone - - the current author/admin checks for resolve/delete are not enough to decide who may list or create comments on each entity class -- Design work needed: - - define which entity types can host comments - - map each entity type to its audience source of truth - - centralize entity visibility checks before any comment read/write -- Recommended path: - - treat comments as a separate architecture ticket, not part of the quick hardening batch - ## Recommended Next Order -1. `comment` architecture design ticket +1. extend the comment entity registry only when a second real consumer exists and its backing audience is explicitly documented ## Slice Definition diff --git a/docs/comment-visibility-architecture.md b/docs/comment-visibility-architecture.md new file mode 100644 index 0000000..5ea99ae --- /dev/null +++ b/docs/comment-visibility-architecture.md @@ -0,0 +1,74 @@ +# Comment Visibility Architecture + +**Date:** 2026-03-30 +**Status:** Phase 1 implemented + +## Problem + +The original comment router accepted arbitrary `entityType` and `entityId` pairs behind `protectedProcedure`. +That was too broad: + +- comment visibility depends on the backing entity, not on the comment record alone +- generic strings allowed clients and assistant tools to imply support for entity types that had no explicit policy +- author/admin checks on `resolve` and `delete` were not enough, because list/create access was still effectively "any authenticated user" + +## Current Product Reality + +There is only one real first-party consumer today: + +- web UI estimate workspace comments via `entityType: "estimate"` + +The older examples for `scope_item`, `estimate_version`, and `demand_line` were aspirational, not backed by an explicit visibility model or active UI. + +## Architecture Decision + +Comments now use an explicit entity registry. + +- supported entity types are allowlisted, not free-form +- each entity type owns: + - its audience rule + - its existence check + - its deep link builder for notifications +- every comment route calls the entity access layer before touching comment data + +## Phase 1 Policy + +Supported entity types: + +- `estimate` + +Audience: + +- same audience as the estimate workspace +- controller, manager, or admin only + +Route effects: + +- `list`, `count`, `create`, `resolve`, and `delete` all require estimate visibility first +- `resolve` and `delete` still require comment author or admin after entity visibility is granted +- replies are only allowed when the parent comment belongs to the same entity tuple +- mention notifications use the entity policy link builder instead of hardcoded route assumptions scattered through the router + +## Why This Shape + +- It closes the real security gap now without pretending a generic multi-entity policy already exists. +- It keeps future comment expansion additive: a new entity type must be onboarded deliberately. +- It gives the assistant and UI one source of truth for what is actually supported today. + +## Extension Rules For Future Entity Types + +To add another commentable entity: + +1. Add the entity type to the registry, not just to input examples. +2. Define the backing audience source of truth. +3. Add an existence check for that entity. +4. Add a notification link builder for that entity. +5. Update assistant tool metadata and assistant visibility gates in the same change. +6. Add router auth tests for unauthenticated, plain authenticated, and elevated callers. +7. Update `docs/route-access-matrix.md`. + +## Non-Goals In Phase 1 + +- generic comment support for arbitrary entities +- row-level polymorphic authorization based only on `entityType` strings +- automatic inheritance for future entities without explicit onboarding diff --git a/docs/route-access-matrix.md b/docs/route-access-matrix.md index f17e886..718d8f4 100644 --- a/docs/route-access-matrix.md +++ b/docs/route-access-matrix.md @@ -73,6 +73,17 @@ Reasoning: - `list`: `controller-finance` - drafting, versioning, export generation, and approval writes: `manager-write` +### `packages/api/src/router/comment.ts` + +- `list`, `count`, `create`, `resolve`, `delete`: `entity-scoped` + +Reasoning: + +- comments must inherit the audience of the backing entity, not the comment row itself +- Phase 1 intentionally supports only `estimate`, because that is the only real product consumer today +- estimate comments therefore inherit the estimate workspace audience: controller, manager, or admin +- future entity types must be added through an explicit registry with per-entity access checks, assistant parity, and router tests in the same slice + ### `packages/api/src/router/system-role-config.ts` - all reads and writes: `admin-only` diff --git a/packages/api/src/__tests__/assistant-router.test.ts b/packages/api/src/__tests__/assistant-router.test.ts index 1e7138c..32f6613 100644 --- a/packages/api/src/__tests__/assistant-router.test.ts +++ b/packages/api/src/__tests__/assistant-router.test.ts @@ -361,6 +361,9 @@ describe("assistant router tool gating", () => { expect(controllerNames).toContain("query_change_history"); expect(controllerNames).toContain("get_entity_timeline"); expect(controllerNames).toContain("search_by_skill"); + expect(controllerNames).toContain("list_comments"); + expect(controllerNames).toContain("create_comment"); + expect(controllerNames).toContain("resolve_comment"); expect(controllerNames).toContain("export_resources_csv"); expect(controllerNames).toContain("export_projects_csv"); expect(controllerNames).toContain("list_audit_log_entries"); @@ -373,6 +376,9 @@ describe("assistant router tool gating", () => { expect(userNames).not.toContain("query_change_history"); expect(userNames).not.toContain("get_entity_timeline"); expect(userNames).not.toContain("search_by_skill"); + expect(userNames).not.toContain("list_comments"); + expect(userNames).not.toContain("create_comment"); + expect(userNames).not.toContain("resolve_comment"); expect(userNames).not.toContain("export_resources_csv"); expect(userNames).not.toContain("export_projects_csv"); expect(userNames).not.toContain("list_audit_log_entries"); @@ -957,6 +963,11 @@ describe("assistant router tool gating", () => { expect(toolDescriptions.get("get_current_user")).toContain("authenticated user's own profile"); expect(toolDescriptions.get("search_resources")).toContain("Resource overview access required"); expect(toolDescriptions.get("search_by_skill")).toContain("Controller/manager/admin access required"); + expect(toolDescriptions.get("list_comments")).toContain("Currently only estimate comments are enabled"); + expect(toolDescriptions.get("list_comments")).toContain("Controller/manager/admin access required"); + expect(toolDescriptions.get("create_comment")).toContain("Currently only estimate comments are enabled"); + expect(toolDescriptions.get("create_comment")).toContain("Controller/manager/admin access required"); + expect(toolDescriptions.get("resolve_comment")).toContain("Currently only estimate comments are enabled"); expect(toolDescriptions.get("list_notifications")).toContain("current user"); expect(toolDescriptions.get("get_unread_notification_count")).toContain("current user"); expect(toolDescriptions.get("list_tasks")).toContain("current user"); diff --git a/packages/api/src/__tests__/assistant-tools-import-export.test.ts b/packages/api/src/__tests__/assistant-tools-import-export.test.ts index 20bb7dd..32a68f2 100644 --- a/packages/api/src/__tests__/assistant-tools-import-export.test.ts +++ b/packages/api/src/__tests__/assistant-tools-import-export.test.ts @@ -4845,10 +4845,12 @@ describe("assistant import/export and dispo tools", () => { const commentFindUnique = vi.fn().mockResolvedValue({ id: "comment_1", authorId: "user_1", + entityType: "estimate", + entityId: "est_1", }); const db = { - user: { - findUnique: vi.fn().mockResolvedValue({ id: "user_1", systemRole: "USER" }), + estimate: { + findUnique: vi.fn().mockResolvedValue({ id: "est_1" }), }, comment: { findMany: vi.fn().mockResolvedValue([ @@ -4931,8 +4933,17 @@ describe("assistant import/export and dispo tools", () => { }, orderBy: { createdAt: "asc" }, }); - expect(db.user.findUnique).toHaveBeenCalledWith({ - where: { email: "assistant@example.com" }, + expect(db.estimate.findUnique).toHaveBeenCalledTimes(3); + expect(db.estimate.findUnique).toHaveBeenNthCalledWith(1, { + where: { id: "est_1" }, + select: { id: true }, + }); + expect(db.estimate.findUnique).toHaveBeenNthCalledWith(2, { + where: { id: "est_1" }, + select: { id: true }, + }); + expect(db.estimate.findUnique).toHaveBeenNthCalledWith(3, { + where: { id: "est_1" }, select: { id: true }, }); expect(db.comment.create).toHaveBeenCalledWith({ @@ -4949,7 +4960,7 @@ describe("assistant import/export and dispo tools", () => { }); expect(commentFindUnique).toHaveBeenCalledWith({ where: { id: "comment_1" }, - select: { id: true, authorId: true }, + select: { id: true, authorId: true, entityType: true, entityId: true }, }); expect(db.comment.update).toHaveBeenCalledWith({ where: { id: "comment_1" }, @@ -5003,7 +5014,7 @@ describe("assistant import/export and dispo tools", () => { it("returns a stable assistant error when creating a comment with an empty body", async () => { const ctx = createToolContext( { - user: { + estimate: { findUnique: vi.fn(), }, comment: { @@ -5026,14 +5037,13 @@ describe("assistant import/export and dispo tools", () => { expect(JSON.parse(result.content)).toEqual({ error: "Comment body is required.", }); - expect(ctx.db.user.findUnique).not.toHaveBeenCalled(); expect(ctx.db.comment.create).not.toHaveBeenCalled(); }); it("returns a stable assistant error when creating a comment with a body that is too long", async () => { const ctx = createToolContext( { - user: { + estimate: { findUnique: vi.fn(), }, comment: { @@ -5056,15 +5066,14 @@ describe("assistant import/export and dispo tools", () => { expect(JSON.parse(result.content)).toEqual({ error: "Comment body must be at most 10000 characters.", }); - expect(ctx.db.user.findUnique).not.toHaveBeenCalled(); expect(ctx.db.comment.create).not.toHaveBeenCalled(); }); it("returns a stable assistant error when the comment author disappears during creation", async () => { const ctx = createToolContext( { - user: { - findUnique: vi.fn().mockResolvedValue({ id: "user_1" }), + estimate: { + findUnique: vi.fn().mockResolvedValue({ id: "est_1" }), }, comment: { create: vi.fn().mockRejectedValue({ @@ -5095,8 +5104,8 @@ describe("assistant import/export and dispo tools", () => { it("returns a stable assistant error when a mentioned user disappears during comment creation", async () => { const ctx = createToolContext( { - user: { - findUnique: vi.fn().mockResolvedValue({ id: "user_1" }), + estimate: { + findUnique: vi.fn().mockResolvedValue({ id: "est_1" }), }, comment: { create: vi.fn().mockResolvedValue({ @@ -5170,8 +5179,13 @@ describe("assistant import/export and dispo tools", () => { findUnique: vi.fn().mockResolvedValue({ id: "comment_1", authorId: "user_2", + entityType: "estimate", + entityId: "est_1", }), }, + estimate: { + findUnique: vi.fn().mockResolvedValue({ id: "est_1" }), + }, }, { userRole: SystemRole.CONTROLLER }, ); diff --git a/packages/api/src/__tests__/comment-router-auth.test.ts b/packages/api/src/__tests__/comment-router-auth.test.ts new file mode 100644 index 0000000..bdd08e6 --- /dev/null +++ b/packages/api/src/__tests__/comment-router-auth.test.ts @@ -0,0 +1,261 @@ +import { SystemRole } from "@capakraken/shared"; +import { describe, expect, it, vi } from "vitest"; +import { commentRouter } from "../router/comment.js"; +import { createCallerFactory } from "../trpc.js"; + +const createCaller = createCallerFactory(commentRouter); + +function createContext( + db: Record, + options: { + role?: SystemRole; + session?: boolean; + } = {}, +) { + const { role = SystemRole.USER, session = true } = options; + + return { + session: session + ? { + user: { email: "user@example.com", name: "User", image: null }, + expires: "2099-01-01T00:00:00.000Z", + } + : null, + db: db as never, + dbUser: session + ? { + id: role === SystemRole.ADMIN ? "user_admin" : "user_1", + systemRole: role, + permissionOverrides: null, + } + : null, + }; +} + +describe("comment router authorization", () => { + it("requires authentication before listing estimate comments", async () => { + const estimateFindUnique = vi.fn(); + const commentFindMany = vi.fn(); + const caller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findMany: commentFindMany, + }, + }, { session: false })); + + await expect(caller.list({ entityType: "estimate", entityId: "est_1" })).rejects.toMatchObject({ + code: "UNAUTHORIZED", + message: "Authentication required", + }); + + expect(estimateFindUnique).not.toHaveBeenCalled(); + expect(commentFindMany).not.toHaveBeenCalled(); + }); + + it("forbids plain users from reading or creating estimate comments", async () => { + const estimateFindUnique = vi.fn(); + const commentFindMany = vi.fn(); + const commentCount = vi.fn(); + const commentCreate = vi.fn(); + const caller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findMany: commentFindMany, + count: commentCount, + create: commentCreate, + }, + })); + + await expect(caller.list({ entityType: "estimate", entityId: "est_1" })).rejects.toMatchObject({ + code: "FORBIDDEN", + message: "Controller access required", + }); + await expect(caller.count({ entityType: "estimate", entityId: "est_1" })).rejects.toMatchObject({ + code: "FORBIDDEN", + message: "Controller access required", + }); + await expect(caller.create({ + entityType: "estimate", + entityId: "est_1", + body: "Please review this estimate.", + })).rejects.toMatchObject({ + code: "FORBIDDEN", + message: "Controller access required", + }); + + expect(estimateFindUnique).not.toHaveBeenCalled(); + expect(commentFindMany).not.toHaveBeenCalled(); + expect(commentCount).not.toHaveBeenCalled(); + expect(commentCreate).not.toHaveBeenCalled(); + }); + + it("allows controllers to list, count, and create estimate comments", async () => { + const estimateFindUnique = vi.fn().mockResolvedValue({ id: "est_1" }); + const commentFindMany = vi.fn().mockResolvedValue([]); + const commentCount = vi.fn().mockResolvedValue(2); + const commentCreate = vi.fn().mockResolvedValue({ + id: "comment_1", + body: "Please review this estimate.", + author: { id: "user_1", name: "Controller User", email: "user@example.com", image: null }, + }); + const caller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findMany: commentFindMany, + count: commentCount, + create: commentCreate, + }, + notification: { + create: vi.fn(), + }, + auditLog: { + create: vi.fn(), + }, + }, { role: SystemRole.CONTROLLER })); + + const listResult = await caller.list({ entityType: "estimate", entityId: "est_1" }); + const countResult = await caller.count({ entityType: "estimate", entityId: "est_1" }); + const createResult = await caller.create({ + entityType: "estimate", + entityId: "est_1", + body: "Please review this estimate.", + }); + + expect(listResult).toEqual([]); + expect(countResult).toBe(2); + expect(createResult.id).toBe("comment_1"); + expect(estimateFindUnique).toHaveBeenCalledTimes(3); + expect(commentCreate).toHaveBeenCalledWith({ + data: { + entityType: "estimate", + entityId: "est_1", + authorId: "user_1", + body: "Please review this estimate.", + mentions: [], + }, + include: { + author: { select: { id: true, name: true, email: true, image: true } }, + }, + }); + }); + + it("rejects unsupported comment entity types before touching the database", async () => { + const estimateFindUnique = vi.fn(); + const commentFindMany = vi.fn(); + const caller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findMany: commentFindMany, + }, + }, { role: SystemRole.CONTROLLER })); + + await expect(caller.list({ + entityType: "scope_item" as never, + entityId: "scope_1", + })).rejects.toMatchObject({ + code: "BAD_REQUEST", + }); + + expect(estimateFindUnique).not.toHaveBeenCalled(); + expect(commentFindMany).not.toHaveBeenCalled(); + }); + + it("rejects replies whose parent comment belongs to another entity", async () => { + const estimateFindUnique = vi.fn().mockResolvedValue({ id: "est_1" }); + const parentFindUnique = vi.fn().mockResolvedValue({ + id: "comment_parent", + entityType: "estimate", + entityId: "est_2", + }); + const commentCreate = vi.fn(); + const caller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findUnique: parentFindUnique, + create: commentCreate, + }, + }, { role: SystemRole.CONTROLLER })); + + await expect(caller.create({ + entityType: "estimate", + entityId: "est_1", + parentId: "comment_parent", + body: "Replying on the right estimate.", + })).rejects.toMatchObject({ + code: "BAD_REQUEST", + message: "Parent comment does not belong to the requested entity", + }); + + expect(commentCreate).not.toHaveBeenCalled(); + }); + + it("requires comment authorship or admin rights after entity visibility is granted", async () => { + const estimateFindUnique = vi.fn().mockResolvedValue({ id: "est_1" }); + const commentFindUnique = vi.fn().mockResolvedValue({ + id: "comment_1", + authorId: "user_2", + entityType: "estimate", + entityId: "est_1", + }); + const commentUpdate = vi.fn(); + const controllerCaller = createCaller(createContext({ + estimate: { + findUnique: estimateFindUnique, + }, + comment: { + findUnique: commentFindUnique, + update: commentUpdate, + }, + }, { role: SystemRole.CONTROLLER })); + + await expect(controllerCaller.resolve({ id: "comment_1", resolved: true })).rejects.toMatchObject({ + code: "FORBIDDEN", + message: "Only the comment author or an admin can resolve comments", + }); + + const adminUpdate = vi.fn().mockResolvedValue({ + id: "comment_1", + body: "Needs review", + resolved: true, + author: { id: "user_2", name: "Other User", email: "other@example.com", image: null }, + }); + const adminCaller = createCaller(createContext({ + estimate: { + findUnique: vi.fn().mockResolvedValue({ id: "est_1" }), + }, + comment: { + findUnique: vi.fn().mockResolvedValue({ + id: "comment_1", + authorId: "user_2", + entityType: "estimate", + entityId: "est_1", + }), + update: adminUpdate, + }, + auditLog: { + create: vi.fn(), + }, + }, { role: SystemRole.ADMIN })); + + const result = await adminCaller.resolve({ id: "comment_1", resolved: true }); + + expect(result.resolved).toBe(true); + expect(adminUpdate).toHaveBeenCalledWith({ + where: { id: "comment_1" }, + data: { resolved: true }, + include: { + author: { select: { id: true, name: true, email: true, image: true } }, + }, + }); + }); +}); diff --git a/packages/api/src/router/assistant-tools.ts b/packages/api/src/router/assistant-tools.ts index d7f1e30..bec040a 100644 --- a/packages/api/src/router/assistant-tools.ts +++ b/packages/api/src/router/assistant-tools.ts @@ -4371,11 +4371,11 @@ export const TOOL_DEFINITIONS: ToolDef[] = [ type: "function", function: { name: "list_comments", - description: "List comments (with replies) for a specific entity such as an estimate, scope item, or demand line.", + description: "List comments (with replies) for a supported comment-enabled entity. Currently only estimate comments are enabled. Controller/manager/admin access required.", parameters: { type: "object", properties: { - entityType: { type: "string", description: "Entity type (e.g. 'estimate', 'estimate_version', 'scope_item', 'demand_line')" }, + entityType: { type: "string", enum: ["estimate"], description: "Supported entity type. Currently only 'estimate'." }, entityId: { type: "string", description: "Entity ID" }, }, required: ["entityType", "entityId"], @@ -4450,11 +4450,11 @@ export const TOOL_DEFINITIONS: ToolDef[] = [ type: "function", function: { name: "create_comment", - description: "Add a comment to an entity (estimate, scope item, demand line, etc.). Supports @mentions. Always confirm with the user first.", + description: "Add a comment to a supported comment-enabled entity. Currently only estimate comments are enabled. Controller/manager/admin access required. Supports @mentions. Always confirm with the user first.", parameters: { type: "object", properties: { - entityType: { type: "string", description: "Entity type (e.g. 'estimate', 'estimate_version', 'scope_item')" }, + entityType: { type: "string", enum: ["estimate"], description: "Supported entity type. Currently only 'estimate'." }, entityId: { type: "string", description: "Entity ID" }, body: { type: "string", description: "Comment body text. Use @[Name](userId) for mentions." }, }, @@ -4466,7 +4466,7 @@ export const TOOL_DEFINITIONS: ToolDef[] = [ type: "function", function: { name: "resolve_comment", - description: "Mark a comment as resolved (or unresolve it). Only the comment author or an admin can do this.", + description: "Mark a comment as resolved (or unresolve it) on a supported comment-enabled entity. Currently only estimate comments are enabled. Controller/manager/admin visibility is required, and only the comment author or an admin can change resolution.", parameters: { type: "object", properties: { @@ -8863,7 +8863,7 @@ const executors = { }; }, - async list_comments(params: { entityType: string; entityId: string }, ctx: ToolContext) { + async list_comments(params: { entityType: "estimate"; entityId: string }, ctx: ToolContext) { const caller = createCommentCaller(createScopedCallerContext(ctx)); const comments = await caller.list({ entityType: params.entityType, @@ -8965,7 +8965,7 @@ const executors = { }, async create_comment(params: { - entityType: string; + entityType: "estimate"; entityId: string; body: string; }, ctx: ToolContext) { diff --git a/packages/api/src/router/assistant.ts b/packages/api/src/router/assistant.ts index 86992f8..feef38d 100644 --- a/packages/api/src/router/assistant.ts +++ b/packages/api/src/router/assistant.ts @@ -251,6 +251,9 @@ const RESOURCE_OVERVIEW_TOOLS = new Set([ /** Tools that follow controllerProcedure access rules in the main API. */ const CONTROLLER_ONLY_TOOLS = new Set([ "search_by_skill", + "list_comments", + "create_comment", + "resolve_comment", "search_projects", "get_project", "search_estimates", diff --git a/packages/api/src/router/comment.ts b/packages/api/src/router/comment.ts index a0e372f..7ff5aa3 100644 --- a/packages/api/src/router/comment.ts +++ b/packages/api/src/router/comment.ts @@ -1,34 +1,12 @@ import { z } from "zod"; import { TRPCError } from "@trpc/server"; import { SystemRole } from "@capakraken/shared"; -import { createTRPCRouter, protectedProcedure } from "../trpc.js"; +import { createTRPCRouter, protectedProcedure, type TRPCContext } from "../trpc.js"; import { createNotification } from "../lib/create-notification.js"; import { createAuditEntry } from "../lib/audit.js"; // ─── Helpers ────────────────────────────────────────────────────────────────── -/** Resolve the DB user id from the session email. Throws UNAUTHORIZED if not found. */ -async function resolveUserId(ctx: { - db: { - user: { - findUnique: (args: { - where: { email: string }; - select: { id: true }; - }) => Promise<{ id: string } | null>; - }; - }; - session: { user?: { email?: string | null } | null }; -}): Promise { - const email = ctx.session.user?.email; - if (!email) throw new TRPCError({ code: "UNAUTHORIZED" }); - const user = await ctx.db.user.findUnique({ - where: { email }, - select: { id: true }, - }); - if (!user) throw new TRPCError({ code: "UNAUTHORIZED" }); - return user.id; -} - /** * Parse @mentions from comment body. * Pattern: @[Display Name](userId) @@ -44,6 +22,74 @@ function parseMentions(body: string): string[] { return Array.from(ids); } +const COMMENT_ENTITY_TYPE_VALUES = ["estimate"] as const; +const CommentEntityTypeSchema = z.enum(COMMENT_ENTITY_TYPE_VALUES); +type CommentEntityType = z.infer; + +type CommentEntityPolicy = { + assertAccess: (ctx: Pick, entityId: string) => Promise; + buildLink: (entityId: string) => string; +}; + +const CONTROLLER_COMMENT_ROLES = new Set([ + SystemRole.ADMIN, + SystemRole.MANAGER, + SystemRole.CONTROLLER, +]); + +async function assertEstimateCommentAccess(ctx: Pick, entityId: string) { + const role = ctx.dbUser?.systemRole as SystemRole | undefined; + if (!role || !CONTROLLER_COMMENT_ROLES.has(role)) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Controller access required", + }); + } + + const estimate = await ctx.db.estimate.findUnique({ + where: { id: entityId }, + select: { id: true }, + }); + if (!estimate) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "Estimate not found", + }); + } +} + +const COMMENT_ENTITY_POLICIES: Record = { + estimate: { + assertAccess: assertEstimateCommentAccess, + buildLink: (entityId) => `/estimates/${entityId}?tab=comments`, + }, +}; + +function isSupportedCommentEntityType(value: string): value is CommentEntityType { + return (COMMENT_ENTITY_TYPE_VALUES as readonly string[]).includes(value); +} + +function getCommentEntityPolicy(entityType: string): CommentEntityPolicy { + if (!isSupportedCommentEntityType(entityType)) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Comments are not supported for entity type: ${entityType}`, + }); + } + + return COMMENT_ENTITY_POLICIES[entityType]; +} + +async function assertCommentEntityAccess( + ctx: Pick, + entityType: string, + entityId: string, +) { + const policy = getCommentEntityPolicy(entityType); + await policy.assertAccess(ctx, entityId); + return policy; +} + // ─── Router ─────────────────────────────────────────────────────────────────── export const commentRouter = createTRPCRouter({ @@ -51,11 +97,13 @@ export const commentRouter = createTRPCRouter({ list: protectedProcedure .input( z.object({ - entityType: z.string(), + entityType: CommentEntityTypeSchema, entityId: z.string(), }), ) .query(async ({ ctx, input }) => { + await assertCommentEntityAccess(ctx, input.entityType, input.entityId); + return ctx.db.comment.findMany({ where: { entityType: input.entityType, @@ -79,11 +127,13 @@ export const commentRouter = createTRPCRouter({ count: protectedProcedure .input( z.object({ - entityType: z.string(), + entityType: CommentEntityTypeSchema, entityId: z.string(), }), ) .query(async ({ ctx, input }) => { + await assertCommentEntityAccess(ctx, input.entityType, input.entityId); + return ctx.db.comment.count({ where: { entityType: input.entityType, @@ -96,25 +146,33 @@ export const commentRouter = createTRPCRouter({ create: protectedProcedure .input( z.object({ - entityType: z.string(), + entityType: CommentEntityTypeSchema, entityId: z.string(), parentId: z.string().optional(), body: z.string().min(1).max(10_000), }), ) .mutation(async ({ ctx, input }) => { - const authorId = await resolveUserId(ctx); + const policy = await assertCommentEntityAccess(ctx, input.entityType, input.entityId); + const authorId = ctx.dbUser?.id; + if (!authorId) throw new TRPCError({ code: "UNAUTHORIZED" }); const mentions = parseMentions(input.body); // If replying, verify the parent exists if (input.parentId) { const parent = await ctx.db.comment.findUnique({ where: { id: input.parentId }, - select: { id: true }, + select: { id: true, entityType: true, entityId: true }, }); if (!parent) { throw new TRPCError({ code: "NOT_FOUND", message: "Parent comment not found" }); } + if (parent.entityType !== input.entityType || parent.entityId !== input.entityId) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Parent comment does not belong to the requested entity", + }); + } } const comment = await ctx.db.comment.create({ @@ -149,7 +207,7 @@ export const commentRouter = createTRPCRouter({ entityId: input.entityId, entityType: input.entityType, senderId: authorId, - link: `/estimates/${input.entityId}?tab=comments`, + link: policy.buildLink(input.entityId), channel: "in_app", }), ), @@ -179,18 +237,21 @@ export const commentRouter = createTRPCRouter({ }), ) .mutation(async ({ ctx, input }) => { - const userId = await resolveUserId(ctx); + const userId = ctx.dbUser?.id; + if (!userId) throw new TRPCError({ code: "UNAUTHORIZED" }); const dbUser = ctx.dbUser; const existing = await ctx.db.comment.findUnique({ where: { id: input.id }, - select: { id: true, authorId: true }, + select: { id: true, authorId: true, entityType: true, entityId: true }, }); if (!existing) { throw new TRPCError({ code: "NOT_FOUND", message: "Comment not found" }); } + await assertCommentEntityAccess(ctx, existing.entityType, existing.entityId); + // Only the author or an admin can resolve const isAdmin = dbUser?.systemRole === SystemRole.ADMIN; if (existing.authorId !== userId && !isAdmin) { @@ -226,18 +287,21 @@ export const commentRouter = createTRPCRouter({ delete: protectedProcedure .input(z.object({ id: z.string() })) .mutation(async ({ ctx, input }) => { - const userId = await resolveUserId(ctx); + const userId = ctx.dbUser?.id; + if (!userId) throw new TRPCError({ code: "UNAUTHORIZED" }); const dbUser = ctx.dbUser; const existing = await ctx.db.comment.findUnique({ where: { id: input.id }, - select: { id: true, authorId: true }, + select: { id: true, authorId: true, entityType: true, entityId: true }, }); if (!existing) { throw new TRPCError({ code: "NOT_FOUND", message: "Comment not found" }); } + await assertCommentEntityAccess(ctx, existing.entityType, existing.entityId); + const isAdmin = dbUser?.systemRole === SystemRole.ADMIN; if (existing.authorId !== userId && !isAdmin) { throw new TRPCError({