feat: RBAC group management#1662
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds organization group RBAC: new pages and components for group listing and edit flows, translations (EN/FR), client changes delegating role_binding updates to a new PATCH function, backend role_binding PATCH handler, RLS/migration updates, settings tab wiring, and analytics/demo utils updates. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Client UI\n(Groups.vue / Groups.[id].vue / GroupsRbacManager)
participant Func as Supabase Function\n(private/role_bindings)
participant BE as Backend Handler\n(role_bindings.ts)
participant DB as Database\n(roles, role_bindings, groups)
User->>UI: Open groups list or edit group
UI->>DB: Fetch groups, roles, role_bindings
DB-->>UI: Return data
User->>UI: Change org/app role or member role
UI->>Func: PATCH /private/role_bindings/{id} { role_name }
Func->>BE: Invoke backend handler
BE->>DB: Resolve role by name and validate scope/assignability
DB-->>BE: Return role record
BE->>DB: Update role_bindings.role_id
DB-->>BE: Return updated binding
BE-->>Func: Return success / error
Func-->>UI: Success / error response
UI->>User: Show toast and refresh data
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: 9
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)supabase/migrations/20260204181424_add_channel_permission_overrides.sql (1)
36-81:⚠️ Potential issue | 🔴 CriticalScope channel_permission_overrides RLS to the row’s channel_id.
The EXISTS subqueries aren’t correlated tochannel_permission_overrides.channel_id, so any user withapp.update_user_roleson any app can read/write all overrides. Tie the join to the row to enforce per-channel scope.🐛 Suggested fix
CREATE POLICY channel_permission_overrides_admin_select ON public.channel_permission_overrides FOR SELECT TO authenticated USING ( EXISTS ( SELECT 1 FROM public.channels JOIN public.apps ON apps.app_id = channels.app_id - WHERE public.rbac_check_permission( + WHERE channels.id = channel_permission_overrides.channel_id + AND public.rbac_check_permission( public.rbac_perm_app_update_user_roles(), apps.owner_org, apps.app_id, NULL::bigint ) ) ); CREATE POLICY channel_permission_overrides_admin_write ON public.channel_permission_overrides FOR ALL TO authenticated USING ( EXISTS ( SELECT 1 FROM public.channels JOIN public.apps ON apps.app_id = channels.app_id - WHERE public.rbac_check_permission( + WHERE channels.id = channel_permission_overrides.channel_id + AND public.rbac_check_permission( public.rbac_perm_app_update_user_roles(), apps.owner_org, apps.app_id, NULL::bigint ) ) ) WITH CHECK ( EXISTS ( SELECT 1 FROM public.channels JOIN public.apps ON apps.app_id = channels.app_id - WHERE public.rbac_check_permission( + WHERE channels.id = channel_permission_overrides.channel_id + AND public.rbac_check_permission( public.rbac_perm_app_update_user_roles(), apps.owner_org, apps.app_id, NULL::bigint ) ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql` around lines 36 - 81, The RLS policies channel_permission_overrides_admin_select and channel_permission_overrides_admin_write use EXISTS subqueries that are not correlated to the current row, allowing any user with app.update_user_roles on any app to affect all rows; update each EXISTS WHERE clause in both the USING and WITH CHECK (for channel_permission_overrides_admin_write) to join/filter on the current row by adding a condition that ties channels.channel_id (or channels.id as used) to channel_permission_overrides.channel_id so the permission check is performed only for the row’s channel_id.
🧹 Nitpick comments (2)
supabase/functions/_backend/private/role_bindings.ts (1)
308-341: Duplicate23505error-code check can be consolidated.The unique-constraint violation (
error?.code === '23505') is checked at line 309 for logging and again at line 336 for the response. Merging these into a single conditional block improves readability.♻️ Suggested consolidation
catch (error: any) { if (error?.code === '23505') { cloudlog({ requestId: c.get('requestId'), message: 'role_binding_duplicate', orgId: org_id, principal_type, principal_id, scope_type, app_id, channel_id, }) + return c.json({ error: 'User already has a role in this family at this scope' }, 409) } - else { - cloudlogErr({ - requestId: c.get('requestId'), - message: 'role_binding_create_failed', - orgId: org_id, - principal_type, - principal_id, - scope_type, - app_id, - channel_id, - error, - }) - } - // Handle unique constraint errors (SSD) - if (error?.code === '23505') { - return c.json({ error: 'User already has a role in this family at this scope' }, 409) - } + cloudlogErr({ + requestId: c.get('requestId'), + message: 'role_binding_create_failed', + orgId: org_id, + principal_type, + principal_id, + scope_type, + app_id, + channel_id, + error, + }) return c.json({ error: 'Internal server error' }, 500) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 308 - 341, Consolidate the duplicate unique-constraint check for error?.code === '23505' in the catch block of the role binding creation: replace the two separate checks with a single conditional that first emits the duplicate-specific cloudlog (cloudlog with message 'role_binding_duplicate' and the same metadata) and then returns the 409 JSON response, otherwise fall through to the existing cloudlogErr and 500 response; locate the catch block around the create role binding logic (references: cloudlog, cloudlogErr, principal_type, principal_id, scope_type, app_id, channel_id) and reorder so the duplicate-path handles both logging and response in one branch.src/components/organization/GroupsRbacManager.vue (1)
16-296: Significant code duplication withGroups.vue.This component duplicates nearly all logic from
src/pages/settings/organization/Groups.vue: interfaces (Group,GroupRow,RoleBinding), fetch functions (fetchGroups,fetchRoles,fetchRoleBindings), helpers (getRoleDisplayName,getGroupOrgRoleName),deleteGroup, column definitions, and template structure.Consider either:
- Using
GroupsRbacManageras the shared component and havingGroups.vuesimply render it, or- Extracting the shared logic into a composable (e.g.,
useGroupsManager).This would eliminate ~200 lines of duplicated code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/organization/GroupsRbacManager.vue` around lines 16 - 296, This file duplicates almost all logic from Groups.vue; extract the shared state and methods into a composable (e.g., useGroupsManager) or make Groups.vue render this component. Move reactive refs and computed state (groups, roles, roleBindings, isLoading, isSubmitting, search, currentPage, columns, groupRows, filteredGroups, dynamicColumns), the fetch helpers (fetchGroups, fetchRoles, fetchRoleBindings), utility functions (getRoleDisplayName, getGroupOrgRoleName), and actions (refreshData, deleteGroup, reload, navigateToCreate) into the composable and return them for reuse, update GroupsRbacManager to import and use useGroupsManager, and update src/pages/settings/organization/Groups.vue to either render GroupsRbacManager or import the same composable to remove the duplicated interfaces (Group, GroupRow, RoleBinding) and logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Around line 161-164: The toast in the catch block of GroupsRbacManager.vue is
using the wrong i18n key; update the toast.error call (the catch handling after
the "Error loading groups:" console.error) to use the groups translation key
(e.g., t('error-fetching-groups')) instead of t('error-fetching-members') so the
user-facing error correctly references groups; ensure the change is made in the
catch block that logs "Error loading groups:" and keep the console.error call
as-is.
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 572-587: The dialog uses French fallback strings for the
add-members key; update the two calls to t('add-members', 'Ajouter des membres')
inside dialogStore.openDialog (title and the primary button text) to use an
English fallback like t('add-members', 'Add members') so missing translations
show consistent English; keep handler addSelectedMembersToGroup and other dialog
options unchanged.
- Around line 302-317: The filter callback in fetchApps is causing a TypeScript
mismatch because Supabase rows have id: string | null while OrgApp expects id:
string; change the filtering to use an inline type predicate so TS can narrow
the element type before assignment to apps (e.g., replace the current
.filter((app: OrgApp) => !!app.id) with a predicate like (app): app is OrgApp =>
app.id !== null && app.id !== undefined) so the result is correctly recognized
as OrgApp[]; ensure this update references the fetchApps function, the apps
variable, and the OrgApp type.
- Around line 513-567: syncAppBindings performs multiple separate supabase
mutations (delete/update/insert) and can leave the DB in a partial state on
failure; replace the client-side sequential mutations with a single atomic
server-side operation (e.g., implement a new RPC/REST endpoint that accepts
group id and the pending map and performs deletes/inserts/updates inside a DB
transaction) and call that from syncAppBindings instead of calling
supabase.from('role_bindings').delete()/update()/insert() directly; ensure the
new server function returns success/failure and then call fetchRoleBindings() on
success (or bubble the returned error) so all changes are committed atomically.
- Line 79: Replace the French fallback string "Ajouter des membres" used in the
i18n calls in this file with the English fallback "Add members" (update the
t(...) or $t(...) invocations that currently supply the French fallback), and
leave the existing computed declaration const groupId = computed(() =>
route.params.id as string) unchanged.
In `@src/pages/settings/organization/Groups.vue`:
- Around line 161-164: The catch block in Groups.vue logs errors and shows a
toast using the wrong translation key; update the toast.error call inside the
catch(error) handler (the block that currently calls console.error('Error
loading groups:', error) and toast.error(t('error-fetching-members'))) to use
the groups-specific translation key (e.g., t('error-fetching-groups')) so it
matches GroupsRbacManager.vue and correctly reflects the error context.
- Around line 268-306: The UI briefly shows the "groups unavailable" fallback
because canManage (a computedAsync) defaults to false while the permission check
is pending; update the template logic to guard rendering until the async
permission resolves by exposing the pending state from the computedAsync (or
derive an isPermissionLoading boolean) and change the v-if condition from
"canShow && canManage" to something like "canShow && (isPermissionLoading ? true
: canManage)" so the unavailable fallback is only shown when the check has
completed and returned false; touch the computedAsync that produces canManage
(and the component local state around it) and adjust any uses in the component
(e.g., the DataTable block and the fallback block) to rely on the new
isPermissionLoading flag.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 390-408: The PATCH handler is missing a scope_type consistency
check: after resolving the new role (role) and before updating the role_bindings
row, load the existing binding (from schema.role_bindings by bindingId) and
compare its scope_type to role.scope_type; if they differ return a 400/403 JSON
error. Reuse the same logic used by the POST path (call or replicate
validateScope behavior) to enforce scope consistency so an app-scoped binding
cannot be changed to an org-scoped role (reference symbols: role,
role.scope_type, schema.role_bindings, bindingId, validateScope).
In `@supabase/migrations/20260221150207_fix_role_bindings_rls_update_insert.sql`:
- Around line 12-41: The role_bindings_insert RLS policy must use
get_identity_org_appid() for app-scoped checks while preserving the single
auth.uid() subquery pattern; update all branches that reference app_id (the app
admin legacy path, the user_has_app_update_user_roles check, and any JOINs
against public.apps or channels) to derive org and app_id via
get_identity_org_appid() (still using the auth_user subquery for auth.uid()),
and then call public.check_min_rights(...) and
public.user_has_app_update_user_roles(...) with the org/app values returned by
get_identity_org_appid(); ensure scope_type comparisons
(public.rbac_scope_app()) remain but replace direct table app_id access with the
helper results so the policy consistently uses get_identity_org_appid() across
the role_bindings_insert logic (also apply the same refactor to the similar
blocks mentioned for lines 53-82).
---
Outside diff comments:
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 36-81: The RLS policies channel_permission_overrides_admin_select
and channel_permission_overrides_admin_write use EXISTS subqueries that are not
correlated to the current row, allowing any user with app.update_user_roles on
any app to affect all rows; update each EXISTS WHERE clause in both the USING
and WITH CHECK (for channel_permission_overrides_admin_write) to join/filter on
the current row by adding a condition that ties channels.channel_id (or
channels.id as used) to channel_permission_overrides.channel_id so the
permission check is performed only for the row’s channel_id.
---
Nitpick comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Around line 16-296: This file duplicates almost all logic from Groups.vue;
extract the shared state and methods into a composable (e.g., useGroupsManager)
or make Groups.vue render this component. Move reactive refs and computed state
(groups, roles, roleBindings, isLoading, isSubmitting, search, currentPage,
columns, groupRows, filteredGroups, dynamicColumns), the fetch helpers
(fetchGroups, fetchRoles, fetchRoleBindings), utility functions
(getRoleDisplayName, getGroupOrgRoleName), and actions (refreshData,
deleteGroup, reload, navigateToCreate) into the composable and return them for
reuse, update GroupsRbacManager to import and use useGroupsManager, and update
src/pages/settings/organization/Groups.vue to either render GroupsRbacManager or
import the same composable to remove the duplicated interfaces (Group, GroupRow,
RoleBinding) and logic.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 308-341: Consolidate the duplicate unique-constraint check for
error?.code === '23505' in the catch block of the role binding creation: replace
the two separate checks with a single conditional that first emits the
duplicate-specific cloudlog (cloudlog with message 'role_binding_duplicate' and
the same metadata) and then returns the 409 JSON response, otherwise fall
through to the existing cloudlogErr and 500 response; locate the catch block
around the create role binding logic (references: cloudlog, cloudlogErr,
principal_type, principal_id, scope_type, app_id, channel_id) and reorder so the
duplicate-path handles both logging and response in one branch.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
messages/en.jsonmessages/fr.jsonsrc/components.d.tssrc/components/dashboard/AppAccess.vuesrc/components/organization/GroupsRbacManager.vuesrc/components/tables/AccessTable.vuesrc/constants/organizationTabs.tssrc/layouts/settings.vuesrc/pages/settings/organization/Groups.[id].vuesrc/pages/settings/organization/Groups.vuesrc/pages/settings/organization/Members.vuesrc/route-map.d.tssupabase/functions/_backend/private/role_bindings.tssupabase/functions/_backend/utils/stats.tssupabase/migrations/20260204181424_add_channel_permission_overrides.sqlsupabase/migrations/20260221150207_fix_role_bindings_rls_update_insert.sql
- Use bracket notation for route.params['id'] to satisfy the params union type - Replace cast-based filter with a proper type predicate to narrow id: string | null to OrgApp Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/settings/organization/Groups.[id].vue (1)
572-587: French fallback strings in dialog text.Lines 574 and 580 still use
'Ajouter des membres'as the i18n fallback. Per the coding guideline that all code comments and strings must be in English, these should use English fallbacks.Proposed fix
- title: t('add-members', 'Ajouter des membres'), + title: t('add-members', 'Add members'),- text: t('add-members', 'Ajouter des membres'), + text: t('add-members', 'Add members'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 572 - 587, The dialog uses French fallback strings for i18n; update the dialogStore.openDialog call for id 'add-group-members' to use English fallback text by replacing occurrences of t('add-members', 'Ajouter des membres') with t('add-members', 'Add members') for both the title and the primary button text (the handler remains addSelectedMembersToGroup and other properties unchanged).
🧹 Nitpick comments (3)
src/pages/settings/organization/Groups.[id].vue (3)
228-231: Misleading error toast inloadAll.The catch block always shows
t('error-fetching-members')regardless of which fetch actually failed (could befetchGroup,fetchRoles,fetchApps, etc.). Consider using a more generic message like'error-loading-group-data'.Proposed fix
catch (error) { console.error('Error loading group data:', error) - toast.error(t('error-fetching-members')) + toast.error(t('error-loading-group-data', 'Error loading group data')) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 228 - 231, The catch in loadAll currently always shows t('error-fetching-members') which is misleading because any of fetchGroup, fetchRoles, fetchApps (or other awaits in loadAll) might have failed; update the catch to show a generic error key such as t('error-loading-group-data') (or derive a specific message based on which awaited call failed if you prefer) and keep the console.error(error) for debugging; locate the catch in the loadAll function and replace the toast.error call to use the new generic key (or conditional message selection) referencing fetchGroup, fetchRoles, and fetchApps as the implicated calls.
293-299: Comment is misleading — pending changes are overwritten, not preserved.The comment says "preserves any pending user changes if partial reload" but the code replaces
pendingAppBindingsentirely with DB state. The behavior is correct (this runs on initial load and after sync), but the comment should be updated to avoid confusion.Proposed fix
- // Sync pending app bindings from DB (preserves any pending user changes if partial reload) + // Reset pending app bindings to match current DB state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 293 - 299, The comment above the sync block is inaccurate: the code in the groupAppBindings.forEach builds a fresh map and assigns it to pendingAppBindings (overwriting any existing pending changes). Update the comment near groupAppBindings and pendingAppBindings to state that this sync replaces pendingAppBindings with the DB state (used on initial load/after sync), and remove the misleading "preserves any pending user changes" phrasing so it reflects the actual behavior.
718-743: DaisyUI radio classes missingd-prefix.The radio inputs use unprefixed classes (
radio radio-primary radio-sm) while the rest of the file consistently uses thed-prefix convention (d-checkbox,d-btn,d-input,d-select). As per coding guidelines, DaisyUI classes should use thed-prefix.Proposed fix
<input v-model="selectedOrgRole" type="radio" - class="radio radio-primary radio-sm" + class="d-radio d-radio-primary d-radio-sm" name="org-role" value="" :disabled="isSubmitting"<input v-model="selectedOrgRole" type="radio" - class="radio radio-primary radio-sm" + class="d-radio d-radio-primary d-radio-sm" name="org-role" :value="role.name" :disabled="isSubmitting"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 718 - 743, The radio input elements for the org role picker use unprefixed DaisyUI classes ("radio radio-primary radio-sm") which breaks the project's `d-` prefix convention; update the class attributes on both the empty-value radio and the v-for radios to use the prefixed variants (e.g., replace "radio", "radio-primary", "radio-sm" with "d-radio", "d-radio-primary", "d-radio-sm") where the inputs are defined (look for the inputs bound to v-model="selectedOrgRole" and the v-for over orgRoleOptions), keeping the rest of the attributes (name, :value, :disabled) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 385-424: The createGroup flow currently inserts the group via
supabase.from('groups').insert(...) in createGroup but if saveGroupOrgRole() or
syncAppBindings() throws the DB row remains without bindings; either implement a
server-side RPC that performs the insert + role/binding writes inside a single
DB transaction, or (simpler) ensure you still set group.value and navigate to
the new group page even when saveGroupOrgRole() or syncAppBindings() fail: catch
errors from saveGroupOrgRole() and syncAppBindings() individually, log/show a
warning toast, but do not rethrow so
router.replace(`/settings/organization/groups/${data.id}`) always runs after a
successful insert; reference createGroup, saveGroupOrgRole, syncAppBindings,
router.replace and the supabase insert/select block when making the change.
- Line 79: The lint failure is caused by bracket access route.params['id'] in
the computed groupId; replace it with dot notation and narrow the params type
inline so TypeScript knows `id` exists (for example cast or assert route.params
to a suitable type or use a type guard) and return the typed value inside the
computed; update the computed definition (groupId and its computed callback
referencing route.params) to perform the inline narrowing so ESLint dot-notation
passes and vue-tsc still type-checks.
- Around line 458-461: The catch block in the saveGroup flow is logging the
correct error but uses the wrong i18n key ('error-creating-group') for update
failures; update the toast.error call inside the saveGroup catch to use the
update-specific translation key (e.g., 'error-updating-group' or the project's
canonical update error key) so the toast reads "Error updating group" and still
passes the fallback string as before; locate this in the saveGroup function's
catch (the lines with console.error('Error saving group:', error) and
toast.error(...)) and replace only the i18n key.
---
Duplicate comments:
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 572-587: The dialog uses French fallback strings for i18n; update
the dialogStore.openDialog call for id 'add-group-members' to use English
fallback text by replacing occurrences of t('add-members', 'Ajouter des
membres') with t('add-members', 'Add members') for both the title and the
primary button text (the handler remains addSelectedMembersToGroup and other
properties unchanged).
---
Nitpick comments:
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 228-231: The catch in loadAll currently always shows
t('error-fetching-members') which is misleading because any of fetchGroup,
fetchRoles, fetchApps (or other awaits in loadAll) might have failed; update the
catch to show a generic error key such as t('error-loading-group-data') (or
derive a specific message based on which awaited call failed if you prefer) and
keep the console.error(error) for debugging; locate the catch in the loadAll
function and replace the toast.error call to use the new generic key (or
conditional message selection) referencing fetchGroup, fetchRoles, and fetchApps
as the implicated calls.
- Around line 293-299: The comment above the sync block is inaccurate: the code
in the groupAppBindings.forEach builds a fresh map and assigns it to
pendingAppBindings (overwriting any existing pending changes). Update the
comment near groupAppBindings and pendingAppBindings to state that this sync
replaces pendingAppBindings with the DB state (used on initial load/after sync),
and remove the misleading "preserves any pending user changes" phrasing so it
reflects the actual behavior.
- Around line 718-743: The radio input elements for the org role picker use
unprefixed DaisyUI classes ("radio radio-primary radio-sm") which breaks the
project's `d-` prefix convention; update the class attributes on both the
empty-value radio and the v-for radios to use the prefixed variants (e.g.,
replace "radio", "radio-primary", "radio-sm" with "d-radio", "d-radio-primary",
"d-radio-sm") where the inputs are defined (look for the inputs bound to
v-model="selectedOrgRole" and the v-for over orgRoleOptions), keeping the rest
of the attributes (name, :value, :disabled) unchanged.
Casting params to { id: string } satisfies both the TS type union
constraint and the eslint dot-notation rule.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace French fallback strings with English in Groups.[id].vue dialog - Fix wrong i18n key in loadAll catch (error-fetching-members → error-loading-group-data) - Fix wrong i18n key in saveGroup catch (error-creating-group → error-updating-group) - Fix wrong i18n key in Groups.vue and GroupsRbacManager.vue catch (error-fetching-members → error-fetching-groups) - Update misleading comment on pendingAppBindings sync block - Fix DaisyUI radio class prefix (radio → d-radio) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Consolidate duplicate 23505 error-code check into a single branch - Add scope_type consistency check in PATCH handler to prevent changing an app-scoped binding to an org-scoped role (or vice-versa) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…annel_id The EXISTS subqueries in both RLS policies were not correlated to the current row, allowing any user with app.update_user_roles on any app to read/write all channel_permission_overrides rows. Add channels.id = channel_permission_overrides.channel_id to scope each policy check to the specific row's channel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Groups.vue was duplicating all the groups-list logic from GroupsRbacManager.vue (interfaces, fetch functions, column definitions, search/pagination state, delete flow). Replace the duplicated implementation with a direct render of GroupsRbacManager, passing orgId and canManage as props. Groups.vue now only owns the permission check and the unavailable fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)supabase/migrations/20260204181424_add_channel_permission_overrides.sql (1)
36-84: 🛠️ Refactor suggestion | 🟠 MajorRedundant
FOR SELECTpolicy —FOR ALLalready covers SELECT.
channel_permission_overrides_admin_writeisFOR ALL, which applies to SELECT, INSERT, UPDATE, and DELETE. The dedicatedchannel_permission_overrides_admin_selectpolicy duplicates the SELECT check with an identical condition. Drop theFOR SELECTpolicy to comply with the one-policy-per-operation rule.Proposed fix
-CREATE POLICY channel_permission_overrides_admin_select ON public.channel_permission_overrides - FOR SELECT - TO authenticated - USING ( - EXISTS ( - SELECT 1 - FROM public.channels - JOIN public.apps ON apps.app_id = channels.app_id - WHERE channels.id = channel_permission_overrides.channel_id - AND public.rbac_check_permission( - public.rbac_perm_app_update_user_roles(), - apps.owner_org, - apps.app_id, - NULL::bigint - ) - ) - ); - CREATE POLICY channel_permission_overrides_admin_write ON public.channel_permission_overrides FOR ALL TO authenticatedAs per coding guidelines: "Create one RLS policy per table per operation; merge multiple conditions into a single policy using OR instead of creating duplicate policies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql` around lines 36 - 84, Remove the redundant SELECT policy and rely on the existing FOR ALL policy: delete the policy named channel_permission_overrides_admin_select and keep channel_permission_overrides_admin_write (FOR ALL) which already covers SELECT; if you intended different logic for SELECT, merge its condition into channel_permission_overrides_admin_write using OR so a single policy (channel_permission_overrides_admin_write) governs all operations on channel_permission_overrides.supabase/functions/_backend/private/role_bindings.ts (1)
122-140:⚠️ Potential issue | 🟠 MajorAdd missing validation for
apikeyprincipal type.When
principalTypeis'apikey', the function falls through to the finalreturn { ok: true, data: null }without verifying the API key exists or has access to the org. This allows creating role bindings for non-existent or unauthorized API key principals.Suggested fix
if (principalType === 'group') { const [group] = await drizzle .select() .from(schema.groups) .where( and( eq(schema.groups.id, principalId), eq(schema.groups.org_id, orgId), ), ) .limit(1) if (!group) { return { ok: false, status: 400, error: 'Group not found in this org' } } } + if (principalType === 'apikey') { + const [key] = await drizzle + .select() + .from(schema.apikeys) + .where(eq(schema.apikeys.id, principalId)) + .limit(1) + + if (!key) { + return { ok: false, status: 400, error: 'API key not found' } + } + + if (key.limited_to_orgs && key.limited_to_orgs.length > 0 && !key.limited_to_orgs.includes(orgId)) { + return { ok: false, status: 400, error: 'API key does not have access to this org' } + } + } + return { ok: true, data: null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 122 - 140, The code misses validation for when principalType === 'apikey' so role bindings can be created for nonexistent/unauthorized API keys; add a branch similar to the 'group' check: when principalType === 'apikey' use drizzle to select from the API key table (e.g., schema.api_keys or your API key model) where id equals principalId and org_id equals orgId (and any additional constraints like not_revoked if applicable), limit(1), and if no record is found return { ok: false, status: 400, error: 'API key not found in this org' } before allowing the final return; follow the same query/return pattern used for schema.groups to locate and implement this check.
♻️ Duplicate comments (4)
supabase/functions/_backend/private/role_bindings.ts (1)
344-401: Scope-type consistency check is now in place — looks good.The PATCH handler correctly validates that the new role's
scope_typematches the existing binding'sscope_type(line 399), preventing scope mismatches. Auth check, role lookup, assignability check, and structured logging are all properly implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 344 - 401, The review indicates the scope_type consistency check in the PATCH handler is correct; no code changes are required—keep the check in the app.patch('/:binding_id') flow that compares role.scope_type to binding.scope_type, and retain the existing auth check (checkPermission), role lookup (drizzle.select from schema.roles), and assignability guard (role.is_assignable) as implemented.src/pages/settings/organization/Groups.vue (1)
28-47:⚠️ Potential issue | 🟡 MinorBrief flash of "groups unavailable" while
canManageresolves.
computedAsyncdefaults tofalse, so while the permission check is in-flight,canShow && canManageevaluates tofalseand the "groups-unavailable" fallback renders momentarily. Consider using theevaluatingref fromcomputedAsyncto show a spinner instead.Suggested approach
-const canManage = computedAsync(async () => { +const canManage = computedAsync(async () => { if (!currentOrganization.value?.gid) return false return await checkPermissions('org.update_user_roles', { orgId: currentOrganization.value.gid }) -}, false) +}, false, { lazy: false }) + +const isPermissionLoading = ref(true) +// Alternative: use computedAsync's evaluating ref: +// const isPermissionLoading = ref(false) +// const canManage = computedAsync(..., false, isPermissionLoading)Then in template:
+ <div v-if="isPermissionLoading" class="flex items-center justify-center py-12"> + <span class="d-loading d-loading-spinner d-loading-lg" /> + </div> <GroupsRbacManager - v-if="canShow && canManage" + v-else-if="canShow && canManage"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups.vue` around lines 28 - 47, The brief flash happens because computedAsync's default false makes (canShow && canManage) false while permissions load; update the component where canManage is created (the computedAsync call) to expose its evaluating ref and in the template use that evaluating ref to render a spinner/loading placeholder instead of the "groups-unavailable" fallback; specifically, reference the evaluating ref returned by your computedAsync hook alongside canManage and change the v-if/v-else logic around GroupsRbacManager and the fallback so that when evaluating is true you show a spinner/loading state, when evaluating is false you use the existing canShow && canManage check, and only show the "groups-unavailable" message after evaluating is finished and permission is false.src/pages/settings/organization/Groups.[id].vue (2)
385-424:⚠️ Potential issue | 🟠 MajorPartial creation: group persists if
saveGroupOrgRoleorsyncAppBindingsfails.If the group
INSERTsucceeds (line 397-406) butsaveGroupOrgRole()orsyncAppBindings()throws, the group row is committed without its role bindings. The user sees an error toast and no navigation occurs, leaving a partially-configured group. Consider catching sub-step errors individually and still navigating to the edit page on partial success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 385 - 424, The group INSERT currently succeeds but if subsequent calls in createGroup (saveGroupOrgRole or syncAppBindings) throw, the new group remains partially configured and the user stays on the form; change createGroup to treat the INSERT as primary success: after setting group.value from the insert, call saveGroupOrgRole() and syncAppBindings() each inside their own try/catch blocks (log errors, accumulate a flag/message for partial failures), still navigate to the group's edit page via router.replace(`/settings/organization/groups/${data.id}`) even if one or both sub-steps fail, and show a success toast on creation plus a warning toast if any sub-step failed (with concise error context); ensure isSubmitting.value is still cleared in finally and do not rethrow sub-step errors so the user lands on the group page to finish configuration.
513-567:⚠️ Potential issue | 🟠 MajorNon-atomic
syncAppBindings: partial failures leave inconsistent state.Sequential deletes and inserts/updates without a transaction mean that if an operation fails mid-loop, earlier mutations are already committed. This was flagged in a prior review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/Groups`.[id].vue around lines 513 - 567, The syncAppBindings function performs multiple separate deletes/inserts/updates against the role_bindings table which can leave the DB in a partially-applied state on failure; instead implement this as a single atomic operation by moving the reconciliation logic into a Postgres function and calling it via supabase.rpc. Concretely: create a server-side stored procedure (e.g., sync_group_app_bindings) that accepts group id and the pending bindings payload, performs the delete/update/insert reconciliation inside a single transaction, and returns success/failure; then replace the per-row supabase.from('role_bindings').delete/update/insert calls in syncAppBindings with a single supabase.rpc('sync_group_app_bindings', { group_id: group.value!.id, pending: pendingAppBindings.value }) call and finally call fetchRoleBindings(); keep references to syncAppBindings, pendingAppBindings, groupAppBindings, fetchRoleBindings and supabase.from('role_bindings') so reviewers can locate the original code.
🧹 Nitpick comments (3)
supabase/migrations/20260204181424_add_channel_permission_overrides.sql (1)
241-244: Consistent use offind_apikey_by_value— consider caching the result.The change to use
find_apikey_by_valuehere aligns with line 150. However, when bothp_user_id IS NULLandp_apikey IS NOT NULL, this function is called twice for the same key (once foruser_id, once forrbac_id). You could fetch both columns in a single call earlier and reuse them.Sketch: resolve apikey fields once
DECLARE v_allowed boolean := false; v_use_rbac boolean; v_effective_org_id uuid := p_org_id; v_effective_user_id uuid := p_user_id; v_legacy_right public.user_min_right; v_apikey_principal uuid; + v_apikey_user_id uuid; v_override boolean; ... BEGIN ... -- Resolve API key fields once IF p_apikey IS NOT NULL THEN - IF v_effective_user_id IS NULL THEN - SELECT user_id INTO v_effective_user_id - FROM public.find_apikey_by_value(p_apikey) - LIMIT 1; - END IF; + SELECT user_id, rbac_id + INTO v_apikey_user_id, v_apikey_principal + FROM public.find_apikey_by_value(p_apikey) + LIMIT 1; + IF v_effective_user_id IS NULL THEN + v_effective_user_id := v_apikey_user_id; + END IF; END IF; ... -- Later, reuse v_apikey_principal directly instead of querying again - SELECT rbac_id INTO v_apikey_principal - FROM public.find_apikey_by_value(p_apikey) - LIMIT 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql` around lines 241 - 244, The code calls public.find_apikey_by_value(p_apikey) multiple times when p_apikey is present; instead, call it once and cache both returned fields into local vars (e.g., v_apikey_principal and v_apikey_user_id) before the permission checks so subsequent logic (the p_user_id NULL check and the rbac_id assignment) reuses the cached values; update the SELECTs using find_apikey_by_value to fetch both columns in a single INTO and remove the duplicate calls, ensuring you only call it when p_apikey IS NOT NULL.src/components/organization/GroupsRbacManager.vue (2)
135-137: ThedynamicColumns → columnswatcher can be simplified.Since
dynamicColumnsis already a computed, you could passdynamicColumnsdirectly to DataTable instead of syncing through a separate ref. The current pattern is only needed ifDataTable'sv-model:columnsmutates the array (e.g., for column reordering). If that's the case, this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/organization/GroupsRbacManager.vue` around lines 135 - 137, The watcher that copies the computed dynamicColumns into the ref columns is unnecessary if DataTable does not mutate columns; remove the watch and the columns ref and pass dynamicColumns directly to DataTable (e.g., bind the DataTable columns prop/v-model to dynamicColumns), otherwise keep the current pattern—if DataTable mutates the array (column reordering) retain the columns ref and watcher; look for the computed dynamicColumns, the ref columns, and the DataTable usage to apply the change.
238-291: Non-atomic group deletion: role_bindings removed before the group row.If the group
DELETEat line 275 fails after the role_bindingsDELETEat line 264 succeeds, the group persists without its bindings. This is a degraded but recoverable state (user can retry), and RLS limits blast radius, so it's acceptable for now. Consider a server-side RPC with a transaction if atomicity becomes important.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/organization/GroupsRbacManager.vue` around lines 238 - 291, deleteGroup currently issues two separate supabase deletes (role_bindings then groups), which is non-atomic; move the deletion logic into a single server-side transactional RPC and call it from deleteGroup: create an RPC (e.g., delete_group_with_bindings) that wraps deletion of role_bindings and the groups row in one transaction, return success/error, then replace the two supabase .from(...).delete() calls in deleteGroup with a single supabase.rpc('delete_group_with_bindings', { group_id: group.id }) call, handle any RPC error the same way (throw/notify), and keep refreshData/toast/isSubmitting logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 198-213: The watch on groupId should validate the incoming id
before calling loadAll/fetchGroup: in the watcher for groupId (the function that
checks id === 'new' and otherwise calls loadAll), add a UUID format check (e.g.,
an isUuid/isUUID helper or a strict regex) and treat non-UUID non-'new' values
as invalid — set group.value = null, reset
editName/editDescription/selectedOrgRole/pendingAppBindings, update
displayStore.NavTitle appropriately, and avoid calling loadAll/fetchGroup;
optionally surface a friendly toast or error message for an invalid route param.
Ensure the change targets the watcher logic around groupId, loadAll, and
fetchGroup so invalid route params never trigger the Supabase UUID query.
- Around line 69-80: This page is missing the RBAC and permission guards present
in Groups.vue; add an async computed permission check and RBAC flag check and
prevent loading the form when unauthorized. Create a computedAsync (or use the
existing computedAsync utility) that calls
checkPermissions('org.update_user_roles', { orgId: currentOrganization.value?.id
|| groupId.value }) and another computed that verifies
currentOrganization.value?.use_new_rbac; use those results to redirect (via
useRouter()) or render a fallback when either check is false (for both create
mode and detail mode), ensuring the form/components in Groups.[id].vue only
mount when permissionCheck.value and useNewRbac.value are true.
---
Outside diff comments:
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 122-140: The code misses validation for when principalType ===
'apikey' so role bindings can be created for nonexistent/unauthorized API keys;
add a branch similar to the 'group' check: when principalType === 'apikey' use
drizzle to select from the API key table (e.g., schema.api_keys or your API key
model) where id equals principalId and org_id equals orgId (and any additional
constraints like not_revoked if applicable), limit(1), and if no record is found
return { ok: false, status: 400, error: 'API key not found in this org' } before
allowing the final return; follow the same query/return pattern used for
schema.groups to locate and implement this check.
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 36-84: Remove the redundant SELECT policy and rely on the existing
FOR ALL policy: delete the policy named
channel_permission_overrides_admin_select and keep
channel_permission_overrides_admin_write (FOR ALL) which already covers SELECT;
if you intended different logic for SELECT, merge its condition into
channel_permission_overrides_admin_write using OR so a single policy
(channel_permission_overrides_admin_write) governs all operations on
channel_permission_overrides.
---
Duplicate comments:
In `@src/pages/settings/organization/Groups`.[id].vue:
- Around line 385-424: The group INSERT currently succeeds but if subsequent
calls in createGroup (saveGroupOrgRole or syncAppBindings) throw, the new group
remains partially configured and the user stays on the form; change createGroup
to treat the INSERT as primary success: after setting group.value from the
insert, call saveGroupOrgRole() and syncAppBindings() each inside their own
try/catch blocks (log errors, accumulate a flag/message for partial failures),
still navigate to the group's edit page via
router.replace(`/settings/organization/groups/${data.id}`) even if one or both
sub-steps fail, and show a success toast on creation plus a warning toast if any
sub-step failed (with concise error context); ensure isSubmitting.value is still
cleared in finally and do not rethrow sub-step errors so the user lands on the
group page to finish configuration.
- Around line 513-567: The syncAppBindings function performs multiple separate
deletes/inserts/updates against the role_bindings table which can leave the DB
in a partially-applied state on failure; instead implement this as a single
atomic operation by moving the reconciliation logic into a Postgres function and
calling it via supabase.rpc. Concretely: create a server-side stored procedure
(e.g., sync_group_app_bindings) that accepts group id and the pending bindings
payload, performs the delete/update/insert reconciliation inside a single
transaction, and returns success/failure; then replace the per-row
supabase.from('role_bindings').delete/update/insert calls in syncAppBindings
with a single supabase.rpc('sync_group_app_bindings', { group_id:
group.value!.id, pending: pendingAppBindings.value }) call and finally call
fetchRoleBindings(); keep references to syncAppBindings, pendingAppBindings,
groupAppBindings, fetchRoleBindings and supabase.from('role_bindings') so
reviewers can locate the original code.
In `@src/pages/settings/organization/Groups.vue`:
- Around line 28-47: The brief flash happens because computedAsync's default
false makes (canShow && canManage) false while permissions load; update the
component where canManage is created (the computedAsync call) to expose its
evaluating ref and in the template use that evaluating ref to render a
spinner/loading placeholder instead of the "groups-unavailable" fallback;
specifically, reference the evaluating ref returned by your computedAsync hook
alongside canManage and change the v-if/v-else logic around GroupsRbacManager
and the fallback so that when evaluating is true you show a spinner/loading
state, when evaluating is false you use the existing canShow && canManage check,
and only show the "groups-unavailable" message after evaluating is finished and
permission is false.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 344-401: The review indicates the scope_type consistency check in
the PATCH handler is correct; no code changes are required—keep the check in the
app.patch('/:binding_id') flow that compares role.scope_type to
binding.scope_type, and retain the existing auth check (checkPermission), role
lookup (drizzle.select from schema.roles), and assignability guard
(role.is_assignable) as implemented.
---
Nitpick comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Around line 135-137: The watcher that copies the computed dynamicColumns into
the ref columns is unnecessary if DataTable does not mutate columns; remove the
watch and the columns ref and pass dynamicColumns directly to DataTable (e.g.,
bind the DataTable columns prop/v-model to dynamicColumns), otherwise keep the
current pattern—if DataTable mutates the array (column reordering) retain the
columns ref and watcher; look for the computed dynamicColumns, the ref columns,
and the DataTable usage to apply the change.
- Around line 238-291: deleteGroup currently issues two separate supabase
deletes (role_bindings then groups), which is non-atomic; move the deletion
logic into a single server-side transactional RPC and call it from deleteGroup:
create an RPC (e.g., delete_group_with_bindings) that wraps deletion of
role_bindings and the groups row in one transaction, return success/error, then
replace the two supabase .from(...).delete() calls in deleteGroup with a single
supabase.rpc('delete_group_with_bindings', { group_id: group.id }) call, handle
any RPC error the same way (throw/notify), and keep
refreshData/toast/isSubmitting logic unchanged.
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 241-244: The code calls public.find_apikey_by_value(p_apikey)
multiple times when p_apikey is present; instead, call it once and cache both
returned fields into local vars (e.g., v_apikey_principal and v_apikey_user_id)
before the permission checks so subsequent logic (the p_user_id NULL check and
the rbac_id assignment) reuses the cached values; update the SELECTs using
find_apikey_by_value to fetch both columns in a single INTO and remove the
duplicate calls, ensuring you only call it when p_apikey IS NOT NULL.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/organization/GroupsRbacManager.vuesrc/pages/settings/organization/Groups.[id].vuesrc/pages/settings/organization/Groups.vuesupabase/functions/_backend/private/role_bindings.tssupabase/migrations/20260204181424_add_channel_permission_overrides.sql
…_direct Call find_apikey_by_value once when p_apikey is present and store both user_id and rbac_id into local vars. Removes the duplicate call that previously fetched them in separate SELECT statements at different points in the function body. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/migrations/20260204181424_add_channel_permission_overrides.sql (1)
36-84: 🛠️ Refactor suggestion | 🟠 MajorRedundant SELECT policy violates the "one RLS policy per table per operation" guideline.
channel_permission_overrides_admin_select(FOR SELECT) andchannel_permission_overrides_admin_write(FOR ALL) both cover SELECT. When a FOR ALL policy and more specific policies exist, both are applied — PostgreSQL ORs them for SELECT, makingadmin_selectentirely redundant since both policies carry identical USING conditions. Dropadmin_select;admin_write(FOR ALL) already satisfies reads.🔧 Proposed fix
-CREATE POLICY channel_permission_overrides_admin_select ON public.channel_permission_overrides - FOR SELECT - TO authenticated - USING ( - EXISTS ( - SELECT 1 - FROM public.channels - JOIN public.apps ON apps.app_id = channels.app_id - WHERE channels.id = channel_permission_overrides.channel_id - AND public.rbac_check_permission( - public.rbac_perm_app_update_user_roles(), - apps.owner_org, - apps.app_id, - NULL::bigint - ) - ) - ); - CREATE POLICY channel_permission_overrides_admin_write ON public.channel_permission_overrides FOR ALL ...Based on learnings: "Create one RLS policy per table per operation; merge multiple conditions into a single policy using OR instead of creating duplicate policies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql` around lines 36 - 84, The SELECT policy channel_permission_overrides_admin_select is redundant because channel_permission_overrides_admin_write is defined FOR ALL and already covers SELECT; remove the channel_permission_overrides_admin_select policy and keep channel_permission_overrides_admin_write (which includes the identical USING and WITH CHECK conditions) so there is only one RLS policy per operation on the channel_permission_overrides table; ensure no other distinct SELECT-only logic is lost when deleting channel_permission_overrides_admin_select.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 36-84: The SELECT policy channel_permission_overrides_admin_select
is redundant because channel_permission_overrides_admin_write is defined FOR ALL
and already covers SELECT; remove the channel_permission_overrides_admin_select
policy and keep channel_permission_overrides_admin_write (which includes the
identical USING and WITH CHECK conditions) so there is only one RLS policy per
operation on the channel_permission_overrides table; ensure no other distinct
SELECT-only logic is lost when deleting
channel_permission_overrides_admin_select.
Add delete_group_with_bindings() SECURITY DEFINER function to the existing channel_permission_overrides migration. It deletes role_bindings and the group in a single server-side transaction, replacing the two separate client-side delete calls in GroupsRbacManager.deleteGroup. Permission check uses rbac_check_permission_direct with auth.uid(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Track the computedAsync evaluating state via isPermissionLoading ref. Show GroupsRbacManager while the check is pending (canShow && (isPermissionLoading || canManage)) and only render the unavailable fallback once the async check has settled and returned false. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…icies
Replace direct auth_user.uid references in the app-scoped branches of
role_bindings_insert and role_bindings_update with
get_identity_org_appid('{all}', apps.owner_org, apps.app_id) so that
API key holders are correctly resolved, matching the pattern used across
other app-scoped RLS policies in the codebase.
Also consolidates the two separate app-scoped conditions (legacy admin
EXISTS + user_has_app_update_user_roles) into a single apps JOIN that
evaluates both checks against the resolved identity.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Catch saveGroupOrgRole() and syncAppBindings() individually in createGroup so a failure in either does not prevent router.replace() from running. Each shows a warning toast so the user knows to retry the assignment, while the group row that was successfully inserted remains accessible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add the same canShow (use_new_rbac flag) and canManage (org.update_user_roles permission check via computedAsync) guards that Groups.vue already has. The template shows a loading spinner while the permission check is pending, the unavailable fallback when either check is false, and the form only when both are satisfied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add a UUID regex check in the groupId watcher. Non-UUID non-'new' values now reset local state and show an error toast instead of triggering fetchGroup() with an invalid UUID, which would cause a Postgres error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/migrations/20260204181424_add_channel_permission_overrides.sql (1)
36-52: 🛠️ Refactor suggestion | 🟠 MajorRedundant
admin_selectpolicy violates the one-policy-per-operation guideline.
channel_permission_overrides_admin_writeis declaredFOR ALL, which already coversSELECTwith itsUSINGclause. If there are multiple permissive policies, a row is accessible if any permissive policy evaluates to true. Having an additionalFOR SELECTpolicy with an identicalUSINGexpression makesadmin_selectcompletely redundant and is a direct violation of the project guideline.🛠️ Proposed fix: remove the redundant policy
-CREATE POLICY channel_permission_overrides_admin_select ON public.channel_permission_overrides - FOR SELECT - TO authenticated - USING ( - EXISTS ( - SELECT 1 - FROM public.channels - JOIN public.apps ON apps.app_id = channels.app_id - WHERE channels.id = channel_permission_overrides.channel_id - AND public.rbac_check_permission( - public.rbac_perm_app_update_user_roles(), - apps.owner_org, - apps.app_id, - NULL::bigint - ) - ) - ); - CREATE POLICY channel_permission_overrides_admin_write ON public.channel_permission_overrides FOR ALL ...As per coding guidelines: "Create one RLS policy per table per operation; merge multiple conditions into a single policy using OR instead of creating duplicate policies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql` around lines 36 - 52, Remove the redundant FOR SELECT RLS policy channel_permission_overrides_admin_select on table channel_permission_overrides because channel_permission_overrides_admin_write (FOR ALL) already covers SELECT with the same USING expression; delete the channel_permission_overrides_admin_select policy block and ensure channel_permission_overrides_admin_write retains the USING clause that calls public.rbac_check_permission(public.rbac_perm_app_update_user_roles(), apps.owner_org, apps.app_id, NULL::bigint) so a single policy governs admin access for all operations (merge conditions with OR only if you add other distinct checks).
🧹 Nitpick comments (2)
src/components/organization/GroupsRbacManager.vue (2)
197-215:role_idis selected but discarded in the mapped object.The query on line 200 fetches
role_id, but the mapping on lines 207–214 never includes it in theRoleBindingshape. Remove it from the select string to avoid fetching unnecessary data.🛠️ Proposed fix
- .select('id, principal_type, principal_id, scope_type, app_id, role_id, roles(name)') + .select('id, principal_type, principal_id, scope_type, app_id, roles(name)')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/organization/GroupsRbacManager.vue` around lines 197 - 215, In fetchRoleBindings(), the Supabase select includes role_id but the mapped RoleBinding object (roleBindings.value) never uses it; remove role_id from the select string in the supabase.from('role_bindings').select(...) call (or alternatively add role_id to the mapped object if it is actually needed), ensuring the select matches the fields you populate (id, principal_type, principal_id, scope_type, app_id and roles(name)).
262-263:isSubmittingis never used in the template — delete action stays clickable during an inflight RPC.
isSubmittingis toggled around thesupabase.rpccall but is not passed to theDataTableor the action buttons, so the user can trigger another confirmation while a deletion is in-flight. Consider passing it as a loading guard to the action or the DataTable's:is-loadingprop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/organization/GroupsRbacManager.vue` around lines 262 - 263, The isSubmitting reactive flag around the supabase.rpc call is never wired to the UI, so users can re-trigger delete while the RPC is inflight; wire this flag into the template and ensure it is always reset: keep the existing isSubmitting.value = true at the start of the delete handler (the function surrounding the supabase.rpc call), set isSubmitting.value = false in a finally block after the try/catch, and bind it to the UI by passing it to the DataTable (e.g., DataTable :is-loading="isSubmitting") or disable the relevant action button(s) (e.g., :disabled="isSubmitting") so the delete action is blocked while supabase.rpc is running.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Line 264: TypeScript errors occur because the new SQL function
delete_group_with_bindings isn't present in the generated Supabase types; run
the project's Supabase type-generation command (e.g., supabase gen types
typescript) after applying the migration so the RPC signature for
supabase.rpc('delete_group_with_bindings', ...) is included, then commit the
updated generated types file to the repo to resolve the TS2345 compile error.
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 303-338: The delete_group_with_bindings function currently deletes
role_bindings and the groups row but forgets to remove
channel_permission_overrides tied to the group; add a DELETE FROM
public.channel_permission_overrides WHERE principal_type =
public.rbac_principal_group() AND principal_id = group_id (execute it before
deleting the groups row, alongside the existing DELETE from
public.role_bindings) so all polymorphic overrides for the group are cleaned up
when delete_group_with_bindings runs.
---
Outside diff comments:
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 36-52: Remove the redundant FOR SELECT RLS policy
channel_permission_overrides_admin_select on table channel_permission_overrides
because channel_permission_overrides_admin_write (FOR ALL) already covers SELECT
with the same USING expression; delete the
channel_permission_overrides_admin_select policy block and ensure
channel_permission_overrides_admin_write retains the USING clause that calls
public.rbac_check_permission(public.rbac_perm_app_update_user_roles(),
apps.owner_org, apps.app_id, NULL::bigint) so a single policy governs admin
access for all operations (merge conditions with OR only if you add other
distinct checks).
---
Nitpick comments:
In `@src/components/organization/GroupsRbacManager.vue`:
- Around line 197-215: In fetchRoleBindings(), the Supabase select includes
role_id but the mapped RoleBinding object (roleBindings.value) never uses it;
remove role_id from the select string in the
supabase.from('role_bindings').select(...) call (or alternatively add role_id to
the mapped object if it is actually needed), ensuring the select matches the
fields you populate (id, principal_type, principal_id, scope_type, app_id and
roles(name)).
- Around line 262-263: The isSubmitting reactive flag around the supabase.rpc
call is never wired to the UI, so users can re-trigger delete while the RPC is
inflight; wire this flag into the template and ensure it is always reset: keep
the existing isSubmitting.value = true at the start of the delete handler (the
function surrounding the supabase.rpc call), set isSubmitting.value = false in a
finally block after the try/catch, and bind it to the UI by passing it to the
DataTable (e.g., DataTable :is-loading="isSubmitting") or disable the relevant
action button(s) (e.g., :disabled="isSubmitting") so the delete action is
blocked while supabase.rpc is running.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/organization/GroupsRbacManager.vuesrc/pages/settings/organization/Groups.vuesupabase/migrations/20260204181424_add_channel_permission_overrides.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/settings/organization/Groups.vue
Add simple-git-hooks dependency and configure pre-commit hook to run 'bun typecheck' Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add DELETE for channel_permission_overrides in delete_group_with_bindings function Update types to include delete_group_with_bindings function signature Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
This was referenced Feb 28, 2026
2 tasks
This was referenced Apr 24, 2026
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 completes the RBAC (Role-Based Access Control) system with three major additions:
AccessTable) allowing granular access control per channel, visible directly from the channel page and app overviewSearchInput,RoleSelect,RoleSelectionModal) and a dedicatedAppAccesspanel to manage who can access each app and with which roleDB / Backend
20260204181424: addchannel_permission_overridestable with RLS20260203201308: remove app/channel/bundle permissions fromorg_memberrole — org members now need explicit app-level bindings20260221150207: fixrole_bindingsRLS INSERT/UPDATE policies — users withapp.update_user_rolespermission could not write bindings via the Supabase clientrole_bindings.ts— new private endpoint to manage role bindingsappsINSERT RLS to useget_identity_org_allowed(app doesn't exist yet at creation time)Frontend
Groups.vue(list) andGroups.[id].vue(detail with member and role binding management)GroupsRbacManager.vueSearchInput,RoleSelect,RoleSelectionModalAccessTablecomponent for channel/app access overridesEnregistrement.de.l.ecran.2026-02-21.a.16.40.26.mov
Test plan
org_memberwithout explicit binding, verify access to apps is deniedapp.update_user_rolespermission (non-admin), verify role binding insert/update worksSummary by CodeRabbit
New Features
Internationalization
Chores