fix(security): clean up role_bindings on member removal

[Dalanir]

Summary

• Fixes RBAC privilege persistence vulnerability where removed org members retained their role bindings (security issues retribution #1667)
• deleteMember now deletes all role_bindings entries for the user within the org (all scopes: org, app, channel, bundle) after removing from org_users
• Uses supabaseAdmin — permission already validated via checkPermission('org.update_user_roles') before any mutations

Test plan

• 'delete organization member' test extended: seeds an org_member role_binding before deletion, asserts it is removed after
• All other member deletion error-case tests unchanged
• Run bun test tests/organization-api.test.ts to verify

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed organization member removal to ensure all associated role bindings and access controls are properly cleaned up when a user is removed from an organization.

• Tests

  • Added test coverage to verify that role bindings are completely removed along with the user when organization members are deleted.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds cleanup logic to remove RBAC role bindings when a user is deleted from an organization, with corresponding test coverage to verify the role_bindings entries are properly removed alongside org_users records.

Changes

Cohort / File(s)	Summary
Role Binding Cleanup supabase/functions/_backend/public/organization/members/delete.ts	Adds deletion of role_bindings table entries (where principal_type='user' and org_id matches) immediately after removing a user from organization, with dedicated error handling.
Test Coverage tests/organization-api.test.ts	Extends member deletion test to seed a role_bindings entry and verify it is cleaned up alongside the org_users record when a member is deleted.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 When members hop away with haste,
We clean their bindings, not a trace to waste!
The roles they had must disappear,
Just as the user's org record, clear.
Hopping through databases with care and grace,
Leaving the organization a tidy place! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: cleaning up role_bindings on member removal, which is the core security fix in this changeset.
Description check	✅ Passed	The description covers the summary (vulnerability fix and solution), test plan with checkbox items, and references the related issue. All major required sections are present and adequately filled out.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-member-removal-rbac

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/organization-api.test.ts (1)

328-339: Consider adding error handling for the role lookup.

The query at line 329 doesn't destructure the error. If the 'org_member' role doesn't exist in seed data, roleData will be null and the ! assertion on line 334 will throw a runtime error with an unclear message.

🔧 Proposed improvement for clearer test failure messages

     // Seed a role_binding to verify it gets cleaned up on member removal
-    const { data: roleData } = await getSupabaseClient().from('roles').select('id').eq('name', 'org_member').single()
+    const { data: roleData, error: roleError } = await getSupabaseClient().from('roles').select('id').eq('name', 'org_member').single()
+    expect(roleError).toBeNull()
     expect(roleData).toBeTruthy()

🤖 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 328 - 339, The roles lookup
ignores possible query errors and may return null roleData, causing a confusing
runtime failure when later using roleData!.id; update the query that calls
getSupabaseClient().from('roles').select('id').eq('name', 'org_member').single()
to destructure both { data: roleData, error: roleError }, assert roleError is
null and add an explicit expect(roleData).toBeTruthy() (or fail with a clear
message) before inserting into role_bindings, so the test fails with a clear
error if the 'org_member' seed is missing.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/organization-api.test.ts`:
- Around line 328-339: The roles lookup ignores possible query errors and may
return null roleData, causing a confusing runtime failure when later using
roleData!.id; update the query that calls
getSupabaseClient().from('roles').select('id').eq('name', 'org_member').single()
to destructure both { data: roleData, error: roleError }, assert roleError is
null and add an explicit expect(roleData).toBeTruthy() (or fail with a clear
message) before inserting into role_bindings, so the test fails with a clear
error if the 'org_member' seed is missing.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8a3ced and 614e451.

📒 Files selected for processing (2)

• supabase/functions/_backend/public/organization/members/delete.ts
• tests/organization-api.test.ts

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud