fix: limit shared chats to ACL grants#26123
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 2 | Last posted: Round 2, 6 findings (1 P1, 1 P2, 2 P3, 1 P4, 1 Nit), APPROVE. Review Finding inventoryFindings
Round logRound 1Panel. 1 P1, 1 P2, 2 P3, 1 Nit. 0 dropped. Reviewed against 69f9b0e..0c7b76d. Round 2Panel. CRF-1 through CRF-5 addressed. 1 P4 new. 0 dropped. Reviewed against 69f9b0e..716a922. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
The approach is sound: stripping RBAC role grants from the subject so only ACL-based access drives the "Shared with me" filter is the right intervention point and avoids duplicating ACL SQL. The fix is minimal and proportional.
The critical issue is that emptying Roles entirely also empties the Rego-derived org_memberships set, which breaks group ACL and "Everyone" group ACL evaluation. User-level ACL sharing works by accident because that Rego path has no is_org_member guard. Group-shared chats silently vanish from the "Shared with me" view. See CRF-1.
Severity summary: 1 P1, 1 P2, 2 P3, 1 Nit.
"Congratulations, the fix just got undone because nobody wrote down why it exists." (Leorio)
🤖 This review was automatically generated with Coder Agents.
| Email: act.Email, | ||
| Type: act.Type, | ||
| ID: act.ID, | ||
| Roles: rbac.RoleIdentifiers{}, |
There was a problem hiding this comment.
P1 [CRF-1] Stripping all roles breaks group ACL and "Everyone" group ACL sharing for SharedOnly queries.
The Rego policy derives org_memberships from input.subject.roles[_].by_org_id[org_id] (policy.rego:90-92). Both group ACL rules gate on is_org_member (policy.rego:345, 358), which requires input.object.org_owner in org_memberships. With Roles: rbac.RoleIdentifiers{}, org_memberships is empty, so is_org_member is always false and both group ACL paths are unreachable.
"User ACL sharing still works because its Rego rule doesn't check
is_org_member." (Hisoka)
Consequence: any chat shared via group ACL silently vanishes from the "Shared with me" tab. The data is intact, but the feature is broken for group shares. The existing TestGetAuthorizedChatsACLSharingGroupACL does not catch this because it calls db.GetAuthorizedChats directly with full roles, bypassing the dbauthz role-stripping path.
Fix direction: preserve org membership while removing permission grants. For example, retain only org-member role identifiers (which establish by_org_id presence without granting admin/owner permissions), or construct synthetic roles with empty permission sets but populated by_org_id keys. (Hisoka P1, Kite P1, Meruem P1, Knov P1)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in ead074ce2617b9ae3261f636bf6e4d7425b5e678. SharedOnly now preserves org membership facts while removing role permission grants, so group and Everyone ACL policy checks can still match without reintroducing owner/admin RBAC-only chat visibility.
| require.NoError(t, err) | ||
| require.GreaterOrEqual(t, len(ownerRows), 5) | ||
|
|
||
| ownerSharedRows, err := authzdb.GetChats(ownerCtx, database.GetChatsParams{ |
There was a problem hiding this comment.
P2 [CRF-2] No positive-case test through the dbauthz layer for SharedOnly with any ACL type.
This test proves that stripping roles blocks RBAC-granted visibility (negative case). It never proves that a user with an explicit ACL share still sees the shared chat when authzdb.GetChats strips their roles. The existing positive tests in TestGetAuthorizedChatsACLSharing and TestGetAuthorizedChatsACLSharingGroupACL call db.GetAuthorizedChats directly with roles intact, bypassing the dbauthz role-stripping entirely.
"If role stripping accidentally breaks ACL evaluation (e.g., Rego requires at least one role for ACL grants to fire), the regression would be silent." (Bisky)
A positive test through authzdb.GetChats with SharedOnly: true would have caught CRF-1 (the group ACL breakage). Two tests are needed: (1) user ACL-shared chat appears, (2) group ACL-shared chat appears, both through the dbauthz path with SharedOnly. (Netero P3, Bisky P3, Mafuuu P3, Pariston P3, Kurapika P4, Kite P3, Meruem P2, Ryosuke P3, Knov)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in ead074ce2617b9ae3261f636bf6e4d7425b5e678. Added positive SharedOnly coverage through the dbauthz GetChats path for both explicit user ACL and group ACL sharing. The group ACL case covers the membership regression, and the user ACL case pins the separate user ACL branch.
| act = rbac.Subject{ | ||
| FriendlyName: act.FriendlyName, | ||
| Email: act.Email, | ||
| Type: act.Type, | ||
| ID: act.ID, | ||
| Roles: rbac.RoleIdentifiers{}, | ||
| Groups: act.Groups, | ||
| Scope: act.Scope, | ||
| }.WithCachedASTValue() |
There was a problem hiding this comment.
P3 [CRF-3] Field-by-field Subject reconstruction will silently drop future fields.
If a new exported field is added to rbac.Subject that participates in authorization, this struct literal will zero it without a compiler warning. act is already a local value copy from ActorFromContext. Mutating it in place is shorter, forward-compatible, and makes intent visible:
act.Roles = rbac.RoleIdentifiers{}
act = act.WithCachedASTValue()(Once CRF-1 is addressed, the assigned value would be the filtered role list rather than an empty one, but the field-assignment approach applies either way.) (Kurapika P3, Ryosuke P2, Knov)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in ead074ce2617b9ae3261f636bf6e4d7425b5e678. I added rbac.Subject.WithRoles(...) so this path avoids field-by-field Subject reconstruction while still clearing and recomputing the cached Rego value after roles change. Mutating the copied subject directly with WithCachedASTValue() would keep a stale cache from the original actor.
| return nil, ErrNoActor | ||
| } | ||
|
|
||
| if arg.SharedOnly { |
There was a problem hiding this comment.
P3 [CRF-4] Role-stripping block has no comment explaining why roles are cleared.
This code looks wrong on first read. A maintainer debugging shared-chat issues at 2 AM sees Roles: rbac.RoleIdentifiers{} and thinks "someone zeroed out all RBAC roles" and reverts it. Every sibling function in this file that does something non-obvious with authorization has a comment.
// Strip role grants from the subject so the authorization filter
// matches only ACL-based access (direct user, group, and scope
// grants). Without this, owner/admin roles include all readable
// chats in the "shared with me" results.
if arg.SharedOnly {(Leorio P3)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in ead074ce2617b9ae3261f636bf6e4d7425b5e678. Added a rationale comment for the SharedOnly authorization rewrite, with wording focused on why RBAC role grants are removed while org membership facts are retained for ACL checks.
|
|
||
| func (q *querier) GetChats(ctx context.Context, arg database.GetChatsParams) ([]database.GetChatsRow, error) { | ||
| prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceChat.Type) | ||
| act, ok := ActorFromContext(ctx) |
There was a problem hiding this comment.
Nit [CRF-5] The commit message has no body. git blame on this block gives "limit shared chats to ACL grants" with no context about why roles were the problem. A one-line body would save future readers a trip to the PR:
fix(coderd/database): limit shared chats to ACL grants
Owner/admin users saw other users' chats in "Shared with me" because
the authorization filter included role-based grants alongside ACL grants.
(Leorio)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in ead074ce2617b9ae3261f636bf6e4d7425b5e678. Amended the commit message body to explain that SharedOnly was polluted by owner/admin and other RBAC grants, and that the fix preserves membership facts for ACL checks without reintroducing RBAC-only visibility.
0c7b76d to
ead074c
Compare
Shared-only chat listing included chats visible through owner, admin, and other RBAC grants. That made the Agents page "Shared with me" source show chats that were not explicitly shared with the viewer. Prepare shared-only chat authorization with role permission grants removed while preserving org membership facts for ACL policy checks. This keeps direct user and group ACL shares visible without reintroducing RBAC-only visibility.
ead074c to
716a922
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All five R1 findings are addressed. The fix is clean and well-tested.
The sharedOnlyChatSubjectRoles function correctly constructs synthetic org-member roles that preserve org_memberships in Rego (via ByOrgID keys) while granting zero permissions. Six reviewers independently verified the CRF-1 fix by tracing the Rego policy chain: org_memberships is populated, is_org_member evaluates true, group and Everyone ACL rules fire, but role_allow stays false because the synthetic roles carry empty permission arrays.
WithRoles is the right abstraction for safe role replacement on a Subject copy. The three new dbauthz-level tests (owner negative, user ACL positive, group ACL positive) cover the exact code paths changed.
Severity summary: 0 P0-P3, 1 P4.
"Traced through the Rego policy:
org_membershipsis populated fromroles[_].by_org_id[org_id](line 91, policy.rego), so the synthetic role establishes membership.is_org_memberevaluates true, enabling group ACL and Everyone ACL checks. No role-level permissions match (empty arrays), sorole_allowstays false. Authorization flows exclusively throughacl_allow. The root cause is addressed, not papered over." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
| require.NoError(t, err) | ||
| require.GreaterOrEqual(t, len(ownerRows), 5) | ||
|
|
||
| ownerSharedRows, err := authzdb.GetChats(ownerCtx, database.GetChatsParams{ |
There was a problem hiding this comment.
P4 [CRF-6] The PR description names "owner or admin RBAC grants" as the motivating case, but only the owner persona is directly tested through the dbauthz SharedOnly path. An org-admin SharedOnly test would pin that persona.
sharedOnlyChatSubjectRoles processes all org-level roles identically (deduplicates by org ID, produces the same synthetic role regardless of input role name), so the code path is already exercised by the owner test. This is documentation-value coverage, not a correctness gap. (Bisky P3, downgraded: mechanism is identical)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Not adding an org-admin-specific test for this round. The SharedOnly branch normalizes all org-scoped roles through the same sharedOnlyChatSubjectRoles path before authorization, so the owner negative test already exercises the RBAC-grant removal mechanism. The user ACL and group ACL positive tests cover the ACL paths that differ behaviorally.
|
/coder-agents-review |
The Agents page "Shared with me" source previously included chats visible through owner or admin RBAC grants. This made owner users see other users' chats even when those chats were not explicitly shared with them.
When listing shared-only chats, prepare the chat read authorization filter with role grants removed from the subject while preserving the subject ID, groups, and scope. This keeps the existing Rego ACL path responsible for direct user, group, and scope checks without duplicating ACL SQL in the chat query.
Note
🤖 This PR was written by Coder Agent on behalf of Danielle Maywood