fix!: require org membership for user ACLs - #26852
Conversation
github-actions
Bot
added
the
release/breaking
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 8 findings (2 P2, 2 P3, 4 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel (19 reviewers). Netero: no findings. 2 P2, 2 P3, 4 Nit. Reviewed against 612b6d4..61b727b. Round 2BLOCKED. CRF-2 (P2), CRF-3 (P3), CRF-6 (Nit) silent. 5 findings addressed by removing CTE scrub and rewriting tests. No review. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Documentation CheckUpdates Needed
Automated review via Coder Agents |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core fix is sound: adding is_org_member to the user ACL rule in policy.rego closes the gap that the TODO comment identified, making user ACLs consistent with the group ACL rules that already required org membership. Test coverage is thorough, with real database tests for the scrub and a well-structured invariant test that locks in both the deny (non-member) and allow (member) paths without breaking site-wide roles.
Severity summary: 2 P2, 2 P3, 4 Nit.
The main concern is the mechanism used for ACL cleanup: the codebase uses BEFORE DELETE triggers on organization_members for sibling cleanup (group membership, AI budget overrides), but this PR uses a CTE embedded in the SQLC query. Seven reviewers flagged this independently. The CTE misses the soft-delete path, where delete_deleted_user_resources() runs a raw DELETE FROM organization_members that fires existing triggers but not the CTE. No security impact (the Rego policy blocks access regardless), but it creates a data hygiene asymmetry.
"A user ACL grant must not authorize a non-member, even one holding a privileged site-wide role." Kite called the test coverage "thorough" and Mafu-san verified genuine test-first development by running the test commits at base SHA and confirming expected failures.
coderd/rbac/acl/updatevalidator.go:30
P3 [CRF-3] Validate checks that user IDs exist (ValidateUserIDs) but never checks whether the user is a member of the resource's organization. An admin can add a user ACL entry for a non-org-member. The Rego policy now blocks the grant from taking effect, but the entry persists. If the user later joins the org for an unrelated reason, the old ACL entry silently activates with no notification.
This gap predates this PR. The PR's is_org_member enforcement makes it newly visible: before, non-member ACL entries were always active (the bug being fixed). After, they lie dormant until a future org join. The write-time check would need the resource's org ID threaded into the validator.
(Hisoka P3, Kurapika P3)
🤖
🤖 This review was automatically generated with Coder Agents.
| WITH deleted_member AS ( | ||
| DELETE FROM organization_members AS organization_member | ||
| WHERE | ||
| organization_id = @organization_id AND | ||
| user_id = @user_id | ||
| ; | ||
| organization_member.organization_id = @organization_id | ||
| AND organization_member.user_id = @user_id | ||
| RETURNING organization_member.organization_id, organization_member.user_id | ||
| ), scrub_template_user_acls AS ( | ||
| UPDATE templates | ||
| SET user_acl = user_acl - deleted_member.user_id::text | ||
| FROM deleted_member | ||
| WHERE | ||
| templates.organization_id = deleted_member.organization_id | ||
| AND templates.user_acl ? deleted_member.user_id::text | ||
| ), scrub_workspace_user_acls AS ( | ||
| UPDATE workspaces | ||
| SET user_acl = user_acl - deleted_member.user_id::text | ||
| FROM deleted_member | ||
| WHERE | ||
| workspaces.organization_id = deleted_member.organization_id | ||
| AND workspaces.user_acl ? deleted_member.user_id::text | ||
| ), scrub_chat_user_acls AS ( | ||
| UPDATE chats | ||
| SET user_acl = user_acl - deleted_member.user_id::text | ||
| FROM deleted_member | ||
| WHERE | ||
| chats.organization_id = deleted_member.organization_id | ||
| AND chats.user_acl ? deleted_member.user_id::text | ||
| ) |
There was a problem hiding this comment.
This should probably be a trigger if it is somthing we want to enforce?
|
/coder-agents-review |
There was a problem hiding this comment.
Five of eight R1 findings were addressed by removing the CTE scrub and rewriting the test comments. The PR is now cleanly scoped to the Rego policy fix + tests.
Three findings have no author response. Further review is blocked until these are addressed or acknowledged:
CRF-2 (P2) The PR description is still empty. This is a breaking change (fix!:). Operators, changelog tooling, and release notes need: what the old behavior was, what the new behavior is, and what downstream impact to expect.
CRF-3 (P3) acl.Validate does not check org membership on the write path. A non-member can be granted an ACL entry that lies dormant and silently activates if they later join the org. This predates the PR; a brief acknowledgment or ticket is sufficient.
CRF-6 (Nit) The is_org_member guard on the user ACL rule in policy.rego:333 has no explanatory comment. The sibling group ACL rules have one. A one-line comment would match the pattern.
To unblock: reply to these threads (fix, acknowledge, or explain why no action is needed) or push changes. The full review panel will run on the next round.
🤖 This review was automatically generated with Coder Agents.
Adds TestAuthorizeUserACLOrgMembership confirming that requiring org
membership for per-user ACL grants does not change how site-wide roles
authorize org-scoped resources:
- A site role that grants the action (template-admin) still authorizes
via its role, with or without an ACL entry, even when the subject is
not a member of the resource's organization.
- A per-user ACL grant does not authorize a non-member, even one with a
privileged site-wide role (user-admin); it only takes effect once the
subject is also an org member.
2 participants
Per-user ACL grants on org-scoped resources were authorized without checking organization membership. A user explicitly granted access to an org-scoped template/workspace/chat retained that access after being removed from the organization, because the user-ACL branch of policy.rego did not require is_org_member (the group-ACL branches already did).
This adds
is_org_memberto the user-ACL rule, matching the group-ACL behavior. After the change, a per-user ACL grant only authorizes a subject who is currently a member of the resource's organization.