feat: RBAC - management app access #1569
Merged
Merged
Conversation
- Add SearchInput component with icon for consistent search UI - Add RoleSelect component for role dropdown selection - Both components support v-model and are fully typed - Reduces code duplication across modals and forms Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Reusable modal for role/permission selection - Integrates with RoleSelect component - Handles validation and loading states - Supports custom titles and descriptions - Emits confirm/cancel events for flexible usage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
AppAccess.vue: - Replace search input with SearchInput component - Replace role select with RoleSelect component - Replace Edit Role Modal with RoleSelectionModal - Remove duplicate modal code (~40 lines) - Refactor updateRoleBinding to handleEditRoleConfirm Members.vue: - Replace search input in App Access modal with SearchInput - Replace role select with RoleSelect component - Remove unused selectedOrgRoleSummary computed property Benefits: - Reduced code duplication - Consistent UI across modals - Easier maintenance and testing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Import getRbacRoleI18nKey for proper role translation - Normalize role names (remove invite_ prefix) - Use i18n translations when available - Fallback to human-readable format (replace _ with spaces) - Fixes display of roles like "app_admin" → "App Admin" Applies the same display logic used in Members.vue and AppAccess.vue for consistent user experience across the application. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR restricts default org_member RBAC (removing many app/channel/bundle grants), adds role-selection UI components and an app-access management flow, deletes the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Members.vue / AppAccess UI
participant Modal as RoleSelectionModal
participant API as Supabase Functions (HTTP)
participant DB as PostgreSQL
User->>UI: Open App-access modal
UI->>API: GET apps & available roles
API->>DB: Query apps, roles, role_bindings
DB-->>API: Return data
API-->>UI: Send apps, roles, bindings
UI->>Modal: Show RoleSelect (roles)
User->>Modal: Select apps + role
User->>UI: Save
UI->>API: POST/PUT role_bindings (create/update)
API->>DB: INSERT/UPDATE role_bindings (RBAC checks)
DB-->>API: Confirm
API-->>UI: Success
UI->>User: Show toast, refresh bindings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.0.0)supabase/migrations/20260203201308_rbac_org_member_no_app_access.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: 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. Comment |
- Change middleware from '/' to '*' for useCors and middlewareAuth - Ensures middleware applies to all routes in groups, role_bindings, and roles - Fixes potential CORS and auth issues on nested routes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Migration: - Remove app, bundle, and channel scope permissions from org_member role - Update role description to reflect org-only access - Ensures org_member has no direct app access by default Seed: - Enable RBAC for test organization - Remove unused variables (v_org, v_migration_result) This enforces the principle that org_member should only have org-level access and requires explicit app-level role assignments. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove app-role-hint key from all language files - Key was not referenced in the codebase - Cleanup to reduce translation maintenance burden Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix import order in AppAccess.vue (SearchInput before RoleSelectionModal) - Fix static class in SearchInput.vue (separate static class attribute) - Fix inconsistent quote-props in RoleSelectionModal.vue emit types - Fix indentation errors in Members.vue template (79 errors total) All errors auto-fixed with eslint --fix Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Dalanir
changed the title
![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: 4
🤖 Fix all issues with AI agents
In `@src/components/dashboard/AppAccess.vue`:
- Around line 12-16: The import order violates the perfectionist/sort-imports
rule: move the SearchInput import to come before the RoleSelectionModal import
so imports are alphabetically ordered (e.g., ensure SearchInput is imported
before RoleSelectionModal among the top-level imports in AppAccess.vue where
IconWrench, DataTable, RoleSelect, RoleSelectionModal, and SearchInput are
declared).
In `@src/components/forms/SearchInput.vue`:
- Around line 29-35: The input element in SearchInput.vue mixes static classes
with a dynamic binding causing vue/prefer-separate-static-class; refactor the
element so static classes ("w-full pl-10 d-input") are placed in the plain class
attribute and the dynamic part remains in :class (e.g. bind only props.class or
an array containing props.class) while preserving v-model="localValue", type,
:placeholder="placeholder" and :disabled="disabled".
In `@src/components/modals/RoleSelectionModal.vue`:
- Around line 33-36: The emit keys in the defineEmits call are inconsistent with
quoting: update the remaining unquoted keys so they match the quoted style (wrap
"confirm" and "cancel" in quotes) in the defineEmits declaration (the emit
constant) to satisfy the quote-props lint rule; ensure the type signatures
remain unchanged (confirm: [role: string], cancel: []) and that only the keys'
quoting is modified.
In `@src/pages/settings/organization/Members.vue`:
- Around line 1675-1762: The Teleport app-access modal template has inconsistent
indentation causing vue/html-indent failures; reformat the block starting at the
Teleport element (contains SearchInput, the app list v-for, input checkbox,
getAppAccessLabel usage, and RoleSelect) to the project's standard indentation
(consistent 2-space nesting) so all child elements (divs, labels, inputs, spans,
and the RoleSelect component) are aligned properly; run the project's formatter
or eslint --fix after adjusting to ensure vue/html-indent passes.
Remove invalid default string argument from t() calls in Members.vue. The vue-i18n t() function expects (key, interpolationObject) not (key, defaultString, interpolationObject).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260203201308_rbac_org_member_no_app_access.sql`:
- Around line 91-165: The has_app_right_apikey function accepts an unchecked
userid and never verifies it against the api key owner returned by
find_apikey_by_value, allowing privilege escalation; fix by validating
api_key.user_id matches the incoming "userid" (or derive userid from
api_key.user_id) right after fetching api_key in has_app_right_apikey, and on
mismatch log a denial (e.g., USERID_MISMATCH) with pg_log and return false
before any permission checks proceed.
🧹 Nitpick comments (3)
src/pages/settings/organization/Members.vue (3)
48-78: Consider extracting shared RBAC types.The
Role,OrgApp, andRoleBindinginterfaces are duplicated betweenMembers.vueandAppAccess.vue. Consider extracting them to a shared types file (e.g.,~/types/rbac.types.ts) to maintain consistency and reduce duplication.
1267-1294: Consider separating concerns infetchMemberAppBindings.Lines 1284-1288 mix data fetching with UI state management (auto-populating
appAccessSelectedRole). This is already handled by the watcher onappAccessSelectedAppIds(lines 456-476), making this code potentially redundant or causing double-updates.🔧 Proposed simplification
appAccessBindings.value = (data || []).filter((binding: RoleBinding) => { return binding.scope_type === 'app' && binding.principal_type === 'user' && binding.principal_id === member.uid }) - - if (appAccessSelectedAppIds.value.length === 1 && !appAccessSelectedRole.value && !appAccessRoleTouched.value) { - const binding = appAccessBindings.value.find(b => b.app_id === appAccessSelectedAppIds.value[0]) - if (binding) - appAccessSelectedRole.value = binding.role_name - } }
1388-1424: Sequential API calls may be slow for bulk app-role assignments.When assigning roles to multiple apps, this loop makes sequential API calls which could be slow. Consider parallelizing with
Promise.allfor better UX, especially for organizations with many apps.⚡ Proposed parallel execution
const bindingMap = appAccessBindingByAppId.value let createdCount = 0 let updatedCount = 0 - for (const appId of appAccessSelectedAppIds.value) { + const operations = appAccessSelectedAppIds.value.map(async (appId) => { const existingBinding = bindingMap.get(appId) if (existingBinding && existingBinding.role_name === appAccessSelectedRole.value) { - continue + return { type: 'skipped' as const } } if (existingBinding) { const { error: updateError } = await supabase .from('role_bindings') .update({ role_id: roleData.id }) .eq('id', existingBinding.id) if (updateError) throw updateError - updatedCount += 1 + return { type: 'updated' as const } } else { const { error } = await supabase.functions.invoke('private/role_bindings', { method: 'POST', body: { principal_type: 'user', principal_id: appAccessMember.value.uid, role_name: appAccessSelectedRole.value, scope_type: 'app', org_id: currentOrganization.value.gid, app_id: appId, channel_id: null, }, }) if (error) throw error - createdCount += 1 + return { type: 'created' as const } } - } + }) + + const results = await Promise.all(operations) + updatedCount = results.filter(r => r.type === 'updated').length + createdCount = results.filter(r => r.type === 'created').length
After removing app/channel/bundle permissions from org_member role, the apps table INSERT policy was still checking for app-level admin permissions. This caused RLS violations when creating apps since the app_id doesn't exist yet during creation. Updated the policy to check org-level write permissions instead, which maps to org.update_settings in RBAC and aligns with the API endpoint's permission check. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260203201308_rbac_org_member_no_app_access.sql`:
- Around line 32-89: The RBAC branches in check_org_user_privileges() and
invite_user_to_org() grant super-admin capabilities by inspecting role_bindings
but skip 2FA checks; update both RBAC paths so that after confirming the
role_binding-based super-admin match you also verify 2FA is enabled for the
caller and org enforcement is on (use public.has_2fa_enabled(auth.uid()) and the
org's enforcing_2fa flag or helper) and only set/allow super-admin actions when
both the role_binding check and the 2FA-enforcement check pass; do the same
pattern in invite_user_to_org() where role_bindings are used.
Previous attempt modified an already-applied migration, which doesn't get re-run in CI. This creates a new migration with a current timestamp to properly fix the RLS policy for app creation. Changes: - Remove RLS policy fix from old migration (already applied) - Create new migration 20260203220918 with the RLS policy fix - Policy now checks org-level 'write' permissions instead of app-level 'admin' permissions for INSERT operations Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This reverts commit 321696b.
When creating a new app, the app_id doesn't exist yet, so we can't use get_identity_org_appid which expects an app_id parameter. Instead, use get_identity_org_allowed which only needs the org_id. This fixes the RLS policy to properly check org-level 'write' permissions when creating apps, which admins and super_admins have through RBAC. Also added debugging to expose-metadata test to help diagnose any remaining issues with app creation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/expose-metadata.test.ts`:
- Around line 113-117: The error logging block that calls createResponse.json()
can throw if the response is not JSON; wrap the JSON parse in a try/catch around
the createResponse.json() call (in the test that checks createResponse.status)
and on failure fall back to await createResponse.text() and include both the
parsing error and the raw text in the process output (e.g., console.error) so
tests don't mask the original HTTP failure; locate the block using the
createResponse variable in tests/expose-metadata.test.ts and update the error
handling accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/public/app/post.ts`:
- Around line 52-60: The post-insert read uses
supabase.from('apps').select().eq('app_id', body.app_id).single() which can
return a same app_id from another org; update the follow-up read to also filter
by owner_org by adding .eq('owner_org', body.owner_org) so the query scopes to
the inserted organization's record (keep the same supabase call and .single()
behavior).
- Around line 43-47: Replace the direct Supabase client usage
(supabaseApikey(...) and the .from('apps').insert(dataInsert) call) with the
Drizzle/PG client pattern: obtain a client via getPgClient() or
getDrizzleClient() from utils/pg.ts, then perform the insert using that client's
API (the Drizzle insert or PG query equivalent) and handle errors via the
returned result/error object; update any variable names and error handling
around the insert to match the new client (e.g., replace dbError handling) so
all DB operations in this handler use getPgClient()/getDrizzleClient() instead
of supabaseApikey.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/public/app/post.ts (1)
28-31:⚠️ Potential issue | 🟠 MajorMissing validation for
owner_orgallows permission check bypass.The condition
body.owner_org && !(await checkPermission(...))short-circuits whenowner_orgis falsy (empty string, null, undefined), skipping the permission check entirely. The subsequent insert will then either fail at the database level or create an app with invalid ownership.Add explicit validation for
owner_orgbefore the permission check:🛡️ Proposed fix
+ if (!body.owner_org) { + throw simpleError('missing_owner_org', 'Missing owner_org', { body }) + } + // Check if the user is allowed to create an app in this organization (auth context set by middlewareKey) - if (body.owner_org && !(await checkPermission(c, 'org.update_settings', { orgId: body.owner_org }))) { + if (!(await checkPermission(c, 'org.update_settings', { orgId: body.owner_org }))) { throw quickError(403, 'cannot_access_organization', 'You can\'t access this organization', { org_id: body.owner_org }) }
🤖 Fix all issues with AI agents
In `@tests/expose-metadata.test.ts`:
- Around line 113-127: The response body is being consumed twice (first with
createResponse.json() then createResponse.text()), so instead read the body as
text once into a variable (e.g., const bodyText = await createResponse.text()),
then try to JSON.parse(bodyText) inside a try/catch to extract structured error
info; use createResponse.status and either the parsed object or the raw bodyText
in the processLogger/console.error message for the App creation failure handling
around the createResponse checks.
Comment on lines
+113
to
+127
| if (createResponse.status !== 200) { | ||
| try { | ||
| const errorBody = await createResponse.json() | ||
| console.error('App creation failed:', createResponse.status, errorBody) | ||
| } | ||
| catch (error) { | ||
| const errorText = await createResponse.text() | ||
| console.error('App creation failed: non-JSON response', { | ||
| status: createResponse.status, | ||
| parseError: error, | ||
| text: errorText, | ||
| }) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Response body can only be consumed once; .text() will fail after .json() throws.
The Fetch API response body is a ReadableStream that can only be read once. When .json() throws due to a parse failure, the stream is already consumed, so the subsequent .text() call will return an empty string or throw.
Read the body as text first, then attempt to parse it as JSON:
🐛 Proposed fix
if (createResponse.status !== 200) {
+ const errorText = await createResponse.text()
try {
- const errorBody = await createResponse.json()
+ const errorBody = JSON.parse(errorText)
console.error('App creation failed:', createResponse.status, errorBody)
}
- catch (error) {
- const errorText = await createResponse.text()
+ catch {
console.error('App creation failed: non-JSON response', {
status: createResponse.status,
- parseError: error,
text: errorText,
})
}
}📝 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
| if (createResponse.status !== 200) { | |
| try { | |
| const errorBody = await createResponse.json() | |
| console.error('App creation failed:', createResponse.status, errorBody) | |
| } | |
| catch (error) { | |
| const errorText = await createResponse.text() | |
| console.error('App creation failed: non-JSON response', { | |
| status: createResponse.status, | |
| parseError: error, | |
| text: errorText, | |
| }) | |
| } | |
| } | |
| if (createResponse.status !== 200) { | |
| const errorText = await createResponse.text() | |
| try { | |
| const errorBody = JSON.parse(errorText) | |
| console.error('App creation failed:', createResponse.status, errorBody) | |
| } | |
| catch { | |
| console.error('App creation failed: non-JSON response', { | |
| status: createResponse.status, | |
| text: errorText, | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@tests/expose-metadata.test.ts` around lines 113 - 127, The response body is
being consumed twice (first with createResponse.json() then
createResponse.text()), so instead read the body as text once into a variable
(e.g., const bodyText = await createResponse.text()), then try to
JSON.parse(bodyText) inside a try/catch to extract structured error info; use
createResponse.status and either the parsed object or the raw bodyText in the
processLogger/console.error message for the App creation failure handling around
the createResponse checks.
|
5 tasks
This was referenced Feb 23, 2026
4 tasks
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
2 participants
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
This PR introduces reusable modal components to reduce code duplication and improves role display consistency across the application.
New Components Created
SearchInput (
src/components/forms/SearchInput.vue)RoleSelect (
src/components/forms/RoleSelect.vue)RoleSelectionModal (
src/components/modals/RoleSelectionModal.vue)Refactoring & Fixes
Benefits
Test Plan
Screenshots
Summary by CodeRabbit
New Features
Documentation
Chores
Tests