fix(rbac): allow org_admin to manage user roles in legacy permission path#2007
Closed
fix(rbac): allow org_admin to manage user roles in legacy permission path#2007
Conversation
Contributor
📝 WalkthroughWalkthroughUpdates legacy RBAC handling so Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.1.0)supabase/migrations/20260429135552_enable_rbac_all_orgs.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: supabase/migrations/20260501161128_fix_org_admin_update_user_roles_legacy_mapping.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/organization-api.test.ts (1)
988-1014: ⚡ Quick winConsider marking this regression test
it.concurrent.The test already uses dedicated org/customer fixtures, so it should be safe to parallelize with the rest of the file. As per coding guidelines,
tests/**/*.test.ts: Design all tests for parallel execution across files; useit.concurrent()instead ofit()to maximize parallelism within test files.Suggested change
- it('org_admin can delete a read member', async () => { + it.concurrent('org_admin can delete a read member', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-api.test.ts` around lines 988 - 1014, The test named "org_admin can delete a read member" should be marked concurrent to allow parallel execution; update the test declaration (the it(...) call for that test) to use it.concurrent(...) so the spec for the function handling organization deletion (the DELETE request to /organization/members in this test) runs concurrently with other tests while keeping the same assertions and fixture usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@supabase/migrations/20260501161128_fix_org_admin_update_user_roles_legacy_mapping.sql`:
- Around line 12-75: The rbac_legacy_right_for_permission SQL function is
created with default PUBLIC execute rights; fix by adding explicit permission
hardening: REVOKE ALL ON FUNCTION public.rbac_legacy_right_for_permission(text)
FROM PUBLIC; then GRANT EXECUTE ON FUNCTION
public.rbac_legacy_right_for_permission(text) TO the intended role(s) (e.g.,
rbac_role or service_role) and set the function owner explicitly with ALTER
FUNCTION ... OWNER TO <owner_role>; ensure the REVOKE runs before the GRANT and
include the same signature (text) when referencing the function.
---
Nitpick comments:
In `@tests/organization-api.test.ts`:
- Around line 988-1014: The test named "org_admin can delete a read member"
should be marked concurrent to allow parallel execution; update the test
declaration (the it(...) call for that test) to use it.concurrent(...) so the
spec for the function handling organization deletion (the DELETE request to
/organization/members in this test) runs concurrently with other tests while
keeping the same assertions and fixture usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26f0e065-1e2a-4e90-b410-f6d43f5fe726
📒 Files selected for processing (4)
supabase/functions/_backend/utils/rbac.tssupabase/migrations/20260429135552_enable_rbac_all_orgs.sqlsupabase/migrations/20260501161128_fix_org_admin_update_user_roles_legacy_mapping.sqltests/organization-api.test.ts
Address CodeRabbit review: - Replace raw UPDATE with rbac_enable_for_org() loop to migrate org_users to role_bindings before enabling the flag - Add rollback documentation in migration header
…path org.update_user_roles was mapped to super_admin in the legacy fallback (rbac_legacy_right_for_permission), preventing org_admin from deleting members. This was inconsistent with RBAC where org_admin explicitly has this permission. The priority_rank guard still prevents org_admin from deleting org_super_admin bindings.
Dalanir
force-pushed
the
fix/org-admin-update-user-roles-legacy-mapping
branch
from
8fbf5b0 to
b55b325
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/organization-api.test.ts`:
- Around line 998-1024: The test "org_admin can delete a read member" currently
verifies the org_users row removal but not the RBAC cleanup; after the existing
verification of org_users (using getSupabaseClient() and adminDeleteOrgId /
userData!.id), add a query against the role_bindings table (select where org_id
= adminDeleteOrgId and user_id = userData!.id) and assert that the query returns
no data and an error or empty result (i.e., role binding is removed); use the
same Supabase client pattern as the existing checks to fetch from
'role_bindings' and assert the binding is gone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 899c639d-8796-48d9-ab28-c1e6410df6a6
📒 Files selected for processing (4)
supabase/functions/_backend/utils/rbac.tssupabase/migrations/20260429135552_enable_rbac_all_orgs.sqlsupabase/migrations/20260501161128_fix_org_admin_update_user_roles_legacy_mapping.sqltests/organization-api.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/migrations/20260429135552_enable_rbac_all_orgs.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/utils/rbac.ts
Comment on lines
+998
to
+1024
| it.concurrent('org_admin can delete a read member', async () => { | ||
| // Add USER_ADMIN as a read member to the org | ||
| const { data: userData, error: userError } = await getSupabaseClient().from('users').select().eq('email', USER_ADMIN_EMAIL).single() | ||
| expect(userError).toBeNull() | ||
| expect(userData).toBeTruthy() | ||
|
|
||
| const { error: addError } = await getSupabaseClient().from('org_users').insert({ | ||
| org_id: adminDeleteOrgId, | ||
| user_id: userData!.id, | ||
| user_right: 'read', | ||
| }) | ||
| expect(addError).toBeNull() | ||
|
|
||
| // USER_ID (admin) deletes USER_ADMIN (read) — should succeed | ||
| const response = await fetch(`${BASE_URL}/organization/members?orgId=${adminDeleteOrgId}&email=${USER_ADMIN_EMAIL}`, { | ||
| headers, | ||
| method: 'DELETE', | ||
| }) | ||
| expect(response.status).toBe(200) | ||
| const responseData = await response.json() as { status: string } | ||
| expect(responseData.status).toBe('ok') | ||
|
|
||
| // Verify the member was actually removed | ||
| const { data, error: orgUserError } = await getSupabaseClient().from('org_users').select().eq('org_id', adminDeleteOrgId).eq('user_id', userData!.id).single() | ||
| expect(orgUserError).toBeTruthy() | ||
| expect(data).toBeNull() | ||
| }) |
Contributor
There was a problem hiding this comment.
Also assert the lingering RBAC binding is removed.
This regression test proves the member row is deleted, but it still misses the authorization state cleanup. If a role_bindings row survives, the legacy-path bug could still slip through unnoticed.
Suggested test addition
const { data, error: orgUserError } = await getSupabaseClient().from('org_users').select().eq('org_id', adminDeleteOrgId).eq('user_id', userData!.id).single()
expect(orgUserError).toBeTruthy()
expect(data).toBeNull()
+
+ const { data: bindingsAfterDelete, error: bindingsAfterDeleteError } = await getSupabaseClient()
+ .from('role_bindings')
+ .select('id')
+ .eq('principal_type', 'user')
+ .eq('principal_id', userData!.id)
+ .eq('org_id', adminDeleteOrgId)
+ expect(bindingsAfterDeleteError).toBeNull()
+ expect(bindingsAfterDelete).toHaveLength(0)
})
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.concurrent('org_admin can delete a read member', async () => { | |
| // Add USER_ADMIN as a read member to the org | |
| const { data: userData, error: userError } = await getSupabaseClient().from('users').select().eq('email', USER_ADMIN_EMAIL).single() | |
| expect(userError).toBeNull() | |
| expect(userData).toBeTruthy() | |
| const { error: addError } = await getSupabaseClient().from('org_users').insert({ | |
| org_id: adminDeleteOrgId, | |
| user_id: userData!.id, | |
| user_right: 'read', | |
| }) | |
| expect(addError).toBeNull() | |
| // USER_ID (admin) deletes USER_ADMIN (read) — should succeed | |
| const response = await fetch(`${BASE_URL}/organization/members?orgId=${adminDeleteOrgId}&email=${USER_ADMIN_EMAIL}`, { | |
| headers, | |
| method: 'DELETE', | |
| }) | |
| expect(response.status).toBe(200) | |
| const responseData = await response.json() as { status: string } | |
| expect(responseData.status).toBe('ok') | |
| // Verify the member was actually removed | |
| const { data, error: orgUserError } = await getSupabaseClient().from('org_users').select().eq('org_id', adminDeleteOrgId).eq('user_id', userData!.id).single() | |
| expect(orgUserError).toBeTruthy() | |
| expect(data).toBeNull() | |
| }) | |
| it.concurrent('org_admin can delete a read member', async () => { | |
| // Add USER_ADMIN as a read member to the org | |
| const { data: userData, error: userError } = await getSupabaseClient().from('users').select().eq('email', USER_ADMIN_EMAIL).single() | |
| expect(userError).toBeNull() | |
| expect(userData).toBeTruthy() | |
| const { error: addError } = await getSupabaseClient().from('org_users').insert({ | |
| org_id: adminDeleteOrgId, | |
| user_id: userData!.id, | |
| user_right: 'read', | |
| }) | |
| expect(addError).toBeNull() | |
| // USER_ID (admin) deletes USER_ADMIN (read) — should succeed | |
| const response = await fetch(`${BASE_URL}/organization/members?orgId=${adminDeleteOrgId}&email=${USER_ADMIN_EMAIL}`, { | |
| headers, | |
| method: 'DELETE', | |
| }) | |
| expect(response.status).toBe(200) | |
| const responseData = await response.json() as { status: string } | |
| expect(responseData.status).toBe('ok') | |
| // Verify the member was actually removed | |
| const { data, error: orgUserError } = await getSupabaseClient().from('org_users').select().eq('org_id', adminDeleteOrgId).eq('user_id', userData!.id).single() | |
| expect(orgUserError).toBeTruthy() | |
| expect(data).toBeNull() | |
| const { data: bindingsAfterDelete, error: bindingsAfterDeleteError } = await getSupabaseClient() | |
| .from('role_bindings') | |
| .select('id') | |
| .eq('principal_type', 'user') | |
| .eq('principal_id', userData!.id) | |
| .eq('org_id', adminDeleteOrgId) | |
| expect(bindingsAfterDeleteError).toBeNull() | |
| expect(bindingsAfterDelete).toHaveLength(0) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/organization-api.test.ts` around lines 998 - 1024, The test "org_admin
can delete a read member" currently verifies the org_users row removal but not
the RBAC cleanup; after the existing verification of org_users (using
getSupabaseClient() and adminDeleteOrgId / userData!.id), add a query against
the role_bindings table (select where org_id = adminDeleteOrgId and user_id =
userData!.id) and assert that the query returns no data and an error or empty
result (i.e., role binding is removed); use the same Supabase client pattern as
the existing checks to fetch from 'role_bindings' and assert the binding is
gone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
org.update_user_roleslegacy permission mapping fromsuper_admintoadminin both SQL (rbac_legacy_right_for_permission) and TypeScript (PERMISSION_TO_LEGACY_RIGHT)org_admincan delete a read member from an organizationMotivation (AI generated)
org_adminhasorg.update_user_rolesexplicitly granted in the RBAC system, but the legacy fallback path mapped this permission tosuper_admin, creating an inconsistency. This preventedorg_adminusers from managing members when the legacy path was used. Thepriority_rankguard (org_admin=90 < org_super_admin=95) already prevents privilege escalation, so thesuper_admingate was unnecessarily restrictive.Business Impact (AI generated)
Org admins can now properly manage members (invite, delete) as intended by the RBAC design. Previously this was silently blocked in the legacy path, causing confusion for customers with admin-level roles.
Test Plan (AI generated)
org_admin can delete a read member— creates a legacy org, adds USER_ID asadmin, verifies they can delete another memberorganization-api.test.tstests pass (no regressions)org.deletestill requiressuper_admin(existing test at line 1298 unchanged)priority_rankguard unchanged —org_adminstill cannot deleteorg_super_adminbindingsGenerated with AI
Summary by CodeRabbit
Bug Fixes
Chores
Tests