feat: Project & Group Visibility (private/shared + two-level inheritance + UX fixes) - #308
feat: Project & Group Visibility (private/shared + two-level inheritance + UX fixes)#308tonyhuangai2026 wants to merge 24 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…etAccessibleProjectUuids) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ility; member CRUD Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ask services (reads + writes) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mment/search/assignment/tracker Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e guards Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ct guards, permission-map Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ager + i18n Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hz + in-memory prisma) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… baselines, write-gate coverage - Document project visibility + member tools in docs/MCP_TOOLS.md and both skill doc sets - Update permission parity baselines for the 3 new member tools - Add write-gate rejection tests (task + document services) to keep coverage >= thresholds - Remove unused imports in idea.service Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… group list Regression from the visibility cascade: listProjectGroups filtered out any group with zero accessible projects for non-super-admins, so a freshly created (empty) group vanished from the list — the UI 'create group' button appeared to do nothing. Groups are organizational containers (consistent with search treating project_group as ungated); surface all company groups, with projectCount reflecting only accessible projects. Adds a regression test for the regular-user empty-group case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ation Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ojects into accessible union + group helpers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ting + project visibility inheritance Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + owner-gate group update/delete Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + i18n Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… service-test group mocks, coverage - Add dynamic group-inheritance E2E cases (group member read+write via group, dynamic revocation, shared-in-private-group invariant) to project-visibility integration test - Document two-level inheritance + group member tools in MCP_TOOLS.md and both skill doc sets - Add projectGroup/projectGroupMember mocks to search/notification/checkin/idea-tracker service tests (getAccessibleProjectUuids now consults groups) - Add group-helper guard-branch tests to keep coverage >= thresholds Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mable predicates Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ted group DELETE Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (mentionables includeUsersOnEmpty) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ember names + user search in dialogs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s, coverage guard tests - E2E: legacy null-owner project/group claimed by first accessible manager; non-member of private owner-less denied; already-owned not reassigned; super_admin no-claim - Docs: claim-on-first-manage + member names in MCP_TOOLS + both skill docs - Added claim/predicate guard-branch tests to keep coverage >= thresholds Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Report
File Coverage |
ChenNima
left a comment
There was a problem hiding this comment.
Review: Request changes
Solid core design — project-access.ts is clean, and the two key invariants hold (the two group-sets are kept rigorously separate, and applyProjectFilter treats an empty accessible set as "match nothing", not "no filter"). The service-layer mainline (project / group / idea / proposal / document / task / activity / comment / checkin / session / idea-tracker / notification.list / search-for-entities) is consistently gated.
However, the "thread auth everywhere" sweep missed a few surfaces, and those happen to be real cross-boundary read/write paths into private projects. Two of them are direct content leaks. Details below, most severe first. I verified each by reading the source.
🔴 BLOCKER 1 — Elaboration subsystem has no visibility gate (non-member read + write into a private project's idea)
src/services/elaboration.service.ts was not touched by this PR and scopes only by companyUuid — no canAccessProject.
Exposed through:
chorus_get_elaboration(src/mcp/tools/public.ts:659) — a public tool, no permission gate — callsgetElaboration({ companyUuid, ideaUuid }).chorus_answer_elaboration(public.ts:621) — also a public tool.answerElaboration(elaboration.service.ts:136) only checks the round ispending_answers; it does not checkcanAccessProject, nor even that the caller is the idea's assignee.chorus_pm_start_elaboration/validate/skip(pm.ts) — gated only on theidea:write/idea:adminpermission bit, which per this PR's own semantics does not widen access to a private project.
Exploit: any company actor (via the two public tools, even an agent with zero resource permissions) who knows/guesses an ideaUuid in a private project they're not a member of can read the full elaboration (rounds, questions, answers, answerer identities) and write answers that advance the idea's lifecycle. Contrast chorus_get_idea, which is correctly gated via getIdea(..., auth).
Fix: add auth to the elaboration service functions and gate on canAccessProject(auth, idea.projectUuid) at entry; thread auth from the MCP tools.
🔴 BLOCKER 2 — getProposalsByIdeaUuid + the two report server actions leak a private project's proposals and report documents
getProposalsByIdeaUuid(companyUuid, projectUuid, ideaUuid) (src/services/proposal.service.ts:490) queries by { projectUuid, companyUuid } only, returns full proposals (incl. documentDrafts / taskDrafts), and has no auth param. Its callers don't gate either (src/app/(dashboard)/projects/[uuid]/dashboard/panels/actions.ts):
getProposalsForIdeaAction(L99)getReportsForIdeaAction(L141) — also feeds approved-proposal uuids into the equally ungatedlistDocumentsByProposalUuids(document.service.ts:152).
Exploit: a non-member who supplies a private project's projectUuid + any ideaUuid reads all of that project's proposals (full draft content) and report documents.
Telling detail: the adjacent getTasksForProposalAction (same file, L116) correctly threads auth into listTasks({ ..., auth }) — so these two are an omission, not intentional.
Fix: add auth + canAccessProject to getProposalsByIdeaUuid (and listDocumentsByProposalUuids), or gate at both server-action entries.
🔴 BLOCKER 3 — SSE /api/events streams every private project's change events to any company user
src/app/api/events/route.ts (not touched by this PR) filters only by event.companyUuid and an optional client-supplied projectUuid query param — it never calls canAccessProject. RealtimeEvent carries projectUuid / entityType / entityUuid / actorUuid (src/lib/event-bus.ts:11).
Exploit: any authenticated company user opens GET /api/events (no filter, or ?projectUuid=<private>) and receives a live stream of every created/updated/deleted event across every private project in the company, including entity and actor uuids. The module header of project-access.ts claims the boundary cascades to real-time fan-out, but it doesn't. (events/notifications is per-recipient keyed and is fine.)
Fix: gate each event with canAccessProject(auth, event.projectUuid) before sending (cache the accessible set per connection; invalidate on membership change, or fall back to per-event checks).
🟠 HIGH — PATCH /api/projects/[uuid]/group uses an access gate instead of a manage gate, and never gates the destination group
src/app/api/projects/[uuid]/group/route.ts + moveProjectToGroup (project-group.service.ts:339, which does no authz at all).
- Wrong level: re-parenting a project is gated only by
canAccessProject(read+write access). So a plain project member, or any non-owner on a shared project withproject:write, can re-group / detach it — this should be owner-only (403), per the PR's stated "group management is owner + super-admin only". - Destination not gated:
body.groupUuidis checked for company membership only — notcanAccessGroup/canManageGroup. An actor can attach a project into a group they neither own nor can access.
Fix: gate source with claimOrCanManageProject (→403); when groupUuid is non-null, require canManageGroup (or at least canAccessGroup) on the destination.
🟡 MEDIUM 1 — Global search leaks private project-group names/descriptions
search.service.ts:457 intersects only the project set with the accessible set; the comment explicitly states "project_group results are unaffected". searchProjectGroups (L373) filters by companyUuid only and never consults getAccessibleGroupUuids.
Exploit: chorus_search / GET /api/search with entityTypes:["project_group"] (or default) returns the uuid + name + description snippet of private groups the actor can't access. chorus_get_project_groups filters correctly via getAccessibleGroupUuids; search bypasses it.
Fix: apply getAccessibleGroupUuids in searchProjectGroups. (Listed as a known follow-up in the PR body, but since name+description leak, I'd treat it as a fix rather than accepted.)
🟡 MEDIUM 2 — Inaccessible task/idea returns 500 instead of 404 on mutation routes (existence oracle)
E.g. tasks/[uuid]/route.ts (PATCH L55, DELETE), tasks/[uuid]/dependencies/*, tasks/[uuid]/claim|release, ideas/[uuid]/route.ts, ideas/[uuid]/claim|release. These pre-check existence with the ungated getTaskByUuid/getIdeaByUuid (company-scoped), so an existing-but-inaccessible entity passes; the service-level gate then throws a plain new Error("…not found"), which withErrorHandler maps to 500 (only ApiError / Prisma P2025 map to 404). Net: nonexistent → 404, existing-but-inaccessible → 500 — the status difference reveals existence. No content leak, but it violates the stated "inaccessible → 404". The idea-move path uses ApiError(...,404) and is the correct pattern to follow.
⚙️ Migrations contain DML (UPDATE) for backfill — violates the "migrations are DDL-only" convention
Both 20260611152319_add_project_visibility and 20260612030604_add_project_group_visibility run UPDATE "Project"/"ProjectGroup" SET visibility='shared'. Project convention is migrations must be DDL-only (no UPDATE/INSERT/DELETE, even for backfill). Please move the backfill to an app-level / one-off step, or confirm an explicit exception with the team.
Minor (non-blocking)
memberUuidsnot validated —addProjectMember/createProjectmember seeding (project.service.ts:436, L195-207) writesmemberUuidverbatim with no check it's a real actor in the company (cf.chorus_pm_assign_task, which doesgetAgentByUuidcompany-scoped). A future uuid collision could silently grant access.getUnreadCount(notification.service.ts:300) isn't visibility-filtered whilelistis — badge can over-count after access is lost. Recipient-scoped, so not a content leak.chorus_session_checkin_task/checkout_task(session.ts:140) accept any task uuid with no project gate — observability-only cross-boundary write; considercanAccessProject(auth, task.projectUuid).docs/design.pennot updated for the new UI (badges, settings modal, members manager) — required per CLAUDE.md; noted as a known follow-up.
Test coverage gap
The integration test is broad, but happens to miss exactly the leaking surfaces: no elaboration cases, no SSE/eventBus cases, no getProposalsByIdeaUuid/report-action cases, and the search assertions cover task/idea/proposal/document/project but not project_group. Please add regressions for these alongside the fixes.
Nice work overall on the authz core and the cascade through the service layer — the gaps are concentrated in the few files the sweep didn't touch. Happy to re-review once BLOCKER 1–3 and HIGH are addressed.
Project Visibility (Private / Shared) + Two-Level Group Inheritance
Part 1 — Project Visibility (Private / Shared)
Summary
Adds a second access dimension to projects on top of multi-tenancy. A project is now either:
shared— visible to the whole company (the historical behavior), orprivate— visible only to its owner and an explicit member list (users and agents).Membership — not the permission bitset — is what grants access to a private project. Holding
project:admindoes not bypass it; only the super admin platform role sees everything (for governance).New projects default to private (owner = creating actor, who is auto-added as the first member). A data migration sets all pre-existing projects to shared, so no current work becomes inaccessible.
Why
Previously every project was visible to everyone in the company (services scoped only by
companyUuid). Teams asked for private workspaces that a subset of people/agents can collaborate in.How it works
A single authz module —
src/lib/authz/project-access.ts— is the source of truth:getAccessibleProjectUuids(auth)→ the set of project UUIDs the actor may see (or anALLsentinel for super admin)canAccessProject(auth, projectUuid)→ read and write gatecanManageProject(auth, projectUuid)→ owner-only gate (visibility / membership / delete)applyProjectFilter(where, accessible)→ injectsprojectUuid: { in: [...] }into existing (company-scoped) queriesAccess is enforced across the whole cascade — the project and all of its ideas, proposals, documents, tasks, activity, comments, notifications, and search results are filtered for non-members. Both reads and writes are gated (e.g. a non-member agent cannot claim/update a private task or post a comment on it).
Surfaces changed
Project.visibility/ownerType/ownerUuid, newProjectMembertable, migration withsharedbackfill.GET/POST /api/projects,GET/PATCH/DELETE /api/projects/[uuid], new/api/projects/[uuid]/members(GET/POST/DELETE, owner-only), andcanAccessProjectguards on every nested route. Leak rule: inaccessible →404, accessible-but-not-owner manage →403.chorus_admin_create_projectgainsvisibility+memberUuids; newchorus_list_project_members(project:read),chorus_admin_add_project_member/chorus_admin_remove_project_member(project:admin); list/get project & group tools and every projectUuid-taking tool gated.docs/MCP_TOOLS.md+ both skill doc sets.Testing
project:admin-non-member denied andprojectUuids[]header does not grant access).src/__tests__/integration/project-visibility.integration.test.ts) that drives the real authz + services over an in-memory Prisma and asserts the full boundary: non-member deny (reads + writes), owner/member allow, super_admin all-access,project:admin-non-member deny, shared-project regression.tsc✓,pnpm test(1889 pass / 1 skip) ✓, coverage 95.06% stmts / 87.94% branches / 95.95% funcs / 96.82% lines (≥ thresholds) ✓,pnpm build✓.Migration / rollout
The migration adds the columns (default
private) and runsUPDATE "Project" SET visibility='shared'for all pre-existing rows, so production data stays fully visible. The standalone Docker entrypoint runsprisma migrate deployautomatically on container start.Known follow-ups (out of scope)
memberrole.project_groupentity names are still searchable in global search (group containers aren't visibility-gated); private-project entities never leak.docs/design.pennot updated in this environment (Pencil MCP tooling unavailable) — to refresh when design tooling is available.🤖 Generated with Claude Code
Part 2 — Two-Level Visibility (ProjectGroup → Project inheritance)
Summary
Project groups gain the same
shared/private+ owner + member model, and a project's effective access becomes the dynamic union of its own accessors and its group's accessors.visibilityflag wins. Asharedproject inside aprivategroup is still company-wide; aprivateproject inside asharedgroup is still restricted — a shared group never exposes its private projects. (Enforced by using only owner/member groups for the project-union, never shared groups.)groupUuiddefaults to its group's visibility.project:adminbypass.Surfaces
ProjectGroup.visibility/ownerType/ownerUuid, newProjectGroupMembertable, migration withsharedbackfill.project-access.ts):getAccessibleProjectUuids+canAccessProjectfold in owned/member groups; newgetAccessibleGroupUuids/canAccessGroup/canManageGroup. Two distinct group-sets kept rigorously separate (project-union = owner/member only; group-visibility = shared∪owned∪member).listProjectGroupsgated bycanAccessGroup(preserving the empty-group fix), new/api/project-groups/[uuid]/members, project inherits group visibility default.chorus_admin_create_project_groupgainsvisibility/memberUuids; newchorus_list_project_group_members(project:read),chorus_admin_add/remove_project_group_member(project:admin);update/deletegroup tools now owner-gated.Testing (whole branch)
project:admin-non-member-denied.project:admin-non-member denied.tsc✓, 1954 tests pass / 1 skip ✓, coverage 95.16% stmts / 88.5% branches / 96.03% funcs / 96.92% lines (≥ thresholds) ✓,pnpm build✓.Migration / rollout
Both migrations (
add_project_visibility,add_project_group_visibility) run automatically via the Docker entrypoint and backfill existing rows toshared. Already deployed to the live standalone instance for validation.Known follow-ups (out of scope)
memberrole for both projects and groups.docs/design.pennot updated (Pencil MCP tooling unavailable in this environment).Part 3 — Visibility UX fixes (claim-on-manage, group DELETE gate, member UX)
Addresses three user-reported bugs, all rooted in legacy null-owner entities (the migration set pre-existing projects/groups to
sharedwith no owner, soisOwnerwas false for everyone → manage controls hidden).updateMany where ownerUuid:null+ lost-race re-read), and super_admin manages without claiming.DELETE /api/project-groups/[uuid]was ungated — addedcanAccessGroup(→404) thenclaimOrCanManageGroup(→403), matching PATCH.getActorName) instead of raw UUIDs; the member-add search surfaces human users (and a default list on empty input) via aforMembersflag on/api/mentionables(the @mention autocomplete default is unchanged).canManageOrClaimable*predicate (no write on a GET; the real claim happens on the manage action).Pure read gates (
canManageProject/Group) stay side-effect-free; all MCP mutating tools + REST manage routes use the claim-aware variant.Testing
tsc✓, 1990 tests pass / 1 skip ✓, coverage 95.23% stmts / 88.77% branches / 96.06% funcs / 96.97% lines ✓,pnpm build✓.Migration / rollout
No schema change (owner columns already exist). Pure logic/UX + the DELETE gate fix. Already deployed to the live standalone instance.