[codex] Fix API key rights editing#2391
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBackend PUT now accepts optional RBAC binding updates and can transactionally replace an API key’s role_bindings with permission checks; frontend adds an edit workflow and unified modal that hydrates bindings via a shared builder; tests and i18n keys added; route typing extended. ChangesAPI Key Edit Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelscodex 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/public/apikey/put.ts (1)
301-321:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the PUT path atomic across metadata and binding changes.
Lines 301-321 commit
updateDatabeforereplaceApiKeyBindings()does its permission checks and transactional rewrite. If the binding step later fails or returns 403, the request errors but the name/expiration change is already persisted. Move theapikeysrow update into the same pg transaction as the binding replacement, or pre-validate all binding auth before updating the row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/functions/_backend/public/apikey/put.ts` around lines 301 - 321, The update currently writes updateData via supabase.from('apikeys').update(...) before calling replaceApiKeyBindings(...), which can leave metadata changes persisted if binding replacement fails; fix by making the metadata update and binding replacements atomic: either move the apikeys update into the same Postgres transaction used by replaceApiKeyBindings (extend replaceApiKeyBindings to accept a transaction/pg client and perform the .update(...) inside that transaction), or pre-validate binding permissions by reusing the same checks replaceApiKeyBindings uses (call its permission/validation logic first) and only then call supabase.from('apikeys').update(...); ensure you use existingApikey.id and auth.userId for the update and propagate any errors back as quickError on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/ApiKeys.vue`:
- Around line 996-1021: The buildApiKeyBindingsFromForm function currently
applies a single selectedOrgRole to every org in selectedOrgsForCreation, which
overwrites per-org roles (see editableOrgBindings usage elsewhere); fix by
detecting mixed org roles and either (A) prevent/save by blocking edits when
editableOrgBindings contains more than one distinct role (show a
validation/error and disable save), or (B) change the form state to model org
role per org (replace selectedOrgRole with a mapping like selectedOrgRolesById
and use that mapping when building bindings) so buildApiKeyBindingsFromForm
pushes the correct role_name for each org instead of reusing one role for all.
In `@tests/apikeys.test.ts`:
- Around line 296-337: Wrap the API key creation, update and assertions in a
try/finally block so the cleanup DELETE call always runs: after creating the key
(createResponse/createData from the POST to /apikey) perform the update and all
expects inside try, and move the final fetch DELETE call that uses createData.id
into the finally block; ensure you still assert status codes and supabase checks
(getSupabaseClient().from('role_bindings') etc.) inside the try, and change the
test declaration to use it.concurrent(...) so the test is safe to run in
parallel.
---
Outside diff comments:
In `@supabase/functions/_backend/public/apikey/put.ts`:
- Around line 301-321: The update currently writes updateData via
supabase.from('apikeys').update(...) before calling replaceApiKeyBindings(...),
which can leave metadata changes persisted if binding replacement fails; fix by
making the metadata update and binding replacements atomic: either move the
apikeys update into the same Postgres transaction used by replaceApiKeyBindings
(extend replaceApiKeyBindings to accept a transaction/pg client and perform the
.update(...) inside that transaction), or pre-validate binding permissions by
reusing the same checks replaceApiKeyBindings uses (call its
permission/validation logic first) and only then call
supabase.from('apikeys').update(...); ensure you use existingApikey.id and
auth.userId for the update and propagate any errors back as quickError on
failure.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f817b59-4279-4c59-8ef3-9c8447c61366
⛔ Files ignored due to path filters (1)
.github/pr-screenshots/apikey-edit-rights.pngis excluded by!**/*.png
📒 Files selected for processing (6)
messages/en.jsonplaywright/e2e/apikeys.spec.tssrc/pages/ApiKeys.vuesrc/route-map.d.tssupabase/functions/_backend/public/apikey/put.tstests/apikeys.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/_backend/public/apikey/put.ts`:
- Around line 327-347: When bindings are updated but other fields are not,
updatedApikey remains the partial existingApikey; change the logic so that after
calling replaceApiKeyBindings (when hasBindingUpdates is true) you always
re-fetch the apikey row from supabase (same query used in the hasUpdates branch)
and assign it to updatedApikey, instead of only doing that when hasUpdates is
true—use the existing supabase .from('apikeys').select().eq('id',
existingApikey.id).eq('user_id', auth.userId').single() flow and preserve the
existing error handling (quickError) so the response contains the full apikey
fields.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01912d52-b536-46da-b97f-02f390a9ae10
📒 Files selected for processing (3)
src/pages/ApiKeys.vuesupabase/functions/_backend/public/apikey/put.tstests/apikeys.test.ts
…ts-edit # Conflicts: # playwright/e2e/apikeys.spec.ts # src/pages/ApiKeys.vue
|
1 participant
Summary (AI generated)
PUT /apikeyto validate and replace API key RBAC bindings atomically.Motivation (AI generated)
API key edit actions only supported name and expiration updates, so users could create API keys with rights but could not later adjust those rights from the API Keys page. The frontend now opens the same rights controls for editing, and the backend now safely replaces bindings after checking the caller can manage all affected organizations.
Business Impact (AI generated)
This restores expected API key management for customers and reduces support friction when teams need to rotate or tighten API key access without deleting and recreating keys.
Test Plan (AI generated)
bun lintbun lint:backendbun typecheckbun run supabase:with-env -- bunx vitest run tests/apikeys.test.ts -t "updates api key role bindings"bun run test:front -- playwright/e2e/apikeys.spec.tsbun run test:front -- playwright/e2e/__tmp_apikey_screenshot.spec.tsfor the PR screenshot capture, then removed the temporary spec before commit.Generated with AI
Summary by CodeRabbit
New Features
Backend
Tests
Localization