feat: add chat-access site-wide role to gate chat creation#23724
feat: add chat-access site-wide role to gate chat creation#23724
Conversation
Documentation CheckUpdates Needed
New Documentation Needed
Automated review via Coder Tasks |
There was a problem hiding this comment.
Pull request overview
Adds a new built-in chat-access site role to gate chat CRUD/creation, with backend RBAC updates, migration to backfill existing chat creators, and frontend role UI/mocks updates.
Changes:
- Introduce
chat-accessbuilt-in role (Go + TS constants) and update RBAC permission matrices/tests. - Remove
ResourceChatfrom default member/org-member/org-service-account permission sets to prevent implicit access. - Add DB migration to grant
chat-accessto users who have created chats; update frontend role display/description and story mocks.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/testHelpers/entities.ts | Adds mock chat-access role and includes it in mock role lists. |
| site/src/pages/OrganizationSettingsPage/UserTable/UserRoleCell.tsx | Adds chat-access to the role sort/display ordering. |
| site/src/pages/OrganizationSettingsPage/UserTable/EditRolesButton.tsx | Adds user-facing description for the new role. |
| site/src/pages/OrganizationSettingsPage/UserTable/EditRolesButton.stories.tsx | Updates story to include chat-access in selected roles. |
| site/src/api/typesGenerated.ts | Adds RoleChatAccess constant for frontend use. |
| codersdk/rbacroles.go | Adds RoleChatAccess constant to the SDK. |
| coderd/rbac/roles_test.go | Updates RBAC test matrix to require chat-access for chat CRUD at user scope. |
| coderd/rbac/roles.go | Defines chat-access role; removes ResourceChat from default member/org-member/service-account permission sets; allows assignment by system/owner/user-admin. |
| coderd/database/migrations/000455_chat_access_role.up.sql | Backfills chat-access to users who have created chats. |
| coderd/database/migrations/000455_chat_access_role.down.sql | Removes chat-access from all users on rollback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Members no longer get chat CRUD by default. A new built-in 'chat-access' role must be assigned to grant chat create, read, update, and delete permissions on the user's own chats. Owners retain full access via site-level permissions. The system, owner, and user-admin roles can assign chat-access. A migration auto-assigns the role to every user who has ever created a chat, so existing users are not disrupted. Also excludes ResourceChat from org member and org service account allPermsExcept calls to prevent permission leaks.
- Rebase onto main, renumber migration 000455 → 000456 (collision) - Revert ResourceChat exclusion from org member/service account allPermsExcept (ChatFile uses InOrg and needs org-member path) - Remove pre-existing duplicate ResourceOrganizationMember - Add ChatFileUsage RBAC test case for org-scoped chat resources - Grant chat-access to test users in querier_test and exp_chats_test - Handle NotAuthorizedError in postChats (return 403 not 500) - Scope down migration with WHERE clause to avoid dead tuples
johnstcn
force-pushed
the
cian/chat-access-role
branch
from
e986e63 to
6b5ca79
Compare
- Add chat-access to pagination user in querier_test.go - Add chat-access to expected site role names in TestListRoles - Add RoleChatAccess to 3 enterprise TestListRoles subtests (false for member/orgAdmin, true for admin)
Filter chat-access from the assignable site roles API response when ExperimentAgents is not enabled. The role still exists in RBAC so existing assignments and enforcement work regardless, but it won't clutter the role picker for deployments that haven't opted into the agents experiment.
Remove MockChatAccessRole from story selectedRoleNames — the role shows up via MockSiteRoles already. Un-export the mock constant since it's only used within entities.ts.
Short-circuit with 403 before expensive validation (workspace selection, model config, MCP servers, labels) when the user lacks chat-access. Matches the pattern in postChatFile.
The role is hidden from the API unless the agents experiment is enabled, so it shouldn't be in default mock data.
Match RequireExperimentWithDevBypass behavior — dev builds show the chat-access role even without the agents experiment.
Instead of including chat-access in SiteBuiltInRoles and then filtering it out, exclude it from the default list and append it in the handler only when the agents experiment is enabled or this is a dev build. Avoids unnecessary slices.DeleteFunc.
johnstcn
changed the title
Modify TestPostChats/Success to use a member with chat-access instead of the owner. Add two new denial subtests: - TestPostChats/MemberWithoutChatAccess: 403 on create - TestListChats/MemberWithoutChatAccess: empty list
Add Chat Access role to prerequisites and insert a new Step 3 explaining how to grant the role. Update early-access.md setup steps to include the role grant.
|
TODO: the agents page could use some love after this change. Update: filed #23831 
|
Any user who can read chats needs to resolve the default model config to create one. The previous ResourceDeploymentConfig check blocked regular members. Check ResourceChat:read instead, which aligns with the actual access requirement.
johnstcn
force-pushed
the
cian/chat-access-role
branch
from
c97f26e to
6ab2a1e
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
The role extraction is well-designed: splitting chat permissions from the implicit member role into an explicit chat-access role is the right approach, the migration grandfathers existing chat users with idempotency, and the conditional visibility in AssignableSiteRoles correctly gates on the experiment flag. The test matrix additions for the new role and the negative MemberWithoutChatAccess tests show good coverage intent.
Severity count: 2×P1, 2×P2, 3×P3, and some notes.
The two P1s need attention before merge: (1) OrgMemberPermissions and OrgServiceAccountPermissions were not updated to exclude ResourceChat, leaving a bypass path for org-scoped chat resources (all six reviewers converged on this), and (2) resolveCreateChatModelConfigID uses user context for a deployment config read that non-admin users cannot perform, breaking the API path when ModelConfigID is omitted. Two things that interact: fixing the OrgMember exclusion without also addressing ChatFile's org-scoped authorization will break file uploads for chat-access users, since the chat-access role only grants User-scope permissions.
Quote from the panel: "The gate looks solid from the front door. Chat create, list, read all check the right resource without org scope, so the member exclusion works. But the side entrance through chat files uses org-scoped resources, and the org-member role was never told about the new rules."
PS. Several unrelated cosmetic changes (formatting in OrgMemberPermissions/OrgServiceAccountPermissions action lists, enterprise test struct collapsing, blank line removal in TestListRoles) add noise to the diff.
🤖 This review was automatically generated with Coder Agents.
johnstcn
force-pushed
the
cian/chat-access-role
branch
from
8039621 to
020bdfc
Compare
|
Smoke-tested with two member users, one with the "chat access" role and one without. |
- Cache chatAccessRole like other built-in roles (P3 perf) - Add ChatAccessRole().Valid() to TestBuiltInRoles (P3 coverage) - Fix vacuous MemberWithoutChatAccess list test: create a chat for the member via AsSystemRestricted before asserting empty - Restore LastModelConfigID assertion dropped during refactor
johnstcn
force-pushed
the
cian/chat-access-role
branch
from
f36c03c to
d6ccb9d
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Design is sound: stripping ResourceChat from the member role's allPermsExcept and introducing a dedicated chat-access role is a clean way to gate an experimental feature. The pre-cached role variable, the migration backfill, the conditional visibility in AssignableSiteRoles, and the pre-flight authorization checks are all well done. Round 1 fixes (role caching, vacuous test, dropped assertion, SiteBuiltInRoles validation) are confirmed resolved.
Severity count: 1 P0, 1 P2.
The P0 is a CI-blocking test failure: the dbauthz mock test for GetDefaultChatModelConfig asserts the wrong resource object. The P2 is a gap in the RBAC test safety net: chatAccessUser is not in requiredSubjects, so accidental role broadening would be undetected.
Deferred findings from round 1 (OrgMemberPermissions not excluding ResourceChat, chatAccessUser lacking org-member role in RBAC test) remain deferred per internal ticket coder/internal#1436. Hisoka's expanded analysis confirms the ChatFile consequence: org members without chatAccess can still pass the dbauthz check on org-scoped ChatFile objects via the wildcard, gated only by UUID obscurity. When org-scoped chats land, this will need addressing.
Drive-by formatting changes in OrgMemberPermissions (lines 1049, 1130) and enterprise/coderd/roles_test.go (Name+APICall collapses) are unrelated to the PR purpose and add diff noise to security-sensitive code paths.
"CI is red for this." -- Hisoka ♦
🤖 This review was automatically generated with Coder Agents.
Every test case now explicitly places chatAccessUser in either the true or false bucket. For non-chat resources it mirrors memberMe (the chatAccess role only grants User-scope chat perms). This catches accidental permission broadening.
|
I think I got all of the failed edits fixed up. |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 1 findings are confirmed resolved. The P0 dbauthz mock test now passes, chatAccessUser is in requiredSubjects covering every test case, and the drive-by formatting changes were reverted. Well addressed.
Severity count: 1 P2.
The P2 is a newly-reachable code path: chatCostSummary (and two sibling handlers) returns 500 instead of 403 when a member without chat-access queries their own cost data. Before this PR, all members had user-scope ResourceChat read, so the dbauthz denial never triggered. Now it does, and the error falls through to InternalServerError instead of Forbidden.
Deferred items (OrgMemberPermissions wildcard, chatAccessUser org-member role in RBAC test) remain tracked in coder/internal#1436. Not re-raised.
Three reviewers independently noted there's no test for experiment-disabled role visibility in AssignableSiteRoles. In test builds, buildinfo.IsDev() always returns true, so the experiment gate is never exercised. This matches the project's RequireExperimentWithDevBypass pattern, so the gap is by design.
Hisoka: "The 500 is the old world's ghost. Before this PR, every member could read. Now they can't, and the handler doesn't know."
🤖 This review was automatically generated with Coder Agents.
| }) | ||
| return | ||
| } | ||
| if dbauthz.IsNotAuthorizedError(err) { |
There was a problem hiding this comment.
P2 This IsNotAuthorizedError check is the right pattern, but the chatCostSummary handler (line 617) doesn't follow it. When targetUser.ID == apiKey.UserID, the handler skips its own Authorize check and defers to dbauthz. For a member without chat-access, GetChatCostSummary (dbauthz.go:2510) denies, and the error falls through to httpapi.InternalServerError (line 628). Same for GetChatCostPerModel (line 638) and GetChatCostPerChat (line 648). (Hisoka)
Before this PR, all members had user-scope ResourceChat read, so this dbauthz denial never triggered on self-access. Now it does. Applying the same IsNotAuthorizedError -> Forbidden check to all three error handlers in chatCostSummary would make the behavior consistent and avoid leaking the internal dbauthz error message.
🤖
There was a problem hiding this comment.
This is the only remaining open item. The endpoint is experiment-gated and only reachable by direct API callers, so it's not blocking. But the fix is one if dbauthz.IsNotAuthorizedError(err) check on each of the three error handlers in chatCostSummary -- could be a quick pre-merge fix or a follow-up.
🤖
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
All round 1 findings are resolved and verified. Clearing the stale CHANGES_REQUESTED.
One open item from round 2: the P2 about chatCostSummary returning 500 instead of 403 for non-chat-access self-access (#comment-3010904924). The endpoint is experiment-gated, the blast radius is limited to direct API callers, and the fix is mechanical (add the same IsNotAuthorizedError check used in postChats). Not blocking, but worth a follow-up or a quick fix before merge.
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Minor note that "chat access" seems slightly disconnected from "Coder Agents". There aren't really any other chats in the product, so I think this is minor. But we should think about how we describe the functionality in a consistent manner.
Yeah I'm happy to rename this; I just punted on the naming. Thanks for lending me your army of review-bots! |
|
Renamed to |
chat-accessbuilt-in role granting chat CRUD at User scopeResourceChatfrom member, org member, and org service accountallPermsExceptcallsmemberMedenied,chatAccessUserallowedBreaking change: Members without
chat-accesslose chat creation ability. Migration covers existing chat creators. Members who have never created a chat do not get this role automatically applied.