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>
- 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>
Remove invalid default string argument from t() calls in Members.vue. The vue-i18n t() function expects (key, interpolationObject) not (key, defaultString, interpolationObject).
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>
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>
|
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:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds channel-scoped permission overrides end-to-end: DB schema, RLS policies, and a direct RBAC check function; backend init tweak; client UI and components for managing per-principal per-channel permissions; locale additions (EN/FR); and client-side permission caches and gating. Changes
Sequence DiagramsequenceDiagram
participant UI as Channel Permissions UI
participant Comp as AccessTable Component
participant RPC as RPC Backend
participant DB as Database
UI->>Comp: Open permissions dialog for principal
Comp->>RPC: request channels + overrides (principal_id)
RPC->>DB: SELECT channels and channel_permission_overrides
DB-->>RPC: channels + overrides
RPC-->>Comp: channel list + overrides
Comp->>UI: render channels and current selections
UI->>Comp: Toggle permission for a channel
Comp->>Comp: compute nextOverride (allow/deny/default)
Comp->>RPC: updateChannelPermission(principal, channel, permission, is_allowed)
alt explicit override needed
RPC->>DB: UPSERT channel_permission_overrides
else explicit override removed (matches default)
RPC->>DB: DELETE channel_permission_overrides
end
DB-->>RPC: ack
RPC-->>Comp: result
Comp->>UI: update state, show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 11
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)
17-31:⚠️ Potential issue | 🔴 CriticalAuthorization bypass when
owner_orgis empty or falsy.The permission check on line 29 uses
body.owner_org && ..., which means ifowner_orgis an empty string (""),null, orundefined, the check is skipped entirely and the insert proceeds without authorization verification. Add explicit validation forowner_orgsimilar toapp_idandname.🔒 Proposed fix to add owner_org validation
if (!body.name) { throw simpleError('missing_name', 'Missing name', { body }) } + 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 `@messages/fr.json`:
- Around line 1499-1514: The locale strings under keys like
"channel-permission-read-required", "channel-permission-associate-required",
"channel-permissions-title", "channel-permissions-description",
"channel-permissions-empty", "channel-permission-read",
"channel-permission-history", "channel-permission-associate",
"error-loading-channel-permissions", and "error-saving-channel-permissions" use
"canal" and should be changed to the consistent term "chaîne" (e.g., "Vous
n'avez pas la permission de voir cette chaîne.", "Associer le bundle" ->
"Associer le bundle à cette chaîne" if needed for clarity) — update each value
to replace "canal" with "chaîne" and ensure grammatical agreement in surrounding
text so all channel-related labels in this file use "chaîne".
In `@src/components/forms/RoleSelect.vue`:
- Around line 1-40: The component uses Vue's computed API but never imports it,
causing a runtime ReferenceError; in RoleSelect.vue add computed to the script
imports (alongside any existing imports such as useI18n) so that the computed()
calls for localValue and placeholderText resolve correctly and the component can
emit 'update:modelValue' and compute placeholder text without crashing.
In `@src/components/modals/RoleSelectionModal.vue`:
- Around line 1-5: The file is missing Vue reactive imports used later (ref,
watch, computed), so add an import from 'vue' that brings in ref, watch, and
computed at the top of the <script setup> block so usages of ref(), watch(), and
computed() (lines referencing those symbols) resolve and avoid runtime errors;
update the import section near useI18n and RoleSelect to include these symbols.
- Around line 76-116: Replace the current :open="open" binding with a template
ref (e.g., dialogRef) and call dialogRef.showModal() when the modal should open
and dialogRef.close() when it should close; in practice, remove the :open
attribute on the <dialog>, add a ref="dialogRef", and in the component watch the
open prop (or call from the method that toggles it) to invoke
dialogRef.showModal() and save document.activeElement as openerEl; on close
(handleClose / `@close`) call dialogRef.close() if not already closed and restore
focus to the saved openerEl; ensure the backdrop click still calls handleClose
and that handleConfirm continues to use selectedRole and isLoading as before.
In `@src/components/tables/AppTable.vue`:
- Around line 165-174: The role lookup uses the wrong key: appRoleByAppId is
keyed by app_id but the code calls appRoleByAppId.value.get(item.id), causing a
permanent fallback to "none"; change the lookup to use item.app_id (i.e.,
appRoleByAppId.value.get(item.app_id)) and keep the surrounding logic
(appRoleLoaded check, normalizedRole processing, getRbacRoleI18nKey) unchanged
so role resolution works correctly.
In `@src/components/tables/ChannelTable.vue`:
- Around line 114-117: Resetting canPromoteChannel only leaves stale read
permissions visible during reload; update the getData function to also clear
canReadChannel before refetching so both permission caches are reset. Inside
async function getData(), alongside the existing canPromoteChannel.value = {},
add a reset for canReadChannel (e.g., canReadChannel.value = {}) so the UI won't
briefly show stale read access while permissions load.
In `@supabase/functions/_backend/private/roles.ts`:
- Around line 10-13: Replace the direct instantiation "new Hono()" with the
shared initializer "createHono()" and update imports accordingly: change the
declaration "export const app = new Hono<MiddlewareKeyVariables>()" to use
"createHono<MiddlewareKeyVariables>()" so this private endpoint uses the common
backend initialization; keep the existing middleware registrations (app.use('*',
useCors) and app.use('*', middlewareAuth)) unchanged.
In `@supabase/functions/_backend/public/app/post.ts`:
- Around line 61-64: The catch block in post.ts currently logs the DB error via
logPgError(c, 'create_app', error) but then exposes the raw error message to the
client in simpleError; remove the sensitive payload by throwing a generic client
error (e.g. throw simpleError('cannot_create_app', 'Cannot create app') or
include only a safe, non-DB-specific code), keep the detailed logPgError(c,
'create_app', error) call for internal diagnostics, and if you need client
correlation include a non-sensitive identifier returned/created when logging
rather than error.message.
In `@supabase/migrations/20260203201308_rbac_org_member_no_app_access.sql`:
- Around line 202-247: The org 2FA enforcement (org.enforcing_2fa +
public.has_2fa_enabled(auth.uid())) is only checked inside the super-admin RBAC
branch; move this check earlier so it runs for all callers after resolving
calling_user_id/org and before any role-specific logic (i.e., immediately after
selecting org and after
public.get_identity_org_allowed()/public.check_min_rights validation), and
remove the duplicate check inside the super-admin branch (references:
invite_user_to_org, calling_user_id, org.enforcing_2fa,
public.has_2fa_enabled(auth.uid()), public.get_identity_org_allowed,
public.check_min_rights). Ensure the check logs the same denial code/message
(e.g., 'SUPER_ADMIN_2FA_REQUIRED' or a more generic '2FA_REQUIRED') and returns
'NO_RIGHTS' when 2FA is not enabled.
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 36-81: The SELECT policy
(channel_permission_overrides_admin_select) and the ALL policy
(channel_permission_overrides_admin_write) lack correlation to
channel_permission_overrides.channel_id, allowing cross-app access; remove the
redundant SELECT policy and consolidate into a single ALL policy
(channel_permission_overrides_admin_write) that covers
SELECT/INSERT/UPDATE/DELETE, and modify both its USING and WITH CHECK clauses to
join channels ON channels.channel_id = channel_permission_overrides.channel_id
and include the same rbac_check_permission(...) call (using
rbac_perm_app_update_user_roles(), apps.owner_org, apps.app_id, NULL::bigint) so
permission is evaluated against the channel's app only.
In `@tests/expose-metadata.test.ts`:
- Around line 113-126: The error-logging branch for createResponse currently
calls createResponse.json() and falls back to createResponse.text(), which can
double-consume the response body; instead read the body once (await
createResponse.text()) into a variable, then attempt to JSON.parse that string
and log either the parsed object or the raw text along with
createResponse.status and the original parse error if JSON.parse fails; update
the block that references createResponse, errorBody, and errorText to use this
single-read approach so the body stream is not consumed twice.
🧹 Nitpick comments (7)
supabase/functions/_backend/public/app/post.ts (1)
42-69: Consider using Drizzle ORM instead of raw SQL for consistency with codebase patterns.The coding guidelines specify using Drizzle ORM query patterns with
schemafrompostgres_schema.tsfor all database operations. Theappstable is defined in the schema. While the parameterized query is safe from SQL injection, refactoring to Drizzle would provide better type safety and match the patterns used elsewhere in the backend:const drizzleClient = getDrizzleClient(pgClient) const data = await drizzleClient .insert(schema.apps) .values(dataInsert) .returning()src/pages/app/[package].channel.[channel].vue (1)
660-664: Use DaisyUId-btnfor this interactive control.This button is interactive and should use DaisyUI button classes to align with frontend guidelines.
♻️ Suggested tweak
- <button - v-if="channel" - class="p-1 transition-colors border border-gray-200 rounded-md dark:border-gray-700 hover:bg-gray-50 hover:border-gray-300 dark:hover:border-gray-600 dark:hover:bg-gray-800 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:bg-transparent disabled:hover:border-gray-200 dark:disabled:hover:border-gray-700" - :disabled="!canPromoteBundle" - `@click`="openSelectVersion()" - > + <button + v-if="channel" + class="d-btn d-btn-ghost d-btn-xs transition-colors border border-gray-200 rounded-md dark:border-gray-700 hover:bg-gray-50 hover:border-gray-300 dark:hover:border-gray-600 dark:hover:bg-gray-800 disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:bg-transparent disabled:hover:border-gray-200 dark:disabled:hover:border-gray-700" + :disabled="!canPromoteBundle" + `@click`="openSelectVersion()" + >As per coding guidelines: Use DaisyUI components (
d-btn,d-input,d-card) for interactive elements in Vue components.src/components/forms/SearchInput.vue (1)
4-15:classprop won’t be populated; use a dedicated prop for input classes.Vue treats
classas a special attribute and doesn’t pass it as a prop, soprops.classstays empty. Rename the prop (e.g.,inputClass) or forward$attrs.classto the input.♻️ Suggested refactor
interface Props { modelValue: string placeholder?: string disabled?: boolean - class?: string + inputClass?: string } const props = withDefaults(defineProps<Props>(), { placeholder: 'Search...', disabled: false, - class: '', + inputClass: '', }) @@ - class="w-full pl-10 d-input" :class="[props.class]" + class="w-full pl-10 d-input" :class="[props.inputClass]"Also applies to: 34-34
src/components/tables/ChannelTable.vue (1)
249-272: Consider batching permission checks to reduce RPC fan‑out.With N rows you currently execute 2N permission calls. If this table grows, a batch RPC or cache layer would reduce latency and load.
src/pages/settings/organization/Members.vue (1)
1219-1443: Consider batching role binding updates when multiple apps are selected.The current loop updates/creates bindings sequentially, which can add noticeable latency for larger selections. A batched
Promise.all(or a backend batch endpoint) would speed this up.src/components/tables/AccessTable.vue (2)
200-220: Potential race condition ifloadChannelPermissionsis called rapidly.The function sets
channelOverridesLoading.value = truebut doesn't guard against being invoked while already loading. IfopenChannelPermissionsis called multiple times in quick succession (e.g., rapid clicks), concurrent fetches could overwrite each other's results.🛡️ Proposed fix: Add early return guard
async function loadChannelPermissions() { if (!selectedPrincipal.value) return + if (channelOverridesLoading.value) + return channelOverridesLoading.value = true
223-231: Type assertionas anybypasses type safety.Using
'channel_permission_overrides' as any(also at Lines 309, 321) suggests the table isn't yet in~/types/supabase.types. This works but loses type checking on the query response shape.Consider regenerating Supabase types to include the new table, or add a local type assertion for the response to maintain some type safety.
There was a problem hiding this comment.
Actionable comments posted: 3
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/private/roles.ts (1)
14-14:⚠️ Potential issue | 🟡 MinorCode comments must be in English.
Lines 14, 26, and 70 have French comments (e.g., "Liste des rôles assignables", "Récupérer tous les rôles assignables", "Récupérer les rôles pour ce scope"). As per coding guidelines, all code comments must be in English regardless of chat language.
✏️ Proposed fix
-// GET /private/roles - Liste des rôles assignables +// GET /private/roles - List assignable roles- // Récupérer tous les rôles assignables + // Fetch all assignable roles- // Récupérer les rôles pour ce scope + // Fetch roles for this scope
🤖 Fix all issues with AI agents
In `@src/components/tables/AppTable.vue`:
- Around line 75-78: The early-return block that checks userId and orgId resets
appRoleLoaded to false and returns, which can permanently block downstream code;
remove the appRoleLoaded.value = false assignment from that early-return so the
existing finally block controls the loaded state, and ensure the watcher or
effect that calls the function depends on userId and orgId (e.g., the reactive
watcher that invokes the loader) so the load re-runs when those IDs become
available; references: userId, orgId, appRoleByAppId, appRoleLoaded and the
watcher that triggers the load.
- Around line 72-111: The app-scoped roles loaded by loadAppRoles (into
appRoleByAppId and appRoleLoaded) are never used; update the app-perm column's
displayFunction to first check appRoleByAppId.value.get(item.app_id) and return
that if present, otherwise fall back to
organizationStore.getOrgByAppId(item.app_id)?.role; also use appRoleLoaded to
avoid flashing “unknown” by rendering a loading/placeholder value while
appRoleLoaded.value is false and only showing the resolved role once true.
Ensure you reference appRoleByAppId, appRoleLoaded, loadAppRoles and
organizationStore.getOrgByAppId in the change so the new logic replaces the
current org-only lookup.
In `@supabase/functions/_backend/private/roles.ts`:
- Line 9: The app initialization incorrectly calls createHono with a type
parameter; remove the generic and call createHono with the required positional
parameters (function name and version) to match the pattern in groups.ts and
role_bindings.ts — e.g., replace the line using
createHono<MiddlewareKeyVariables>() with createHono('roles', 'v1') (or the same
functionName/version pair used in the other files) so createHono is invoked with
two positional args and no type args.
🧹 Nitpick comments (2)
src/components/tables/AppTable.vue (1)
108-111: DuplicatewatchEffecttrigger — both watchers fire on the same condition.This
watchEffectand the one at Line 67-70 share the same guard (props.apps.length > 0), so every reactive change toprops.appsfires bothloadMauNumbersandloadAppRolesin parallel. That's likely fine functionally, but consider consolidating them into a single watcher for clarity, or at least adding a distinct reactive dependency (e.g.organizationStore.currentOrganization?.gid) to avoid re-fetching roles when only app data refreshes but the org hasn't changed.src/components/tables/ChannelTable.vue (1)
250-273: Performance: N×2 sequential-per-row permission checks on every page load.
loadChannelPermissionsfires2 × rows.lengthconcurrentcheckPermissionscalls (up to 20 for a full page of 10). IfcheckPermissionshits the backend each time, this can noticeably delay table rendering sincegetDataawaits it before clearingisLoading.Consider batching: if the permissions service supports checking multiple channels in one call, that would reduce round-trips. Otherwise, this is acceptable for a page size of 10 but worth keeping in mind if the offset grows.
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/functions/_backend/private/roles.ts (1)
15-15:⚠️ Potential issue | 🟡 MinorCode comments must be in English.
Lines 15, 27, 54, and 71 contain French comments (e.g., "Liste des rôles assignables", "Récupérer tous les rôles assignables"). As per coding guidelines, all code comments must be in English regardless of the chat language.
♻️ Proposed fix
-// GET /private/roles - Liste des rôles assignables +// GET /private/roles - List assignable roles ... - // Récupérer tous les rôles assignables + // Fetch all assignable roles ... -// GET /private/roles/:scope_type - Liste des rôles par scope +// GET /private/roles/:scope_type - List roles by scope type ... - // Récupérer les rôles pour ce scope + // Fetch roles for this scopeAs per coding guidelines, "All code comments must be in English, regardless of the chat language."
Also applies to: 27-27, 54-54, 71-71
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260204181424_add_channel_permission_overrides.sql`:
- Around line 238-242: The lookup of the API key is wrong: instead of querying
public.apikeys WHERE key = p_apikey (which fails for hashed keys), call the
existing helper public.find_apikey_by_value(p_apikey) to resolve either plain or
hashed keys and assign its returned rbac_id into v_apikey_principal; ensure you
handle the case where the helper returns NULL (i.e., no principal found) the
same way the code expects in the surrounding IF/THEN logic.
|
2 participants
Summary (AI generated)
Motivation (AI generated)
We need channel-scoped permission overrides to allow fine-grained access (e.g., allow development only), and surface this in the UI.
Business Impact (AI generated)
Enables safer delegation for app teams, reducing accidental production changes while keeping development velocity.
Test Plan (AI generated)
channel.readis deniedchannel.promote_bundleGenerated with AI
Summary by CodeRabbit