feat: add organization scoping to chats#23827
Conversation
johnstcn
changed the title
Chats were previously user-scoped only (owner_id). This adds organization_id to the chats table so they align with the existing site → org → user RBAC convention. Key changes: - Migration adds organization_id NOT NULL to chats with 3-tier backfill (workspace org → user's org membership → default org) - RBAC: Chat.RBACObject() now includes .InOrg(), ChatConverter uses organizationOwnerMatcher(), org admins gain chat access - API: CreateChatRequest requires organization_id, validates membership, rejects cross-org workspace binding - Frontend: AgentCreateForm gets OrganizationAutocomplete picker (auto-selects for single-org users) - chatd: CreateOptions and subagent creation propagate org ID - All tests updated for org-scoped chat resources
The enterprise chatd test helpers (seedChatDependencies, seedWaitingChat, seedRemoteRunningChat) now create an organization and pass organization_id through to all CreateOptions and InsertChatParams call sites.
Capture firstUser from coderdenttest.New and pass firstUser.OrganizationID to all CreateChatRequest calls.
johnstcn
force-pushed
the
cian/chat-org-scoping
branch
from
53722e0 to
8aec990
Compare
Fixes querier_test, chatprompt_test, chattool/startworkspace_test and remaining chatd_test InsertChatParams that were missing the new required organization_id field.
ChatConverter now uses organizationOwnerMatcher() instead of the hardcoded empty string, so the expected SQL in compile tests must reference organization_id :: text instead of ''. Also renamed ChatOrgScopedNeverMatches to ChatOrgScopedMatches since org-scoped queries now actually match.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Self-review via deep-review skill (9 parallel reviewers). The compile_test P0 is already fixed in the latest push. Remaining items are test coverage gaps and a migration nit.
1 P1 (test coverage), 3 P2, 2 Obs, 2 Nits across 6 inline comments.
This review contains findings that may need attention before merge.
- Add OrganizationID to all 51 CreateChatRequest callsites in exp_chats_test.go (CI blocker) - Migration: add ORDER BY for deterministic backfill, add index on chats.organization_id - Consolidate identical ChatConverter into NoACLConverter - Fix stale 'not org-scoped' comment in querier_test.go - Pre-select default org in AgentCreateForm picker
- Renumber migration 000457 -> 000458 (000457 now taken by chat_access_role on main) - Merge RBAC test: agentsAccessUser goes to false set since it has no org role for org-scoped chats - Merge exp_chats_test: keep agents-access member pattern from main, add OrganizationID - Merge querier_test: keep agents-access role, add org member
…g test callsites The great purge of 2026: 25 InsertChatParams and 2 CreateChatRequests walk into a bar. The bartender says 'Where's your OrganizationID?' They all get bounced by the FK constraint bouncer. - db2sdk_test: TestChat_AllFieldsPopulated now populates all fields (shocking) - dbauthz_test: InsertChat RBAC assertion includes .InOrg() (as it should) - chatparam_test: TestChatParam/Found creates an actual org before inserting - gitsync/worker_test: TestWorker gets its own org to play with - exp_chats_test: 25 InsertChatParams + 1 CreateChatRequest get their papers - mcp_test: CreateChatRequest finally declares its organizational allegiance
johnstcn
force-pushed
the
cian/chat-org-scoping
branch
from
69fc07d to
1984d10
Compare
…ast straggler Two issues: 1. The UsageLimitExceeded test's InsertChatParams at line 510 was the world's best hide-and-seek champion — survived two rounds of grep due to a nearby OrganizationID in a different struct within the 15-line scan window. 2. OrgMemberPermissions excluded ResourceChat from the site-level member role but NOT from the org-level member permissions. With chats now org-scoped via .InOrg(), org members got implicit chat access through their member permissions — a privilege escalation that would make any security reviewer's eye twitch.
johnstcn
force-pushed
the
cian/chat-org-scoping
branch
from
1984d10 to
ba3c756
Compare
The _ in 'user, _, model' was doing a fine job ignoring the org, right up until CreateChat politely refused to exist without one.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Round 2 deep review (6 parallel reviewers). Prior P2s for ORDER BY, index, and ChatConverter consolidation all verified fixed.
2 P2, 1 P3, 1 Obs across 4 inline comments. Overall this is in good shape — the RBAC model change is well-reasoned and the migration is solid.
Covers the primary user path: a regular org member accessing their own org-scoped chat. Previously only owner and orgAdmin were in the true list, leaving the most common access pattern validated only by integration tests.
Three new subtests in TestPostChats covering the org validation code paths that previously had zero direct test coverage: - NilOrganizationID: uuid.Nil → 400 - NonMemberOrganization: wrong org → 403 - CrossOrgWorkspaceMismatch: workspace in org A, chat for org B → 400
|
🤖 Design decisions noted from deep review round 2 (not actionable in this PR):
|
… fix new callsites Main added 202 commits including dynamic tools, system prompts, chat files, recording, purge tests, and telemetry tests — all with new InsertChatParams/CreateChatRequest/CreateOptions structs that needed OrganizationID.
The RefreshesStaleStatusWithExternalAuth test was deleted on main in 7861fcf (inline diff resolution removed in favor of background gitsync), but our merge conflict resolution accidentally brought it back from the dead. Remove the zombie test and its orphaned imports.
TestChatMessageWithFiles/FilesLinkedOnSend and TestPatchChatMessage/FilesLinkedOnEdit were missing OrganizationID in their CreateChatRequest calls — stragglers from the main merge.
…pace creation The create_workspace tool could silently bind a workspace from org A to a chat scoped to org B. Now it looks up the template's org before calling CreateFn and returns a tool error if they don't match. Closes the cross-org gap noted in the deep review.
# Conflicts: # coderd/x/chatd/subagent.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 56 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Panel re-review (round 4). 5 reviewers: Bisky, Mafuuu, Kurapika, Meruem, Zoro (wildcard). Netero first pass.
All R3 findings resolved. agents-access gates added to both promoteChatQueuedMessage and postChatToolResults. Em-dashes replaced. OrganizationID assertion added to child chat test. Clean fix commit with no scope drift.
Kurapika verified the full authorization model and found no additional security issues. Mafuuu traced the contract end-to-end and confirmed all entry points enforce the org constraint. Meruem verified no TOCTOU gaps in the org membership check (template org is immutable, membership revocation is caught by dbauthz).
Two convergent P3 findings: the new agents-access gates on promoteChatQueuedMessage (line 2143) and postChatToolResults (line 5869) have no corresponding tests. The identical pattern on postChatMessages does (line 4128). (Netero, Kurapika, Meruem)
Meruem notes ChatOwnerMe at compile_test.go:288 now tests a SQL clause (organization_id :: text = '') that can never match a row post-migration. The test validates compilation correctness but the compiled SQL is vestigial. Consider a comment noting this path is dead at the data level.
Bisky notes GetAuthorizedChats at querier_test.go:1355 only tests the negative case (cross-org admin sees 0 chats). No test confirms a same-org admin sees chats. The RBAC unit test covers the policy, but the SQL filter generator path is untested for the positive case.
"Five-line null check equivalent to newOrg?.id !== selectedOrg?.id. The five-line version obscures the intent without handling any edge case the one-liner misses." (Zoro)
Severity count: 2x P3 (missing gate tests), 3x P3 (frontend), 2x Nit. 3 dropped as P4.
site/src/pages/AgentsPage/AgentCreatePage.tsx:33
P3 Workspace dropdown shows workspaces from all organizations, not filtered by the selected org. (Mafuuu)
workspacesQueryusesworkspaces({ q: "owner:me", limit: 0 })which fetches every workspace the user owns across all orgs. When the user changes the org picker,handleWorkspaceChange(null)clears the selection but does not re-filter the options. A user can select Org A, pick a workspace from Org B, and submit. The server returns 400 but the error message requires org/workspace knowledge to diagnose.
Filter workspaceOptions by the selected org, or pass organizationId into the query.
🤖
coderd/rbac/roles.go:448
Nit Comment says "Without this role, members cannot create or interact with chats" but org members CAN read, update, and delete their own chats via OrgMemberPermissions (verified by OrgMemberWithoutAgentsAccessCanAccessOwnChats). (Meruem)
roles.godefines the RBAC model. A developer reading this comment would conclude agents-access is the sole gate for chat interaction, which is false at the RBAC layer. The handler-level gates are the actual enforcement for creation and inference.
🤖
🤖 This review was automatically generated with Coder Agents.
Backend: - Add MemberWithoutAgentsAccess tests for promoteChatQueuedMessage and postChatToolResults handlers (both had gates but no test coverage) - Add descriptive comments to templateAdmin and userAdmin role entries to match the style of neighboring roles in builtInRoles Frontend (AgentCreateForm): - Simplify orgChanged comparison to use optional chaining - Replace window.confirm with ConfirmDialog for consistency and testability - Filter workspace dropdown by selected organization (client-side) - Add OrgChangeConfirmation story with play function assertions
Addressed all 7 items from the review in 51d249c: Backend (
Backend (
Frontend (
|
The comment claimed members cannot 'create or interact with chats' without this role, but they CAN read/update/delete their own chats via OrgMemberPermissions. The role gates creation and AI inference.
Responding to the two top-level findings from the round 4 review:
Done. Added client-side filtering of
Fixed in 6694df9. The old comment said "members cannot create or interact with chats" — but members CAN read/update/delete their own chats via Also added descriptive comments to the previously-uncommented |
# Conflicts: # site/src/pages/AgentsPage/components/AgentCreateForm.tsx
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Mostly minor stuff, so I feel confident to approve.
| -e | ||
| # Agent planning documents (local working files). | ||
| docs/plans/ | ||
| amend-review+self-review.zip |
Three TestStartWorkspace subtests (InProgressBuild, FailedBuild, StartTriggeredBuildFailure) were missing OrganizationID in their InsertChatParams, causing FK constraint violations against PG.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Code Review Summary — PR #23827
Findings: 6 | P0: 0 | P1: 1 | P2: 1 | P3: 4 All findings are posted as inline annotations on the Pluto Code Review check run. This review is complete. Approval must be performed by a human. Note 🤖 This comment was written by Coder Agent on behalf of Danielle Maywood |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Panel re-review (round 5). 5 reviewers: Bisky, Mafuuu, Kurapika, Meruem, Killua (wildcard). Netero first pass.
All R4 findings resolved. H8 (ChatOwnerMe vacuous SQL) and H10 (same-org admin test gap) closed by panel consensus: all 4 reviewers accepted the author's defenses. No performance concerns (Killua). Authorization chain verified correct across all 8 paths (Kurapika).
Three findings converge on error handling in the org membership validation.
list_templates returns templates from all organizations without org identifiers. The cross-org template guard tells the LLM "use list_templates to find templates in the correct organization," but the LLM receives the same unfiltered list with no way to distinguish orgs. In multi-org deployments this creates an inference loop. Aligns with @mafredri's question in #comment-3072017260. (Mafuuu P3, Meruem P3)
"The boundary between 'requires agents-access because it triggers inference' and 'does not require agents-access because it's metadata management' is a design decision worth pinning with tests on both sides. Right now only one side has a nail in it." (Bisky)
Severity count: 3x P3, 1x Nit.
🤖 This review was automatically generated with Coder Agents.
Frontend (AgentCreateForm.tsx): - Validate localStorage workspace ID against initial org on mount, preventing stale cross-org workspace from being silently submitted Backend (exp_chats.go): - Remove dead Is404Error branch on OrganizationMembers (:many query) - Remove redundant dbauthz.IsNotAuthorizedError check on workspace lookup Backend (listtemplates.go, chatd.go): - Filter list_templates by chat organization - Include organization_id in template output for LLM context Tests: - Add same-org admin chat visibility test (positive case) - Add positive-side agents-access boundary test (UpdateChat) - Add error message assertions to NilOrganizationID/NonMemberOrganization - Fix squashed struct fields in active_chat_internal_test
|
[P2 warning] Persisted attachments lack org affinity
Suggestion: Either store Found by react-robustness-bottom-up as owl.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P3 suggestion] Fragile positional uuid.UUID parameter lists in test helpers
Suggestion: Consider using a params struct or named arguments pattern to prevent silent swaps. Found by go-design-bottom-up as eeyore.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P2 suggestion] ListTemplates org-scoping filter is untested
The PR adds Suggestion: Add a test that creates templates in two orgs and verifies Found by go-testing-top-down as buzz.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P2 suggestion] Org picker uses raw organizations instead of permittedOrganizations()
Suggestion: Use Found by react-conventions as hades.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P2 warning] Misleading comment claims filteredWorkspaces clears stale selection, but no code does
The comment at line 195 says "It will be re-validated once the list arrives via filteredWorkspaces clearing the selection if stale" — but Suggestion: Either implement the reconciliation the comment describes, or correct the comment to reflect that the backend catches the mismatch with a 400 error and no client-side clearing happens. Found by react-correctness-top-down as mulan.md (also independently confirmed by 4 other reviewers) Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P3 suggestion] Stale comment on setupChatInfra
Comment says "Returns the store, user ID, and model config ID" but the function now returns four values (store, userID, modelConfigID, orgID). Suggestion: Update the comment to mention the org ID. Found by go-design-bottom-up as eeyore.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
|
[P3 suggestion] Defensive guard silently skips org validation on uuid.Nil
The Suggestion: Consider logging or returning an error when Found by go-design-bottom-up as eeyore.md Note This comment was written by Coder Agent on behalf of Danielle Maywood |
Code Review Summary (Round 2) — PR #23827
Findings: 7 | P0: 0 | P1: 0 | P2: 4 | P3: 3 9 additional findings were filtered as re-reports of round 1 issues or duplicates. 1 finding (RDB002) was rejected by fact-checking. This review is complete. Approval must be performed by a human. Note This comment was written by Coder Agent on behalf of Danielle Maywood |
Fixes coder/internal#1436
Chats were user-scoped only (
owner_id). This addsorganization_idto align with the site → org → user RBAC convention.Changes
000466addsorganization_id NOT NULLtochatswith 3-tier backfill (workspace org → user org membership → default org)Chat.RBACObject()includes.InOrg(),ConfigChats()usesNoACLConverter()CreateChatRequestrequiresorganization_id, handler validates membership and rejects cross-org workspace bindingcreate_workspacetool validates template org matches chat org before callingCreateFnAgentCreateFormuses controlledOrganizationAutocomplete(auto-selects for single-org users)chatd.CreateOptionsand subagent creation propagate org IDorganization_idin chats-api.mdImplementation plan & decision log
Key design decisions
organization_id.workspace.organization_id == chat.organization_idto prevent mismatches. Thecreate_workspacetool also validates template org before calling CreateFn.workspacestable (all workspaces have org). Fallbacks for deleted workspaces: user's oldest org membership (ORDER BY created_at ASC), then default org.OrganizationAutocompletecomponent (refactor(site): convert OrganizationAutocomplete to fully controlled component #24211). Auto-selects first org, dropdown for multi-org.ResourceChatstays inOrgMemberPermissions(not excluded). TheMemberlevel in Rego is owner-scoped (subject.id == object.owner), so org members can only CRUD their own chats — same as workspaces.agents-accessrole gates chat creation at the handler level.postChatsusesResourceChat.WithOwner()without.InOrg()for the agents-access role gate (intentional belt-and-suspenders).Scope boundaries
chatstable getsorganization_id. Child tables (messages, queued messages, diff statuses) inherit through FK.chat_providersandchat_model_configsremain deployment-wide.Chatis not auditable — noenterprise/audit/table.gochanges.