[Orgs] 3 Chat permission parity: chats join the project role ladder (stacked on #267) - #363
Open
amal66 wants to merge 6 commits into
Open
Conversation
|
|
… has
WHY THIS MATTERS
The org/RBAC migration (20260816_01) added org_id to projects, documents,
workflows and tabular_reviews — and skipped chats. That left chats the only
content family still on the pre-RBAC model: no way to share a standalone
chat at all, and no column for the access helpers or the overview RPC to
derive a role from. It also left a real authorization drift: the chat list
RPC took no email parameter, so a collaborator on a shared project could
open a colleague's chat by URL (the detail route checks the project's
shared_with) while the same chat never appeared in their GET /chat list.
List and detail disagreeing about what exists is exactly the failure mode
the overview-RPC lockstep convention (20260816_03's header) exists to
prevent.
WHAT IS THE SHARED PERMISSION SCHEMA
Every shareable resource carries the same three anchors:
user_id — the hard ON DELETE CASCADE owner anchor (unchanged);
shared_with — a per-row email list, so standalone rows can be shared;
org_id — a nullable FK with ON DELETE SET NULL, so dropping an org
never orphan-deletes a user's rows.
From those, lib/access.ts derives one of four roles (owner / manager /
editor / viewer) and routes gate on a capability matrix instead of ad-hoc
ownership checks.
HOW IT WORKS
- 20260820_01 adds chats.org_id + chats.shared_with (+ index) and
backfills org_id with creation-time semantics: a chat inside a project
inherits the project's org; everything else lands in the owner's
PERSONAL org. A personal org has exactly one member, so this stamping is
the privacy decision in data form — a standalone chat grants nothing to
anyone until it is explicitly shared, and org visibility only ever flows
through project membership.
- 20260820_02 drops and recreates get_chats_overview with p_user_email
and an is_owner output column. The predicate now mirrors
ensureChatAccess branch for branch: chat owner, chat shared_with, chat
org membership, or accessible project (owner / shared_with / org). Drop
rather than overload: the function has exactly one caller, and a
leftover 3-arg overload would ambiguate PostgREST's named-argument
resolution.
- schema.sql is updated byte-identically so fresh installs and upgraded
deployments converge (schema-drift check replicated locally: NO DRIFT).
Part of the chat-permission-parity follow-up to open-legal-products#267.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS Authorization bugs are rarely in the policy — they are in the third, slightly different copy of the policy. Chats had exactly that: a private getAccessibleChat helper inside routes/chat.ts that re-implemented part of the access logic (owner, else delegate to the project) and silently lacked the rest (no direct-share branch, no org branch, no strongest-wins merge). Two code paths answering "who can touch this row?" differently is how a collaborator ends up editor on one endpoint and 404 on another. WHAT IS STRONGEST-WINS MERGING A caller can hold several grants at once: org member (viewer tier) AND in the row's shared_with (editor tier). Branches must merge by taking the strongest role, never the first match — otherwise adding someone to a share list can DEMOTE them (the org branch shadowing the share branch was a real bug caught in open-legal-products#267's post-rebase review). The merge lives in strongerRole(); every derivation must route through it. HOW IT WORKS ensureReviewAccess's body — owner short-circuit, direct shared_with → editor, project verdict via checkProjectAccess, own-org membership → manager/viewer, all merged strongest-wins — was already the general algorithm: nothing in it is review-specific. It is extracted verbatim into a private ensureSharedResourceAccess, and both exported functions become three-line delegates: ensureReviewAccess(review, ...) -> ensureSharedResourceAccess(review, ...) ensureChatAccess(chat, ...) -> ensureSharedResourceAccess(chat, ...) Reviews and chats now cannot drift apart: there is one derivation to test, one to read, and one for the overview RPCs to mirror. ensureChatAccess's own-org branch is kept for structural symmetry even though standalone chats are stamped with a single-member personal org (20260820_01), which makes it a practical no-op — the privacy decision lives in the stamping policy, not in a special-cased code path. Part of the chat-permission-parity follow-up to open-legal-products#267. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… add sharing
WHY THIS MATTERS
Before this commit, three chat endpoints made their own law. PATCH (rename)
and DELETE filtered on `.eq("user_id", userId)` — a raw ownership check
that bypassed the role ladder entirely, so an org admin could rewrite a
chat's title through POST /chat/:id/generate-title (gated content.edit)
but got 404 from PATCH /chat/:id on the very same column. DELETE returned
204 even when it deleted nothing, so a forbidden delete looked like a
successful one. And GET /chat used a list predicate that disagreed with
the detail route, hiding chats the caller could open by URL.
WHAT IS THE DECLARED-CAPABILITY IDIOM
Routes never compare roles or re-derive rights from an isOwner flag; they
declare the capability they need and ask can(role, capability)
(lib/permissions.ts). The whole policy is one table, so tightening or
widening a tier is a one-line diff there — not an audit of every route.
HOW IT WORKS
- getAccessibleChat is now a thin wrapper over ensureChatAccess: it loads
the row and returns {chat, isOwner, projectRole}. Reads gate on the
verdict itself (the same convention project/review reads use).
- GET /chat/:chatId returns access_role + is_owner, mirroring the project
and review detail responses, so clients render per-role affordances
instead of guessing from one boolean.
- PATCH /chat/:chatId now takes {title?, shared_with?}: title needs
content.edit (the tier that already writes titles via generate-title),
shared_with needs members.manage (mirroring the project share PATCH:
emails lowercased + deduped, self-share and unknown users rejected).
Chats — including standalone ones — are shareable for the first time.
- DELETE /chat/:chatId needs container.delete: still the top of the
ladder, but derived — the chat owner, or the owner of the project it
lives in (who could already delete the whole project). Everyone else
gets an honest 403/404 instead of a silent no-op 204.
- GET /chat passes the caller's email to get_chats_overview, whose
predicate now mirrors ensureChatAccess — the list and the detail
endpoint can no longer disagree about what exists.
- GET /chat/:chatId/people exposes the owner + shared_with roster, the
same shape as the project/review people endpoints.
- Both chat-create paths (and projectChat's) stamp org_id via
resolveContentOrgId: project chats inherit the project's org, standalone
chats land in the caller's single-member personal org — private until
explicitly shared.
Part of the chat-permission-parity follow-up to open-legal-products#267.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…th to the access check
WHY THIS MATTERS
Two authorization holes in the review-chat sub-resource, found while
auditing chat permissions for parity:
1. DELETE and PATCH /tabular-review/:reviewId/chats/:chatId never looked
at reviewId at all — no check that the review exists, that the caller
can access it, or that the chat even belongs to it. Any chat id could
be renamed or deleted through any (or a nonexistent) review path. The
.eq("user_id") filter limited the blast radius to the caller's own
chats, but a URL path that lies about the resource hierarchy is a bug
class in itself (IDOR-shaped: object references must be validated
against their claimed parent, not trusted).
2. Two read routes selected the review row WITHOUT shared_with, which
silently disabled ensureReviewAccess's direct-share branch — the
branch can only match against a column that was actually fetched. A
review-level collaborator could open the review itself (that route
selects *) but got 404 on its chat list and chat messages.
HOW IT WORKS
- ensureReviewChatWriteAccess centralizes the write preamble: load the
review (WITH shared_with), run ensureReviewAccess, then verify
chat.review_id === reviewId. Both writes call it, and their final
UPDATE/DELETE also filters on review_id so the row touched is provably
the one the URL named. Writes stay chat-owner-only — review chats are
personal threads within a shared review, not shared containers.
- The two starved selects gain shared_with, un-breaking direct-share
collaborators on review-chat reads.
Part of the chat-permission-parity follow-up to open-legal-products#267.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY THIS MATTERS A capability matrix is only as trustworthy as the tests that pin it. open-legal-products#267 established the pattern — every role x capability cell asserted, plus regression tests for the overlap-precedence bug its own review caught. This extends the same rigor to the resource family that just joined the schema, so a future refactor that quietly re-adds a user_id filter or drops a merge branch fails a named test instead of shipping. WHAT IS BEING PINNED - ensureChatAccess derivation (lib/__tests__/access.test.ts): owner branch; case-insensitive direct-share -> editor; project inheritance (project owner -> owner, org admin -> manager, org member -> viewer); chat-own-org branch; the overlap-precedence regressions (org member + direct share must be editor, never viewer; project owner + direct share must stay owner); cross-tenant denial; fail-closed with no email. - Chat route gates (integration/chat.routes.test.ts): rename allowed for managers, 403 for viewers; share-list writes manager-only, with the normalization (lowercase/trim/dedupe), self-share and unknown-user rejections asserted on the exact payload persisted; delete 204 for the owner, 403 for an org admin, 404 for a stranger — and the recorded update/delete filters are asserted to be [{id}] alone, which is the machine-checkable form of "the raw user_id filter is gone, the matrix decides now". Also: access_role/is_owner on the detail response, the people roster, the RPC receiving p_user_email, and a standalone direct-share chat walking read/write/delete tiers. - Review-chat path binding (integration/tabular.routes.test.ts): rename and delete 404 when the review is missing, inaccessible, or the chat belongs to a different review; 204 for the owner when review and chat line up. Full backend suite: 712 passed | 27 skipped. Part of the chat-permission-parity follow-up to open-legal-products#267. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
amal66
force-pushed
the
olp-pr/organizations-rbac
branch
from
August 21, 2026 17:28
305db48 to
4aced37
Compare
amal66
force-pushed
the
olp-pr/chat-permissions
branch
from
August 21, 2026 17:29
a280ba9 to
4fcb739
Compare
…a silent 204
The review-chat rename and delete routes gate on review access, then
scope the write itself with .eq("user_id", userId). For a collaborator
who can see the review but did not create the chat, the write matched
zero rows and the route still answered 204 — a success-shaped response
for an update that never happened. Callers (and tests) reading the
status would believe the rename or delete took effect.
ensureReviewChatWriteAccess now selects the chat's user_id and refuses
non-creators with a 403 that says so. The user_id filter on the write
stays as belt and braces; the review-access and review-binding checks
in front are unchanged, so the bogus-review-id 404s still come first
and reads stay at the viewer tier.
Replication (before): share a review with a second account, insert a
chat as the owner, then as the collaborator PATCH or DELETE
/tabular-review/:reviewId/chats/:chatId — 204, but the title and row
are untouched. After: 403 "Only the chat's creator can modify it";
the owner's own writes still 204.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0128b5tXqvX4GghBELHhyVnC
Collaborator
Author
Live verification round — 2026-08-21 (tip
|
amal66
marked this pull request as ready for review
August 21, 2026 21:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #267 — the base is its branch, so the diff below is just this PR's 5 commits.
What this is
#267 gave projects and tabular reviews one permission model:
shared_with+org_idcolumns, a role ladder (owner/manager/editor/viewer), routes gating oncan(role, capability), and list RPCs that match the detail routes. Chats never got any of it — no columns, an ad-hoc access check private to the route file, rename/delete doing rawuser_idfilters, and a chat list that disagreed with the detail endpoint. This PR makes chats the third resource on the same schema. Nothing new is invented; the existing model is applied.Concretely:
chatsgetsorg_id+shared_with, same shape astabular_reviews(migration + backfill).ensureChatAccessinlib/access.ts. It turns outensureReviewAccesswas already the general algorithm, so both are now thin wrappers over one shared derivation — they can't drift apart again.PATCH /chat/:idacceptsshared_with(manager+), andGET /chat/:id/peoplereturns the roster.get_chats_overviewnow takes the caller's email and returnsis_owner, and its predicate matchesensureChatAccessbranch for branch.access_role+is_ownerlike projects/reviews.Privacy: standalone chats stay private. They're stamped with the owner's single-member personal org, so org visibility only ever flows through project membership. Sharing a standalone chat is explicit, per-email.
Behaviour changes
GET /chat. List and detail now agree.content.edit). This resolves a real contradiction —generate-titlealready rewrote the same column at that tier whilePATCH404'd.container.deleteinstead of auser_idfilter, so a project owner can now delete a collaborator's chat inside their own project. They could already delete the whole project, so no net new power.:reviewIdin the path — previously any chat could be hit through any (or a made-up) review id, with no review access check.shared_with, so direct-share collaborators could open a review but 404'd on its chat list.Replication
Before (on #267): A shares a project with B and chats in it. As B,
GET /chatdoesn't list A's chat butGET /chat/<id>opens it. As B (editor),PATCH /chat/<id> {"title":"x"}→ 404 whilegenerate-titlesucceeds. AndDELETE /tabular-review/<any-uuid>/chats/<chatId>as the chat owner returns 204 despite the bogus review id.After: the chat shows up in B's list with
is_owner: false, the rename works for B (an org viewer gets 403), the bogus review path 404s. New: A shares a standalone chat with B viaPATCH {"shared_with": [...]}— B can open and chat in it (access_role: "editor") but can't delete it; a third user still 404s everywhere.Tradeoffs
content.editrather thanstructure.manage— consistency with the existing title-write path won over narrowness. A chat title is content, not structure.org_idbranch inensureChatAccessis a practical no-op for standalone chats (personal org). Kept anyway for symmetry with reviews — the privacy decision lives in the stamping policy, not in special-cased code.Testing
tsc --noEmitclean;vitest run→ 803 passed | 27 skipped (current tipa5e72d18, after the rebase ontod8183be4). Tests pin the fullensureChatAccessbranch/overlap matrix, the route tier walks (including asserting the update/delete filters are[{id}]alone — i.e. the rawuser_idfilters are really gone), the review-chat binding gates, and the non-creator 403 on review-chat writes.9a1277b+ migrations vs freshschema.sql):NO DRIFT.is_owner: false, private ones invisible; backfill stamps project chats with the project's org and standalone chats with the personal org.20260821_10/11applied over a database carrying real prior-round data — backfill stamped 21/21 existing chats; 8 scripted case groups green end-to-end against the running stack, including one real Anthropic stream through a shared standalone chat; recorded UI flows onassets/orgs-smoke-2026-08-21.a5e72d18from that round: review-chat rename/delete previously answered a success-shaped 204 no-op to a collaborator who wasn't the chat's creator (write scoped byuser_idmatched zero rows after the access gate passed).ensureReviewChatWriteAccessnow refuses non-creators with 403 "Only the chat's creator can modify it"; replicated live before/after.Note for CI: checks cannot run while the base is the
olp-pr/organizations-rbacstaging branch (workflows trigger on PRs tomainonly). Retargeting tomainafter #267/#268 merge gives this PR its first CI run.🤖 Generated with Claude Code