feat(api): atomic API key creation with RBAC role bindings#2012
Open
feat(api): atomic API key creation with RBAC role bindings#2012
Conversation
Allow creating an API key with role bindings in a single POST /apikey call. The mode field is now nullable (managed via RBAC when NULL). Backend handles rollback if any binding fails.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNullable apikey.mode added; API key creation accepts atomic RBAC bindings. Backend adds role-binding creation API, permission checks, and rollback on binding failures. Types, DB migration, middleware, frontend flows, and tests updated for RBAC-managed keys and null-mode semantics. ChangesAtomic apikey + RBAC bindings
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Vue ApiKeys
participant API as POST /apikey Function
participant RoleBind as role_bindings.ts
participant DB as PostgreSQL/Drizzle
User->>Frontend: Submit create request (name, bindings?, org_id,...)
Frontend->>API: POST { name, bindings, limited_to_*, expires_at, ... }
API->>API: Validate body & bindings
API->>DB: CALL create_hashed_apikey(p_mode=NULL...)
DB-->>API: return apikey row (id, rbac_id)
API->>DB: CHECK org.update_user_roles for referenced orgs
alt permissions OK and bindings present
loop per binding
API->>RoleBind: createRoleBindingForPrincipal(binding, principal)
RoleBind->>DB: validate role, scope, ownership, principal membership
RoleBind->>DB: INSERT role_binding
DB-->>RoleBind: binding created
RoleBind-->>API: success
end
API-->>Frontend: 200 OK (apikey + bindings)
else any failure
API->>DB: DELETE apikey (rollback)
API-->>Frontend: 4xx error (permission/binding_failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/types/supabase.types.ts (1)
26-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe CLI Supabase typings are still partially stale.
apikeysnow allowsmode: null, but later RPC typings still model the legacy non-null shape—for examplefind_apikey_by_valuestill returns a non-nullmode. CLI consumers of RPC results will keep assuming the old contract.As per coding guidelines, "Run
bun typesafter schema changes in migrations to regenerate TypeScript types for Supabase".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/types/supabase.types.ts` around lines 26 - 52, The Supabase types are out of sync: the table type for apikeys allows mode: null but RPC return types (e.g., the find_apikey_by_value RPC type) still mark mode as non-null; update the RPC/result types to match the table shape by regenerating the Supabase TypeScript types and committing the updated file. Run the project’s type generation command (bun types) after the migration so cli/src/types/supabase.types.ts (and RPC typings such as the return type for find_apikey_by_value) reflect mode: Database["public"]["Enums"]["key_mode"] | null and any other schema changes.supabase/functions/_backend/utils/supabase.types.ts (1)
26-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate the RPC typings too.
Only the table interfaces were updated here. Later in this file,
create_hashed_apikey*still require non-nullp_mode, and RPC returns likefind_apikey_by_value/regenerate_hashed_apikey*still expose non-nullmode. That leaves backend callers with a stale contract for RBAC-managed API keys.As per coding guidelines, "Run
bun typesafter schema changes in migrations to regenerate TypeScript types for Supabase".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/supabase.types.ts` around lines 26 - 54, The RPC typings were not regenerated so RPC methods still declare non-null p_mode/mode; run the TS type regeneration and update RPC interfaces to match the new nullable key mode: regenerate Supabase types (run "bun types" per repo guideline) and ensure RPC signatures for create_hashed_apikey*, regenerate_hashed_apikey*, and find_apikey_by_value reflect nullable p_mode/mode (and any other RPC returns/params that reference Database["public"]["Enums"]["key_mode"]) so backend callers see the updated RBAC-managed API key contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/organization/ApiKeys`.[id].vue:
- Around line 524-560: The current submit can send an empty bindings array
(built from selectedOrgRole and pendingAppBindings) which the server rejects;
before calling supabase.functions.invoke('apikey') validate that at least one
binding exists (i.e., selectedOrgRole.value is set or pendingAppBindings.value
contains at least one non-empty role) and if none, set/return a client-side
validation error (update the component's error state / show a message and abort
the POST) so the request is not sent with bindings = [].
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 466-469: The principal validation currently allows stale API-key
principals; update the validatePrincipalAccess logic (the call site using
principal_type and principal_id) to apply the same checks used for user
principals: when principal_type === 'apikey' reject any membership rows whose
role starts with 'invite_' and ensure RBAC access isn't expired (expires_at must
be null or in the future). Modify validatePrincipalAccess to filter out invite_%
rows and to check expiry for apikey principals before returning ok=true so that
the role_bindings creation path cannot bind stale or invite-only API keys.
- Around line 447-450: The rank check uses getCallerMaxPriorityRank which only
inspects direct role_bindings and therefore blocks callers whose effective admin
rights come from org_users.user_right or group/apikey bindings; replace the
direct call with an effective-rank computation that mirrors checkPermission(...)
semantics (e.g., introduce or call
getEffectiveMaxPriorityRank/getCallerEffectiveRank that considers
org_users.user_right, group bindings, and apikey flows) and use that result in
place of callerMaxRank in the anti-escalation check so non-direct RBAC admins
are not incorrectly 403’ed.
In `@supabase/functions/_backend/public/apikey/post.ts`:
- Around line 171-172: The rollback delete call to
supabase.from('apikeys').delete().eq('id', apikeyData.id) is ignored — check the
delete result and handle failures instead of proceeding to throw the binding
error; after awaiting the delete, inspect the returned error (e.g.,
result.error) and if non-null, throw or return a 500 quickError (e.g.,
quickError(500, 'rollback_failed', `Rollback failed deleting apikey
${apikeyData.id}: ${deleteError.message}`)) or otherwise surface/log the
deletion error, and only if the delete succeeded continue to throw the original
forbidden_binding quickError; apply this change to all three places where the
delete+throw pattern appears (the delete call followed immediately by
quickError).
- Around line 10-11: The imports from '../../private/role_bindings.ts' are out
of order for the perfectionist/sort-imports rule; fix by combining and/or
reordering them into a single sorted import so the type import is properly
grouped—e.g., import both createRoleBindingForPrincipal and type
CreateBindingParams from the same module in one statement (reference:
createRoleBindingForPrincipal, CreateBindingParams) and ensure imports are
alphabetically sorted to satisfy the lint rule.
- Around line 225-229: The catch block currently rethrows any error with
error?.code which can skip the rollback/cleanup path and break atomicity; update
the catch (error: any) handler so it only rethrows known internal
quickError/simpleError shapes (e.g., check for error?.status or a specific
marker used by our app) OR ensure rollback/cleanup is invoked unconditionally
before rethrowing when a DB/runtime error has occurred; in practice modify the
condition around throw error in the catch block to first perform the existing
rollback/cleanup logic (the same rollback called elsewhere in this function) for
errors with error.code and only skip rollback for trusted internal errors
(quickError/simpleError) detected by their explicit shape.
---
Outside diff comments:
In `@cli/src/types/supabase.types.ts`:
- Around line 26-52: The Supabase types are out of sync: the table type for
apikeys allows mode: null but RPC return types (e.g., the find_apikey_by_value
RPC type) still mark mode as non-null; update the RPC/result types to match the
table shape by regenerating the Supabase TypeScript types and committing the
updated file. Run the project’s type generation command (bun types) after the
migration so cli/src/types/supabase.types.ts (and RPC typings such as the return
type for find_apikey_by_value) reflect mode:
Database["public"]["Enums"]["key_mode"] | null and any other schema changes.
In `@supabase/functions/_backend/utils/supabase.types.ts`:
- Around line 26-54: The RPC typings were not regenerated so RPC methods still
declare non-null p_mode/mode; run the TS type regeneration and update RPC
interfaces to match the new nullable key mode: regenerate Supabase types (run
"bun types" per repo guideline) and ensure RPC signatures for
create_hashed_apikey*, regenerate_hashed_apikey*, and find_apikey_by_value
reflect nullable p_mode/mode (and any other RPC returns/params that reference
Database["public"]["Enums"]["key_mode"]) so backend callers see the updated
RBAC-managed API key contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94580966-5c63-4bd2-97d1-2d57d7c182b8
📒 Files selected for processing (7)
cli/src/types/supabase.types.tssrc/pages/settings/organization/ApiKeys.[id].vuesrc/types/supabase.types.tssupabase/functions/_backend/private/role_bindings.tssupabase/functions/_backend/public/apikey/post.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260501193952_apikey_nullable_mode_with_bindings.sql
…indings endpoint - Fix mode=NULL auth: checkKey, checkKeyPg, checkKeyById, checkKeyByIdPg now allow API keys with mode=NULL (RBAC-managed) to authenticate - Switch role_bindings endpoint from middlewareAuth to middlewareV2 with guard blocking limited-scope API keys - Fix migration: use CREATE OR REPLACE instead of DROP+CREATE to preserve existing function grants (fixes hashed key creation) - Update existing tests to include mode:'all' in POST /apikey requests (required by new validation when no bindings are provided) - Add atomic bindings test suite (9 tests)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
supabase/functions/_backend/private/role_bindings.ts (2)
459-462:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis anti-escalation check still ignores effective admin access.
checkPermission(...)is what lets the caller into this flow, butgetCallerMaxPriorityRank()only looks at directrole_bindings. A caller authorized via legacyorg_users.user_rightor another indirect/effective path is still treated as rank0here, so valid admins can be blocked from assigning higher-ranked roles.🤖 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 459 - 462, The anti-escalation check uses getCallerMaxPriorityRank(...) which only looks at direct role_bindings and ignores effective/legacy admin access granted via checkPermission(...) or org_users.user_right; update the logic so the caller's effective max priority rank is computed (either by extending getCallerMaxPriorityRank to consider checkPermission results and org_users.user_right or by calling checkPermission and mapping that to a high priority rank) before comparing with role.priority_rank; ensure the comparison uses that effective rank variable (e.g., effectiveCallerMaxRank) so callers authorized via legacy/effective paths are not incorrectly blocked.
478-481:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAPI-key principal validation still accepts stale owner access.
The helper delegates to
validatePrincipalAccess(), but theapikeybranch still treats any ownerorg_usersrow as valid and does not require unexpired RBAC access. That means an API key whose owner only has aninvite_%membership or an expired user binding can still receive fresh role bindings.🤖 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 478 - 481, The apikey branch currently treats any owner org_users row as valid even if it's an invite or expired; update the logic so when principal_type === 'apikey' (in role_bindings.ts where validatePrincipalAccess is called), you require the owner's org_users binding to be active and unexpired instead of accepting any owner row. Concretely: adjust validatePrincipalAccess (or add an immediate check after its return) to verify the owner binding from the org_users lookup is not an invite_* membership and that its RBAC expiry (or binding expiry) is in the future; if not, return ok: false with an appropriate status/error so stale/INVITED/expired owners cannot mint fresh role bindings. Ensure you reference the existing symbols validatePrincipalAccess, principal_type, principal_id, org_id and the org_users membership fields when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/utils/supabase.ts`:
- Around line 1471-1474: The current check treats data.mode === null as passing
legacy mode checks; instead, change the logic so NULL does NOT bypass legacy
mode but triggers an explicit RBAC validation path: keep the existing branch for
non-null modes (if data.mode !== null && !allowed.includes(data.mode) ... return
null), but when data.mode === null call the RBAC-specific authorization helper
(validate RBAC role binding / permission for the requested action) and only
allow the request if that RBAC check succeeds—otherwise return null; apply the
same change in the mirrored helper in hono_middleware.ts so both places use the
non-null legacy check and an explicit RBAC-only check for null modes.
In `@tests/apikey-atomic-bindings.test.ts`:
- Line 264: Rename the test title string in the failing it(...) declaration so
it begins with a lowercase letter; locate the it('RBAC-only key (mode=NULL) can
authenticate', ...) call and change the leading character to lowercase (for
example "rbac-only key (mode=NULL) can authenticate") to satisfy the
test/prefer-lowercase-title rule.
- Around line 159-193: Replace the global-count rollback assertion with a direct
check that the uniquely named key created in this test does not exist: capture
the test key name (the string `rollback-test-key-${TEST_ID.slice(0, 8)}`), then
query the `apikeys` table (using the same Supabase client) filtering by that
`name` and `user_id` and assert the result is empty after the failed POST in the
test "rolls back the API key when a binding fails"; update the assertion block
that currently compares `keysBefore`/`keysAfter` lengths to instead use this
name-based query so the test is safe to run in parallel.
---
Duplicate comments:
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 459-462: The anti-escalation check uses
getCallerMaxPriorityRank(...) which only looks at direct role_bindings and
ignores effective/legacy admin access granted via checkPermission(...) or
org_users.user_right; update the logic so the caller's effective max priority
rank is computed (either by extending getCallerMaxPriorityRank to consider
checkPermission results and org_users.user_right or by calling checkPermission
and mapping that to a high priority rank) before comparing with
role.priority_rank; ensure the comparison uses that effective rank variable
(e.g., effectiveCallerMaxRank) so callers authorized via legacy/effective paths
are not incorrectly blocked.
- Around line 478-481: The apikey branch currently treats any owner org_users
row as valid even if it's an invite or expired; update the logic so when
principal_type === 'apikey' (in role_bindings.ts where validatePrincipalAccess
is called), you require the owner's org_users binding to be active and unexpired
instead of accepting any owner row. Concretely: adjust validatePrincipalAccess
(or add an immediate check after its return) to verify the owner binding from
the org_users lookup is not an invite_* membership and that its RBAC expiry (or
binding expiry) is in the future; if not, return ok: false with an appropriate
status/error so stale/INVITED/expired owners cannot mint fresh role bindings.
Ensure you reference the existing symbols validatePrincipalAccess,
principal_type, principal_id, org_id and the org_users membership fields when
implementing the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27bde0f9-7b85-4e88-af54-5bccb9170009
📒 Files selected for processing (7)
supabase/functions/_backend/private/role_bindings.tssupabase/functions/_backend/utils/hono_middleware.tssupabase/functions/_backend/utils/supabase.tssupabase/migrations/20260501193952_apikey_nullable_mode_with_bindings.sqltests/apikey-atomic-bindings.test.tstests/apikeys-expiration.test.tstests/apikeys.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20260501193952_apikey_nullable_mode_with_bindings.sql
Contributor
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/apikey/post.ts (1)
126-157:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis is still a best-effort rollback, not an atomic create.
The key is inserted first through Supabase, then bindings are written through a separate PG client, and cleanup happens later with a compensating delete. A worker crash/timeout or mid-flight connection failure between those steps can still leave partially-applied state, so the endpoint does not actually provide the atomicity promised by the PR.
Use a single database transaction for the key row and all binding inserts, or move the whole flow into one RPC/SQL function so commit/rollback is decided once. As per coding guidelines
supabase/functions/_backend/**/*.ts: “Use getPgClient()/getDrizzleClient() for multi-step SQL, transactions, joins, and schema-backed writes; only use supabaseAdmin for simple single-statement internal helper writes when it keeps code smaller and equally correct”.Also applies to: 159-243
🧹 Nitpick comments (1)
tests/apikey-atomic-bindings.test.ts (1)
85-298: ⚡ Quick winUse
it.concurrent()here to match the test contract.These cases are still serialized with
it(...), even though this file already uses dedicated seed data and unique resource names. Converting them toit.concurrent(...)will align the suite with the repo’s test expectations.As per coding guidelines
tests/**/*.test.ts: “Design all tests for parallel execution across files; use it.concurrent() instead of it() to maximize parallelism within test files; create dedicated seed data for tests that modify resources”.Also applies to: 301-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikey-atomic-bindings.test.ts` around lines 85 - 298, Convert the serialized tests in the describe.skipIf(USE_CLOUDFLARE) block to run concurrently by replacing each it(...) call with it.concurrent(...); specifically update the tests with titles like 'creates an API key with bindings and no mode', 'creates an API key with mode and no bindings (backward compat)', 'rejects creating an API key without mode and without bindings', 'rolls back the API key when a binding fails', 'creates multiple bindings in a single call', 'validates binding shape before creation', and 'RBAC-only key (mode=NULL) can authenticate' to use it.concurrent so they run in parallel while keeping the same test bodies and unique resource names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/types/supabase.types.ts`:
- Around line 3389-3402: The get_org_apikeys RPC return type currently declares
mode as non-nullable Database["public"]["Enums"]["key_mode"]; update the return
object shape for get_org_apikeys so mode is nullable
(Database["public"]["Enums"]["key_mode"] | null) to match the schema where
apikeys.mode can be NULL for RBAC-only keys; locate the get_org_apikeys entry
and change the mode field's type accordingly so callers can handle null values.
In `@supabase/functions/_backend/public/apikey/post.ts`:
- Around line 78-90: The request handler currently allows a non-null legacy
`mode` together with RBAC `bindings` (checked via `hasBindings`), which can
create hybrid keys; update the validation so that if `hasBindings` is true and
`mode` is not null you immediately reject the request (e.g., throw
`simpleError('mode_with_bindings_not_allowed', 'Mode cannot be set when bindings
are provided')`), and keep the existing checks for when `mode` is present alone
(use `Constants.public.Enums.key_mode` to validate); locate this logic around
the `mode = body.mode ?? null`, `hasBindings`, and subsequent `if (mode !==
null)` block and add the new rejection before validating `mode`.
In `@supabase/functions/_backend/utils/supabase.types.ts`:
- Around line 3389-3402: The RPC type for get_org_apikeys incorrectly marks mode
as non-null; update the Returns type for get_org_apikeys so mode is nullable
(e.g., Database["public"]["Enums"]["key_mode"] | null) to match apikeys.Row.mode
and RBAC-only keys that set mode = null; locate the get_org_apikeys declaration
in supabase.types.ts and change the mode field's type accordingly so callers see
the nullable type.
---
Nitpick comments:
In `@tests/apikey-atomic-bindings.test.ts`:
- Around line 85-298: Convert the serialized tests in the
describe.skipIf(USE_CLOUDFLARE) block to run concurrently by replacing each
it(...) call with it.concurrent(...); specifically update the tests with titles
like 'creates an API key with bindings and no mode', 'creates an API key with
mode and no bindings (backward compat)', 'rejects creating an API key without
mode and without bindings', 'rolls back the API key when a binding fails',
'creates multiple bindings in a single call', 'validates binding shape before
creation', and 'RBAC-only key (mode=NULL) can authenticate' to use it.concurrent
so they run in parallel while keeping the same test bodies and unique resource
names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0de2e617-a5d3-4109-997c-fcf661764612
📒 Files selected for processing (6)
cli/src/types/supabase.types.tssrc/pages/settings/organization/ApiKeys.[id].vuesrc/types/supabase.types.tssupabase/functions/_backend/public/apikey/post.tssupabase/functions/_backend/utils/supabase.types.tstests/apikey-atomic-bindings.test.ts
Dalanir
force-pushed
the
feat/apikey-atomic-bindings
branch
from
271efa5 to
1bb6767
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
supabase/functions/_backend/utils/supabase.types.ts (1)
3389-3402:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_org_apikeysstill typesmodeas non-null.RBAC-only keys created by this PR persist
apikeys.mode = null, but this RPC still exposesmodeaskey_modewithout| null. This leaves the generated types out of sync withapikeys.Row.modeand can cause runtime surprises for callers.Suggested fix
get_org_apikeys: { Args: { p_org_id: string } Returns: { created_at: string expires_at: string id: number limited_to_apps: string[] limited_to_orgs: string[] - mode: Database["public"]["Enums"]["key_mode"] + mode: Database["public"]["Enums"]["key_mode"] | null name: string owner_email: string rbac_id: string user_id: string }[] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/supabase.types.ts` around lines 3389 - 3402, The generated RPC type for get_org_apikeys incorrectly marks mode as non-null; change the Returns type for mode in get_org_apikeys to allow null (i.e. Database["public"]["Enums"]["key_mode"] | null) so it matches apikeys.Row.mode, update the type annotation for get_org_apikeys's Returns accordingly, and re-generate/verify the supabase types to ensure get_org_apikeys, apikeys.mode and Database["public"]["Enums"]["key_mode"] are consistent across the typings.tests/apikey-atomic-bindings.test.ts (1)
260-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename this test to start with lowercase.
ESLint rule
test/prefer-lowercase-titlerequires test titles to begin with a lowercase letter.Suggested fix
- it('RBAC-only key (mode=NULL) can authenticate', async () => { + it('rbac-only key (mode=NULL) can authenticate', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikey-atomic-bindings.test.ts` at line 260, The test title "RBAC-only key (mode=NULL) can authenticate" in the it(...) declaration should be renamed to start with a lowercase letter to satisfy test/prefer-lowercase-title; update the it(...) title string in tests/apikey-atomic-bindings.test.ts (the test for RBAC-only key) to begin with a lowercase character (e.g., "rbac-only key (mode=NULL) can authenticate") keeping the rest of the text unchanged.supabase/functions/_backend/public/apikey/post.ts (1)
78-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject requests that send both
modeandbindings.The validation allows a non-null
modealongside RBACbindings. This creates a hybrid key whose effective permissions depend on how downstream auth code interprets both fields, potentially broadening access unexpectedly. Ifbindingsare present,modeshould be forced tonullor the request rejected.Suggested fix
const hasBindings = bindings.length > 0 // mode is required when no bindings are provided, optional (null) otherwise const mode = body.mode ?? null if (!name) { throw simpleError('name_is_required', 'Name is required') } + if (hasBindings && mode !== null) { + throw simpleError('invalid_mode', 'mode must be null when bindings are provided') + } if (!hasBindings && !mode) { throw simpleError('mode_is_required', 'Mode is required when no bindings are provided') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/apikey/post.ts` around lines 78 - 91, The current validation allows a non-null body.mode together with RBAC bindings (hasBindings), creating an ambiguous hybrid key; update the validation in post.ts to explicitly reject requests that provide both bindings and a non-null mode by adding a guard after computing const mode = body.mode ?? null and hasBindings: if hasBindings && mode !== null throw a simpleError (e.g., 'mode_with_bindings_not_allowed', 'Mode must be null when bindings are provided'); keep the existing validModes check for when mode is non-null and no bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dashboard/Usage.vue`:
- Around line 251-252: The code uses the nullish coalescing fallback (??) so
when state.stats_updated_at or state.stats_refresh_requested_at is explicitly
null the previous effectiveOrganization.value timestamps are preserved; change
the assignment to accept null from the server (i.e., assign
effectiveOrganization.value.stats_updated_at = state.stats_updated_at and
effectiveOrganization.value.stats_refresh_requested_at =
state.stats_refresh_requested_at or otherwise only treat undefined as "no value"
so null clears the value) so downstream checks like isChartRefreshInProgress /
isChartDataStale see null as the cleared state.
In `@src/services/apikeys.ts`:
- Around line 82-84: The sorter for the 'mode' case is comparing (a.mode ?? '')
and (b.mode ?? '') which treats null as an empty string while the UI in
ApiKeys.vue renders null as "RBAC"; update the 'mode' case in the sorter (the
switch branch handling 'mode' in src/services/apikeys.ts) to map null/undefined
to the same rendered label ("RBAC") before lowercasing so comparisons use the
same value users see (i.e., compute aValue and bValue by replacing nullish modes
with "RBAC" and then .toLowerCase()).
In `@src/types/supabase.types.ts`:
- Around line 3389-3403: The get_org_apikeys RPC return type incorrectly marks
apikeys.mode as non-nullable; update the return shape for get_org_apikeys so the
mode field is unioned with null (i.e. change mode:
Database["public"]["Enums"]["key_mode"] to mode:
Database["public"]["Enums"]["key_mode"] | null) to reflect the migration that
made apikeys.mode nullable; modify the type in src/types (the get_org_apikeys
block) so consumers see mode may be null.
In `@tests/cli-sdk-utils.ts`:
- Around line 236-237: The current guard treats apiKey.mode falsy and rejects
null-mode keys; update the check so null (explicitly === null) is allowed and
only undefined/missing modes or non-null modes lacking access are rejected.
Concretely, replace the condition using !apiKey.mode with a check like: if
(!apiKey || apiKey.mode === undefined || (apiKey.mode !== null &&
!hasModeAccess(apiKey.mode, allowedModes))) return the same error, and replicate
the same fix for the other occurrence that uses !apiKey.mode (the one noted in
the comment). Ensure you reference apiKey.mode, hasModeAccess, and allowedModes
when making this change.
---
Duplicate comments:
In `@supabase/functions/_backend/public/apikey/post.ts`:
- Around line 78-91: The current validation allows a non-null body.mode together
with RBAC bindings (hasBindings), creating an ambiguous hybrid key; update the
validation in post.ts to explicitly reject requests that provide both bindings
and a non-null mode by adding a guard after computing const mode = body.mode ??
null and hasBindings: if hasBindings && mode !== null throw a simpleError (e.g.,
'mode_with_bindings_not_allowed', 'Mode must be null when bindings are
provided'); keep the existing validModes check for when mode is non-null and no
bindings.
In `@supabase/functions/_backend/utils/supabase.types.ts`:
- Around line 3389-3402: The generated RPC type for get_org_apikeys incorrectly
marks mode as non-null; change the Returns type for mode in get_org_apikeys to
allow null (i.e. Database["public"]["Enums"]["key_mode"] | null) so it matches
apikeys.Row.mode, update the type annotation for get_org_apikeys's Returns
accordingly, and re-generate/verify the supabase types to ensure
get_org_apikeys, apikeys.mode and Database["public"]["Enums"]["key_mode"] are
consistent across the typings.
In `@tests/apikey-atomic-bindings.test.ts`:
- Line 260: The test title "RBAC-only key (mode=NULL) can authenticate" in the
it(...) declaration should be renamed to start with a lowercase letter to
satisfy test/prefer-lowercase-title; update the it(...) title string in
tests/apikey-atomic-bindings.test.ts (the test for RBAC-only key) to begin with
a lowercase character (e.g., "rbac-only key (mode=NULL) can authenticate")
keeping the rest of the text unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34d67e90-4c23-4b81-ac54-3e4c4a709ca6
📒 Files selected for processing (10)
cli/src/types/supabase.types.tssrc/components/dashboard/Usage.vuesrc/pages/ApiKeys.vuesrc/pages/settings/organization/ApiKeys.[id].vuesrc/services/apikeys.tssrc/types/supabase.types.tssupabase/functions/_backend/public/apikey/post.tssupabase/functions/_backend/utils/supabase.types.tstests/apikey-atomic-bindings.test.tstests/cli-sdk-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/settings/organization/ApiKeys.[id].vue
- cli/src/types/supabase.types.ts
Dalanir
force-pushed
the
feat/apikey-atomic-bindings
branch
from
1bb6767 to
c38671f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tests/apikey-atomic-bindings.test.ts (1)
260-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename this test title to start with lowercase.
This fails the
test/prefer-lowercase-titlelint rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikey-atomic-bindings.test.ts` at line 260, Rename the test title string in the it(...) call that currently reads "RBAC-only key (mode=NULL) can authenticate" to start with a lowercase letter (e.g., "rbac-only key (mode=NULL) can authenticate") so it satisfies the test/prefer-lowercase-title lint rule; update the title in the it declaration in tests/apikey-atomic-bindings.test.ts accordingly.src/services/apikeys.ts (1)
83-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSort
nullmode using the same label users see.
nullis currently sorted as'', but the table renders it as RBAC. Sorting should use the same normalized value to avoid inconsistent ordering behavior.Suggested fix
case 'mode': - aValue = (a.mode ?? '').toLowerCase() - bValue = (b.mode ?? '').toLowerCase() + aValue = (a.mode ?? 'rbac').toLowerCase() + bValue = (b.mode ?? 'rbac').toLowerCase() break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apikeys.ts` around lines 83 - 84, The comparator currently normalizes null/undefined mode to '' which differs from the UI label "RBAC" and causes inconsistent ordering; update the normalization where aValue and bValue are computed (using a.mode and b.mode) to map null/undefined to the same normalized label used by the table (e.g., 'rbac') and then lowercase it (e.g., (a.mode ?? 'rbac').toLowerCase()) so sorting matches the rendered "RBAC" entries; apply the same change for bValue.src/pages/settings/organization/ApiKeys.[id].vue (1)
492-528:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBlock submit when no bindings are selected before calling
/apikey.This path can still send
bindings: [], which the backend rejects. Add a client-side guard so users get immediate validation instead of a failed request.Suggested fix
function validateApiKeyForm() { if (!editName.value.trim()) { toast.error(t('please-enter-api-key-name')) return false } + + if (isCreateMode.value) { + const hasAnyBinding = !!selectedOrgRole.value || Object.values(pendingAppBindings.value).some(roleName => !!roleName) + if (!hasAnyBinding) { + toast.error(t('select-user-role')) + return false + } + } if (hasIncompleteAppBindings()) { toast.error(t('select-role-for-each-app')) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/organization/ApiKeys`.[id].vue around lines 492 - 528, The current submit path can call supabase.functions.invoke('apikey', ...) with bindings potentially equal to [], which the backend rejects; before the invoke (just after building the bindings array in the submit handler that references selectedOrgRole, pendingAppBindings, bindings, etc.) add a client-side guard to check if bindings.length === 0 and abort submission: set a visible validation error state (or return early) and prevent the API call (do not call supabase.functions.invoke) so users get immediate feedback; ensure the same check is used to disable the submit button or show an inline error so empty bindings cannot be sent.supabase/functions/_backend/utils/supabase.types.ts (1)
3389-3402:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_org_apikeysstill typesmodeas non-null.
apikeys.modeis nullable now, so this RPC typing is out of sync and can mislead callers into unsafe assumptions.Suggested fix
get_org_apikeys: { Args: { p_org_id: string } Returns: { created_at: string expires_at: string id: number limited_to_apps: string[] limited_to_orgs: string[] - mode: Database["public"]["Enums"]["key_mode"] + mode: Database["public"]["Enums"]["key_mode"] | null name: string owner_email: string rbac_id: string user_id: string }[] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/supabase.types.ts` around lines 3389 - 3402, The RPC typing for get_org_apikeys incorrectly marks the mode field as non-nullable; update the Returns type so the mode property is nullable (e.g., change mode: Database["public"]["Enums"]["key_mode"] to allow null) so callers see that apikeys.mode can be null; adjust the get_org_apikeys return type accordingly wherever it appears to use Database["public"]["Enums"]["key_mode"] | null.cli/src/types/supabase.types.ts (1)
3389-3402:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
get_org_apikeys().modenullable.Line 3397 still exposes
modeas non-null, but RBAC-only keys can legitimately returnNULL. This makes the CLI contract unsafe for a valid runtime state.Suggested fix
get_org_apikeys: { Args: { p_org_id: string } Returns: { created_at: string expires_at: string id: number limited_to_apps: string[] limited_to_orgs: string[] - mode: Database["public"]["Enums"]["key_mode"] + mode: Database["public"]["Enums"]["key_mode"] | null name: string owner_email: string rbac_id: string user_id: string }[] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/types/supabase.types.ts` around lines 3389 - 3402, The get_org_apikeys return type declares mode as non-null but RBAC-only keys can be NULL; update the return type for get_org_apikeys (refer to get_org_apikeys in supabase.types.ts) so the mode field is nullable (e.g., change its type to allow null, Database["public"]["Enums"]["key_mode"] | null) ensuring callers handle possible null values.
🧹 Nitpick comments (1)
tests/apikey-atomic-bindings.test.ts (1)
86-341: 🏗️ Heavy liftUse
it.concurrent()in this.test.tssuite.This new file uses serial
it(...)throughout; it should be designed for parallel execution.As per coding guidelines, "Design all tests for parallel execution across files; use it.concurrent() instead of it()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikey-atomic-bindings.test.ts` around lines 86 - 341, The test file uses serial it(...) blocks which prevents parallel execution; update every test declaration to use it.concurrent(...) instead (i.e., replace calls to it('...', async () => { ... }) and the nested it(...) usages inside both describe blocks with it.concurrent('...', async () => { ... })); ensure all tests that assert async behavior (e.g., the "creates an API key..." tests, "rolls back the API key..." test, "creates multiple bindings..." test, "validates binding shape..." test, "RBAC-only key..." test, and the role_bindings tests) are converted so they run concurrently without changing their bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/types/supabase.types.ts`:
- Around line 3043-3050: The RPC arg type for p_mode is currently optional but
disallows null; update the type in the create_hashed_apikey Args (and the other
RPC Args block around lines 3072-3079) to allow explicit null by changing
p_mode's type from Database["public"]["Enums"]["key_mode"] to
Database["public"]["Enums"]["key_mode"] | null so the generated types match the
RPCs' default NULL behavior.
In `@tests/apikey-atomic-bindings.test.ts`:
- Around line 231-235: In the failure branch replace the loose check with an
explicit DB assertion that the API key created earlier in this test was rolled
back: after parsing response.json(), query the same storage used elsewhere in
this test for the key name (the variable holding the test key name) and assert
it does not exist (e.g.
expect(getKeyByName(testKeyName)).toBeNull()/toBeUndefined() or
expect(rows).toHaveLength(0)); keep the existing
expect(data).toHaveProperty('error') as well to verify the error response.
---
Duplicate comments:
In `@cli/src/types/supabase.types.ts`:
- Around line 3389-3402: The get_org_apikeys return type declares mode as
non-null but RBAC-only keys can be NULL; update the return type for
get_org_apikeys (refer to get_org_apikeys in supabase.types.ts) so the mode
field is nullable (e.g., change its type to allow null,
Database["public"]["Enums"]["key_mode"] | null) ensuring callers handle possible
null values.
In `@src/pages/settings/organization/ApiKeys`.[id].vue:
- Around line 492-528: The current submit path can call
supabase.functions.invoke('apikey', ...) with bindings potentially equal to [],
which the backend rejects; before the invoke (just after building the bindings
array in the submit handler that references selectedOrgRole, pendingAppBindings,
bindings, etc.) add a client-side guard to check if bindings.length === 0 and
abort submission: set a visible validation error state (or return early) and
prevent the API call (do not call supabase.functions.invoke) so users get
immediate feedback; ensure the same check is used to disable the submit button
or show an inline error so empty bindings cannot be sent.
In `@src/services/apikeys.ts`:
- Around line 83-84: The comparator currently normalizes null/undefined mode to
'' which differs from the UI label "RBAC" and causes inconsistent ordering;
update the normalization where aValue and bValue are computed (using a.mode and
b.mode) to map null/undefined to the same normalized label used by the table
(e.g., 'rbac') and then lowercase it (e.g., (a.mode ?? 'rbac').toLowerCase()) so
sorting matches the rendered "RBAC" entries; apply the same change for bValue.
In `@supabase/functions/_backend/utils/supabase.types.ts`:
- Around line 3389-3402: The RPC typing for get_org_apikeys incorrectly marks
the mode field as non-nullable; update the Returns type so the mode property is
nullable (e.g., change mode: Database["public"]["Enums"]["key_mode"] to allow
null) so callers see that apikeys.mode can be null; adjust the get_org_apikeys
return type accordingly wherever it appears to use
Database["public"]["Enums"]["key_mode"] | null.
In `@tests/apikey-atomic-bindings.test.ts`:
- Line 260: Rename the test title string in the it(...) call that currently
reads "RBAC-only key (mode=NULL) can authenticate" to start with a lowercase
letter (e.g., "rbac-only key (mode=NULL) can authenticate") so it satisfies the
test/prefer-lowercase-title lint rule; update the title in the it declaration in
tests/apikey-atomic-bindings.test.ts accordingly.
---
Nitpick comments:
In `@tests/apikey-atomic-bindings.test.ts`:
- Around line 86-341: The test file uses serial it(...) blocks which prevents
parallel execution; update every test declaration to use it.concurrent(...)
instead (i.e., replace calls to it('...', async () => { ... }) and the nested
it(...) usages inside both describe blocks with it.concurrent('...', async () =>
{ ... })); ensure all tests that assert async behavior (e.g., the "creates an
API key..." tests, "rolls back the API key..." test, "creates multiple
bindings..." test, "validates binding shape..." test, "RBAC-only key..." test,
and the role_bindings tests) are converted so they run concurrently without
changing their bodies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c44151b-1bba-472e-b989-5a242e2cbaab
📒 Files selected for processing (11)
cli/src/types/supabase.types.tssrc/components/dashboard/Usage.vuesrc/pages/ApiKeys.vuesrc/pages/settings/organization/ApiKeys.[id].vuesrc/services/apikeys.tssrc/types/supabase.types.tssupabase/functions/_backend/public/apikey/post.tssupabase/functions/_backend/utils/supabase.types.tstests/apikey-atomic-bindings.test.tstests/cli-sdk-utils.tstests/organization-api.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/cli-sdk-utils.ts
- src/components/dashboard/Usage.vue
- src/pages/ApiKeys.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/public/apikey/post.ts
…, empty bindings validation, and stale invite check - getCallerMaxPriorityRank now considers legacy org_users.user_right for JWT callers so that admins without RBAC bindings are not blocked by anti-escalation - validateApiKeyPrincipalAccess filters out invite_% memberships and rejects pending invites, mirroring the user-principal validation - Client-side validateApiKeyForm requires at least one binding in create mode to prevent a useless 400 roundtrip
…et_and_seed_* functions)
|
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
1 participant
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
POST /apikeycall instead of requiring two separate API calls (create key + assign bindings)modecolumn nullable in theapikeystable —NULLmeans permissions are managed exclusively via RBACrole_bindingsChanges
Database
20260501193952_apikey_nullable_mode_with_bindings.sql):ALTER COLUMN mode DROP NOT NULL, recreatecreate_hashed_apikeyandcreate_hashed_apikey_for_userRPCs withp_mode DEFAULT NULLBackend
POST /apikey(post.ts): Accepts optionalbindings[]array in the request body. Validates mode/bindings consistency, creates the key, then creates each binding via the extracted helper. Rolls back on failure.role_bindings.ts: ExtractedcreateRoleBindingForPrincipal()— a reusable function encapsulating the 7-step validation + insert pipeline, usable outside the Hono route context.Frontend
ApiKeys.[id].vue:createApiKeyRecord()now sends bindings in the same request body. Removed client-side rollback logic and hardcodedmode: 'all'.Types
supabase.types.tsfiles (backend, frontend, CLI):modeisstring | nullon Row, optional on Insert.Testing
Manual testing recommended:
mode→ should work as beforemode→ key created withmode = NULL, bindings attachedFollow-up (not in this PR)
/private/role_bindingsmiddleware frommiddlewareAuth(JWT-only) tomiddlewareV2to allow API keys to manage their own bindingsSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests