feat: add chat model config RBAC resource - #27957
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
@codex review |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 27 findings (1 P1, 5 P2, 14 P3, 5 Nit, 2 Note), REQUEST_CHANGES. Review Finding inventoryFinding inventory, PR #27957Findings
Cross-check notes
Round logRound 1Netero pass: 2 P3 (CRF-1, CRF-2). Panel of 21 (19 trigger-matched + 2 wildcards: Takumi, Melody). New: 1 P1, 5 P2, 12 P3, 5 Nit, 2 Note. Takumi: no findings. Reviewed against 9f6d499..6c26421. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
This PR gets the hard parts of the read cutover right, and the panel said so repeatedly: the GetDefaultChatModelConfigCandidates split prevents a silent no-default commit that an authorized-filter candidate list would have shipped (five reviewers traced it, and the write-window test pins the rollback semantics against real Postgres), the converter mirrors the established WorkspaceConverter shape, the migration follows the enum-add precedent, and every interim write gate carries a ticketed TODO. Takumi ran the concurrency trace and found nothing: "The timing is fine."
Severity count: 1 P1, 5 P2, 14 P3, 5 Nit, 2 Note.
The P1 is the headline: six reviewers converged independently, three with empirical probes against Postgres, on org admins and org auditors of non-default organizations regressing from the full enabled model list to an empty list for the whole fallback window, while plain members of the same org keep the full list. The PR description discloses the custom-site-role regression but not this one, and this one hits built-in roles on every multi-org deployment through the UI model picker.
Two P2s are description/comment honesty: the PR body attributes member read to the seeded org ACL when the code preserves it via the chatd subject grant, and the create-gate comment justifies itself with a dbauthz create-in-org check that does not exist in this PR. One P2 is a test fixture that silently inverts (Enabled: false is dbgen's zero value), turning the centerpiece disabled-visibility assertions into no-ops.
Process observations: CI shows "Pixel / Review" failing on this head. The only frontend changes are generated files, so the diff is an unlikely cause, but a failing check is not "pre-existing"; resolve it or state why it is out of scope before merge. Separately, Gon audited all 33 new comments and found a pattern: several were written for the PR's development narrative (plan milestones, revert history) rather than for the next reader of the file; the individual findings are inline.
Fun quote, from Hisoka: "I pulled the isAdmin thread and something on the other end moved. Shall I show you?"
coderd/exp_chats.go:6551
P2 [CRF-4] The PR description justifies the accepted regression with a mechanism the code does not implement: "members read through the org ACL seeded in the schema PR." (Mafu-san P2, Pariston Note)
Mafu-san traced every member-subject read path: "The list handler routes non-admins to GetEnabledChatModelConfigs(dbauthz.AsChatd(ctx)), so members read because the chatd service subject gained site-level chat_model_config:read (dbauthz.go:795), not through any ACL." ChatModelConfig.RBACObject() has zero non-test callers, and an ACL-only grantee can never reach the SQL filter because the AnyOrganization gate evaluates a bare object with empty ACL lists.
Consequence: the human accepting the tradeoff is told member read is preserved by the seeded ACL. If M3 removes or narrows the chatd grant on that assumption, member read breaks silently. Pariston adds the regression itself is overstated: custom deployment_config:read roles fall to the else branch and still see every enabled config; what they lose is the management view (disabled rows), not "the model list."
Fix: correct the PR description to name the chatd-subject path as the member read mechanism and to scope the stated loss to the management view.
🤖
coderd/exp_chats.go:6551
P3 [CRF-14] The non-admin branch is the only interim authorization site in this PR without a cutover TODO. (Kurapika P3; Hisoka, Mafuuu, Meruem Notes)
Kurapika: "Every authenticated user of any org therefore enumerates every org's enabled configs (model names, provider IDs, context limits, reasoning options), and the org ACLs the schema PR seeded decide nothing on this path. ... After the M3 cutover, when configs are per-org, this branch becomes a standing cross-org disclosure unless someone remembers to scope it. Every other interim decision in this PR carries TODO(mafredri): ... after CODAGT-709 M3; this branch carries only a nolint:gocritic comment."
Three other reviewers flagged the same asymmetry: org-scoped readers are SQL-filtered to their orgs while every plain member sees all orgs' enabled configs. Fine during the window; untagged interim behavior is the kind that survives its window. Add the M3 TODO stating the branch must switch to an org-scoped query at cutover.
🤖
🤖 This review was automatically generated with Coder Agents.
| // sufficient for using the chat feature. AnyOrganization routes | ||
| // org-scoped readers (org admin, org auditor) to the authorized list as | ||
| // well as site readers; the SQL filter then restricts them to their orgs. | ||
| isAdmin := api.Authorize(r, policy.ActionRead, rbac.ResourceChatModelConfig.AnyOrganization()) |
There was a problem hiding this comment.
P1 [CRF-3] Org admins and org auditors of non-default organizations regress from the full enabled model list to an empty list for the entire fallback window. (Pariston P1, Kite P1, Melody P2; also Hisoka P2, Mafuuu P2, Kurapika P2)
The chain, verified link by link and reproduced empirically by three reviewers with probes against Postgres:
createChatModelConfigpins every insert to the default org with an everyone-read ACL keyed to the default org's everyone group; nothing seeds configs at org creation.- An org admin holds
chat_model_config:readin-org viaallPermsExcept(roles.go:585); an org auditor gains it in this PR (roles.go:617). Both now passAuthorize(read, ResourceChatModelConfig.AnyOrganization())and take the filtered branch. - For a non-default-org reader the filter admits rows in their org (none exist during the window) or ACL matches (the seeded key is the default org's everyone group, which they are not in). Empty list.
- Before this PR the same users failed the
deployment_config:readgate and got every enabled config through the chatd branch.
Melody's probe: "The fresh org's org admin lists 0 configs; a plain member of the same org lists 1." The endpoint feeds the model picker (site/src/api/queries/chats.ts, AgentsPage, ModelsPage), so the org admin's picker goes blank while their members' pickers are full. The PR description discloses the custom-site-role regression but not this one, which hits built-in roles in every multi-org deployment.
The test suite cannot catch this: TestChatModelConfigListReadContracts only exercises default-org readers, where all real-window configs live.
Fix options from the panel, smallest first: keep the handler check site-scoped (rbac.ResourceChatModelConfig, no AnyOrganization()) until M3 so org readers keep the pre-PR path and gain the management view at cutover; or union the filtered result with the enabled list (dedupe by ID) so use-visibility never shrinks below the else branch; or fall through to the enabled branch when the filtered list is empty. Whichever way, the list test needs a non-default-org admin/auditor subtest.
🤖
| // sufficient for using the chat feature. AnyOrganization routes | ||
| // org-scoped readers (org admin, org auditor) to the authorized list as | ||
| // well as site readers; the SQL filter then restricts them to their orgs. | ||
| isAdmin := api.Authorize(r, policy.ActionRead, rbac.ResourceChatModelConfig.AnyOrganization()) |
There was a problem hiding this comment.
P3 [CRF-13] A multi-org user who passes the gate through one org sees other orgs' disabled configs through the everyone-group ACL matchers. (Meruem)
"An org auditor in org A also holding plain membership in org B passes AnyOrganization(), lands in the GetChatModelConfigs branch, and the filter returns org B's disabled configs via the everyone-group ACL, rows the enabled-only branch was supposed to hide from them." Every config carries an everyone-read group ACL keyed on the org ID (seeded by migrations 000565/000566), and ChatModelConfigConverter registers ACL matchers, so the filter admits ACL-granted rows.
Consequence: disabled config metadata (model, display name, options, context limit) is disclosed in orgs where the user holds no management or audit role, and the "members see only enabled configs" contract in the comment above this line is enforced nowhere in policy; it is handler discipline only.
🤖
| // sufficient for using the chat feature. AnyOrganization routes | ||
| // org-scoped readers (org admin, org auditor) to the authorized list as | ||
| // well as site readers; the SQL filter then restricts them to their orgs. | ||
| isAdmin := api.Authorize(r, policy.ActionRead, rbac.ResourceChatModelConfig.AnyOrganization()) |
There was a problem hiding this comment.
P3 [CRF-15] ACL-only read grants never reach the authorized list, so the seeded ACLs and any future share grant are dead in the read path this PR cuts over. (Knov P3, Razor Note, Ryosuke Note)
Knov: "the schema PR seeded every config's group_acl with an everyone-in-org read entry precisely for this RBAC PR, and this PR wires those ACLs into the SQL filter and defines ActionShare; through which path does an ACL-only reader ever reach the filtered list? None." The gate tests role permissions against an object with empty ACL lists, so ACL-only readers always fall to the enabled-only branch. Razor traces the consequence forward: the moment share is exposed, "a user shared a disabled config will not see it anywhere."
Latent today (no share endpoint writes per-user ACLs), but the two authorization layers encode different reader sets and nothing pins the asymmetry. At minimum, name it in the handler comment; a test when share lands.
🤖
| // sufficient for using the chat feature. AnyOrganization routes | ||
| // org-scoped readers (org admin, org auditor) to the authorized list as | ||
| // well as site readers; the SQL filter then restricts them to their orgs. | ||
| isAdmin := api.Authorize(r, policy.ActionRead, rbac.ResourceChatModelConfig.AnyOrganization()) |
There was a problem hiding this comment.
P3 [CRF-1] Variable isAdmin no longer describes the branch condition after the authorization refactor. (Netero)
"The gate changed from ActionRead on ResourceDeploymentConfig (admin-only) to ActionRead on ResourceChatModelConfig.AnyOrganization(), which now admits site auditors, org auditors, and custom roles holding only chat_model_config:read. None of these are admins." The next reader extending the if isAdmin branch will key admin-only behavior on a set that includes read-only auditors. Rename to something like useAuthorizedList or canReadConfigs.
🤖
| // ChatModelConfigConverter qualifies columns against the chat_model_configs | ||
| // alias used by GetChatModelConfigs. Chat model configs have no user owner, | ||
| // only an organization owner. | ||
| func ChatModelConfigConverter() *sqltypes.VariableConverter { |
There was a problem hiding this comment.
P3 [CRF-2] ChatModelConfigConverter has no compile test, and its ACL matchers are asserted by no test at any level. (Netero P3, refined by Chopper)
Netero: "coderd/rbac/regosql/compile_test.go has per-converter SQL assertions for WorkspaceConverter ... and ChatConverter .... The new converter gets none." Chopper refines the scope: the integration test does exercise the converter's compile and org_owner path end to end against Postgres, but every subject in it reads through role permissions, never through group_acl/user_acl alone, so the ACLMappingMatcher branches produce SQL no test asserts. Pariston adds those branches are also unreachable through any production path until M3, so no failure can surface a mistake in them before the cutover; Melody notes ChatModelConfig.RBACObject() likewise has no non-test caller, so the Go-side and SQL-side authorization shapes can drift silently.
Add compile_test cases mirroring the UserChatACLAllow pattern with the cmc. alias.
🤖
| return q.db.GetAuthorizedChatModelConfigs(ctx, prepared) | ||
| } | ||
|
|
||
| // GetDefaultChatModelConfigCandidates enumerates the configs that |
There was a problem hiding this comment.
Nit [CRF-23] The doc comment claims the method "enumerates the configs that ensureDefaultChatModelConfig may promote to default", but it returns every non-deleted config in every organization. (Zoro)
The org filtering and ID exclusion happen in the caller (exp_chats.go:7145-7151), so the returned set contains configs the caller may never promote. Zoro's redraft keeps the name and widens the contract: "returns the unfiltered, deployment-wide config list from which ensureDefaultChatModelConfig selects a promotion candidate for one organization," which also absorbs the rationale currently split with modelqueries.go:61-64.
🤖
| ) | ||
| } | ||
|
|
||
| // ChatModelConfigConverter qualifies columns against the chat_model_configs |
There was a problem hiding this comment.
Nit [CRF-24] "the chat_model_configs alias" is wrong; the alias is cmc. (Gon)
The matchers hardcode cmc.id, cmc.organization_id, cmc.group_acl, cmc.user_acl, and the query aliases chat_model_configs cmc. A reader grepping the SQL for a chat_model_configs alias finds nothing. Say "the cmc alias".
🤖
| // Allow auditors to query deployment stats and insights. | ||
| ResourceDeploymentStats.Type: {policy.ActionRead}, | ||
| ResourceDeploymentConfig.Type: {policy.ActionRead}, | ||
| // Chat model configs are org-scoped but auditors retain their |
There was a problem hiding this comment.
Nit [CRF-25] "auditors retain their deployment-config read parity with a site-level read here" does not parse on first read. (Gon, Leorio)
Leorio: "'parity' has no stated counterpart." What it means: site auditors used to read model configs through deployment_config:read, and this grant preserves that access. Say that: "Reads of chat model configs authorized against deployment_config:read before the chat_model_config resource existed; this site-level read preserves that auditor access."
🤖
| } | ||
|
|
||
| func (q *querier) DeleteChatModelConfigByID(ctx context.Context, id uuid.UUID) error { | ||
| // Interim: keep the deployment-config update check so the old write |
There was a problem hiding this comment.
P3 [CRF-26] Comment duplication is a pattern across the PR's interim gates. (Gon P2, Leorio Nit; consolidated at P3)
Two shapes. First, four dbauthz comments duplicate their own TODO line: "The fetch-then-authorize object delete check lands with the cutover." (2180) says what the TODO on the next line says with the ticket and timing; same at 3460, 6044, 7410. Second, per Leorio, the interim rationale is copy-pasted near-verbatim five times (2178, 3458, 6042, 7226, 7408) while "exp_chats.go already shows the right treatment in this very PR: full rationale once on createChatModelConfig, then 'Interim write gate: see createChatModelConfig.' on the siblings." Gon also flags three comments restating adjacent code or assertions: roles.go:617 restates the map entry below it, exp_chats_test.go:4197 restates its parent comment and require messages, and exp_chats_test.go:4238's "and is not candidate-list coverage" restates the clause before it.
When the cutover happens, the person doing it edits one explanation instead of hunting five drifting copies. One anchor comment, four pointers, and the trims listed above.
🤖
| @@ -0,0 +1,6 @@ | |||
| ALTER TYPE api_key_scope ADD VALUE IF NOT EXISTS 'chat_model_config:*'; | |||
There was a problem hiding this comment.
Note [CRF-27] Five of the six irreversible enum values are speculative in this PR. (Pariston)
"Only chat_model_config:read has any behavior behind it here; create, update, delete, share, and * gate write paths that this PR deliberately keeps on deployment_config:update until the API PR. Postgres cannot drop enum values, so if the cutover reshapes the action set (share in particular has no current consumer anywhere in the diff), the values are permanent residue." Seeding scopes ahead of the API PR is coherent; worth weighing only whether share is certain enough to burn an enum value on now.
🤖
6c26421 to
2026b3f
Compare
Part of CODAGT-709.
Adds a
chat_model_configRBAC resource (create/read/update/delete/share), a regosql converter so list reads filter in SQL (GetAuthorizedChatModelConfigs), and migration000567with six newapi_key_scopeenum values (Postgres can't drop enum values, so the down is deliberately a no-op). Site and org auditors gain read, as does the chatd subject.Only the read path switches here; the write handlers keep their interim
deployment_config:updategates until the API PR completes the cutover.The management gate in
listChatModelConfigsstays site-scoped while the enabled-config fallback is deployment-wide. Org admins and org auditors remain on the enabled-config path until the API PR makes config use org-scoped. This prevents an org-scoped read grant from shrinking the usable model list for organizations created after the migration or for configs created after it.Worth flagging: custom site roles holding only
deployment_config:readlose the management view in this PR, since reads now authorise against the new resource. They still see every enabled config through the fallback branch; what they no longer see is disabled rows. The fallback works because the chatd subject gained site-levelchat_model_config:read. The seeded org ACL does not preserve member reads in this interim path because ACLs are evaluated only after a reader reaches the authorized SQL filter.