feat: RBAC system v1 #1429
feat: RBAC system v1 #1429
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a full RBAC system: DB schema + migrations, SQL functions, backend RBAC utilities/middleware and private REST APIs, many public handlers switched to RBAC checks, frontend permission service/components/routes, i18n updates, tests, and a comprehensive RBAC design doc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Frontend as "Frontend (checkPermissions)"
participant API as "Backend API"
participant RBAC as "RBAC utils (rbac.ts)"
participant DB as "Database / RPC"
Client->>Frontend: user action (e.g. promote bundle)
Frontend->>Frontend: call checkPermissions('channel.promote_bundle', {appId})
Frontend->>API: HTTP request to protected endpoint
API->>RBAC: requirePermission / checkPermission(c, permission, scope)
RBAC->>DB: rpc('rbac_check_permission', p_permission_key, p_org_id, p_app_id, p_channel_id)
DB-->>RBAC: permission result
RBAC-->>API: boolean
API-->>Frontend: 200 / 403
Frontend-->>Client: render success / show no-permission
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
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: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (22)
supabase/functions/_backend/public/channel/delete.ts (1)
50-55: Delete operation result is not checked—potential silent failure.The delete query result is not captured or validated. If the deletion fails due to RLS policies, foreign key constraints, or database errors, the function will still return a success response, misleading the caller.
🔧 Proposed fix to check delete result
- await supabaseApikey(c, apikey.key) + const { error: deleteError } = await supabaseApikey(c, apikey.key) .from('channels') .delete() .eq('app_id', body.app_id) .eq('name', body.channel) + if (deleteError) { + throw simpleError('cannot_delete_channel', 'Failed to delete channel', { supabaseError: deleteError }) + } return c.json(BRES)planetscale/replicate_to_planetscale.sh (1)
80-88: Stop logging DB credentials (password is printed directly, and also via echoed SQL).
Line 81 printsSOURCE_PASSWORD, and the subscription SQL echoed at Line 185 includes the password; this is a secrets leak risk.Proposed fix (mask/remove sensitive logging)
echo "SOURCE_USER: $SOURCE_USER" -echo "SOURCE_PASSWORD: $SOURCE_PASSWORD" +echo "SOURCE_PASSWORD: [redacted]" echo "HOST: $SOURCE_HOST" echo "PORT: $SOURCE_PORT" echo "DB: $SOURCE_DB" # exit() # Restore file DUMP_FILE='data_replicate.dump'echo "Subscription creation SQL:" -echo "$SQL_QUERY_SUB" +echo "[redacted: subscription SQL contains credentials]" psql-17 "$TARGET_DB_URL" -v ON_ERROR_STOP=1 <<SQL $SQL_QUERY_SUB SQLsupabase/functions/_backend/utils/pg.ts (1)
187-204: Don’t log rawdbUrl(it can contain DB credentials).This is a high-risk secret leak in logs; redact user/password at minimum.
Proposed fix (redact credentials)
export function getPgClient(c: Context, readOnly = false) { const dbUrl = getDatabaseURL(c, readOnly) const requestId = c.get('requestId') const appName = c.res.headers.get('X-Worker-Source') ?? 'unknown source' const dbName = c.res.headers.get('X-Database-Source') ?? 'unknown source' - cloudlog({ requestId, message: 'SUPABASE_DB_URL', dbUrl, dbName, appName }) + let safeDbUrl = dbUrl + try { + const u = new URL(dbUrl) + if (u.username) + u.username = 'REDACTED' + if (u.password) + u.password = 'REDACTED' + safeDbUrl = u.toString() + } + catch { + // ignore parse errors; keep minimal info + safeDbUrl = 'UNPARSEABLE_DB_URL' + } + cloudlog({ requestId, message: 'SUPABASE_DB_URL', dbUrl: safeDbUrl, dbName, appName }) const isPooler = dbName.startsWith('sb_pooler') const options = { connectionString: dbUrl,messages/en.json (1)
153-155: Fix duplicate translation keys in messages/en.json (JSON last-write-wins behavior).This file contains duplicate keys (
no-organization-selected,date,create) that will silently override earlier definitions and can break i18n tooling or export flows. Remove the duplicate entries from the later block:Minimal fix
- "no-organization-selected": "No organization selected", "assigned": "Assigned", "removed": "Removed", "event": "Event", "by": "By", - "date": "Date", "bundle-deployed": "Bundle Deployed",Also note:
createappears twice with different values ("create" vs "Create" at lines 498 and 1320), and there is a case-sensitive collision risk withaccessvsAccess(lines 1482 and 1516).supabase/functions/_backend/public/build/start.ts (1)
43-75: Handle hashed API keys:apikey.key!can be null → runtime crash.
checkPermissionexplicitly supports hashed keys by falling back toc.get('capgkey'), but this function still doesconst apikeyKey = apikey.key!and uses it for DB updates. With hashed keys, that can benulland will blow up / skip marking builds failed.Proposed diff
export async function startBuild( c: Context, jobId: string, appId: string, apikey: Database['public']['Tables']['apikeys']['Row'], ): Promise<Response> { let alreadyMarkedAsFailed = false - const apikeyKey = apikey.key! + // `apikey.key` can be null for hashed keys; use the raw key from request context. + const apikeyKey = apikey.key ?? c.get('capgkey') + if (!apikeyKey) { + cloudlogErr({ + requestId: c.get('requestId'), + message: 'Missing API key in context (cannot update build_requests)', + job_id: jobId, + app_id: appId, + user_id: apikey.user_id, + }) + throw simpleError('not_authorized', 'Missing API key') + } try { @@ if (!(await checkPermission(c, 'app.build_native', { appId }))) { const errorMsg = 'You do not have permission to start builds for this app' @@ await markBuildAsFailed(c, jobId, errorMsg, apikeyKey)supabase/functions/_backend/public/build/cancel.ts (1)
23-67: Handle hashed API keys when updatingbuild_requestsafter cancel.The RBAC check supports hashed keys, but the DB update path still uses
supabaseApikey(c, apikey.key)which may be null for hashed keys (same issue asstartBuild).Proposed diff
// Update build_requests status to cancelled // Use authenticated client for data queries - RLS will enforce access - const supabase = supabaseApikey(c, apikey.key) + const apikeyKey = apikey.key ?? c.get('capgkey') + if (!apikeyKey) { + cloudlogErr({ + requestId: c.get('requestId'), + message: 'Missing API key in context (cannot update build_requests)', + job_id: jobId, + app_id: appId, + user_id: apikey.user_id, + }) + throw simpleError('not_authorized', 'Missing API key') + } + const supabase = supabaseApikey(c, apikeyKey)supabase/functions/_backend/private/download_link.ts (1)
1-21: UsecreateHonoto initialize the Hono app, but the proposed fix is incomplete.The guideline requires using
createHonofromutils/hono.tsfor Hono initialization. However,createHono(functionName: string, version: string, sentryDsn?: string)requires at least two parameters (functionNameandversion). The proposed diff calls it with no arguments, which will fail at runtime.Additionally, the actual codebase pattern uses
honoFactory.createApp()(as seen in other backend files), which differs from this guideline requirement.Corrected approach needed
Option 1: Use
createHonowith proper parameters:-import { Hono } from 'hono/tiny' +import { createHono } from '../utils/hono.ts' import { middlewareAuth, parseBody, simpleError, useCors } from '../utils/hono.ts' -export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono('download_link', '1.0.0')Option 2: Align with actual backend pattern:
-import { Hono } from 'hono/tiny' +import { honoFactory } from '../utils/hono.ts' -import { middlewareAuth, parseBody, simpleError, useCors } from '../utils/hono.ts' +import { honoFactory, middlewareAuth, parseBody, simpleError, useCors } from '../utils/hono.ts' -export const app = new Hono<MiddlewareKeyVariables>() +export const app = honoFactory.createApp()Verify which approach aligns with the intended guideline requirement.
supabase/functions/_backend/private/delete_failed_version.ts (2)
20-25: Do not logapikey/capgkey(secret leakage)
These logs can leak credentials into log storage and third-party sinks.Proposed change
const apikey = c.get('apikey') const capgkey = c.get('capgkey') as string - cloudlog({ requestId: c.get('requestId'), message: 'apikey', apikey }) - cloudlog({ requestId: c.get('requestId'), message: 'capgkey', capgkey }) + cloudlog({ + requestId: c.get('requestId'), + message: 'auth context', + hasApikey: !!apikey, + capgkeyPrefix: capgkey ? capgkey.slice(0, 8) : null, + })
18-35: Move body validation before RBAC check (avoid “401” for malformed requests)
Right now a missingapp_id/namewon’t hit the 400s becausecheckPermissionexecutes first.Proposed change
app.delete('/', middlewareKey(['all', 'write', 'upload']), async (c) => { const body = await parseBody<DataUpload>(c) cloudlog({ requestId: c.get('requestId'), message: 'delete failed version body', body }) + + if (!body.app_id) { + return quickError(400, 'error_app_id_missing', 'Error app_id missing', { body }) + } + if (!body.name) { + return quickError(400, 'error_bundle_name_missing', 'Error bundle name missing', { body }) + } @@ - // Auth context is already set by middlewareKey - if (!(await checkPermission(c, 'bundle.delete', { appId: body.app_id }))) { + // Auth context is already set by middlewareKey + if (!(await checkPermission(c, 'bundle.delete', { appId: body.app_id }))) { return quickError(401, 'not_authorized', 'You can\'t access this app', { app_id: body.app_id }) } @@ - if (!body.app_id) { - return quickError(400, 'error_app_id_missing', 'Error bundle name missing', { body }) - } - if (!body.name) { - return quickError(400, 'error_bundle_name_missing', 'Error bundle name missing', { body }) - }Also applies to: 45-50
supabase/functions/_backend/private/upload_link.ts (1)
23-36: Do not logapikey/capgkey(secret leakage) + validatebody.app_idbefore RBAC checkProposed change
const apikey = c.get('apikey') as Database['public']['Tables']['apikeys']['Row'] const capgkey = c.get('capgkey') as string - cloudlog({ requestId: c.get('requestId'), message: 'apikey', apikey }) - cloudlog({ requestId: c.get('requestId'), message: 'capgkey', capgkey }) + cloudlog({ + requestId: c.get('requestId'), + message: 'auth context', + apikeyId: apikey?.id ?? null, + capgkeyPrefix: capgkey ? capgkey.slice(0, 8) : null, + }) + + if (!body.app_id) { + return quickError(400, 'error_app_id_missing', 'Error app_id missing', { body }) + } @@ // Auth context is already set by middlewareKey if (!(await checkPermission(c, 'app.upload_bundle', { appId: body.app_id }))) { throw simpleError('app_access_denied', 'You can\'t access this app', { app_id: body.app_id }) }src/components/tables/HistoryTable.vue (1)
416-472: Server-side permission enforcement for rollback is insufficientThe RLS UPDATE policy on the
channelstable uses genericcheck_min_rights('write')permission, while the frontend enforces a specificchannel.rollback_bundlepermission. A user with 'write' access but without thechannel.rollback_bundlepermission can bypass the UI check by calling Supabase directly:supabase.from('channels').update({version: item.version_id}).eq('id', channelId)The RLS policy will allow this since it only validates 'write' permission, not the specific rollback permission. Update the RLS policy to enforce the RBAC permission explicitly using
rbac_check_permission_direct('channel.rollback_bundle', ...)for version updates, or create a more restrictive trigger that blocks non-authorized version changes.src/pages/app/[package].channel.[channel].vue (1)
448-475: Fix local channel state update guard inonSelectAutoUpdate.The assignment currently won’t run if
disable_auto_updatewas falsy; it should update wheneverchannel.valueexists.Proposed fix
- if (channel.value?.disable_auto_update) - channel.value.disable_auto_update = value + if (channel.value) + channel.value.disable_auto_update = valuesupabase/functions/_backend/public/organization/delete.ts (2)
69-73: Usecapgkey(original key string) forsupabaseApikeyto support hashed keys safely.If
apikey.keyisnullfor hashed keys, this call can break auth (or behave unexpectedly). Other endpoints in this PR usec.get('capgkey')for this reason.Proposed fix
- const { error } = await supabaseApikey(c, apikey.key) + const { error } = await supabaseApikey(c, c.get('capgkey') as string) .from('orgs') .delete() .eq('id', orgId)
26-67: Re-checksupabaseAdminusage in a public endpoint (least privilege).This is user-triggered deletion code; using the admin client for storage deletion is high impact if anything upstream is bypassed. If this must remain, consider adding extra guardrails (defense-in-depth) and ensure logs don’t leak sensitive paths.
supabase/functions/_backend/public/organization/members/get.ts (2)
14-31: Remove PII logs (cloudlograw members data.
cloudlog(..., data)andcloudlog(..., data: parsed.data)will emit user emails (and possibly more) into logs.Proposed fix
- cloudlog({ requestId: c.get('requestId'), message: 'data', data, error }) + cloudlog({ requestId: c.get('requestId'), message: 'get_org_members result', hasData: !!data, hasError: !!error }) @@ - cloudlog({ requestId: c.get('requestId'), message: 'Members', data: parsed.data }) + cloudlog({ requestId: c.get('requestId'), message: 'Members fetched', count: parsed.data.length })Also applies to: 53-63
14-31: Update schema to handleimage_urlas optional.The RPC return for pending invitations sets
image_urlto an empty string, but for regular members it usesusers.image_urlwhich may be nullable.z.string()will fail validation if the value is null, causing a 400 error. Update toz.string().optional().Note:
supabaseApikey()already supports hashed keys via fallback toc.get('capgkey')whenapikey.keyis null, so no change needed there.supabase/functions/_backend/public/organization/members/post.ts (1)
22-34: Potential null safety issue withcapgkeytype assertion.The parameter is correctly renamed to
_apikeyto indicate it's unused. However, line 34 usesc.get('capgkey') as stringwhich could beundefinedbased on theMiddlewareKeyVariablesinterface wherecapgkey?: stringis optional.Consider adding a null check or using a fallback:
Suggested improvement
- const supabase = supabaseApikey(c, c.get('capgkey') as string) + const capgkey = c.get('capgkey') + if (!capgkey) { + throw simpleError('missing_api_key', 'API key is required') + } + const supabase = supabaseApikey(c, capgkey)supabase/functions/_backend/public/app/get.ts (2)
37-96: Blocker:simpleErroris used but not imported in this file.Line 58/78/93 call
simpleError(...), but the imports (Line 4) only includequickError. This should fail typecheck/build.Proposed fix
-import { quickError } from '../../utils/hono.ts' +import { quickError, simpleError } from '../../utils/hono.ts'
16-34: Hashed API keys likely break app fetch path.
checkPermissionexplicitly supports hashed keys by usingc.get('capgkey')whenapikey.keyis null, but Line 24 usessupabaseApikey(c, apikey.key). Ifapikey.keyis null, this will likely fail to query.Proposed fix
- const { data, error: dbError } = await supabaseApikey(c, apikey.key) + const apikeyString = apikey.key ?? c.get('capgkey') + if (!apikeyString) { + throw quickError(401, 'invalid_apikey', 'Missing API key') + } + + const { data, error: dbError } = await supabaseApikey(c, apikeyString) .from('apps') .select('*') .eq('app_id', appId) .single()supabase/functions/_backend/public/organization/members/delete.ts (2)
38-47: Don’t assumecapgkeyexists; derive apikey string safely.Line 39 (
c.get('capgkey') as string) can beundefinedat runtime; the assertion only silences types. Use the same apikey-string derivation pattern ascheckPermission(auth apikey key or capgkey) and fail fast if missing.Proposed fix
- const supabase = supabaseApikey(c, c.get('capgkey') as string) + const apikeyString = c.get('auth')?.apikey?.key ?? c.get('capgkey') + if (!apikeyString) { + throw simpleError('invalid_apikey', 'Missing API key') + } + const supabase = supabaseApikey(c, apikeyString)
27-36: AvoidsupabaseAdminin user-facing APIs (service key); prefer a constrained RPC.Per repo guidelines, using the admin SDK/service key in user-facing endpoints should be avoided. Here it’s used to look up a user by email (Line 28-32). Consider a SECURITY DEFINER SQL function like
get_user_id_by_email_in_org(org_id, email)(or similar) that:
- checks caller’s org permission / membership server-side
- returns only a UUID (or null)
Then call it via the authenticated client (supabaseApikey).src/pages/settings/organization/Members.vue (1)
299-416: RBAC invites will fail—RBAC role names are not valid in theuser_min_rightenum.When RBAC is enabled, the frontend passes
org_member,org_admin,org_billing_admin, ororg_super_admindirectly tosendInvitation(), which calls theinvite_user_to_orgRPC. However, that RPC expects theuser_min_rightenum type, which only contains legacy values:read,upload,write,admin,super_admin,invite_read,invite_upload,invite_write,invite_admin,invite_super_admin. RBAC role names are not part of this enum and will cause a type error at runtime.The
as anycast inhandlePermissionSelectionmasks the mismatch at compile time but does not resolve the underlying incompatibility.
🤖 Fix all issues with AI agents
In @RBAC_SYSTEM.md:
- Around line 1-35: CI flags French words in RBAC_SYSTEM.md ("ressources",
"Fonctions", "supporte") causing the pipeline to fail; fix by either translating
the document to English or updating the typo allowlist: add these exact French
tokens (and any other valid French terms in the doc) to the .typos.toml
allowlist/extend-words entry so the linter ignores them, ensuring you update the
repository's .typos.toml rather than changing the RBAC_SYSTEM.md content unless
you choose translation.
- Around line 801-915: The function rbac_check_permission_direct currently
accepts p_user_id and is executable by the authenticated role, allowing clients
to probe other users' permissions; fix by validating the caller: at the start of
rbac_check_permission_direct add a check that if p_user_id IS NOT NULL AND
p_user_id != auth.uid() then RETURN false; alternatively implement a public
wrapper without the p_user_id parameter that calls the internal
rbac_check_permission_direct (which you then restrict to service_role only) so
client-facing RPCs cannot supply arbitrary user IDs; update permission grants
accordingly.
In @src/components/dashboard/AppAccess.vue:
- Line 73: The file uses the Ref type (e.g., Ref<TableColumn[]>) for the columns
constant but did not import Ref from Vue; add an import for Ref alongside
existing Vue imports (so the Ref type is available) and ensure the columns
declaration remains: const columns: Ref<TableColumn[]> = ref<TableColumn[]>([
... ]). This fixes the missing type reference for Ref used with TableColumn and
the columns symbol.
In @src/pages/settings/organization/Members.vue:
- Around line 82-101: The function checkRbacEnabled is missing its closing
brace, causing subsequent top-level declarations like isInviteNewUserDialogOpen
to be nested inside the function; fix by inserting the missing closing "}"
immediately after the catch block (or before the line declaring const
isInviteNewUserDialogOpen = ref(false)) so checkRbacEnabled() properly ends and
isInviteNewUserDialogOpen remains a top-level reactive ref; verify no other
braces are unbalanced in the surrounding script.
In @src/stores/organization.ts:
- Around line 30-75: The file is missing three exports referenced by
auto-imports: implement and export LEGACY_TO_RBAC_ROLE_MAPPING (reverse of
RBAC_TO_LEGACY_ROLE_MAPPING, mapping each legacy OrganizationRole to the array
of RbacRoleName(s) that include it—ensure 'owner' remains org-scoped only), add
RBAC_ORG_ROLE_DISPLAY_NAMES as a Record<RbacRoleName, string> with
human-friendly labels for each RBAC role, and implement
getRbacRoleDisplayName(role: string): string | undefined to return
RBAC_ORG_ROLE_DISPLAY_NAMES[role]; ensure all three are exported and types match
the existing RbacRoleName and OrganizationRole usages and that names exactly
match LEGACY_TO_RBAC_ROLE_MAPPING, RBAC_ORG_ROLE_DISPLAY_NAMES, and
getRbacRoleDisplayName so auto-imports compile.
In @supabase/functions/_backend/utils/hono_middleware.ts:
- Around line 26-105: middlewareRbacContext has three issues: replace direct
c.req.json() with cloning (use c.req.raw.clone().json()) before parsing so
downstream handlers can still read the body; change the resolver handling from
resolved instanceof Promise ? await resolved : resolved to await
Promise.resolve(options.orgIdResolver(c)) (or await Promise.resolve(...)) to
safely handle thenables; and await the closeClient(...) calls after both DB
usages (the block resolving app org and the RBAC-enabled check) because
closeClient returns a Promise and must be awaited to avoid leaking connections
when using getPgClient/getDrizzleClient.
In @supabase/functions/_backend/utils/rbac.ts:
- Around line 35-81: The Permission union type and the
PERMISSION_TO_LEGACY_RIGHT mapping in rbac.ts are missing the
'app.update_user_roles' entry (the DB/migrations and helper
user_has_app_update_user_roles() expect it), causing a TypeScript error; add
'app.update_user_roles' to the exported Permission type and add an entry in
PERMISSION_TO_LEGACY_RIGHT mapping that maps 'app.update_user_roles' -> 'admin'
to match other app-level admin permissions like 'app.delete' and
'app.read_audit'.
In @supabase/seed.sql:
- Around line 544-547: Replace the stray semicolon that terminates the VALUES
list after the tuple starting with (13, NOW(), 'com.stats.app', '1.0.0', ...)
with a comma so the subsequent tuples (14, …), (15, …), (16, …) are part of the
same INSERT VALUES list, and ensure the final tuple (16, …) ends the statement
with a single semicolon.
- Around line 482-485: The VALUES block is malformed: a stray semicolon ends the
first tuple and the following tuples are left dangling. Replace the semicolon
after the first tuple with a comma (or prepend a single INSERT INTO ... VALUES
(...) before the first tuple if the INSERT is missing), ensure all tuples are
comma-separated, and terminate the final tuple with a single semicolon; in
short, make the rows with the 'user_min_right'::"public"."user_min_right" casts
(e.g., tuples containing UUIDs like 'f6a7b8c9-d0e1-4f2a-9b3c-4d5e6f7a8b92') part
of one well-formed INSERT INTO ... VALUES (...) , (...) , (...) ; statement.
🟠 Major comments (21)
tests/events.test.ts-3-3 (1)
3-3: UsegetEndpointUrl()instead ofBASE_URLin tests (per repo testing guidelines).
Line 3 importsBASE_URL, and the file builds URLs via${BASE_URL}/...; this can break Cloudflare Workers routing expectations. As per coding guidelines, prefergetEndpointUrl(path).Proposed fix (switch to getEndpointUrl)
-import { APP_NAME, BASE_URL, getSupabaseClient, headers, resetAndSeedAppData, resetAndSeedAppDataStats, resetAppData, resetAppDataStats, USER_EMAIL } from './test-utils.ts' +import { APP_NAME, getEndpointUrl, getSupabaseClient, headers, resetAndSeedAppData, resetAndSeedAppDataStats, resetAppData, resetAppDataStats, USER_EMAIL } from './test-utils.ts'- const response = await fetch(`${BASE_URL}/private/events`, { + const response = await fetch(getEndpointUrl('/private/events'), { method: 'POST', headers: { capgkey: headers.Authorization, },(Apply the same replacement for the other
${BASE_URL}/private/eventsoccurrences in this file.)messages/de.json-1454-1463 (1)
1454-1463: Add missing RBAC translation keys to de.json (and other locales).This hunk adds only the role labels at lines 1454–1463. The verification reveals de.json is missing 58 keys found in en.json, including critical RBAC strings:
app-access-control,assign-app-role,assign-role,error-assigning-role,error-removing-role,error-role-already-assigned,no-role-bindings,principal-type,role-assigned,role-assignments,role-assignments-description,role-removed,scope,scope-type, and others. If the UI references these keys, they will render as raw key names in German.This gap exists across most locales (es.json, hi.json, id.json, it.json, ja.json, ko.json, pl.json, pt-br.json, ru.json, tr.json, vi.json, zh-cn.json also missing 58 keys; fr.json missing 21).
supabase/functions/_backend/utils/postgres_schema.ts-164-169 (1)
164-169: Add explicit composite primary key togroup_memberstable in Drizzle schema.The
group_memberstable definition lacks aprimaryKey()declaration. The actual database migration definesPRIMARY KEY (group_id, user_id), but the Drizzle schema must explicitly declare this:Required fix
export const group_members = pgTable('group_members', { group_id: uuid('group_id').notNull(), user_id: uuid('user_id').notNull(), added_by: uuid('added_by'), added_at: timestamp('added_at').notNull().defaultNow(), }, (table) => ({ pk: primaryKey({ columns: [table.group_id, table.user_id] }), }))supabase/functions/_backend/private/roles.ts-1-13 (1)
1-13: UsecreateHonoinstead of instantiatingHonodirectly.All backend files must use
createHonofor Hono app initialization. This sets up essential middleware for Sentry error tracking, request logging, request ID generation (Cloudflare Ray ID, Supabase Execution ID, or UUID), and error handling. Direct instantiation withnew Hono()bypasses these.Replace
export const app = new Hono<MiddlewareKeyVariables>()with an appropriate call tocreateHono('roles', <version>)(ensurefunctionNameandversionare provided).RBAC_SYSTEM.md-665-707 (1)
665-707: Legacy mapping likely references non-existent enum values (invite_*).
rbac_permission_for_legacy()mapspublic.user_min_rightvalues likeinvite_super_admin,invite_admin,invite_write,invite_upload(Line 679-682, 687-690, 695-698). Based on learnings,user_min_rightisread, upload, write, admin, super_adminonly, so this section may be wrong/outdated and should be aligned to the actual enum values. Based on learnings, avoid introducing “owner” semantics into app-level checks as well.RBAC_SYSTEM.md-189-259 (1)
189-259: Doc correctness: conflicting schemas/types + scope examples don’t line up.
role_bindingsis defined once withscope_type,channel_id uuid REFERENCES channels(rbac_id), audit fields, etc. (Line 193-216), but later you show a simplifiedCREATE TABLE role_bindings (...)that conflicts (e.g.,channel_id bigint REFERENCES channels(id)and missing scope/audit columns) (Line 1239-1247). Pick one canonical definition (ideally the real migration schema) and delete/annotate the simplified one.- Bundle permissions are described as “scope: app” (Line 587-596) while later “Bundle permissions (scope: 'bundle')” (Line 1182-1186) — make this consistent with the
permissions.scope_typeCHECK (Line 158-161) and actual implementation.- Best-practices snippet uses
{ appId, bundleId }(Line 1906-1907) but the documentedPermissionScopeonly has orgId/appId/channelId (Line 970-974). Update the example or the scope type.Also applies to: 587-596, 1208-1247, 1901-1907
supabase/functions/_backend/private/devices.ts-1-7 (1)
1-7: UsecreateHonoper backend guidelines (instead ofnew Hono)
This repo’s backend guidelines requirecreateHonofromutils/hono.tsfor app initialization.Proposed change
import type { MiddlewareKeyVariables } from '../utils/hono.ts' import type { Order } from '../utils/types.ts' -import { Hono } from 'hono/tiny' -import { parseBody, simpleError, useCors } from '../utils/hono.ts' +import { createHono, parseBody, simpleError, useCors } from '../utils/hono.ts' import { middlewareV2 } from '../utils/hono_middleware.ts' import { cloudlog } from '../utils/logging.ts' import { checkPermission } from '../utils/rbac.ts' import { countDevices, readDevices } from '../utils/stats.ts' @@ -export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono<MiddlewareKeyVariables>()Also applies to: 25-25
src/components/tables/HistoryTable.vue-47-51 (1)
47-51: Likely incorrect scope: passchannelIdforchannel.rollback_bundle
Using only{ appId }for a channel-scoped permission risks over-authorizing if your RBAC differentiates per-channel access.Proposed change
const canRollbackBundle = computedAsync(async () => { if (!props.appId) return false - return await hasPermission('channel.rollback_bundle', { appId: props.appId }) + return await hasPermission('channel.rollback_bundle', { + appId: props.appId, + channelId: props.channelId, + }) }, false)supabase/functions/_backend/files/files.ts-380-381 (1)
380-381: Use'app.upload_bundle'instead of'app.read_bundles'for upload access.The
checkWriteAppAccessfunction guards upload endpoints (POST, PATCH at lines 461, 466) and is named for write access validation, but it checks the'app.read_bundles'permission (line 381). The RBAC system defines'app.upload_bundle'(upload level) as the correct permission for uploads. Other upload endpoints likeupload_link.tscorrectly use'app.upload_bundle'. Using the read permission allows users with only read access to perform uploads, bypassing intended access controls.Change line 381 from:
const hasPermission = await checkPermissionPg(c, 'app.read_bundles', { appId: app_id }, drizzleClient, userId, capgkey)to:
const hasPermission = await checkPermissionPg(c, 'app.upload_bundle', { appId: app_id }, drizzleClient, userId, capgkey)src/pages/app/[package].access.vue-21-33 (1)
21-33: Avoid stale “app found” state: clearappon refresh and handle Supabase errors explicitly.
app.value = dataApp || app.valuekeeps old data when the next app doesn’t exist (or query fails), so the UI can incorrectly renderAccessTablefor the previous app. Also.single()errors are ignored.Proposed fix
async function loadAppInfo() { try { - const { data: dataApp } = await supabase + const { data: dataApp, error } = await supabase .from('apps') .select() .eq('app_id', id.value) .single() - app.value = dataApp || app.value + if (error) { + console.error('Error loading app info:', error) + app.value = undefined + return + } + app.value = dataApp ?? undefined } catch (error) { console.error('Error loading app info:', error) + app.value = undefined } } async function refreshData() { isLoading.value = true try { + app.value = undefined await loadAppInfo() } catch (error) { console.error('Error refreshing data:', error) } finally { isLoading.value = false } }Also applies to: 35-46
supabase/tests/35_test_is_admin_rbac.sql-22-65 (1)
22-65: Make the test idempotent: protectrole_bindingsinsert against duplicates and assert prerequisites.If
public.role_bindingshas a uniqueness constraint for the binding, this can hard-fail the test run (not just anok(false)).Possible adjustment
-- 5) Grant platform_super_admin role to test_admin +SELECT ok( + EXISTS (SELECT 1 FROM public.roles WHERE name = 'platform_super_admin'), + 'platform_super_admin role exists' +); + INSERT INTO public.role_bindings ( @@ FROM public.roles r WHERE r.name = 'platform_super_admin'; +-- If a unique constraint exists for this binding, keep the test idempotent: +-- ON CONFLICT DO NOTHING;src/pages/app/[package].channel.[channel].vue-47-57 (1)
47-57: UsechannelIdinstead ofappIdfor channel permission checks—the backend auto-derivesappIdandorgId.Lines 50 and 56 should pass
{ channelId: id.value }not{ appId: packageId.value }. The permission service documentation explicitly shows channel-scoped permissions are checked viachannelId, and the backend automatically derives parent scopes.Affected locations
- Lines 47–57:
canUpdateChannelSettingsandcanPromoteBundlecomputed definitions- Lines 138–143, 263–269, 292–296: Functions using these computed values
supabase/tests/34_test_rbac_rls.sql-23-36 (1)
23-36: Replace tautological assertions for roles/permissions reads.Line 26 and Line 33 (
COUNT(*) >= 0) will always pass even if RLS blocks everything (returns 0). Prefer asserting existence of known rows (seeded), or create a row as admin and assert the regular user can see that exact row.Proposed fix (example)
-- 3) Regular user can read roles SELECT tests.authenticate_as('test_user'); SELECT ok( - (SELECT COUNT(*) FROM public.roles) >= 0, + EXISTS (SELECT 1 FROM public.roles), 'Regular user can read roles' ); -- 4) Regular user can read permissions SELECT tests.authenticate_as('test_user'); SELECT ok( - (SELECT COUNT(*) FROM public.permissions) >= 0, + EXISTS (SELECT 1 FROM public.permissions), 'Regular user can read permissions' );supabase/functions/_backend/private/role_bindings.ts-11-12 (1)
11-12: Middleware registration may not cover all routes.Same issue as in
groups.ts- middleware registered on'/'may not apply to sub-routes like/:org_idor/:binding_id.Proposed fix
-app.use('/', useCors) -app.use('/', middlewareAuth) +app.use('*', useCors) +app.use('*', middlewareAuth)supabase/functions/_backend/private/role_bindings.ts-99-101 (1)
99-101: Missing validation forapikeyprincipal type.The code accepts
'apikey'as a validprincipal_typebut there's no validation logic to verify the API key exists or belongs to the organization, unlike the validation for'user'(lines 150-165) and'group'(lines 168-183).Proposed fix
Add validation for apikey principal type:
// Si principal_type est apikey, vérifier qu'il appartient à l'org if (principal_type === 'apikey') { const [apikey] = await drizzle .select() .from(schema.apikeys) .where( and( eq(schema.apikeys.id, principal_id), eq(schema.apikeys.owner_org, org_id), ), ) .limit(1) if (!apikey) { return c.json({ error: 'API key not found in this org' }, 400) } }supabase/functions/_backend/private/role_bindings.ts-122-124 (1)
122-124: String comparison for permission levels is unreliable.Same issue as in
groups.ts- using< 'admin'for string comparison doesn't reliably determine privilege levels.Proposed fix
- if (!orgAccess.length || orgAccess[0].user_right < 'admin') { + const adminRoles = ['admin', 'super_admin'] + if (!orgAccess.length || !adminRoles.includes(orgAccess[0].user_right)) { return c.json({ error: 'Forbidden - Admin rights required' }, 403) }Consider creating a shared helper function for permission checks to ensure consistency across all endpoints.
supabase/functions/_backend/private/groups.ts-94-96 (1)
94-96: String comparison for permission levels may not work correctly.The comparison
orgAccess[0].user_right < 'admin'uses lexicographic string comparison. Ifuser_rightis an enum like['read', 'upload', 'write', 'admin', 'super_admin'], this comparison won't correctly determine privilege levels. Consider comparing against an explicit list or using a helper function.Proposed fix
- if (!orgAccess.length || orgAccess[0].user_right < 'admin') { + const adminRoles = ['admin', 'super_admin'] + if (!orgAccess.length || !adminRoles.includes(orgAccess[0].user_right)) { return c.json({ error: 'Forbidden - Admin rights required' }, 403) }src/services/permissions.ts-209-221 (1)
209-221:checkPermissionsBatchkeying bypermissionwill corrupt results across scopes.
Example: two checks for'app.read'on different apps will collide and one overwrites the other. Use a composite key (permission + scope) or return an array preserving order.One possible fix (stable composite key)
export async function checkPermissionsBatch( checks: Array<{ permission: Permission, scope: PermissionScope }>, ): Promise<Record<string, boolean>> { const results = await Promise.all( checks.map(async ({ permission, scope }) => ({ - key: permission, + key: `${permission}:${JSON.stringify(scope)}`, allowed: await hasPermission(permission, scope), })), ) return Object.fromEntries( results.map(({ key, allowed }) => [key, allowed]), ) }supabase/functions/_backend/utils/rbac.ts-170-207 (1)
170-207:isRbacEnabledForOrg()cache is incorrect across multiple orgIds in the same request.
It caches a single boolean inc.set('rbacEnabled', enabled)without tying it toorgId, so a second call with a different orgId may reuse the wrong value.One possible fix (cache orgId + value)
export async function isRbacEnabledForOrg( c: Context<MiddlewareKeyVariables>, orgId: string | null, ): Promise<boolean> { - const cached = c.get('rbacEnabled') - if (cached !== undefined) { - return cached - } + const cached = c.get('rbacEnabled') + const cachedOrgId = c.get('resolvedOrgId') + if (cached !== undefined && cachedOrgId === orgId) { + return cached + } if (!orgId) { return false } let pgClient try { pgClient = getPgClient(c) const drizzleClient = getDrizzleClient(pgClient) const result = await drizzleClient.execute( sql`SELECT public.rbac_is_enabled_for_org(${orgId}::uuid) as enabled`, ) const enabled = (result.rows[0] as any)?.enabled === true - c.set('rbacEnabled', enabled) + c.set('rbacEnabled', enabled) + c.set('resolvedOrgId', orgId) return enabled }src/pages/settings/organization/Members.vue-238-271 (1)
238-271: RBAC member deletion is broken:aid: 0mapping prevents all RBAC members from being deleted.RBAC members are mapped with
aid: 0(line 252), butdeleteMember()explicitly blocks deletion whenaid === 0with the "cannot-delete-owner" error message. This makes every RBAC member undeletable. Additionally,_deleteMember()attempts to delete fromorg_userstable bymember.aid, which would also fail for RBAC members sinceaidis not populated from the RBAC data model.src/auto-imports.d.ts-11-15 (1)
11-15: Remove non-existent exports from auto-imports declarations.The following exports are declared in auto-imports.d.ts but do not exist in
src/stores/organization.ts:
LEGACY_TO_RBAC_ROLE_MAPPING(lines 11, 353)RBAC_ORG_ROLE_DISPLAY_NAMES(lines 12, 353)getRbacRoleDisplayName(line 46)Only
RBAC_TO_LEGACY_ROLE_MAPPING,RBAC_ORG_ROLE_I18N_KEYS,RBAC_ROLE_HIERARCHY, andgetRbacRoleI18nKeyare actually exported from organization.ts. Remove the non-existent entries from auto-imports.d.ts or add them to organization.ts if they are intended.
🟡 Minor comments (15)
messages/es.json-1454-1463 (1)
1454-1463: Spanish consistency: consider matching existing “Súper administrador” capitalization/diacritics.
role-org-super-adminis “Super Administrador”; elsewhere the UI uses “Súper administrador”.Proposed tweak
- "role-org-super-admin": "Super Administrador", + "role-org-super-admin": "Súper administrador",messages/en.json-1482-1487 (1)
1482-1487: Remove the unused capitalized"Access"key at line 1516; use the existing lowercase"access"key instead.The lowercase
"access"key (line 1482) is actively used in the codebase—it's referenced insrc/constants/appTabs.tsas a tab label and translated viat(tab.label)insrc/components/Tabs.vue. The capitalized"Access"key at line 1516 has no references in the codebase and should be removed to avoid confusion and accidental misuse.messages/ko.json-1454-1463 (1)
1454-1463: Korean “role-app-reader” translation is likely misleading (“리더” reads as “leader”).Consider changing
"role-app-reader": "앱 리더"to something unambiguous like “앱 뷰어”, “읽기 전용”, or “읽기 권한”.
Also consider"zip-bundle": "지프 앱 번들"→ “ZIP 앱 번들” for consistency with “ZIP” as a term.messages/tr.json-1454-1463 (1)
1454-1463: Consider a more natural Turkish label for “role-app-reader”.“Uygulama Okuyucu” is understandable, but “Uygulama Görüntüleyici” (or similar) may read more naturally for “read-only/viewer”.
supabase/functions/_backend/private/devices.ts-29-35 (1)
29-35: Addbody.appIdvalidation before callingcheckPermissionMissing
appIdcurrently results in a 403 authorization error instead of a 400-style input validation error. Following the established pattern in the codebase (e.g.,stripe_checkout.ts), validate the required field before permission checks to provide clearer error semantics.Proposed change
app.post('/', middlewareV2(['read', 'write', 'all', 'upload']), async (c) => { const body = await parseBody<DataDevice>(c) cloudlog({ requestId: c.get('requestId'), message: 'post devices body', body }) + if (!body.appId) { + throw simpleError('missing_app_id', 'Missing appId', { body }) + } if (!(await checkPermission(c, 'app.read_devices', { appId: body.appId }))) { throw simpleError('app_access_denied', 'You can\'t access this app', { app_id: body.appId }) }supabase/functions/_backend/private/stripe_portal.ts-44-45 (1)
44-45: Typo in error code and message:not_authorizeshould benot_authorized.Same issue as in
stripe_checkout.ts- the error code and message are inconsistent with the rest of the file (lines 23, 31, 40 usenot_authorized/Not authorized).Proposed fix
if (!await checkPermission(c, 'org.update_billing', { orgId: body.orgId })) - throw simpleError('not_authorize', 'Not authorize') + throw simpleError('not_authorized', 'Not authorized')supabase/functions/_backend/private/stripe_checkout.ts-54-55 (1)
54-55: Typo in error code and message:not_authorizeshould benot_authorized.The error code and message are inconsistent with the rest of the file (lines 33, 41, 50 all use
not_authorized/Not authorized). This could cause issues for clients parsing error codes.Proposed fix
if (!await checkPermission(c, 'org.update_billing', { orgId: body.orgId })) - throw simpleError('not_authorize', 'Not authorize') + throw simpleError('not_authorized', 'Not authorized')src/pages/settings/organization/Plans.vue-176-177 (1)
176-177: Potential race condition between organization change and permission check.When
currentOrganizationchanges,canUpdateBillingwill re-compute asynchronously. At line 177,canUpdateBilling.valuemay still reflect the previous organization's permission result or the defaultfalsevalue, potentially triggering the "cannot-view-plans" dialog incorrectly.Consider awaiting the permission check or using
watchEffectwith the permission result as a dependency:Potential fix using explicit await
watch(currentOrganization, async (newOrg, prevOrg) => { - if (newOrg && !canUpdateBilling.value) { + if (newOrg) { + const hasBillingPermission = await hasPermission('org.update_billing', { orgId: newOrg.gid }) + if (!hasBillingPermission) { if (!initialLoad.value) { // ... rest of the logic } + } + }src/pages/app/[package].access.vue-1-17 (1)
1-17: UseuseRouter()instead of$routerin the template (per repo guidance).Proposed fix
<script setup lang="ts"> @@ -import { useRoute } from 'vue-router' +import { useRoute, useRouter } from 'vue-router' @@ const route = useRoute('/app/[package].access') +const router = useRouter() @@ </script> @@ - <button class="mt-4 text-white d-btn d-btn-primary" @click="$router.push(`/app`)"> + <button class="mt-4 text-white d-btn d-btn-primary" @click="router.push('/app')"> {{ t('back-to-apps') }} </button>Also applies to: 76-78
supabase/functions/_backend/public/app/post.ts-27-30 (1)
27-30: Remove the conditionalbody.owner_org &&from the permission check.The permission
'org.update_settings'is the correct choice—no dedicated'org.create_app'permission exists in the RBAC system. However, sinceowner_orgis a required field in theCreateAppinterface (line 12), the conditional check bypasses permission validation ifowner_orgis falsy, creating a security gap.Align with the pattern used in
organization/put.tsandorganization/delete.ts, which perform the permission check unconditionally: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 }) }Alternatively, add explicit validation for
owner_orgbefore the permission check if optional handling is intentional.tests/rbac-permissions.test.ts-11-14 (1)
11-14: Drop unused constant or use it.
TEST_RBAC_ORG_ID(Line 13) isn’t referenced.supabase/functions/_backend/public/organization/members/delete.ts-10-13 (1)
10-13: Add UUID validation fororgId
orgIdshould be validated as a UUID on line 11, not just a string. For a permission-critical endpoint that manages organization membership, UUID validation is essential:const deleteBodySchema = z.object({ - orgId: z.string(), + orgId: z.string().uuid(), email: z.email(), })The
z.email()validation is correct and fully supported in zod/mini.supabase/functions/_backend/private/groups.ts-14-14 (1)
14-14: Fix typo flagged by pipeline.Proposed fix
-// GET /private/groups/:org_id - Liste des groupes d'un org +// GET /private/groups/:org_id - List groups for an orgsrc/components/tables/AccessTable.vue-162-162 (1)
162-162: Fix typo flagged by pipeline.The pipeline failure indicates a spelling error in the French comment.
Proposed fix
- // Mettre à jour le role_id existant + // Mettre à jour le role_id existentOr consider using English comments for consistency:
- // Mettre à jour le role_id existant + // Update the existing role_idmessages/fr.json-1454-1500 (1)
1454-1500: Translation map: consider removing the capitalized duplicate key and fix remaining English strings.
Having both"access"and"Access"is easy to misuse; also there are still values like"Open in new tab"/"Your API Key"in the FR file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/constants/organizationTabs.ts (1)
8-10: Duplicate icon import:IconShieldandIconSecurityreference the same icon.Both
IconSecurity(line 8) andIconShield(line 9) import from~icons/heroicons/shield-check. Either reuseIconSecurityfor therole-assignmentstab, or choose a different icon if visual distinction is intended.♻️ Proposed fix: Reuse existing IconSecurity
import IconSecurity from '~icons/heroicons/shield-check' -import IconShield from '~icons/heroicons/shield-check' import IconUserGroup from '~icons/heroicons/user-group'Then update line 20:
- { label: 'role-assignments', key: '/settings/organization/role-assignments', icon: IconShield }, + { label: 'role-assignments', key: '/settings/organization/role-assignments', icon: IconSecurity },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/constants/organizationTabs.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{ts,tsx,vue,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
~/alias for imports fromsrc/directory in frontend TypeScript and Vue components
Files:
src/constants/organizationTabs.ts
src/**/*.{vue,ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend ESLint must pass before commit; run
bun lint:fixto auto-fix issues in frontend files
Files:
src/constants/organizationTabs.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
src/constants/organizationTabs.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per @antfu/eslint-config rules
Files:
src/constants/organizationTabs.ts
src/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use path alias
~/to map tosrc/for imports in TypeScript and JavaScript files
Files:
src/constants/organizationTabs.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
src/constants/organizationTabs.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: riderx
Repo: Cap-go/capgo PR: 1297
File: src/components/dashboard/DeploymentBanner.vue:77-79
Timestamp: 2025-12-23T01:19:15.067Z
Learning: In the Cap-go codebase, the 'owner' role exists only at the organization level (used in src/stores/organization.ts), not as an app-level permission. App-level permissions use the user_min_right database enum which includes: read, upload, write, admin, super_admin (but NOT owner). When checking app permissions, never include 'owner' in the role checks.
📚 Learning: 2025-10-30T14:58:37.007Z
Learnt from: Dalanir
Repo: Cap-go/capgo PR: 1238
File: src/layouts/settings.vue:7-13
Timestamp: 2025-10-30T14:58:37.007Z
Learning: In the Cap-go/capgo repository, heroicons are imported using the pattern `~icons/heroicons/icon-name` (e.g., `~icons/heroicons/user`, `~icons/heroicons/bell`) without requiring `/outline/` or `/solid/` path segments. Solid variants use a `-solid` suffix in the icon name itself (e.g., `~icons/heroicons/arrow-path-solid`).
Applied to files:
src/constants/organizationTabs.ts
🔇 Additional comments (1)
src/constants/organizationTabs.ts (1)
19-20: New RBAC tabs structure looks correct.The
groupsandrole-assignmentstabs are appropriately placed and follow the existing tab definition pattern. The route paths are consistent with the organization settings structure.
Dalanir
force-pushed
the
rbac-system-v1
branch
from
75934f6 to
403d5ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
supabase/seed.sql (1)
474-485: Fix brokenorg_usersseed INSERT (SQL syntax error).Line 482 ends the
VALUESlist with);, but Lines 483-485 continue with additional tuples—this will fail parsing.Proposed fix
INSERT INTO "public"."org_users" ("org_id", "user_id", "user_right", "app_id", "channel_id") VALUES @@ - ('c3d4e5f6-a7b8-4c9d-8e0f-1a2b3c4d5e6f', '8b2c3d4e-5f6a-4b7c-8d9e-0f1a2b3c4d5e', 'super_admin'::"public"."user_min_right", null, null), - ('f6a7b8c9-d0e1-4f2a-9b3c-4d5e6f7a8b92', 'e5f6a7b8-c9d0-4e1f-8a2b-3c4d5e6f7a81', 'super_admin'::"public"."user_min_right", null, null); - ('046a36ac-e03c-4590-9257-bd6c9dba9ee8', 'c591b04e-cf29-4945-b9a0-776d0672061a', 'admin'::"public"."user_min_right", null, null), - ('34a8c55d-2d0f-4652-a43f-684c7a9403ac', '6aa76066-55ef-4238-ade6-0b32334a4097', 'write'::"public"."user_min_right", null, null); - ('a7b8c9d0-e1f2-4a3b-9c4d-5e6f7a8b9ca4', 'f6a7b8c9-d0e1-4f2a-9b3c-4d5e6f708193', 'super_admin'::"public"."user_min_right", null, null); + ('c3d4e5f6-a7b8-4c9d-8e0f-1a2b3c4d5e6f', '8b2c3d4e-5f6a-4b7c-8d9e-0f1a2b3c4d5e', 'super_admin'::"public"."user_min_right", null, null), + ('f6a7b8c9-d0e1-4f2a-9b3c-4d5e6f7a8b92', 'e5f6a7b8-c9d0-4e1f-8a2b-3c4d5e6f7a81', 'super_admin'::"public"."user_min_right", null, null), + ('046a36ac-e03c-4590-9257-bd6c9dba9ee8', 'c591b04e-cf29-4945-b9a0-776d0672061a', 'admin'::"public"."user_min_right", null, null), + ('34a8c55d-2d0f-4652-a43f-684c7a9403ac', '6aa76066-55ef-4238-ade6-0b32334a4097', 'write'::"public"."user_min_right", null, null), + ('a7b8c9d0-e1f2-4a3b-9c4d-5e6f7a8b9ca4', 'f6a7b8c9-d0e1-4f2a-9b3c-4d5e6f708193', 'super_admin'::"public"."user_min_right", null, null);supabase/functions/_backend/utils/hono.ts (1)
111-136: JWT validation should return 401 Unauthorized, not 400 Bad Request.Line 125:
simpleError()always returns status 400, but authentication failures should return 401. Change toquickError(401, 'invalid_jwt', 'Invalid JWT').Note: The
jwtfield in the auth context stores the rawAuthorizationheader value (including the "Bearer " prefix), which is correct for the current usage insupabaseWithAuth()but should be documented to prevent downstream confusion if the auth object is used elsewhere.Proposed fix
if (authError || !authData.user) { cloudlog({ requestId: c.get('requestId'), message: 'Invalid JWT', error: authError }) - return simpleError('invalid_jwt', 'Invalid JWT') + throw quickError(401, 'invalid_jwt', 'Invalid JWT') }supabase/functions/_backend/public/bundle/set_channel.ts (1)
15-27: Bug: check channel-scoped permission withchannelId, not justappId.With
{ appId: body.app_id }(Line 25), a principal that only has a channel-level role binding can be denied incorrectly. Use{ channelId: body.channel_id }(and optionally includeappIdtoo).Proposed fix
- if (!(await checkPermission(c, 'channel.promote_bundle', { appId: body.app_id }))) { + if (!(await checkPermission(c, 'channel.promote_bundle', { channelId: body.channel_id }))) { throw simpleError('cannot_access_app', 'You can\'t access this app', { app_id: body.app_id }) }src/components/dashboard/AppSetting.vue (1)
104-147: Fix incorrect org role value at line 826.The code checks
org.role === 'org_super_admin', butorg.rolecontains legacy role values ('super_admin'or'owner'), not RBAC names. Change toorg.role === 'super_admin' || org.role === 'owner'to match the actual role values stored in the organization object.This also applies to lines 301-306, 430-435, 637-642, and 825-830 if they have the same issue.
supabase/functions/_backend/private/delete_failed_version.ts (1)
20-29: Do not logcapgkey(API key); remove or heavily redact.Line 24 logs the full
capgkey, which is a credential. This is a production security incident waiting to happen (logs are widely accessible and long-lived).Proposed diff
const apikey = c.get('apikey') const capgkey = c.get('capgkey') as string cloudlog({ requestId: c.get('requestId'), message: 'apikey', apikey }) - cloudlog({ requestId: c.get('requestId'), message: 'capgkey', capgkey }) + cloudlog({ + requestId: c.get('requestId'), + message: 'capgkey', + capgkey: capgkey ? `${capgkey.slice(0, 6)}...` : 'missing', + }) - const { data: userId, error: _errorUserId } = await supabaseApikey(c, capgkey) - .rpc('get_user_id', { apikey: capgkey, app_id: body.app_id }) - if (_errorUserId) { - return quickError(404, 'user_not_found', 'Error User not found', { _errorUserId }) - }supabase/functions/_backend/files/files.ts (1)
373-403: Changeapp.read_bundlestoapp.upload_bundleincheckWriteAppAccess.Line 381:
checkWriteAppAccessis a middleware protecting file upload/write operations (POST, PATCH routes), yet it checksapp.read_bundles(read permission). This is an authorization regression. The RBAC system clearly distinguishesapp.read_bundles(read) fromapp.upload_bundle(upload), and similar upload endpoints (e.g.,bundle/create.ts) correctly useapp.upload_bundle. File write operations should require upload permission, not read permission.src/layouts/settings.vue (1)
54-123: Billing tab can “stick” on native because removal only happens when!canReadBilling.Current logic only removes
/billingwhen!canReadBilling.value, not whenCapacitor.isNativePlatform()is true. If the tab ever gets added in a non-native context (or base tabs change), it won’t be forcibly removed on native.Proposed tweak
- if (!Capacitor.isNativePlatform() - && canReadBilling.value - && !organizationTabs.value.find(tab => tab.key === '/billing')) { + if (!Capacitor.isNativePlatform() + && canReadBilling.value + && !organizationTabs.value.find(tab => tab.key === '/billing')) { organizationTabs.value.push({ label: 'billing', icon: IconBilling, key: '/billing', onClick: () => openPortal(organizationStore.currentOrganization?.gid ?? '', t), }) } - else if (!canReadBilling.value) { + else if (Capacitor.isNativePlatform() || !canReadBilling.value) { organizationTabs.value = organizationTabs.value.filter(tab => tab.key !== '/billing') }
🤖 Fix all issues with AI agents
In @src/components/dashboard/AppAccess.vue:
- Line 3: The file imports computed, onMounted, ref, watch from 'vue' but uses
the Vue type Ref (e.g., Ref<TableColumn[]>) on line 73, causing a TS error; fix
it by adding Ref to the import list from 'vue' (include Ref alongside
computed/onMounted/ref/watch) so the type Ref<TableColumn[]> is recognized where
used in AppAccess.vue.
In @src/pages/settings/organization/Members.vue:
- Around line 82-101: The function checkRbacEnabled is missing its closing
brace, causing a syntax error; close the function body by adding a single '}'
right after the existing catch block (before the declaration of
isInviteNewUserDialogOpen) so the try/catch and the async function are properly
terminated and ensure indentation matches surrounding code.
In @src/stores/organization.ts:
- Around line 287-372: The comment block around invalidateRoleBindingsCache and
hasPermissionsInRbac contains French text and typos causing CI spellcheck
failures; replace or rewrite those block comments in English and fix
misspellings (e.g., "Invalide" → "Invalidate", "FONCTION" → "FUNCTION",
"EXEMPLE"/"AVANTAGES"/"autorisation" → "EXAMPLE"/"ADVANTAGES"/"authorization"),
keeping the same intent and references to RBAC migration and the TODO for
hasPermission; update the JSDoc above hasPermissionsInRbac and the surrounding
explanatory comments to clean, consistent English phrasing while preserving
mentions of RBAC_ROLE_HIERARCHY, role_bindings, role_permissions, and the
migration steps so spellcheck passes.
In @supabase/functions/_backend/private/groups.ts:
- Around line 94-96: The permission check using orgAccess[0].user_right <
'admin' is invalid because it relies on lexicographic string comparison; replace
it with an explicit privilege check (e.g., verify user_right is one of
['admin','super_admin'] or map enum values to numeric ranks and compare ranks)
inside the same conditional that returns the 403; update the check near
orgAccess[0].user_right to either test inclusion in the allowed set or call a
new utility (e.g., hasRequiredRight(user_right, 'admin')) that encapsulates the
enum-to-rank logic.
In @supabase/functions/_backend/private/invite_new_user_to_org.ts:
- Around line 51-56: validateInvite currently returns { status: 403 } on RBAC
denial which the invite_new_user_to_org handler ignores and later throws a 400;
change validateInvite to throw a quickError(403, ...) (or otherwise throw an
error) immediately when checkPermission fails so the route handler won’t
continue to the simpleError path. Specifically, inside validateInvite replace
the return { status: 403 } branch with throw quickError(403, 'not authorized
(insufficient permissions)') (or equivalent) and apply the same change for any
other permission-denial branches referenced by the invite_new_user_to_org flow
so callers stop execution and the correct 403 is returned.
In @supabase/functions/_backend/private/role_bindings.ts:
- Around line 251-253: The condition uses a lexicographic string comparison
(orgAccess[0].user_right < 'admin') which is incorrect; replace it with an
explicit role check (e.g., ensure orgAccess exists and orgAccess[0].user_right
=== 'admin' or use a role-to-rank lookup and compare numeric ranks) so the
handler correctly enforces admin-only access before returning c.json({ error:
'Forbidden - Admin rights required' }, 403).
- Around line 122-124: The current check uses lexicographic string comparison on
orgAccess[0].user_right which is invalid for role hierarchy; replace it with an
explicit hierarchy check (e.g., define an ordered roles array like
['read','upload','write','admin','super_admin'] and compare indices, or simply
check membership in allowedRoles = ['admin','super_admin']) and use that to
decide if the user is permitted before returning c.json({ error: 'Forbidden -
Admin rights required' }, 403). Ensure you reference orgAccess[0].user_right for
the lookup and keep the same response when unauthorized.
In @supabase/functions/_backend/public/build/start.ts:
- Around line 5-7: The code uses a non-null assertion apikey.key! before calling
checkPermission which will throw if the DB row has key=null; change the logic in
the start handler to defensively handle nullable keys: read apikey without using
!, prefer apikey.key ?? apikey.hashed_key (or pass the whole apikey object to
checkPermission if it understands hashed keys), and ensure any failure to
resolve a usable key triggers markBuildAsFailed and returns the controlled error
response rather than letting an exception bubble; update references to
apikey.key! to these safe checks and add a guarded error path that calls
markBuildAsFailed when no valid key is available.
🟠 Major comments (13)
supabase/functions/_backend/utils/postgres_schema.ts-164-169 (1)
164-169:group_membersneeds a PK/unique constraint + FK to prevent duplicates/orphans.Without a composite PK/unique index, the same
(group_id, user_id)can be inserted multiple times, which can break effective-permissions calculations.Possible patch
-import { bigint, boolean, integer, pgEnum, pgTable, serial, text, timestamp, uuid, varchar } from 'drizzle-orm/pg-core' +import { bigint, boolean, integer, pgEnum, pgTable, serial, text, timestamp, uuid, varchar, primaryKey } from 'drizzle-orm/pg-core' -export const group_members = pgTable('group_members', { - group_id: uuid('group_id').notNull(), +export const group_members = pgTable('group_members', { + group_id: uuid('group_id').notNull().references(() => groups.id, { onDelete: 'cascade' }), user_id: uuid('user_id').notNull(), added_by: uuid('added_by'), added_at: timestamp('added_at').notNull().defaultNow(), -}) +}, (t) => ({ + pk: primaryKey({ columns: [t.group_id, t.user_id] }), +}))supabase/functions/_backend/utils/postgres_schema.ts-154-162 (1)
154-162: Add FK + per-org uniqueness forgroups.
groups.org_idshould likely referenceorgs.id, andnameis typically unique within an org (otherwise UI/API gets ambiguous).Possible patch
-import { bigint, boolean, integer, pgEnum, pgTable, serial, text, timestamp, uuid, varchar } from 'drizzle-orm/pg-core' +import { bigint, boolean, integer, pgEnum, pgTable, serial, text, timestamp, uuid, varchar, uniqueIndex } from 'drizzle-orm/pg-core' -export const groups = pgTable('groups', { +export const groups = pgTable('groups', { id: uuid('id').primaryKey().defaultRandom(), - org_id: uuid('org_id').notNull(), + org_id: uuid('org_id').notNull().references(() => orgs.id, { onDelete: 'cascade' }), name: text('name').notNull(), description: text('description'), is_system: boolean('is_system').notNull().default(false), created_by: uuid('created_by'), created_at: timestamp('created_at').notNull().defaultNow(), -}) +}, (t) => ({ + orgNameUq: uniqueIndex('groups_org_id_name_uq').on(t.org_id, t.name), +}))supabase/functions/_backend/utils/postgres_schema.ts-171-186 (1)
171-186: Add missing foreign key constraints and indexes to match the database schema in role_bindings.The Drizzle schema for
role_bindings(lines 171–186) is missing critical constraints and indexes that are defined in the migration (20251222140030_rbac_system.sql). The types themselves are correct—channel_idisuuidand correctly referenceschannels(rbac_id)(alsouuid), andapp_idis intentionallyuuidto referenceapps.idrather thanapps.name. However:
- Missing foreign keys:
role_id,org_id,app_id,channel_id, andbundle_idall lack.references()constraints that exist in the database migration.- Missing CHECK constraint:
scope_typeshould validate against('platform', 'org', 'app', 'bundle', 'channel')as enforced in the migration.- Missing indexes: The migration defines five unique scope-based indexes and two composite indexes for efficient lookups and SSD enforcement.
Sync the Drizzle schema with the authoritative migration schema to ensure ORM queries reflect actual constraints:
Update to add FKs and basic index hint
export const role_bindings = pgTable('role_bindings', { id: uuid('id').primaryKey().defaultRandom(), principal_type: text('principal_type').notNull(), principal_id: uuid('principal_id').notNull(), - role_id: uuid('role_id').notNull(), + role_id: uuid('role_id').notNull().references(() => roles.id, { onDelete: 'cascade' }), scope_type: text('scope_type').notNull(), - org_id: uuid('org_id'), - app_id: uuid('app_id'), + org_id: uuid('org_id').references(() => orgs.id, { onDelete: 'cascade' }), + app_id: uuid('app_id').references(() => apps.id, { onDelete: 'cascade' }), bundle_id: bigint('bundle_id', { mode: 'number' }), - channel_id: uuid('channel_id'), + channel_id: uuid('channel_id').references(() => channels.rbac_id, { onDelete: 'cascade' }), granted_by: uuid('granted_by').notNull(), granted_at: timestamp('granted_at').notNull().defaultNow(), expires_at: timestamp('expires_at'), reason: text('reason'), is_direct: boolean('is_direct').notNull().default(true), })supabase/functions/_backend/private/stripe_portal.ts-5-5 (1)
5-5: Fix auth error code/message + consider checking permission before querying org.
- Line 45:
not_authorize/Not authorizelooks accidental and inconsistent with the rest of the file (and likely client expectations).- Line 44: You can run
checkPermissionbefore fetchingorg.customer_idto fail fast.Proposed fix
- if (!await checkPermission(c, 'org.update_billing', { orgId: body.orgId })) - throw simpleError('not_authorize', 'Not authorize') + if (!await checkPermission(c, 'org.update_billing', { orgId: body.orgId })) + throw simpleError('not_authorized', 'Not authorized')Also applies to: 9-12, 44-45
RBAC_SYSTEM.md-631-915 (1)
631-915: Security note: don’t expose SECURITY DEFINER functions that accept arbitraryp_user_id.The doc demonstrates calling
rbac_check_permission_direct(p_user_id => 'user-uuid'::uuid, ...)while the function isSECURITY DEFINER. If this RPC is granted toauthenticated, clients could probe permissions for other users unless the SQL enforcesp_user_id = auth.uid()(or you only expose a no-arg wrapper that usesauth.uid()). This mirrors prior learnings about not granting inner functions withuser_idparams.Also applies to: 797-945
tests/rbac-permissions.test.ts-26-177 (1)
26-177: Use dedicated TEST_RBAC_ORG_ID and a dedicated test user/app for RBAC tests instead of reusing ORG_ID.Tests that enable/disable RBAC mode modify shared global state (
rbac_settingsWHEREid = 1and per-orguse_new_rbacflags). Since tests run in parallel and share ORG_ID with other test files, these mutations can interfere with other tests' setup. TEST_RBAC_ORG_ID is already defined in seed but unused. Create dedicated RBAC-specific test fixtures for org, user, and app or reuse TEST_RBAC_ORG_ID; ensure all state mutations are properly reverted in afterEach hooks (currently line 156 updates only the org flag, not the global setting).src/stores/organization.ts-102-103 (1)
102-103: RBAC cache won’t be reactive withRef<Map>mutations; UI can get stuck showing “no access”.
_roleBindingsCacheis aref(new Map())(Line 102) and you mutate it via.set/.delete/.clear(Lines 278-279, 291-296, 508-509). Vue won’t track internalMapmutations through aRef, so anything relying on this for rendering can fail to update after role bindings load.One workable pattern (replace Map on writes)
- const _roleBindingsCache: Ref<Map<string, RoleBinding[]>> = ref(new Map()) // Cache des role_bindings par orgId + const _roleBindingsCache: Ref<Map<string, RoleBinding[]>> = ref(new Map()) // Cache des role_bindings par orgId + + const setRoleBindingsCache = (orgId: string, bindings: RoleBinding[]) => { + // Replace the Map instance to trigger reactivity + const next = new Map(_roleBindingsCache.value) + next.set(orgId, bindings) + _roleBindingsCache.value = next + } ... - _roleBindingsCache.value.set(orgId, userBindings) + setRoleBindingsCache(orgId, userBindings) ... - _roleBindingsCache.value.delete(orgId) + const next = new Map(_roleBindingsCache.value) + next.delete(orgId) + _roleBindingsCache.value = next ... - _roleBindingsCache.value.clear() + _roleBindingsCache.value = new Map() ... - _roleBindingsCache.value.set(org.gid, bindingsData) + setRoleBindingsCache(org.gid, bindingsData)Also applies to: 251-285, 290-297, 299-332, 496-515
supabase/functions/_backend/utils/rbac.ts-170-207 (1)
170-207:rbacEnabledcontext cache should be per-org (or include orgId).
isRbacEnabledForOrg()caches a single boolean inc.get('rbacEnabled')(Lines 175-178, 195-196). If any request ever checks multiple orgIds, the first result will be reused incorrectly.Safer cache shape
export async function isRbacEnabledForOrg( c: Context<MiddlewareKeyVariables>, orgId: string | null, ): Promise<boolean> { - // Check cache first - const cached = c.get('rbacEnabled') - if (cached !== undefined) { - return cached - } + const cachedOrgId = c.get('resolvedOrgId') + const cachedEnabled = c.get('rbacEnabled') + if (cachedOrgId === orgId && cachedEnabled !== undefined) + return cachedEnabled if (!orgId) { return false } ... - c.set('rbacEnabled', enabled) + c.set('resolvedOrgId', orgId) + c.set('rbacEnabled', enabled) return enabledsupabase/functions/_backend/utils/rbac.ts-239-312 (1)
239-312: Enforce “non-empty scope” and use read-only PG client for SELECT-only checks.
checkPermission()acceptsscope: PermissionScopebut doesn’t enforce the “at least one must be provided” contract (Lines 257-258). Also it usesgetPgClient(c)for a SELECT (Line 270) wheregetPgClient(c, true)seems more appropriate.Proposed diff
export async function checkPermission( c: Context<MiddlewareKeyVariables>, permission: Permission, scope: PermissionScope, ): Promise<boolean> { ... const { orgId = null, appId = null, channelId = null } = scope + if (orgId == null && appId == null && channelId == null) { + cloudlog({ + requestId: c.get('requestId'), + message: 'checkPermission: missing scope', + permission, + }) + return false + } ... - pgClient = getPgClient(c) + pgClient = getPgClient(c, true)supabase/functions/_backend/public/build/status.ts-5-7 (1)
5-7: Avoid assumingapikey.keyis present; hashed API keys likely break this path
checkPermissionexplicitly supports hashed keys viac.get('capgkey'), butsupabaseApikey(c, apikey.key)(Line 39) still assumes a non-null key. Ifapikey.keyis null for hashed keys, this can break build status reads (and subsequent DB updates) even though the RBAC check passes.Proposed diff
+import type { MiddlewareKeyVariables } from '../../utils/hono.ts' import type { Context } from 'hono' import type { Database } from '../../utils/supabase.types.ts' import { simpleError } from '../../utils/hono.ts' import { cloudlog, cloudlogErr } from '../../utils/logging.ts' import { recordBuildTime, supabaseApikey } from '../../utils/supabase.ts' import { checkPermission } from '../../utils/rbac.ts' import { getEnv } from '../../utils/utils.ts' export async function getBuildStatus( - c: Context, + c: Context<MiddlewareKeyVariables>, params: BuildStatusParams, apikey: Database['public']['Tables']['apikeys']['Row'], ): Promise<Response> { @@ - const supabase = supabaseApikey(c, apikey.key) + const apikeyString = apikey.key ?? c.get('capgkey') ?? null + if (!apikeyString) + throw simpleError('unauthorized', 'Missing API key') + const supabase = supabaseApikey(c, apikeyString)Also applies to: 33-40
supabase/tests/33_test_rbac_phase1.sql-1-7 (1)
1-7: MakeCREATE EXTENSIONidempotent to avoid flaky test runs.
CREATE EXTENSION "basejump-supabase_test_helpers";will error if the extension is already installed.Proposed fix
-CREATE EXTENSION "basejump-supabase_test_helpers"; +CREATE EXTENSION IF NOT EXISTS "basejump-supabase_test_helpers";supabase/functions/_backend/public/organization/delete.ts-1-24 (1)
1-24: Create a dedicatedorg.deletepermission instead of reusingorg.update_settingsfor deletion.The
org.update_settingspermission is semantically incorrect for organization deletion. It should only handle settings updates. Other resources follow the established pattern with dedicated delete permissions (app.delete,bundle.delete,channel.delete), but org deletion is missing this. Using an overly broad permission creates a privilege escalation risk: iforg.update_settingsis granted more widely in the future, deletion rights would inadvertently expand. Addorg.deleteto the permissions table and use it here instead.supabase/functions/_backend/private/groups.ts-9-9 (1)
9-9: UsecreateHonofrom utils/hono.ts per coding guidelines.Per the coding guidelines, Hono app initialization should use
createHonofromutils/hono.tsrather than instantiating directly.♻️ Use createHono helper
-import { Hono } from 'hono/tiny' -import { middlewareAuth, useCors } from '../utils/hono.ts' +import { createHono, middlewareAuth, useCors } from '../utils/hono.ts' -export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono()Based on learnings: "Use
createHonofromutils/hono.tsfor all Hono framework application initialization and routing"
🟡 Minor comments (9)
messages/zh-cn.json-1454-1463 (1)
1454-1463: Translations look OK; consider terminology consistency for role names.Keys/format look fine. Minor: double-check whether “计费经理 / 应用阅读者” matches your existing zh-CN terminology elsewhere (some products prefer “计费管理员 / 应用读者”).
messages/ja.json-1454-1463 (1)
1454-1463: Consider adjusting JP wording for “role-app-reader”.Line 1457 uses
アプリ読者, which can read a bit unnatural in UI. Consider閲覧者(common in admin UIs) orリーダーfor consistency with other locales.src/components/dashboard/DeploymentBanner.vue-28-34 (1)
28-34: Don’t requireuserRolefor an RBAC-based permission gate (and update the doc comment)
hasAdminPermissionnow hinges onhasPermission('channel.promote_bundle', ...), but it still returnsfalsewhenuserRole.valueis unset (Line 98-99). This can hide the banner for authorized users if role loading fails/returns null. Also the “admin/super_admin roles” comment is no longer accurate.Proposed diff
const hasAdminPermission = computedAsync(async () => { - if (!props.appId || !userRole.value) + if (!props.appId) return false return await hasPermission('channel.promote_bundle', { appId: props.appId }) }, false)Also applies to: 97-101
src/components/tables/ChannelTable.vue-54-65 (1)
54-65: Potential UX glitch: permission refs default tofalseand can block the auto “add channel” flowBecause
canUpdateChannelstarts asfalse, thecount === 0path that callsshowAddModal()can race and showno-permissionfor users who actually have permission (untilcomputedAsyncresolves). Consider deferring that auto-open until the permission check is finished, or separating “unknown yet” from “false”.Also applies to: 286-287, 302-306
messages/en.json-1504-1506 (1)
1504-1506: Confirm the confirmation text matches the actual scope being removed.
"remove-role-confirm": "Do you want to remove this user's access to the app?"reads app-specific, but it’s under generic role assignment management. If this confirm is also used for org/channel scope removals, the text will be misleading.supabase/functions/_backend/private/groups.ts-14-14 (1)
14-14: Fix French comments to English.The pipeline is failing due to French comments. Replace French text with English throughout the file.
🔧 Fix French comments
-// GET /private/groups/:org_id - Liste des groupes d'un org +// GET /private/groups/:org_id - List groups for an orgLine 42:
- // Récupérer les groupes + // Fetch groupsLine 63:
-// POST /private/groups/:org_id - Créer un groupe +// POST /private/groups/:org_id - Create a groupLine 98:
- // Créer le groupe + // Create the groupLine 118:
-// PUT /private/groups/:group_id - Modifier un groupe +// PUT /private/groups/:group_id - Update a groupAnd similar changes for all other French comments (lines 26, 82, 133, 148, 164, 182, 194, 209, 225, 238, 250, 261, 277, 297, 316, 327, 343, 359, 378, 391, 402, 418).
src/components/tables/AccessTable.vue-254-266 (1)
254-266: Comment typo flagged by pipeline: "fonction" should be "function".The pipeline flagged this French comment. Consider using English comments for consistency.
🔧 Suggested fix
-// Filtrer les éléments en fonction de la recherche +// Filter elements based on searchsupabase/functions/_backend/private/role_bindings.ts-75-78 (1)
75-78: Handle malformed JSON input gracefully.
c.req.json()throws if the request body is not valid JSON. Wrap it in a try-catch to return a 400 response for malformed input.Suggested fix
app.post('/', async (c: Context<MiddlewareKeyVariables>) => { const userId = c.get('auth')?.userId - const body = await c.req.json() + let body + try { + body = await c.req.json() + } + catch { + return c.json({ error: 'Invalid JSON body' }, 400) + }supabase/functions/_backend/public/organization/put.ts-27-30 (1)
27-30: Inconsistent error response for permission failure.Permission failures typically return 401 (Unauthorized) or 403 (Forbidden) status codes. This uses
simpleErrorwhich returns 400. Compare withapp/put.tswhich usesquickError(401, ...)for the same type of check.🐛 Suggested fix for consistency
// Auth context is already set by middlewareKey if (!(await checkPermission(c, 'org.update_settings', { orgId: body.orgId }))) { - throw simpleError('cannot_access_organization', 'You can\'t access this organization', { orgId: body.orgId }) + throw quickError(401, 'cannot_access_organization', 'You can\'t access this organization', { orgId: body.orgId }) }
🧹 Nitpick comments (24)
supabase/functions/_backend/public/device/get.ts (1)
45-55: RBAC migration looks correct.The permission check
'app.read_devices'with{ appId: body.app_id }scope is appropriate for this endpoint. The defensive ordering (validation before authorization) is good practice.As per coding guidelines, consider typing the context parameter more specifically for better type safety:
💡 Optional: Use typed Context
-export async function get(c: Context, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']): Promise<Response> { +export async function get(c: Context<MiddlewareKeyVariables>, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']): Promise<Response> {This would require importing
MiddlewareKeyVariablesfrom../../utils/hono.ts.supabase/functions/_backend/utils/postgres_schema.ts (1)
142-152: Consider constrainingrolesmore tightly (enum + uniqueness).
scope_type: text+name: textwith no uniqueness/indexing makes it easy to create ambiguous roles (same name in same scope). Also, ifdefaultRandom()is used, ensure the DB hasgen_random_uuid()available. As per learnings, keeping schema here is good—please ensure migrations match it.Proposed direction (sketch)
- Add
pgEnumforscope_type(and reuse it inrole_bindings.scope_type)- Add a unique constraint like
(scope_type, name)- Add an index on
priority_rankif it’s used for ordering- Verify
gen_random_uuid()support in your Supabase/Postgres setupsupabase/functions/_backend/private/download_link.ts (1)
7-7: Avoid double-auth and keep a single source of truth for user identity.Now that
middlewareAuthsetsc.get('auth'), this handler can drift ifsupabase.auth.getUser()ever disagrees (or if you later optimize middleware). Consider derivinguserIdfromc.get('auth')here (and only using Supabase client calls for data), so the RBAC decision + URL generation use the same identity.Also applies to: 38-40
supabase/functions/_backend/utils/hono.ts (1)
40-42: New RBAC context variables: ensure they’re set (or remove).
rbacEnabled/resolvedOrgIdare added toMiddlewareKeyVariables, but nothing here populates them—please ensure the intended middleware sets them, otherwise they’ll silently stayundefined.tests/rbac-permissions.test.ts (1)
179-215: Considerit.concurrent()for the pure read-only mapping loop.The permission→legacy mapping checks (Line 179+) are read-only and can safely be
it.concurrent()to align with the repo’s parallel testing guidance. Based on learnings.src/components/dashboard/AppSetting.vue (1)
42-53: Harden computed permission checks against thrown errors/timeouts.If
hasPermission(...)can throw (network/RPC),computedAsyncwill reject; consider a local try/catch and default tofalse.Proposed fix
const canUpdateSettings = computedAsync(async () => { if (!appRef.value) return false - return await hasPermission('app.update_settings', { appId: props.appId }) + try { + return await hasPermission('app.update_settings', { appId: props.appId }) + } + catch { + return false + } }, false) const canDeleteApp = computedAsync(async () => { if (!appRef.value) return false - return await hasPermission('app.delete', { appId: props.appId }) + try { + return await hasPermission('app.delete', { appId: props.appId }) + } + catch { + return false + } }, false)src/stores/organization.ts (2)
30-75: Tighten types for i18n helper + avoid stringly-typed map key access.
getRbacRoleI18nKey(role: string)(Line 73) defeats the point ofRbacRoleNameand can mask typos at callsites. Preferrole: RbacRoleName(orrole: stringbut with a type-guard).Proposed diff
-export function getRbacRoleI18nKey(role: string): string | undefined { +export function getRbacRoleI18nKey(role: RbacRoleName): string | undefined { return RBAC_ORG_ROLE_I18N_KEYS[role] }
299-405:hasPermissionsInRole()legacy path is brittle (no legacy hierarchy); consider “minimum right” semantics.Legacy fallback currently does strict equality (Line 404) against mapped legacy roles, so callers must remember to always include higher roles in
requiredRoles(e.g. pass bothorg_adminandorg_super_admin). That’s easy to get wrong and will silently deny legitimate access.If the legacy model is “minimum right”, consider a simple rank table (
read < upload < write < admin < super_admin, plusownerat org scope) and checkrank(currentRole) >= rank(requiredMinRole).supabase/functions/_backend/utils/rbac.ts (1)
339-366: Avoid N DB connections incheckPermissions/checkAnyPermission.Both helpers call
checkPermissionin a loop (Lines 344-347, 360-364), which opens/closes a PG client per permission. For endpoints checking multiple permissions, this is a big overhead. Prefer opening one client and reusing viacheckPermissionPg.supabase/functions/_backend/private/delete_failed_version.ts (1)
31-34: Permission check migration looks good; consider returning 403 (not 401) for “authenticated but forbidden”.The RBAC check itself (Line 32) is the right direction. For consistency, the failure path currently returns 401 (Line 33) which usually means “not authenticated”, while this is “forbidden”.
supabase/functions/_backend/public/build/logs.ts (1)
5-5: Align Hono context typing with RBAC helper expectations
checkPermissionis typed asContext<MiddlewareKeyVariables>(persupabase/functions/_backend/utils/rbac.ts), but this function acceptsContextwithout variables typing, which can drift from the repo convention and reduce type-safety aroundc.get('auth')/c.get('requestId'). As per coding guidelines, consider updating the signature (and imports) to useContext<MiddlewareKeyVariables>here.Proposed diff
-import type { Context } from 'hono' +import type { Context } from 'hono' +import type { MiddlewareKeyVariables } from '../../utils/hono.ts' export async function streamBuildLogs( - c: Context, + c: Context<MiddlewareKeyVariables>, jobId: string, appId: string, apikey: Database['public']['Tables']['apikeys']['Row'], ): Promise<Response> {Also applies to: 74-84
messages/en.json (1)
1478-1524: Avoid generic + mixed-case i18n keys (collision risk / hard-to-debug overrides).The newly added RBAC strings include very generic keys (
"access","assign","remove","user","group", etc.) and also both"access"and"Access"(Line 1482 vs Line 1516). Even if JSON parsing “works”, these keys are prone to accidental reuse across features and subtle key mismatches in code.Recommended: namespace these keys (e.g.,
rbac-access,rbac-assign,rbac-user, …) and pick one consistent casing.src/layouts/app.vue (1)
10-25: Avoid(… as any)?.use_new_rbac; prefer typed store + stable filter key.Two concerns:
(organizationStore.currentOrganization as any)?.use_new_rbaccan silently break if the field name/type changes.baseAppTabs.filter(t => t.label !== 'access')is brittle if labels change (or get localized); filtering on a stable route key is safer.supabase/functions/_backend/public/channel/get.ts (1)
15-55: Consider updating helper function context types for consistency.The internal helper functions
getAllandgetOnestill use the plainContexttype while the exportedgetfunction usesContext<MiddlewareKeyVariables>. While this works since these helpers are only called fromget, updating them toContext<MiddlewareKeyVariables>would improve type consistency and enable better IDE support for context variables.♻️ Optional consistency improvement
-async function getAll(c: Context, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']) { +async function getAll(c: Context<MiddlewareKeyVariables>, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']) {-async function getOne(c: Context, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']) { +async function getOne(c: Context<MiddlewareKeyVariables>, body: GetDevice, apikey: Database['public']['Tables']['apikeys']['Row']) {supabase/functions/_backend/public/channel/post.ts (1)
26-40: Optional: Update helper function context type.Similar to
get.ts, thefindVersionhelper uses plainContextwhile the exported function usesContext<MiddlewareKeyVariables>. Consider updating for consistency.supabase/functions/_backend/public/bundle/create.ts (1)
24-132: Large commented-out code block.This ~100 line block of commented code for URL verification could be removed or moved to a separate utility if the feature is not planned for near-term re-enablement. Keeping it adds maintenance burden.
src/components/dashboard/AppAccess.vue (2)
176-202: N+1 query pattern when enriching role bindings.Each binding triggers a separate query to fetch user email or group name. For organizations with many bindings, this could cause performance issues. Consider batching the user/group lookups.
♻️ Suggested optimization
// Batch fetch all user emails const userIds = appBindings .filter((b: RoleBinding) => b.principal_type === 'user') .map((b: RoleBinding) => b.principal_id) const { data: usersData } = await supabase .from('users') .select('id, email') .in('id', userIds) const userMap = new Map(usersData?.map(u => [u.id, u.email]) || []) // Then map bindings using the lookup map
361-385: Duplicate initialization logic between watch and onMounted.The same sequence of
fetchAppDetails,checkRbacEnabled, and conditional fetches appears in both the watcher andonMounted. Extract to a shared function for DRY.♻️ Suggested refactor
+async function initializeData() { + await fetchAppDetails() + await checkRbacEnabled() + if (useNewRbac.value) { + await Promise.all([ + fetchAppRoleBindings(), + fetchAvailableAppRoles(), + fetchAvailableMembers(), + fetchAvailableGroups(), + ]) + } +} -watch(() => props.appId, async () => { - await fetchAppDetails() - await checkRbacEnabled() - if (useNewRbac.value) { - await Promise.all([ - fetchAppRoleBindings(), - fetchAvailableAppRoles(), - fetchAvailableMembers(), - fetchAvailableGroups(), - ]) - } -}) +watch(() => props.appId, initializeData) -onMounted(async () => { - await fetchAppDetails() - await checkRbacEnabled() - if (useNewRbac.value) { - await Promise.all([ - fetchAppRoleBindings(), - fetchAvailableAppRoles(), - fetchAvailableMembers(), - fetchAvailableGroups(), - ]) - } -}) +onMounted(initializeData)supabase/functions/_backend/private/role_bindings.ts (2)
4-12: UsecreateHonofromutils/hono.tsfor app initialization.Per coding guidelines, all Hono application initialization should use
createHonofromutils/hono.tsinstead of directly instantiatingnew Hono(). This ensures consistent middleware setup across all backend endpoints.Suggested fix
-import { Hono } from 'hono/tiny' -import { middlewareAuth, useCors } from '../utils/hono.ts' +import { createHono, middlewareAuth, useCors } from '../utils/hono.ts' ... -export const app = new Hono<MiddlewareKeyVariables>() - -app.use('/', useCors) -app.use('/', middlewareAuth) +export const app = createHono() +app.use('*', middlewareAuth)Based on learnings,
createHonofromutils/hono.tsshould be used for all Hono framework application initialization and routing.
68-71: Use structured logging withcloudloginstead ofconsole.error.Per coding guidelines, backend logging should use
cloudlog({ requestId: c.get('requestId'), message: '...' })for structured logging.Suggested fix
+import { cloudlog } from '../utils/cloudflare.ts' ... catch (error) { - console.error('Error fetching role bindings:', error) + cloudlog({ requestId: c.get('requestId'), message: 'Error fetching role bindings', error }) return c.json({ error: 'Internal server error' }, 500) }src/services/permissions.ts (1)
169-222: Consider batch RPC optimization for future iterations.These helper functions execute N parallel RPC calls. For typical use cases with 2-5 permissions, this is acceptable. If permission checks grow, consider adding a batch RPC endpoint (e.g.,
rbac_check_permissions_batch) to reduce round trips.supabase/functions/_backend/utils/hono_middleware.ts (2)
65-93: Consider consolidating DB queries to reduce connection overhead.The middleware opens two separate database connections: one for resolving
app_id → orgIdand another for checkingrbac_is_enabled_for_org. These could be combined into a single connection/query whenorgIdis resolved fromapp_id.♻️ Suggested consolidation
if (appId) { let pgClient try { pgClient = getPgClient(c, true) const drizzleClient = getDrizzleClient(pgClient) - const appResult = await drizzleClient - .select({ ownerOrg: schema.apps.owner_org }) - .from(schema.apps) - .where(eq(schema.apps.app_id, appId)) - .limit(1) - if (appResult.length > 0 && appResult[0].ownerOrg) { - orgId = appResult[0].ownerOrg + // Resolve org and check RBAC in one query + const result = await drizzleClient.execute( + sql`SELECT a.owner_org, public.rbac_is_enabled_for_org(a.owner_org) as rbac_enabled + FROM public.apps a WHERE a.app_id = ${appId} LIMIT 1`, + ) + if (result.rows.length > 0) { + orgId = (result.rows[0] as any).owner_org + c.set('resolvedOrgId', orgId) + c.set('rbacEnabled', (result.rows[0] as any).rbac_enabled === true) + // Skip the second DB call below } } // ... rest of error handling } } - - // If we have an orgId, check if RBAC is enabled - if (orgId) { + // Only check RBAC separately if orgId was provided via resolver (not from app_id) + if (orgId && c.get('rbacEnabled') === undefined) {
424-432: Redundant null check onsubkey.The
if (subkey)check on line 424 is redundant since the function returns early on lines 416-418 ifsubkeyis null. The code works correctly, but the check adds unnecessary cognitive load.♻️ Simplify by removing redundant check
if (!subkey) { cloudlog({ requestId: c.get('requestId'), message: 'Invalid subkey', subkey_id }) return quickError(401, 'invalid_subkey', 'Invalid subkey') } if (subkey?.limited_to_apps && subkey?.limited_to_apps.length === 0 && subkey?.limited_to_orgs && subkey?.limited_to_orgs.length === 0) { cloudlog({ requestId: c.get('requestId'), message: 'Invalid subkey, no limited apps or orgs', subkey }) return quickError(401, 'invalid_subkey', 'Invalid subkey, no limited apps or orgs') } - if (subkey) { - c.set('subkey', subkey) - // Override auth context with subkey for RBAC - c.set('auth', { - userId: apikey!.user_id, - authType: 'apikey', - apikey: subkey, - } as AuthInfo) - } + c.set('subkey', subkey) + // Override auth context with subkey for RBAC + c.set('auth', { + userId: apikey!.user_id, + authType: 'apikey', + apikey: subkey, + } as AuthInfo) }src/constants/organizationTabs.ts (1)
8-10: Duplicate icon import.
IconSecurity(line 8) andIconShield(line 9) both import from~icons/heroicons/shield-check. Either reuseIconSecurityfor the role-assignments tab, or rename one import to clarify the distinction.♻️ Remove duplicate import
import IconInfo from '~icons/heroicons/information-circle' import IconSecurity from '~icons/heroicons/shield-check' -import IconShield from '~icons/heroicons/shield-check' import IconUserGroup from '~icons/heroicons/user-group' import IconUsers from '~icons/heroicons/users' export const organizationTabs: Tab[] = [ { label: 'general', key: '/settings/organization', icon: IconInfo }, { label: 'members', key: '/settings/organization/members', icon: IconUsers }, // Security tab is added dynamically in settings.vue for super_admins only { label: 'security', key: '/settings/organization/security', icon: IconSecurity }, { label: 'audit-logs', key: '/settings/organization/auditlogs', icon: IconAudit }, { label: 'groups', key: '/settings/organization/groups', icon: IconUserGroup }, - { label: 'role-assignments', key: '/settings/organization/role-assignments', icon: IconShield }, + { label: 'role-assignments', key: '/settings/organization/role-assignments', icon: IconSecurity },
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/pages/settings/organization/Members.vue`:
- Around line 732-764: The updateRbacMemberRole function currently only handles
the error object from supabase.rpc and the success case when data === 'OK',
leaving other RPC responses silent; update it to treat non-OK data as failures
by checking if data !== 'OK' and then surface the problem (e.g., call
handleRbacRoleUpdateError with an object containing a message derived from data
or call toast.error with `${t('cannot-change-permission')}: ${String(data ??
t('unexpected-response'))}`), log the unexpected response for debugging, and
avoid proceeding to reloadData unless data === 'OK'; reference
updateRbacMemberRole, supabase.rpc('update_org_member_role'),
handleRbacRoleUpdateError, toast, and reloadData to locate where to add this
logic.
- Around line 82-94: The mapRbacRoleToLegacy function currently maps 'org_admin'
to 'write' causing under-granted invites; update mapRbacRoleToLegacy so that the
'org_admin' case returns 'admin' instead of 'write', preserving the other cases
and default cast to Database['public']['Enums']['user_min_right'] to keep
backward compatibility.
🧹 Nitpick comments (1)
src/pages/settings/organization/Members.vue (1)
34-64: Consider centralizing RBAC activation behind the feature flag.
IfVITE_FEATURE_RBAC_SYSTEMis intended as a global kill‑switch, RBAC branches currently key offuseNewRbaconly. A single computed guard keeps permission options and data loading consistent.♻️ Suggested refactor
const rbacSystemEnabled = import.meta.env.VITE_FEATURE_RBAC_SYSTEM === 'true' +const isRbacActive = computed(() => rbacSystemEnabled && useNewRbac.value) - if (useNewRbac.value) { + if (isRbacActive.value) { // Options RBAC } else { // Options legacy } - if (useNewRbac.value && currentOrganization.value) { + if (isRbacActive.value && currentOrganization.value) { // Utiliser la RPC RBAC pour récupérer les membres }Also applies to: 146-175, 257-289
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/private-error-cases.test.ts (3)
127-185: Align test titles with expected status codes; consider de-duping identical cases.
The upload_link block has mismatched “500” titles while asserting 400/404, and two cases are effectively identical. Consider renaming and/or varying inputs to keep intent clear.✅ Suggested title updates (example)
- it('should return 500 when user not found', async () => { + it('should return 400 when app access is denied (unknown app)', async () => { ... - it('should return 500 when app not found', async () => { + it('should return 404 when version is not found for app', async () => { ... - it('should return 500 when version already exists', async () => { + it('should return 404 when version is not found (unknown version)', async () => {
237-264: Delete-failed-version tests are duplicated and the first title is misleading.
Both tests send the same request and assert the same outcome; also the first title says “500” while asserting 401. Consider merging or differentiating inputs.✅ Suggested title update (minimal)
- it('should return 500 when user not found', async () => { + it('should return 401 when user is not authorized', async () => {
367-386: Restore org mutations even when assertions fail.
If the request or expectations throw, the customer_id restore never runs, which can cascade into later tests in this file. Wrap the mutation in a try/finally to guarantee cleanup.🔧 Suggested fix
- await getSupabaseClient().from('orgs').update({ customer_id: null }).eq('id', testOrgId) - - const response = await fetch(getEndpointUrl('/private/set_org_email'), { - method: 'POST', - headers, - body: JSON.stringify({ - org_id: testOrgId, - email: 'test@example.com', - }), - }) - - expect(response.status).toBe(400) - const data = await response.json() as { error: string } - expect(data.error).toBe('org_does_not_have_customer') - - // Restore customer_id for other tests - await getSupabaseClient().from('orgs').update({ customer_id: testCustomerId }).eq('id', testOrgId) + await getSupabaseClient().from('orgs').update({ customer_id: null }).eq('id', testOrgId) + try { + const response = await fetch(getEndpointUrl('/private/set_org_email'), { + method: 'POST', + headers, + body: JSON.stringify({ + org_id: testOrgId, + email: 'test@example.com', + }), + }) + + expect(response.status).toBe(400) + const data = await response.json() as { error: string } + expect(data.error).toBe('org_does_not_have_customer') + } + finally { + await getSupabaseClient().from('orgs').update({ customer_id: testCustomerId }).eq('id', testOrgId) + }
There was a problem hiding this comment.
Actionable comments posted: 4
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/utils/postgres_schema.ts (1)
96-105: Adduse_new_rbacboolean column to the Drizzleorgsschema.The
use_new_rbaccolumn exists in the database migrations withboolean NOT NULL DEFAULT false(see20251222140030_rbac_system.sql:534), but is missing from the Drizzle schema definition. This drift will cause runtime errors when backend code attempts to access this field. The column is actively used in RLS functions and triggers.🛠️ Suggested fix
export const orgs = pgTable('orgs', { id: uuid('id').primaryKey().notNull(), created_by: uuid('created_by').notNull(), logo: text('logo'), name: text('name').notNull(), management_email: text('management_email').notNull(), customer_id: text('customer_id'), require_apikey_expiration: boolean('require_apikey_expiration').notNull().default(false), max_apikey_expiration_days: integer('max_apikey_expiration_days'), + use_new_rbac: boolean('use_new_rbac').notNull().default(false), })
🤖 Fix all issues with AI agents
In `@src/pages/settings/organization/Members.vue`:
- Around line 808-836: In updateRbacInviteRole, add handling for non-'OK' RPC
responses: after the error check, if data exists but !== 'OK', log the response,
call handleRbacInviteUpdateError (or a new helper) with the returned value, and
show a user-facing toast.error with a localized message that includes or maps
the returned string; keep existing success path (toast.success + reloadData) for
'OK' only. Ensure you reference the RPC result variables (data, error) and the
existing helpers (handleRbacInviteUpdateError, toast, reloadData) when
implementing this branch.
- Around line 261-298: The code misclassifies bigint-string org_user_id because
Number.isFinite(orgUserId) returns false for strings; in the RBAC branch inside
checkRbacEnabled() update the hasOrgUserInvite logic to check for null/undefined
(e.g., org_user_id != null && org_user_id !== '') instead of Number.isFinite,
and when setting aid convert the string id to a number or bigint as appropriate
(e.g., aid: hasOrgUserInvite ? Number(orgUserId) : -1) so members.value mapping
(fields org_user_id, hasOrgUserInvite, aid) correctly preserves legacy org_users
invites.
In `@supabase/functions/_backend/private/accept_invitation.ts`:
- Around line 187-249: The current flow inserts into org_users then later
inserts into role_bindings (in the useRbacInvite branch), which can leave a user
with legacy org membership if the role_bindings insert fails; modify
accept_invitation flow to perform the org_users and role_bindings writes
atomically: either run both statements in a single DB transaction using
supabaseAdmin.rpc/pg transaction or, if a transaction is not feasible, add a
compensating delete that removes the org_users row when insertBindingError
occurs (use the same user.user.id and invitation.org_id to identify the row) and
return the original quickError; also ensure you check and handle errors from the
compensating delete (and log/return a 500 if that delete fails) so there is no
partial membership state (refer to the org_users insert, deleteBindingError,
insertBindingError, and role_bindings insert logic).
In `@supabase/functions/_backend/public/organization/members/post.ts`:
- Around line 26-27: The ESLint error arises because legacyInviteRoles is only
used for typing and never at runtime; update the code by either renaming
legacyInviteRoles to _legacyInviteRoles to mark it as intentionally unused at
runtime or remove the constant and inline its type where needed (e.g., replace
uses of typeof legacyInviteRoles[number] with an explicit union type). Ensure
rbacInviteRoles remains unchanged for runtime use and update any type references
to use the new name or inlined union so type derivation still works.
♻️ Duplicate comments (5)
src/types/supabase.types.ts (1)
3739-3746: Access control for user‑scoped RPCs still needs hardening.
These helper RPCs accept arbitraryp_user_idand were previously flagged for exposure risk. Please ensure grants or caller checks are restricted.supabase/functions/_backend/utils/postgres_schema.ts (1)
206-216: Fixrole_bindings.channel_idtype mismatch in Drizzle schema.The database migration defines
channel_idasuuidwith a foreign key tochannels.rbac_id, but the Drizzle schema incorrectly declares it asbigint. This type mismatch will cause Drizzle queries to fail or return incorrect results.Apply this fix
- channel_id: bigint('channel_id', { mode: 'number' }), + channel_id: uuid('channel_id'),supabase/functions/_backend/utils/supabase.types.ts (2)
454-454: Regenerate types to capture build_logs foreign key relationships.The
build_logstable hasorg_id_fkeyanduser_id_fkeyconstraints in the database schema, butRelationships: []is empty here. Runbun typesto regenerate after ensuring the schema is correctly defined.
2585-2614: Security: The 5-parametercheck_min_rights(user_id)overload remains granted to public roles.This overload accepting an explicit
user_idparameter enables cross-user permission probing. The fix requires a migration to revoke execute permissions on the 5-parameter overload fromanonandauthenticatedroles, keeping only the 4-parameter wrapper that usesauth.uid()internally.src/pages/settings/organization/Members.vue (1)
789-806: Handle non‑OK responses fromupdate_org_member_role.
This was already flagged previously and still appears unresolved.
🧹 Nitpick comments (3)
supabase/migrations/20260120165047_rbac_invites.sql (3)
688-692: Empty conditional block with no-op.This block contains only
NULL;which does nothing. If the transition from admin/super_admin to read/upload/write requires no additional handling beyond what's already done, consider removing the block entirely or adding a comment explaining why no action is needed.Consider removing or documenting
- -- Handle transition from admin/super_admin to read/upload/write: - IF OLD.user_right IN (public.rbac_right_super_admin(), public.rbac_right_admin()) - AND NEW.user_right IN (public.rbac_right_read(), public.rbac_right_upload(), public.rbac_right_write()) THEN - NULL; - END IF; + -- NOTE: Transition from admin/super_admin to read/upload/write is fully handled + -- by the delete/insert logic above (old org binding deleted, new org_member + app bindings created)
1143-1146: Consider usingGRANT EXECUTEfor clarity.
GRANT ALLon functions is equivalent toGRANT EXECUTE(EXECUTE is the only privilege for functions), but explicitly usingGRANT EXECUTEis more conventional and clearer about intent.Optional style fix
ALTER FUNCTION public.get_orgs_v7() OWNER TO postgres; -GRANT ALL ON FUNCTION public.get_orgs_v7() TO anon; -GRANT ALL ON FUNCTION public.get_orgs_v7() TO authenticated; -GRANT ALL ON FUNCTION public.get_orgs_v7() TO service_role; +GRANT EXECUTE ON FUNCTION public.get_orgs_v7() TO anon; +GRANT EXECUTE ON FUNCTION public.get_orgs_v7() TO authenticated; +GRANT EXECUTE ON FUNCTION public.get_orgs_v7() TO service_role;
64-154: Clarify the function's scope in its documentation.This function only handles inviting existing users to an org. For non-existing users (lines 133–151), it returns error codes but never creates a
tmp_usersentry. New-user invites are created by the separateinvite_new_user_to_orgEdge Function, which populatestmp_userswith both legacy and RBAC role information.Update the function comment to make this distinction explicit and reference the companion mechanism in the application layer.
supabase/functions/_backend/public/organization/members/post.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/public/organization/members/post.ts`:
- Around line 26-35: The build fails because the constant rbacInviteRoles
referenced by the RbacInviteRole type and runtime checks is missing; restore or
reintroduce a definitive rbacInviteRoles array of allowed string roles and use
it to define the RbacInviteRole type and any runtime validations. Specifically,
add a top-level const rbacInviteRoles = [...] containing 'org_member',
'org_admin', 'org_super_admin' (and any other valid RBAC roles), keep
RbacInviteRole as (typeof rbacInviteRoles)[number], and ensure legacyToRbac maps
to those exact values so runtime checks that reference rbacInviteRoles succeed.
🧹 Nitpick comments (1)
supabase/functions/_backend/public/organization/members/post.ts (1)
84-94: VariablelegacyInviteTypeis redeclared.Line 85 redeclares
legacyInviteTypewhich was already declared at line 63. While this works (due to block scoping), it's redundant—the outer declaration can be reused here since this branch only executes when!useNewRbac, where the legacy type is applicable.♻️ Suggested cleanup
else { - const legacyInviteType = body.invite_type as LegacyInviteRole const inviteType = `invite_${legacyInviteType}` as Database['public']['Enums']['user_min_right'] const result = await supabase.rpc('invite_user_to_org', {
supabase/functions/_backend/public/organization/members/post.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260120165047_rbac_invites.sql`:
- Around line 253-261: The permission checks inside the IF/ELSE block use
public.rbac_check_permission_direct but omit the api_key_text parameter; update
both calls in this snippet (the ones checking
public.rbac_perm_org_update_user_roles() and public.rbac_perm_org_invite_user())
to pass the api_key_text argument (the same param used elsewhere, e.g., in
update_org_invite_role_rbac) so the signature matches
public.rbac_check_permission_direct(..., p_org_id, NULL, NULL, api_key_text).
- Around line 740-742: The permission check calling
public.rbac_check_permission_direct with rbac_perm_org_read(), auth.uid(),
p_org_id is missing the api_key_text parameter; update the call to include the
sixth argument (e.g., pass NULL or the appropriate api_key_text variable) so it
matches the expected signature of rbac_check_permission_direct and stays
consistent with other usages.
- Around line 192-200: The permission checks call rbac_check_permission_direct
without the api_key_text parameter; copy the same api_key_text declaration and
retrieval logic used in invite_user_to_org_rbac (declare api_key_text text and
set it the same way) and pass api_key_text as the final argument to both
rbac_check_permission_direct calls in this block so API-key authenticated
requests are evaluated consistently.
🧹 Nitpick comments (1)
supabase/migrations/20260120165047_rbac_invites.sql (1)
688-692: Dead code: empty IF block with NULL statement.This conditional block contains only a
NULLstatement, which is a no-op. If there's intended logic for the admin-to-member transition, it's missing. Otherwise, this block should be removed.Proposed fix (remove dead code)
- -- Handle transition from admin/super_admin to read/upload/write: - IF OLD.user_right IN (public.rbac_right_super_admin(), public.rbac_right_admin()) - AND NEW.user_right IN (public.rbac_right_read(), public.rbac_right_upload(), public.rbac_right_write()) THEN - NULL; - END IF; - -- Handle transition from read/upload/write to admin/super_admin:
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260120165047_rbac_invites.sql`:
- Around line 984-1031: The query currently redacts fields but still returns org
rows when enforcing_2fa=true and "2fa_has_access" is false; update the selection
logic to block those orgs entirely by filtering on the two_fa_access CTE. After
joining/including two_fa_access (alias tfa) in the main query, add a condition
to exclude rows where tfa.enforcing_2fa = true AND tfa."2fa_has_access" = false
(or simply require tfa."2fa_has_access" = true), so orgs enforcing 2FA are not
returned until public.has_2fa_enabled(userid) is true.
♻️ Duplicate comments (3)
supabase/migrations/20260120165047_rbac_invites.sql (3)
212-218: Add missingapi_key_textparameter in permission checks (API key auth).
Line 212-218 still callsrbac_check_permission_directwithoutapi_key_text, which breaks API-key authenticated flows.
279-285: Same API-key parameter omission in permission checks.
Line 279-285 is missingapi_key_textinrbac_check_permission_direct, same issue as earlier.
774-776: Permission check missing API-key argument.
Line 774-776 callsrbac_check_permission_directwithoutapi_key_text, same issue as earlier.
🧹 Nitpick comments (1)
supabase/migrations/20260120165047_rbac_invites.sql (1)
516-523: Create helper functions for channel-level developer and uploader roles.
Lines 520-521 use hardcoded strings'channel_developer'and'channel_uploader'while other roles use helper functions. Helper functions exist for their app-level equivalents (rbac_role_app_developer(),rbac_role_app_uploader()), so createrbac_role_channel_developer()andrbac_role_channel_uploader()functions to align with the established pattern.
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/public/organization/members/post.ts (1)
105-105: PII in logs: email address is logged.Logging
body.emailmay create compliance concerns (GDPR/CCPA). Consider logging a hashed/masked identifier or omitting PII from structured logs.🔧 Proposed fix — remove email from log
- cloudlog({ requestId: c.get('requestId'), message: 'User invited to organization', data: { email: body.email, org_id: body.orgId } }) + cloudlog({ requestId: c.get('requestId'), message: 'User invited to organization', data: { org_id: body.orgId } })
🤖 Fix all issues with AI agents
In `@src/pages/settings/organization/Members.vue`:
- Around line 93-95: The isInviteMember function can throw when member.role is
undefined; update isInviteMember (OrganizationMemberRow) to defensively check
role before calling includes, e.g., treat missing/falsey role as non-matching by
using a guarded check like typeof member.role === 'string' or Array.isArray
depending on type, then call includes only when safe; ensure the logic still
returns true for member.is_invite or member.is_tmp and otherwise safely
evaluates the role check.
In `@supabase/functions/_backend/private/accept_invitation.ts`:
- Around line 187-207: The code allows accepting invitations for RBAC-enabled
orgs with invitation.rbac_role_name == null, causing legacy bindings; before
inserting into org_users in accept_invitation, enforce that if org.use_new_rbac
=== true (useRbacInvite logic) then rbacRoleName must be non-null and return
quickError (e.g., 'failed_to_accept_invitation' / 'Missing RBAC role name') if
it is missing; move the null-check for rbacRoleName earlier (before the insert
into org_users) and only allow insertion when rbacRoleName is present, so the
database trigger sync_org_user_to_role_binding will create RBAC bindings
correctly.
In `@supabase/functions/_backend/public/organization/members/post.ts`:
- Line 52: The unsafe cast passes possibly undefined c.get('capgkey') into
supabaseApikey; change the call to use the provided _apikey parameter (or a safe
fallback) instead of casting: call supabaseApikey(c, _apikey) or compute a
safeApiKey = c.get('capgkey') ?? _apikey and pass that to supabaseApikey, and
remove the redundant cast so supabaseApikey receives a proper string or handles
undefined itself; update usages of supabaseApikey and ensure the function
signature/behavior aligns with this fallback logic.
♻️ Duplicate comments (1)
src/pages/settings/organization/Members.vue (1)
808-812: Missing handler for non-OK responses fromupdate_org_member_role.If the RPC returns a value other than
'OK'(but no error), the response is silently ignored. The user receives no feedback about the operation status.🐛 Proposed fix
if (data === 'OK') { toast.success(t('permission-changed')) await reloadData() } + else if (data) { + console.warn('Unexpected RPC response:', data) + toast.warning(`${t('unexpected-response')}: ${String(data)}`) + } }
🧹 Nitpick comments (3)
supabase/functions/_backend/public/organization/members/post.ts (1)
86-89: Variable shadowing:legacyInviteTypeis redeclared.
legacyInviteTypeis already declared at line 66. While block scoping makes this valid, shadowing reduces readability. The outer declaration is sufficient since this branch only executes when!useNewRbac, where the legacy type is needed.♻️ Proposed fix — remove redundant declaration
else { - const legacyInviteType = body.invite_type as LegacyInviteRole const inviteType = `invite_${legacyInviteType}` as Database['public']['Enums']['user_min_right'] const result = await supabase.rpc('invite_user_to_org', {src/pages/settings/organization/Members.vue (1)
748-751: Consider clarifying theaid === 0check for RBAC compatibility.This check appears to be legacy logic. In RBAC mode,
aidis set to-1for pure RBAC members orNumber(orgUserId)for legacy invites. Theaid === 0case could theoretically occur iforgUserIdis"0", but this is unlikely. Consider adding a comment explaining this guard or consolidating it with the RBAC member detection logic.supabase/functions/_backend/private/accept_invitation.ts (1)
247-256: Consider using inviter/system forgranted_by.Line 255 sets
granted_byto the invited user, which can muddy audit trails. If the invitation stores an inviter (e.g.,invited_by) or you have a system principal, consider using that instead.
supabase/functions/_backend/public/organization/members/post.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@supabase/tests/38_test_get_orgs_v7_2fa_enforcement.sql`:
- Around line 244-262: Remove or change the duplicate Test 25 in the SQL file:
Test 24 and Test 25 currently run the exact same assertion against
public.get_orgs_v7(tests.get_supabase_uid('test_no_2fa_user_v7')) checking for
current_setting('test.org_with_2fa_v7')::uuid; either delete the redundant Test
25 block or replace it with a different scenario (e.g., use a different UID,
different org setting, or assert a different field) so it tests a distinct
behavior, and then decrement the test plan count by updating SELECT plan(26) to
SELECT plan(25).
In `@tests/hashed-apikey-rls.test.ts`:
- Line 27: Tests modify orgs.enforcing_2fa using the shared ORG_ID_RLS which can
race with other tests; instead, create and use a dedicated org/seed for this
file (e.g., a new constant ORG_ID_2FA_TEST) and update the beforeAll/afterAll
hooks and the variable originalEnforcing2fa to operate on that dedicated org;
ensure any seeding/cleanup mirrors existing seeding utilities used elsewhere so
the new org is created before tests and fully cleaned up after to avoid
cross-test interference.
🧹 Nitpick comments (2)
supabase/migrations/20260120165047_rbac_invites.sql (2)
523-529: Prefer centralized role constants for channel roles.
Hard-coded'channel_developer'/'channel_uploader'can drift; if helper functions exist (or can be added), use them for consistency.
591-663: Consider extracting user_right → role mapping into a helper.
The OLD/NEW CASE blocks duplicate logic; a shared function would reduce divergence risk.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/settings/organization/Members.vue`:
- Around line 925-927: The legacy branch in changeMemberPermission may call
member.role.includes('invite') and throw if member.role is undefined; always
call the defensive helper isInviteMember instead. Update changeMemberPermission
to determine isInvite via isInviteMember(member) regardless of useNewRbac.value
(keep useNewRbac only if still needed elsewhere), and then pass that boolean
into showPermModal; reference changeMemberPermission, isInviteMember,
useNewRbac, showPermModal and member.role when locating the change.
🧹 Nitpick comments (6)
supabase/functions/_backend/public/organization/members/post.ts (2)
40-52: Remove underscore prefix fromapikeyparameter since it's used.The
_apikeyparameter is actually used on line 52 (_apikey?.key), so the underscore prefix suggesting it's unused is misleading.♻️ Proposed fix
-export async function post(c: Context<MiddlewareKeyVariables>, bodyRaw: any, _apikey: Database['public']['Tables']['apikeys']['Row']) { +export async function post(c: Context<MiddlewareKeyVariables>, bodyRaw: any, apikey: Database['public']['Tables']['apikeys']['Row']) { const bodyParsed = inviteBodySchema.safeParse(bodyRaw) if (!bodyParsed.success) { throw simpleError('invalid_body', 'Invalid body', { error: bodyParsed.error }) } const body = bodyParsed.data // Auth context is already set by middlewareKey if (!(await checkPermission(c, 'org.invite_user', { orgId: body.orgId }))) { throw simpleError('cannot_access_organization', 'You can\'t access this organization', { orgId: body.orgId }) } - const supabase = supabaseApikey(c, _apikey?.key) + const supabase = supabaseApikey(c, apikey?.key)
87-97: Variable shadowing:legacyInviteTypeis already declared at line 66.The variable
legacyInviteTypeis re-declared inside the else block, shadowing the outer declaration. Remove the redundant declaration.♻️ Proposed fix
else { - const legacyInviteType = body.invite_type as LegacyInviteRole const inviteType = `invite_${legacyInviteType}` as Database['public']['Enums']['user_min_right'] const result = await supabase.rpc('invite_user_to_org', { email: body.email, org_id: body.orgId, invite_type: inviteType, }) data = result.data error = result.error }supabase/tests/38_test_get_orgs_v7_2fa_enforcement.sql (1)
119-129: Tests 4, 7-11, 14-15 are redundant given Test 3.Since Test 3 proves
COUNT(*) = 0(no row returned), all subsequent tests checking individual fields forNULLwill pass trivially—not because of redaction logic, but because theWHEREclause matches nothing.Consider either:
- Keeping only Test 3 and removing the redundant field checks, or
- Restructuring these tests to verify redaction on a separate org where the user has partial access (if such a scenario exists)
This is not a blocking issue since the tests do pass and correctly reflect the expected behavior.
supabase/functions/_backend/private/accept_invitation.ts (1)
248-260:granted_byis set to the newly created user—verify this is intentional.The
granted_byfield (line 256) referencesuser.user.id, meaning the user who accepted the invitation "granted themselves" the role. This may be intentional for self-service invitations, but typicallygranted_byreflects the inviter or system. Consider using a system/service account UUID or the inviter's ID from the invitation record if available.src/pages/settings/organization/Members.vue (2)
799-816: Inconsistent handling of non-OK responses compared toupdateRbacInviteRole.
updateRbacInviteRole(lines 849-854) properly handles unexpected RPC responses with logging and error toast, butupdateRbacMemberRolesilently ignores responses that are neither'OK'nor an error. For defensive coding consistency, add similar handling here.🔧 Suggested fix
if (data === 'OK') { toast.success(t('permission-changed')) await reloadData() + return + } + + if (data) { + const responseMessage = typeof data === 'string' ? data : JSON.stringify(data) + console.warn('Unexpected RBAC member update response:', responseMessage) + handleRbacRoleUpdateError({ message: responseMessage }) } }
1263-1267: Consider using a dialog identifier instead of matching translated titles.The teleport condition relies on matching translated strings (
t('select-user-perms')andt('select-user-role', 'Select a role')), which is fragile if translations change or differ across locales. Consider adding atypeoridfield todialogOptionsfor more robust matching.This follows existing patterns in the codebase, so it's not blocking — but worth considering for future maintainability.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@messages/de.json`:
- Around line 1267-1270: The German translation for the RBAC role hint uses
informal address; update the value of the "select-user-role-expanded" key to
formal address so it matches surrounding lines (replace "Wähle die zuzuweisende
RBAC-Rolle. Legacy-Rollen bleiben während der Migration sichtbar." with a
version starting "Wählen Sie …"). Ensure punctuation and capitalization remain
consistent with other keys like "select-role" and "select-user-role" and keep
the explanatory sentence about legacy roles unchanged except for the formal
pronoun.
In `@messages/it.json`:
- Around line 1515-1526: Change the value for the "zip-bundle" locale key from
"Pacchetto app Zip" to use standard uppercase "ZIP": update the "zip-bundle"
entry so it reads "Pacchetto app ZIP"; locate the "zip-bundle" key in the
messages/it.json bundle to make this one-word casing change.
In `@supabase/functions/_backend/utils/hono.ts`:
- Around line 119-136: Replace the inconsistent return with a thrown 401
Unauthorized error when JWT validation fails: in the block that calls
supabaseClientFn(...) and awaits supabase.auth.getUser(), change the flow that
currently does "return simpleError(...)" to "throw simpleError(...)" and ensure
that simpleError is invoked/configured to produce a 401 status for invalid JWTs;
keep the existing cloudlog(...) call and then throw the
simpleError('invalid_jwt', 'Invalid JWT') so the pattern matches other throws in
this file (references: supabaseClientFn, supabase.auth.getUser, cloudlog,
simpleError, c.set, AuthInfo).
In `@supabase/functions/_backend/utils/org_email_notifications.ts`:
- Around line 158-244: The DB reads using supabaseAdmin(c) (the queries that
populate legacyMembers, rbacUserBindings, rbacGroupBindings, groupMembers, and
users inside getEligibleOrgMemberEmails in org_email_notifications.ts) must be
refactored to use the backend Postgres/Drizzle client (getPgClient() or
getDrizzleClient()) instead of supabaseAdmin; replace each
.from(...).select(...).in/... chain with equivalent parameterized Drizzle/PG
queries, keep the same filtering logic (org_id, principal_type, scope_type,
roles.name, group IN, user IN), preserve error handling and cloudlog calls
(messages like 'getEligibleOrgMemberEmails legacy error', 'rbac user error',
'rbac group error', 'group members error', 'users error'), ensure expires_at
filtering and userIds collection logic remains identical, and import/obtain the
PG/Drizzle client from the request context (c) before running the queries.
♻️ Duplicate comments (1)
messages/en.json (1)
1540-1540: Add Oxford comma for clarity (user, group, or role).Minor copy polish for readability.
Suggested edit
- "search-role-bindings": "Search by user, group or role...", + "search-role-bindings": "Search by user, group, or role...",
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/org_email_notifications.ts (1)
235-247: Refactor to use Drizzle ORM for large user lookupsThe
.in('id', userIdList)approach withsupabaseAdmincan hit URL length limits when orgs have large membership sets (groups + role bindings). This violates the coding guideline to use Drizzle ORM for database operations. Replace with a Drizzle query usinginArray()to avoid URL constraints and stay consistent with the rest of the file:const users = await getDrizzle(getPgClient(c, true)) .select({ id: schema.users.id, email: schema.users.email, email_preferences: schema.users.email_preferences, }) .from(schema.users) .where(inArray(schema.users.id, userIdList))This performs the join server-side and eliminates the URL parameter overhead.
| "select-role": "App-Rolle auswählen", | ||
| "select-user-role": "Rolle auswählen", | ||
| "select-user-role-expanded": "Wähle die zuzuweisende RBAC-Rolle. Legacy-Rollen bleiben während der Migration sichtbar.", | ||
| "rbac-not-enabled-for-org": "RBAC ist für diese Organisation nicht aktiviert.", |
There was a problem hiding this comment.
Use formal address for the RBAC role hint.
“Wähle …” is informal and stands out from nearby formal phrasing (“Wählen Sie …”). Consider aligning tone for consistency.
Proposed tweak
- "select-user-role-expanded": "Wähle die zuzuweisende RBAC-Rolle. Legacy-Rollen bleiben während der Migration sichtbar.",
+ "select-user-role-expanded": "Wählen Sie die zuzuweisende RBAC-Rolle aus. Legacy-Rollen bleiben während der Migration sichtbar.",🤖 Prompt for AI Agents
In `@messages/de.json` around lines 1267 - 1270, The German translation for the
RBAC role hint uses informal address; update the value of the
"select-user-role-expanded" key to formal address so it matches surrounding
lines (replace "Wähle die zuzuweisende RBAC-Rolle. Legacy-Rollen bleiben während
der Migration sichtbar." with a version starting "Wählen Sie …"). Ensure
punctuation and capitalization remain consistent with other keys like
"select-role" and "select-user-role" and keep the explanatory sentence about
legacy roles unchanged except for the formal pronoun.
| "zip-bundle": "Pacchetto app Zip", | ||
| "access": "Accesso", | ||
| "role-app-admin": "amministratore di app", | ||
| "role-app-developer": "sviluppatore di app", | ||
| "role-app-uploader": "caricatore di app", | ||
| "role-app-reader": "lettore di app", | ||
| "remove-role": "Rimuovi ruolo", | ||
| "remove-role-confirm": "Vuoi rimuovere l'accesso di questo utente all'app?", | ||
| "role-org-super-admin": "super amministratore", | ||
| "role-org-admin": "amministratore", | ||
| "role-org-billing-admin": "gestore fatturazione", | ||
| "role-org-member": "membro" |
There was a problem hiding this comment.
Use standard ZIP casing for consistency.
Most locales use “ZIP”; aligning the Italian string avoids mixed casing in the UI.
🔧 Proposed tweak
- "zip-bundle": "Pacchetto app Zip",
+ "zip-bundle": "Pacchetto app ZIP",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "zip-bundle": "Pacchetto app Zip", | |
| "access": "Accesso", | |
| "role-app-admin": "amministratore di app", | |
| "role-app-developer": "sviluppatore di app", | |
| "role-app-uploader": "caricatore di app", | |
| "role-app-reader": "lettore di app", | |
| "remove-role": "Rimuovi ruolo", | |
| "remove-role-confirm": "Vuoi rimuovere l'accesso di questo utente all'app?", | |
| "role-org-super-admin": "super amministratore", | |
| "role-org-admin": "amministratore", | |
| "role-org-billing-admin": "gestore fatturazione", | |
| "role-org-member": "membro" | |
| "zip-bundle": "Pacchetto app ZIP", | |
| "access": "Accesso", | |
| "role-app-admin": "amministratore di app", | |
| "role-app-developer": "sviluppatore di app", | |
| "role-app-uploader": "caricatore di app", | |
| "role-app-reader": "lettore di app", | |
| "remove-role": "Rimuovi ruolo", | |
| "remove-role-confirm": "Vuoi rimuovere l'accesso di questo utente all'app?", | |
| "role-org-super-admin": "super amministratore", | |
| "role-org-admin": "amministratore", | |
| "role-org-billing-admin": "gestore fatturazione", | |
| "role-org-member": "membro" |
🤖 Prompt for AI Agents
In `@messages/it.json` around lines 1515 - 1526, Change the value for the
"zip-bundle" locale key from "Pacchetto app Zip" to use standard uppercase
"ZIP": update the "zip-bundle" entry so it reads "Pacchetto app ZIP"; locate the
"zip-bundle" key in the messages/it.json bundle to make this one-word casing
change.
|
|
||
| // Validate JWT and set auth context | ||
| const { supabaseClient: supabaseClientFn } = await import('./supabase.ts') | ||
| const supabase = supabaseClientFn(c, authorization) | ||
| const { data: authData, error: authError } = await supabase.auth.getUser() | ||
| if (authError || !authData.user) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'Invalid JWT', error: authError }) | ||
| return simpleError('invalid_jwt', 'Invalid JWT') | ||
| } | ||
|
|
||
| // Set auth context for RBAC | ||
| c.set('auth', { | ||
| userId: authData.user.id, | ||
| authType: 'jwt', | ||
| apikey: null, | ||
| jwt: authorization, | ||
| } as AuthInfo) | ||
|
|
There was a problem hiding this comment.
Use throw for consistency and return 401 for auth failures.
Two issues:
-
Inconsistent error pattern: Line 126 uses
return simpleError(...)while line 116 usesthrow simpleError(...). SincesimpleErrorreturnsnever, both work, butthrowis clearer and consistent with the rest of the file. -
Incorrect HTTP status:
simpleErrorreturns 400 Bad Request, but JWT validation failures should return 401 Unauthorized per HTTP semantics.
Suggested fix
if (authError || !authData.user) {
cloudlog({ requestId: c.get('requestId'), message: 'Invalid JWT', error: authError })
- return simpleError('invalid_jwt', 'Invalid JWT')
+ throw quickError(401, 'invalid_jwt', 'Invalid JWT')
}🤖 Prompt for AI Agents
In `@supabase/functions/_backend/utils/hono.ts` around lines 119 - 136, Replace
the inconsistent return with a thrown 401 Unauthorized error when JWT validation
fails: in the block that calls supabaseClientFn(...) and awaits
supabase.auth.getUser(), change the flow that currently does "return
simpleError(...)" to "throw simpleError(...)" and ensure that simpleError is
invoked/configured to produce a 401 status for invalid JWTs; keep the existing
cloudlog(...) call and then throw the simpleError('invalid_jwt', 'Invalid JWT')
so the pattern matches other throws in this file (references: supabaseClientFn,
supabase.auth.getUser, cloudlog, simpleError, c.set, AuthInfo).
| const { data: legacyMembers, error: legacyError } = await supabaseAdmin(c) | ||
| .from('org_users') | ||
| .select(` | ||
| user_id, | ||
| user_right, | ||
| users!inner ( | ||
| ) | ||
| `) | ||
| .select('user_id') | ||
| .eq('org_id', orgId) | ||
| .in('user_right', ['admin', 'super_admin']) | ||
| .is('app_id', null) // org-level membership only, not app-specific | ||
| .is('app_id', null) | ||
|
|
||
| if (legacyError) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'getEligibleOrgMemberEmails legacy error', orgId, error: legacyError }) | ||
| } | ||
| else { | ||
| for (const member of legacyMembers ?? []) { | ||
| if (member.user_id) | ||
| userIds.add(member.user_id) | ||
| } | ||
| } | ||
|
|
||
| if (error || !members) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'getEligibleOrgMemberEmails error', orgId, error }) | ||
| const { data: rbacUserBindings, error: rbacUserError } = await supabaseAdmin(c) | ||
| .from('role_bindings') | ||
| .select('principal_id, expires_at, roles!inner(name)') | ||
| .eq('org_id', orgId) | ||
| .eq('principal_type', 'user') | ||
| .eq('scope_type', 'org') | ||
| .in('roles.name', adminRoleNames) | ||
|
|
||
| if (rbacUserError) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'getEligibleOrgMemberEmails rbac user error', orgId, error: rbacUserError }) | ||
| } | ||
| else { | ||
| for (const binding of rbacUserBindings ?? []) { | ||
| const expiresAt = (binding as any).expires_at as string | null | undefined | ||
| if (expiresAt && new Date(expiresAt) <= now) | ||
| continue | ||
| const principalId = (binding as any).principal_id as string | null | undefined | ||
| if (principalId) | ||
| userIds.add(principalId) | ||
| } | ||
| } | ||
|
|
||
| const { data: rbacGroupBindings, error: rbacGroupError } = await supabaseAdmin(c) | ||
| .from('role_bindings') | ||
| .select('principal_id, expires_at, roles!inner(name)') | ||
| .eq('org_id', orgId) | ||
| .eq('principal_type', 'group') | ||
| .eq('scope_type', 'org') | ||
| .in('roles.name', adminRoleNames) | ||
|
|
||
| if (rbacGroupError) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'getEligibleOrgMemberEmails rbac group error', orgId, error: rbacGroupError }) | ||
| } | ||
| else { | ||
| const groupIds = (rbacGroupBindings ?? []) | ||
| .filter((binding) => { | ||
| const expiresAt = (binding as any).expires_at as string | null | undefined | ||
| return !expiresAt || new Date(expiresAt) > now | ||
| }) | ||
| .map(binding => (binding as any).principal_id as string | null | undefined) | ||
| .filter((groupId): groupId is string => Boolean(groupId)) | ||
|
|
||
| if (groupIds.length > 0) { | ||
| const { data: groupMembers, error: groupError } = await supabaseAdmin(c) | ||
| .from('group_members') | ||
| .select('user_id') | ||
| .in('group_id', groupIds) | ||
|
|
||
| if (groupError) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'getEligibleOrgMemberEmails group members error', orgId, error: groupError }) | ||
| } | ||
| else { | ||
| for (const member of groupMembers ?? []) { | ||
| if (member.user_id) | ||
| userIds.add(member.user_id) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const userIdList = Array.from(userIds) | ||
| if (userIdList.length === 0) { | ||
| return [] | ||
| } | ||
|
|
||
| // Fetch user email_preferences separately since it might not be in generated types yet | ||
| const userIds = members.map(m => m.user_id) | ||
| const { data: users } = await supabaseAdmin(c) | ||
| const { data: users, error: usersError } = await supabaseAdmin(c) | ||
| .from('users') | ||
| .select('id, email, email_preferences') | ||
| .in('id', userIds) | ||
| .in('id', userIdList) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Drizzle/PG clients for these new DB reads
The new org/user RBAC lookups are executed via supabaseAdmin, which conflicts with the backend requirement to use getPgClient() / getDrizzleClient() for PostgreSQL access. Please refactor these queries to Drizzle/PG to align with the migration path and shared backend conventions. As per coding guidelines, ...
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/utils/org_email_notifications.ts` around lines
158 - 244, The DB reads using supabaseAdmin(c) (the queries that populate
legacyMembers, rbacUserBindings, rbacGroupBindings, groupMembers, and users
inside getEligibleOrgMemberEmails in org_email_notifications.ts) must be
refactored to use the backend Postgres/Drizzle client (getPgClient() or
getDrizzleClient()) instead of supabaseAdmin; replace each
.from(...).select(...).in/... chain with equivalent parameterized Drizzle/PG
queries, keep the same filtering logic (org_id, principal_type, scope_type,
roles.name, group IN, user IN), preserve error handling and cloudlog calls
(messages like 'getEligibleOrgMemberEmails legacy error', 'rbac user error',
'rbac group error', 'group members error', 'users error'), ensure expires_at
filtering and userIds collection logic remains identical, and import/obtain the
PG/Drizzle client from the request context (c) before running the queries.
Summary (AI generated)
Motivation (AI generated)
The RBAC system enables granular access control to Capgo platform resources. This documentation is necessary to:
Business Impact (AI generated)
This documentation positively impacts Capgo's business by:
Test Plan (AI generated)
supabase/migrations/Screenshots
Checklist
bun run lint:backend && bun run lint.Summary by CodeRabbit
New Features
Refactor
Documentation
Localization
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.