Skip to content

fix(rbac): allow org_admin to manage user roles in legacy permission path #2007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Dalanir wants to merge 3 commits into from
+177 −1
Closed

fix(rbac): allow org_admin to manage user roles in legacy permission path #2007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a89bfae
feat(db): enable RBAC for all existing organizations
Dalanir Apr 29, 2026
add5d3c
fix(db): use rbac_enable_for_org() to properly backfill role_bindings
Dalanir Apr 29, 2026
b55b325
fix(rbac): allow org_admin to manage user roles in legacy permission …
Dalanir May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion supabase/functions/_backend/utils/rbac.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const PERMISSION_TO_LEGACY_RIGHT: Record<Permission, Database['public']['Enums']
'org.delete': 'super_admin',
'org.read_members': 'read',
'org.invite_user': 'admin',
'org.update_user_roles': 'super_admin',
'org.update_user_roles': 'admin',
'org.read_billing': 'admin',
'org.update_billing': 'super_admin',
'org.read_invoices': 'admin',
Expand Down
19 changes: 19 additions & 0 deletions supabase/migrations/20260429135552_enable_rbac_all_orgs.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- Enable RBAC for all existing organizations.
-- Uses rbac_enable_for_org() to properly backfill role_bindings from org_users
-- before flipping the use_new_rbac flag.
--
-- Rollback (if critical issues are discovered):
-- UPDATE "public"."orgs" SET "use_new_rbac" = false WHERE "use_new_rbac" = true;
-- Note: role_bindings created by this migration will remain but become unused
-- when the flag is false. They do not need to be deleted for a safe rollback.
DO $$
DECLARE
v_org_id uuid;
v_result jsonb;
BEGIN
FOR v_org_id IN
SELECT id FROM "public"."orgs" WHERE "use_new_rbac" = false
LOOP
v_result := "public"."rbac_enable_for_org"(v_org_id, NULL);
END LOOP;
END $$;
82 changes: 82 additions & 0 deletions supabase/migrations/20260501161128_fix_org_admin_update_user_roles_legacy_mapping.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
-- Fix: Allow org_admin to manage user roles in legacy fallback path
--
-- Previously org.update_user_roles mapped to super_admin in the legacy
-- permission resolver, meaning only super_admins could delete/manage
-- org members. This was inconsistent with the RBAC system where
-- org_admin explicitly has org.update_user_roles granted.
--
-- Change: org.update_user_roles -> admin (was super_admin)
-- The priority_rank guard in the application layer still prevents
-- org_admin (rank 90) from deleting org_super_admin (rank 95) bindings.

CREATE OR REPLACE FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") RETURNS "public"."user_min_right"
LANGUAGE "plpgsql" IMMUTABLE
SET "search_path" TO ''
AS $$
BEGIN
-- Map permissions to their legacy equivalents
-- This mapping should match PERMISSION_TO_LEGACY_RIGHT in utils/rbac.ts
CASE p_permission_key
-- Read permissions -> public.rbac_right_read()
WHEN public.rbac_perm_org_read() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_org_read_members() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_app_read() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_app_read_bundles() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_app_read_channels() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_app_read_logs() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_app_read_devices() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_channel_read() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_channel_read_history() THEN RETURN public.rbac_right_read();
WHEN public.rbac_perm_channel_read_forced_devices() THEN RETURN public.rbac_right_read();

-- Upload permissions -> public.rbac_right_upload()
WHEN public.rbac_perm_app_upload_bundle() THEN RETURN public.rbac_right_upload();

-- Write permissions -> public.rbac_right_write()
WHEN public.rbac_perm_app_update_settings() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_app_create_channel() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_app_manage_devices() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_app_build_native() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_channel_update_settings() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_channel_promote_bundle() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_channel_rollback_bundle() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_channel_manage_forced_devices() THEN RETURN public.rbac_right_write();
WHEN public.rbac_perm_org_create_app() THEN RETURN public.rbac_right_write();

-- Admin permissions -> public.rbac_right_admin()
WHEN public.rbac_perm_org_update_settings() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_org_invite_user() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_org_read_billing() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_org_read_invoices() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_org_read_audit() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_app_delete() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_app_read_audit() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_bundle_delete() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_channel_delete() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_channel_read_audit() THEN RETURN public.rbac_right_admin();
WHEN public.rbac_perm_org_update_user_roles() THEN RETURN public.rbac_right_admin();

-- Super admin permissions -> public.rbac_right_super_admin()
WHEN public.rbac_perm_org_update_billing() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_org_read_billing_audit() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_org_delete() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_app_transfer() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_impersonate_user() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_manage_orgs_any() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_manage_apps_any() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_manage_channels_any() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_run_maintenance_jobs() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_delete_orphan_users() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_read_all_audit() THEN RETURN public.rbac_right_super_admin();
WHEN public.rbac_perm_platform_db_break_glass() THEN RETURN public.rbac_right_super_admin();

ELSE RETURN NULL; -- Unknown permission
END CASE;
END;
$$;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

ALTER FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") OWNER TO "postgres";
REVOKE ALL ON FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") FROM PUBLIC;
GRANT EXECUTE ON FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") TO "service_role";
GRANT EXECUTE ON FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") TO "authenticated";
GRANT EXECUTE ON FUNCTION "public"."rbac_legacy_right_for_permission"("p_permission_key" "text") TO "anon";
75 changes: 75 additions & 0 deletions tests/organization-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,81 @@ describe('[DELETE] /organization/members', () => {
})
})

describe('[DELETE] /organization/members - org_admin can delete members', () => {
const adminDeleteOrgId = randomUUID()
const adminDeleteGlobalId = randomUUID()
const adminDeleteCustomerId = `cus_test_admin_del_${adminDeleteOrgId}`

beforeAll(async () => {
const { error: stripeError } = await getSupabaseClient().from('stripe_info').insert({
customer_id: adminDeleteCustomerId,
status: 'succeeded',
product_id: 'prod_LQIregjtNduh4q',
subscription_id: `sub_${adminDeleteGlobalId}`,
trial_at: new Date(Date.now() + 15 * 24 * 60 * 60 * 1000).toISOString(),
is_good_plan: true,
})
if (stripeError)
throw stripeError

// Create org with legacy RBAC path (use_new_rbac: false) to test the fixed legacy mapping
const { error: orgError } = await getSupabaseClient().from('orgs').insert({
id: adminDeleteOrgId,
name: `Admin Delete Test Org ${adminDeleteGlobalId}`,
management_email: TEST_EMAIL,
created_by: USER_ID,
customer_id: adminDeleteCustomerId,
website: 'https://admin-delete-test.example/',
use_new_rbac: false,
})
if (orgError)
throw orgError

// Add USER_ID as admin (NOT super_admin) — this is the key scenario under test
const { error: orgUserError } = await getSupabaseClient().from('org_users').insert({
org_id: adminDeleteOrgId,
user_id: USER_ID,
user_right: 'admin',
})
if (orgUserError)
throw orgUserError
})

afterAll(async () => {
await getSupabaseClient().from('org_users').delete().eq('org_id', adminDeleteOrgId)
await getSupabaseClient().from('orgs').delete().eq('id', adminDeleteOrgId)
await getSupabaseClient().from('stripe_info').delete().eq('customer_id', adminDeleteCustomerId)
})

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()
})
Comment on lines +998 to +1024
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

})

describe('[POST] /organization', () => {
it.concurrent('create organization', async () => {
const name = `Created Organization ${new Date().toISOString()}`
Expand Down
Loading