feat: add RBAC API key management and app creation permissions#1863
Open
feat: add RBAC API key management and app creation permissions#1863
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (17)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds organization-scoped API Keys v2 with RBAC bindings: new UI components and pages, localized strings across many languages, DataTable tweaks, new routes/tab, backend Supabase function and migration changes to prioritize API-key role bindings, CLI DB wrappers, role_bindings access updates, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ApiKeyRbacManager (Frontend)
participant Backend as Supabase Functions (serverless)
participant DB as PostgreSQL (RBAC / apikeys)
participant Storage as RoleBindings / Apps
User->>UI: Open "Create API Key" form
UI->>User: Display roles/apps selection
User->>UI: Submit form (roles, apps, expiration)
UI->>Backend: call serverless `apikey` function (create)
Backend->>DB: INSERT apikeys, create rbac_id, store metadata
Backend->>Storage: INSERT role_bindings (org / app scope)
DB-->>Backend: success
Backend-->>UI: return plain key (one-time)
UI->>User: show copy modal
sequenceDiagram
participant Client as External Client (uses API key)
participant API as Server (request handler / CLI wrapper)
participant RBAC as rbac_check_permission_direct
participant DB as PostgreSQL (role_bindings, apikeys, apps)
Client->>API: Request with apikey header
API->>DB: resolve apikey -> user_id
API->>RBAC: rbac_check_permission_direct(permission, user_id, org, app, channel, apikey)
RBAC->>DB: query role_bindings where rbac applies (apikey first)
alt apikey has explicit role_bindings
RBAC->>RBAC: evaluate permission against apikey bindings only
else
RBAC->>DB: enforce limited_to_orgs/apps, then fallback to owner user's permissions
end
RBAC-->>API: allow / deny boolean
API-->>Client: respond or 403
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
supabase/functions/_backend/private/role_bindings.ts (1)
150-175: Align org membership validation with RBAC system's group support.The
validatePrincipalAccesscheck uses hard-codedorg_users+ directrole_bindingslookups but doesn't include group-derived membership. Meanwhile, the RBAC system (viarbac_has_permission) includesgroup_membersin permission checks at all scopes (org, app, channel). A user with only group-based org access would pass RBAC checks but fail this validation. Consider replacing the hard-coded org eligibility logic with a shared helper that routes through the same RBAC path used bycheckPermission().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 150 - 175, The org membership check in validatePrincipalAccess currently queries org_users and role_bindings directly (see usage of schema.org_users and schema.role_bindings) which misses group-derived membership; replace this hard-coded logic with a call into the same RBAC evaluation used elsewhere (e.g. rbac_has_permission / checkPermission) or extract a shared helper (e.g. isPrincipalInOrgViaRbac) that considers schema.group_members and role_bindings when validating org eligibility so users who have only group-based org access pass the check.src/layouts/settings.vue (1)
128-145: Remove the redundant API keys tab reinsertion block.
organizationTabsis already initialized from[...]baseOrgTabs, andsrc/constants/organizationTabs.ts:12-23already includes/settings/organization/api-keys. This branch no longer changes behavior, but it leaves a second place to maintain tab presence and ordering.♻️ Suggested simplification
- const needsApiKeys = true - const hasApiKeys = organizationTabs.value.find(tab => tab.key === '/settings/organization/api-keys') - if (needsApiKeys && !hasApiKeys) { - const base = baseOrgTabs.find(t => t.key === '/settings/organization/api-keys') - const insertAfterKeys = [ - '/settings/organization/groups', - '/settings/organization/members', - '/settings/organization', - ] - const insertAfterIndex = insertAfterKeys - .map(key => organizationTabs.value.findIndex(tab => tab.key === key)) - .find(index => index >= 0) ?? -1 - - if (base && insertAfterIndex >= 0) - organizationTabs.value.splice(insertAfterIndex + 1, 0, { ...base }) - else if (base) - organizationTabs.value.push({ ...base }) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layouts/settings.vue` around lines 128 - 145, Remove the redundant API keys reinsertion logic: delete the whole conditional block that defines needsApiKeys, computes hasApiKeys, finds base from baseOrgTabs, builds insertAfterKeys/insertAfterIndex and calls organizationTabs.value.splice or push; organizationTabs is already initialized from baseOrgTabs (which contains '/settings/organization/api-keys'), so keep only the initial organizationTabs initialization and remove the code referencing needsApiKeys, hasApiKeys, baseOrgTabs lookup, insertAfterKeys, insertAfterIndex, and the splice/push logic to avoid duplicate maintenance locations.supabase/tests/45_test_org_create_app_permission.sql (1)
31-50: This file still doesn't exercise a directly bound API key.The seeded key never gets a
role_bindingsrow withprincipal_type = public.rbac_principal_apikey(), so the final insert only proves the non-bound/legacy fallback. A regression in the new bound-key path would still pass this test. Adding a second key with a direct API-key binding would close that gap.Also applies to: 124-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/45_test_org_create_app_permission.sql` around lines 31 - 50, Add a second API key and a matching direct role_binding entry so the bound-key path is exercised: insert a new row into public.apikeys (e.g., id 45002, a distinct key string and same limited_to_orgs value) and then insert into public.role_bindings using principal_type = public.rbac_principal_apikey(), principal_id = the new apikey id, role_id from public.roles (same selection as the existing member binding), scope_type = public.rbac_scope_org(), org_id = '70000000-0000-4000-8000-000000000001', and granted_by = tests.get_supabase_uid('org_create_app_admin') so the test covers the directly-bound API key path.src/pages/ApiKeys.vue (1)
850-897: Switch these new controls to the repo’sd-DaisyUI classes.These radios and checkboxes are using unprefixed DaisyUI classes (
radio,checkbox,form-control,label). In this repo, interactive primitives are expected to use thed-prefixed variants, so this modal can drift from the standard Capgo styling/behavior. As per coding guidelinesUse DaisyUI (d-prefixed classes) for buttons, inputs, and other interactive primitives to keep behavior and spacing consistent.Also applies to: 903-929, 937-971, 981-987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ApiKeys.vue` around lines 850 - 897, The radio inputs in the ApiKeys modal (the blocks binding v-model="selectedKeyType" with name="key-type" and surrounding divs using classes like "form-control", "label", "radio", "radio-primary", "label-text") are using unprefixed DaisyUI classes; replace those with the repo's d- prefixed counterparts (e.g., "d-form-control", "d-label", "d-radio", "d-radio-primary", "d-label-text" or the repo's exact d-variants) so the interactive primitives use the standard styling/behavior; apply the same replacements to the other similar blocks referenced (lines ~903-929, 937-971, 981-987) to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/en.json`:
- Line 482: Add the new "copy" translation key to all locale bundles so
t('copy') in ApiKeys.[id].vue resolves in every language; update each
messages/*.json (e.g., messages/hi.json and any other locale files present) to
include the "copy" key with an appropriate localized string (matching the
English "Copy" entry) so no locale falls back to the raw key.
In `@src/components/DataTable.vue`:
- Around line 598-612: Make the action tooltip keyboard-accessible by making the
wrapper focusable and wiring the tooltip to the button via aria-describedby: add
tabindex="0" to the outer div (the element with class "relative inline-flex
group"), generate or derive a stable tooltip id (e.g., using the action and elem
index) and set that id on the span (role="tooltip"), then add
:aria-describedby="getActionTitle(action, elem) ? tooltipIdFor(action, elem) :
undefined" to the <button> instead of relying on disabled title behavior; also
ensure the wrapper forwards Enter/Space to invoke action.onClick(elem) when the
action is not disabled (handle keydown on the wrapper to call
action.onClick(elem) only if !(action.disabled && action.disabled(elem))). Use
getActionTitle(action, elem), action.onClick, action.icon and elem to locate
code.
In `@src/components/organization/ApiKeyRbacManager.vue`:
- Around line 126-160: The column definitions in computedV2Columns set sortable:
true but DataTable only emits column state and does not reorder elementList, so
clicking headers only toggles icons; either remove sortable: true from the
columns (in computedV2Columns and the analogous legacy columns around lines
203-236) or implement a sorted computed like the one in src/pages/ApiKeys.vue
that listens to DataTable's emitted column state and returns a sorted
v2Keys/legacyKeys array; update the component to use that sorted computed for
the table's rows (reference computedV2Columns, v2Keys, legacyKeys, and the
sorting logic in src/pages/ApiKeys.vue).
- Around line 181-184: Replace inline English fallbacks passed to the i18n
helper by using translation keys only: in ApiKeyRbacManager.vue update the t()
call used in the menu item (icon: IconWrench, title: t('manage', 'Manage')) to
title: t('manage'), and update the error notification call where
t('error-removing-apikey', 'Error removing API key') is used to
t('error-removing-apikey'); then add a corresponding "error-removing-apikey"
entry to messages/en.json (with the English string) so the new key resolves.
In `@src/pages/ApiKeys.vue`:
- Around line 952-978: The selectedApps set can contain apps that are no longer
visible after the allowed orgs shrink (so createApiKey() can submit hidden app
IDs); fix by pruning displayStore.selectedApps whenever the allowed-org
selection changes or right before submission: when filteredAppsForSelectedOrgs
updates (or inside the watcher for displayStore.selectedOrganizations) remove
any entries from displayStore.selectedApps that are not present in
filteredAppsForSelectedOrgs; alternatively, ensure createApiKey() filters
displayStore.selectedApps to only include apps whose app_id appears in
filteredAppsForSelectedOrgs before building the request. Reference symbols:
displayStore.selectedApps, filteredAppsForSelectedOrgs, handleAppSelection,
createApiKey.
In `@src/pages/settings/organization/ApiKeys`.[id].vue:
- Around line 411-468: The current flow calls
supabase.functions.invoke('apikey') to create the API key and then issues
separate supabase.functions.invoke('private/role_bindings') calls for org and
app roles, which can leave a half-configured credential if any binding fails;
update the flow to either (A) move creation + bindings into a single backend
transaction by adding/using an API that accepts name, expires_at, hashed,
selectedOrgRole and pendingAppBindings and returns the created rbac_id only
after all bindings succeed, or (B) implement compensating cleanup in this
client: if any role binding (the calls that reference 'private/role_bindings',
newRbacId, selectedOrgRole, pendingAppBindings) fails, immediately call the
backend to revoke/delete the created API key (use the existing 'apikey' function
or a new DELETE/cleanup function) and surface the original error to the user;
ensure special handling for plaintext vs hashed (createAsHashed) so plaintext
secrets are never lost on partial failure.
- Line 202: The code uses inline fallback strings passed as a second argument to
the t() translation calls (e.g., the assignment to displayStore.NavTitle =
t('create-api-key', 'Create API key') and other t(...) usages noted), which
violates the i18n guideline; remove the inline English fallbacks from all t()
calls in ApiKeys.[id].vue so each call is t('translation.key') only, then add
the corresponding English copy into messages/en.json under the same keys (e.g.,
create-api-key and the other keys referenced at the reported locations) so
missing translations come from the messages file rather than inline fallbacks;
update any tests or imports if needed to ensure keys are present and reload the
i18n messages.
- Around line 294-296: The current logic in fetchRoleBindings() sets
selectedOrgRole.value to '' when keyOrgBinding.value?.role_name is in
unsupportedApiKeyOrgRoles, and saveOrgRole() treats an empty selectedOrgRole as
an instruction to delete the org binding; this causes unsupported roles like
"org_billing_admin" to be silently removed on save. Preserve the original
unsupported role by storing it in a separate variable (e.g.,
originalUnsupportedOrgRole) when populating selectedOrgRole in
fetchRoleBindings(), display selectedOrgRole as '' but in saveOrgRole() only
perform the delete if the user actually changed selectedOrgRole away from the
original value (compare against originalUnsupportedOrgRole and
keyOrgBinding.value?.role_name), and apply the same fix for the duplicate logic
around lines 531-536 so unsupported roles are not removed unless explicitly
changed by the user.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 146-178: Collapse the two distinct API-key failure responses into
a single client-facing error: when apiKey is missing, when membership is absent,
or when ownerRbacAccess is absent, return the same generic { ok: false, status:
400, error: '<generic message>' } (choose a neutral message like "Invalid API
key or access") while preserving detailed reasons only in server logs; update
the checks around the apiKey variable, the membership query result, and the
ownerRbacAccess check in role_bindings.ts (referencing the apiKey variable, the
membership destructure, and the ownerRbacAccess destructure) so they all yield
the same public error, and add processLogger.error or similar log statements
that include the specific condition (missing apiKey, membership not found,
ownerRbacAccess missing) for internal debugging.
In `@supabase/functions/_backend/public/app/post.ts`:
- Around line 29-30: Ensure owner_org presence is validated before the RBAC
check: add an explicit check for body.owner_org at the top of the creation flow
and throw a quickError when it's missing (so the request fails
deterministically), then keep the existing permission call to checkPermission(c,
'org.create_app', { orgId: body.owner_org }) unchanged; this guarantees
authorization is always evaluated only when owner_org is present and avoids
failing later at insert time.
In `@supabase/migrations/20260305120000_rbac_apikey_bindings_priority.sql`:
- Around line 411-416: get_org_apikeys() currently allows any org reader because
it only calls public.check_min_rights('read', ...); change the RPC to require
the same management permission used by the UI (org.update_user_roles) or
add/require a dedicated right like 'manage_api_keys' instead of 'read' — replace
the check_min_rights call in get_org_apikeys() to call
public.check_min_rights('org.update_user_roles'::public.user_min_right,
auth.uid(), p_org_id, NULL::varchar, NULL::bigint) (or 'manage_api_keys' if you
introduce that new right) and keep the existing RAISE EXCEPTION flow so callers
without the management permission cannot enumerate API keys.
- Around line 434-451: The predicate currently only checks for user ownership or
user-scoped role_bindings (role_bindings with principal_type =
public.rbac_principal_user()), which lets API keys explicitly bound to the org
via role_bindings (principal_type = public.rbac_principal_apikey()) drop out of
get_org_apikeys() when the owner relation changes; update the EXISTS check that
queries public.role_bindings (the block referencing rb.principal_type =
public.rbac_principal_user(), rb.scope_type = public.rbac_scope_org(),
rb.principal_id = ak.user_id, rb.org_id = p_org_id) to also consider bindings
where principal_type = public.rbac_principal_apikey() and the principal_id
matches the api key identifier (ak.id or the correct apikey identifier field) so
keys with apikey-scoped bindings remain included in the predicate.
- Around line 72-75: The legacy API-key branch resolves v_effective_user_id from
find_apikey_by_value(p_apikey) but continues to call the legacy helpers with the
original p_user_id, causing capgkey-only requests to be treated as NULL; update
the legacy-check calls (the places that invoke the legacy helpers referenced by
rbac_check_permission_request() and the second occurrence around lines 282-287)
to pass v_effective_user_id when it is set (i.e., use COALESCE-like behavior:
use v_effective_user_id if NOT NULL, otherwise fall back to p_user_id) so the
resolved key owner is used for permission checks.
In `@supabase/migrations/20260325161611_add_org_create_app_permission.sql`:
- Around line 67-68: The migration changed the legacy mapping for org.create_app
to return rbac_right_admin(), which breaks non-RBAC write/all users and keys
that previously relied on legacy write-level access; update the mapping so
public.rbac_perm_org_create_app() returns rbac_right_write() (use the
rbac_right_write() symbol instead of rbac_right_admin()) and apply the same
change to the other occurrences of this mapping (the other
rbac_perm_org_create_app() branches around the referenced ranges) so the
pre-create storage.objects and public.apps INSERT paths preserve the legacy
write access.
---
Nitpick comments:
In `@src/layouts/settings.vue`:
- Around line 128-145: Remove the redundant API keys reinsertion logic: delete
the whole conditional block that defines needsApiKeys, computes hasApiKeys,
finds base from baseOrgTabs, builds insertAfterKeys/insertAfterIndex and calls
organizationTabs.value.splice or push; organizationTabs is already initialized
from baseOrgTabs (which contains '/settings/organization/api-keys'), so keep
only the initial organizationTabs initialization and remove the code referencing
needsApiKeys, hasApiKeys, baseOrgTabs lookup, insertAfterKeys, insertAfterIndex,
and the splice/push logic to avoid duplicate maintenance locations.
In `@src/pages/ApiKeys.vue`:
- Around line 850-897: The radio inputs in the ApiKeys modal (the blocks binding
v-model="selectedKeyType" with name="key-type" and surrounding divs using
classes like "form-control", "label", "radio", "radio-primary", "label-text")
are using unprefixed DaisyUI classes; replace those with the repo's d- prefixed
counterparts (e.g., "d-form-control", "d-label", "d-radio", "d-radio-primary",
"d-label-text" or the repo's exact d-variants) so the interactive primitives use
the standard styling/behavior; apply the same replacements to the other similar
blocks referenced (lines ~903-929, 937-971, 981-987) to keep consistency.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 150-175: The org membership check in validatePrincipalAccess
currently queries org_users and role_bindings directly (see usage of
schema.org_users and schema.role_bindings) which misses group-derived
membership; replace this hard-coded logic with a call into the same RBAC
evaluation used elsewhere (e.g. rbac_has_permission / checkPermission) or
extract a shared helper (e.g. isPrincipalInOrgViaRbac) that considers
schema.group_members and role_bindings when validating org eligibility so users
who have only group-based org access pass the check.
In `@supabase/tests/45_test_org_create_app_permission.sql`:
- Around line 31-50: Add a second API key and a matching direct role_binding
entry so the bound-key path is exercised: insert a new row into public.apikeys
(e.g., id 45002, a distinct key string and same limited_to_orgs value) and then
insert into public.role_bindings using principal_type =
public.rbac_principal_apikey(), principal_id = the new apikey id, role_id from
public.roles (same selection as the existing member binding), scope_type =
public.rbac_scope_org(), org_id = '70000000-0000-4000-8000-000000000001', and
granted_by = tests.get_supabase_uid('org_create_app_admin') so the test covers
the directly-bound API key path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aad52d00-25bd-46ed-b268-c6087ca895b5
📒 Files selected for processing (31)
messages/de.jsonmessages/en.jsonmessages/es.jsonmessages/fr.jsonmessages/hi.jsonmessages/id.jsonmessages/it.jsonmessages/ja.jsonmessages/ko.jsonmessages/pl.jsonmessages/pt-br.jsonmessages/ru.jsonmessages/tr.jsonmessages/vi.jsonmessages/zh-cn.jsonsrc/components.d.tssrc/components/DataTable.vuesrc/components/organization/ApiKeyRbacManager.vuesrc/constants/organizationTabs.tssrc/layouts/settings.vuesrc/pages/ApiKeys.vuesrc/pages/settings/organization/ApiKeys.[id].vuesrc/pages/settings/organization/ApiKeys.vuesrc/route-map.d.tssupabase/functions/_backend/private/role_bindings.tssupabase/functions/_backend/public/app/post.tssupabase/functions/_backend/utils/rbac.tssupabase/migrations/20260305120000_rbac_apikey_bindings_priority.sqlsupabase/migrations/20260325153223_cli_rbac_permission_wrappers.sqlsupabase/migrations/20260325161611_add_org_create_app_permission.sqlsupabase/tests/45_test_org_create_app_permission.sql
Comment on lines
+181
to
+184
| { | ||
| icon: IconWrench, | ||
| title: t('manage', 'Manage'), | ||
| onClick: (apiKey: ApiKeyRow) => router.push(`/settings/organization/api-keys/${apiKey.rbac_id}`), |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/organization/ApiKeyRbacManager.vue | sed -n '175,190p'Repository: Cap-go/capgo
Length of output: 657
🏁 Script executed:
cat -n src/components/organization/ApiKeyRbacManager.vue | sed -n '400,410p'Repository: Cap-go/capgo
Length of output: 419
🏁 Script executed:
find . -name "messages" -o -name "*en.json" | head -20Repository: Cap-go/capgo
Length of output: 328
🏁 Script executed:
cat ./messages/en.json | grep -E '"(manage|error-removing-apikey)"'Repository: Cap-go/capgo
Length of output: 77
Remove inline English fallbacks from t() calls.
These should use translation keys only. At line 183, t('manage', 'Manage') should be t('manage') since the key already exists in messages/en.json. At line 407, add error-removing-apikey to messages/en.json and change t('error-removing-apikey', 'Error removing API key') to t('error-removing-apikey').
Minimal cleanup
- title: t('manage', 'Manage'),
+ title: t('manage'),
...
- toast.error(t('error-removing-apikey', 'Error removing API key'))
+ toast.error(t('error-removing-apikey'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/organization/ApiKeyRbacManager.vue` around lines 181 - 184,
Replace inline English fallbacks passed to the i18n helper by using translation
keys only: in ApiKeyRbacManager.vue update the t() call used in the menu item
(icon: IconWrench, title: t('manage', 'Manage')) to title: t('manage'), and
update the error notification call where t('error-removing-apikey', 'Error
removing API key') is used to t('error-removing-apikey'); then add a
corresponding "error-removing-apikey" entry to messages/en.json (with the
English string) so the new key resolves.
Comment on lines
+29
to
30
| if (body.owner_org && !(await checkPermission(c, 'org.create_app', { orgId: body.owner_org }))) { | ||
| throw quickError(403, 'cannot_access_organization', 'You can\'t access this organization', { org_id: body.owner_org }) |
Contributor
There was a problem hiding this comment.
Validate owner_org before the permission check.
At Line 29, an empty/missing owner_org skips RBAC enforcement (body.owner_org && ...) and only fails later at insert time. Add an explicit required check first so authorization is always evaluated deterministically.
Suggested patch
+ if (!body.owner_org) {
+ throw simpleError('missing_owner_org', 'Missing owner_org', { body })
+ }
+
// Check if the user is allowed to create an app in this organization (auth context set by middlewareKey)
- if (body.owner_org && !(await checkPermission(c, 'org.create_app', { orgId: body.owner_org }))) {
+ if (!(await checkPermission(c, 'org.create_app', { orgId: body.owner_org }))) {
throw quickError(403, 'cannot_access_organization', 'You can\'t access this organization', { org_id: body.owner_org })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/functions/_backend/public/app/post.ts` around lines 29 - 30, Ensure
owner_org presence is validated before the RBAC check: add an explicit check for
body.owner_org at the top of the creation flow and throw a quickError when it's
missing (so the request fails deterministically), then keep the existing
permission call to checkPermission(c, 'org.create_app', { orgId: body.owner_org
}) unchanged; this guarantees authorization is always evaluated only when
owner_org is present and avoids failing later at insert time.
Comment on lines
+72
to
+75
| IF v_effective_user_id IS NULL AND p_apikey IS NOT NULL THEN | ||
| SELECT user_id INTO v_effective_user_id | ||
| FROM public.find_apikey_by_value(p_apikey) | ||
| LIMIT 1; |
Contributor
There was a problem hiding this comment.
Legacy API-key fallback drops the resolved owner user.
You resolve v_effective_user_id from find_apikey_by_value() at Lines 72-75, but the legacy branch still passes the original p_user_id into the legacy helpers. rbac_check_permission_request() supplies auth.uid(), so capgkey-only requests hit this branch with NULL and org-scoped legacy checks deny even though the key owner was resolved.
⚙️ Suggested fix
- RETURN public.has_app_right_apikey(p_app_id, v_legacy_right, p_user_id, p_apikey);
+ RETURN public.has_app_right_apikey(p_app_id, v_legacy_right, v_effective_user_id, p_apikey);
@@
- RETURN public.check_min_rights_legacy(v_legacy_right, p_user_id, v_effective_org_id, p_app_id, p_channel_id);
+ RETURN public.check_min_rights_legacy(v_legacy_right, v_effective_user_id, v_effective_org_id, p_app_id, p_channel_id);Also applies to: 282-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260305120000_rbac_apikey_bindings_priority.sql` around
lines 72 - 75, The legacy API-key branch resolves v_effective_user_id from
find_apikey_by_value(p_apikey) but continues to call the legacy helpers with the
original p_user_id, causing capgkey-only requests to be treated as NULL; update
the legacy-check calls (the places that invoke the legacy helpers referenced by
rbac_check_permission_request() and the second occurrence around lines 282-287)
to pass v_effective_user_id when it is set (i.e., use COALESCE-like behavior:
use v_effective_user_id if NOT NULL, otherwise fall back to p_user_id) so the
resolved key owner is used for permission checks.
Comment on lines
+411
to
+416
| -- Permission check: caller must be a member of the org with at least read access. | ||
| IF NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, auth.uid(), p_org_id, NULL::varchar, NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; |
Contributor
There was a problem hiding this comment.
get_org_apikeys() is readable by callers who cannot manage API keys.
The RPC only enforces check_min_rights('read', ...), but the management page itself gates access on org.update_user_roles in src/pages/settings/organization/ApiKeys.[id].vue Lines 76-80. That means any authenticated org reader who can call RPCs can bypass the page guard and enumerate key names plus owner_email. The RPC should enforce the same management permission as the UI, or a dedicated manage-api-keys permission.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260305120000_rbac_apikey_bindings_priority.sql` around
lines 411 - 416, get_org_apikeys() currently allows any org reader because it
only calls public.check_min_rights('read', ...); change the RPC to require the
same management permission used by the UI (org.update_user_roles) or add/require
a dedicated right like 'manage_api_keys' instead of 'read' — replace the
check_min_rights call in get_org_apikeys() to call
public.check_min_rights('org.update_user_roles'::public.user_min_right,
auth.uid(), p_org_id, NULL::varchar, NULL::bigint) (or 'manage_api_keys' if you
introduce that new right) and keep the existing RAISE EXCEPTION flow so callers
without the management permission cannot enumerate API keys.
Comment on lines
+434
to
+451
| ( | ||
| EXISTS ( | ||
| SELECT 1 | ||
| FROM public.org_users ou | ||
| WHERE ou.user_id = ak.user_id | ||
| AND ou.org_id = p_org_id | ||
| ) | ||
| OR EXISTS ( | ||
| SELECT 1 | ||
| FROM public.role_bindings rb | ||
| WHERE rb.principal_type = public.rbac_principal_user() | ||
| AND rb.scope_type = public.rbac_scope_org() | ||
| AND rb.principal_id = ak.user_id | ||
| AND rb.org_id = p_org_id | ||
| ) | ||
| ) | ||
| -- Key scope: either unlimited (no org restriction) or includes this org | ||
| AND (ak.limited_to_orgs IS NULL OR cardinality(ak.limited_to_orgs) = 0 OR p_org_id = ANY(ak.limited_to_orgs)) |
Contributor
There was a problem hiding this comment.
Keys with explicit RBAC bindings can still disappear from the org listing.
This predicate only keeps keys whose owner is an org member or has an org-scoped user binding, then applies limited_to_orgs. A key that is explicitly bound to this org/app via public.role_bindings stays authorized by public.rbac_check_permission_direct() but can drop out of get_org_apikeys() once the owner relationship changes, leaving an active org key unlistable and uneditable. Include principal_type = public.rbac_principal_apikey() bindings in the relevance check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260305120000_rbac_apikey_bindings_priority.sql` around
lines 434 - 451, The predicate currently only checks for user ownership or
user-scoped role_bindings (role_bindings with principal_type =
public.rbac_principal_user()), which lets API keys explicitly bound to the org
via role_bindings (principal_type = public.rbac_principal_apikey()) drop out of
get_org_apikeys() when the owner relation changes; update the EXISTS check that
queries public.role_bindings (the block referencing rb.principal_type =
public.rbac_principal_user(), rb.scope_type = public.rbac_scope_org(),
rb.principal_id = ak.user_id, rb.org_id = p_org_id) to also consider bindings
where principal_type = public.rbac_principal_apikey() and the principal_id
matches the api key identifier (ak.id or the correct apikey identifier field) so
keys with apikey-scoped bindings remain included in the predicate.
Comment on lines
+67
to
+68
| WHEN public.rbac_perm_org_create_app() THEN RETURN public.rbac_right_admin(); | ||
| WHEN public.rbac_perm_org_update_settings() THEN RETURN public.rbac_right_admin(); |
Contributor
There was a problem hiding this comment.
org.create_app no longer preserves the old legacy access level.
rbac_check_permission_request() now backs both public.apps inserts and the pre-create storage.objects branch, but the legacy mapping for org.create_app is admin. The previous public.apps INSERT policy in supabase/migrations/20260203201308_rbac_org_member_no_app_access.sql:305-322 required legacy write, so non-RBAC write/all users and keys will lose app-creation behavior after this migration.
⚙️ Suggested fix
- WHEN public.rbac_perm_org_create_app() THEN RETURN public.rbac_right_admin();
+ WHEN public.rbac_perm_org_create_app() THEN RETURN public.rbac_right_write();Also applies to: 131-139, 174-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260325161611_add_org_create_app_permission.sql` around
lines 67 - 68, The migration changed the legacy mapping for org.create_app to
return rbac_right_admin(), which breaks non-RBAC write/all users and keys that
previously relied on legacy write-level access; update the mapping so
public.rbac_perm_org_create_app() returns rbac_right_write() (use the
rbac_right_write() symbol instead of rbac_right_admin()) and apply the same
change to the other occurrences of this mapping (the other
rbac_perm_org_create_app() branches around the referenced ranges) so the
pre-create storage.objects and public.apps INSERT paths preserve the legacy
write access.
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
org.create_appwith RLS support for app creation.fix(rbac): support org api keys with bindingsfeat(api-keys): add organization rbac management uifeat(rbac): add cli permission wrappers and app creationMotivation (AI generated)
API Keys v2 and the CLI were still partially tied to legacy authorization paths. This change aligns the organization API key management flow with RBAC, fixes missing bindings/listing issues, and adds the backend permission primitives needed to migrate app creation away from legacy role proxies.
Business Impact (AI generated)
This reduces authorization inconsistencies between the console, backend, and CLI, makes RBAC API keys usable in production workflows, and gives Capgo a cleaner path to ship fine-grained permissions without relying on the old
modemodel.Test Plan (AI generated)
bun lint:backendbun typecheckrole_bindingsare created as expectedorg.create_appallows app creation for RBAC org membersGenerated with AI
Summary by CodeRabbit
New Features
Localization