feat: API key list, revoke, and specific 401 errors (OPE-164)#288
Merged
DevanshuNEU merged 4 commits intoMar 8, 2026
Merged
Conversation
Backend:
- GET /api/v1/keys -- list user's API keys (masked, with metadata)
- DELETE /api/v1/keys/{key_id} -- revoke by ID (soft delete, verifies ownership)
- APIKeyManager.list_keys(user_id) -- returns masked keys with preview
- APIKeyManager.revoke_key_by_id(key_id, user_id) -- ownership-safe revoke
Auth:
- Specific 401 errors for ci_ keys: 'not found', 'revoked', 'no linked user'
instead of generic 'Invalid token or API key'
- Would have saved us 2 hours of debugging today
Existing endpoints (unchanged):
- POST /api/v1/keys/generate -- already existed, works
- GET /api/v1/keys/usage -- already existed, works
392 tests pass.
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds precise auth error reporting for Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MW as AuthMiddleware
participant Service as APIKeyService
participant DB as Database
rect rgba(200,220,255,0.5)
Client->>MW: Request with token (e.g., "ci_...")
MW->>MW: Attempt primary JWT auth -> fails
MW->>Service: Lookup/validate API key by token
Service->>DB: Query api_keys (by hash / suffix)
DB-->>Service: Record (found / not found / revoked / no_user)
Service-->>MW: Key status & metadata
alt key valid with linked user
MW->>Client: Proceed with authenticated context
else failure cases
MW->>Client: 401 with specific failure reason
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/routes/api_keys.py (1)
96-136: Add return annotations to the new route handlers.
list_api_keys()andrevoke_api_key()are both new backend endpoints, but neither signature declares a return type. As per coding guidelines, "Type hints on all function signatures in Python backend".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/api_keys.py` around lines 96 - 136, The two new route handlers list_api_keys and revoke_api_key lack return type annotations; update their signatures to include explicit return types (e.g., from typing import Dict, Any and annotate as -> Dict[str, Any] or use FastAPI Response types) so both functions declare their return types (add "-> Dict[str, Any]" to list_api_keys and revoke_api_key), and add any required imports (Dict, Any) at the top of the module to satisfy the "Type hints on all function signatures" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/middleware/auth.py`:
- Around line 163-179: The linked-user check for ci_ keys is currently
unreachable because _validate_api_key() returns before verifying user_id; move
the "has no linked user" validation into _validate_api_key() so it inspects the
Supabase row (key_hash/user_id/active) prior to constructing/returning the
AuthContext and raise a 401 (or return the error detail) when user_id is
missing. Concretely, inside _validate_api_key() use the same
get_supabase_service() lookup used for token.startswith("ci_"), verify
result.data[0].get("user_id") and return unauthorized with the detail "API key
has no linked user. Contact admin." if absent, ensuring the
token.startswith("ci_") block no longer needs to attempt that check after
_validate_api_key() returns.
In `@backend/routes/api_keys.py`:
- Around line 113-136: The route revoke_api_key currently accepts key_id as str
which lets malformed IDs reach the DB; change the parameter type to UUID (import
UUID from uuid) in the function signature for revoke_api_key so FastAPI
validates incoming key_id at the boundary and returns a 422 for bad UUIDs,
leaving the existing try/except logic (api_key_manager.revoke_key_by_id call,
logging, and error handling) unchanged.
In `@backend/services/rate_limiter.py`:
- Around line 221-235: The key preview is currently derived from the stored
SHA-256 key_hash (variable h) which won't match the user's raw key; instead,
change this code to read and return a dedicated persisted display suffix (e.g.,
"display_suffix" or "key_preview_suffix") that must be saved when the key is
created. Update the keys loop to use row.get("display_suffix") (or similar) to
build "key_preview" (e.g., f"ci_...{display_suffix}") with a safe fallback like
"ci_..." if absent, and ensure the API key creation code (the function that
inserts new api_keys rows) is updated to generate and persist that display
suffix at creation time.
- Around line 212-236: Convert revoke_key_by_id and list_keys to asynchronous
functions: change def revoke_key_by_id(...) -> bool and def list_keys(...) ->
list to async def, and replace synchronous .execute() calls with awaited async
calls (await self.db.table(...).update(...).eq(...).execute() and await
self.db.table(...).select(...).eq(...).order(...).execute()). Keep return values
the same (bool from revoke_key_by_id and list from list_keys) and preserve the
result.data handling and key masking logic (references: revoke_key_by_id,
list_keys, and usage of result.data and key_hash).
---
Nitpick comments:
In `@backend/routes/api_keys.py`:
- Around line 96-136: The two new route handlers list_api_keys and
revoke_api_key lack return type annotations; update their signatures to include
explicit return types (e.g., from typing import Dict, Any and annotate as ->
Dict[str, Any] or use FastAPI Response types) so both functions declare their
return types (add "-> Dict[str, Any]" to list_api_keys and revoke_api_key), and
add any required imports (Dict, Any) at the top of the module to satisfy the
"Type hints on all function signatures" guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 545bc382-1fba-44b0-872f-858abfe8f00b
📒 Files selected for processing (3)
backend/middleware/auth.pybackend/routes/api_keys.pybackend/services/rate_limiter.py
3 issues found during frontend planning: 1. Tier self-escalation: users could set tier='enterprise' in the request body. Fixed: tier locked to auth.tier, removed from request. 2. No key limit: unlimited key generation. Fixed: max 5 active keys per user, returns 403 with clear message. 3. generate_key returned only the raw key string, no ID for frontend list. Fixed: returns dict with key, id, name, tier. Also: generate route now returns 'id' so frontend can show the key in the list immediately without refetching. Tests updated: mock returns dict, CreateAPIKeyRequest no longer accepts tier. 392 tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/routes/api_keys.py (1)
64-65: Redundant fallback to"free".Based on
backend/middleware/auth.py,AuthContext.tieris declared astier: str = "free"and all code paths that createAuthContextexplicitly set tier to a non-None value. Theor "free"fallback is unreachable.Suggested simplification
- # Tier is locked to the user's auth tier (no self-escalation) - tier = auth.tier or "free" + # Tier is locked to the user's auth tier (no self-escalation) + tier = auth.tier🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/api_keys.py` around lines 64 - 65, The assignment "tier = auth.tier or \"free\"" redundantly falls back to "free" because AuthContext.tier is always set; remove the unreachable fallback and assign directly from auth.tier by replacing that expression with a direct use of auth.tier (referencing the variable name tier and the auth.tier property and AuthContext.tier declaration) so the code simply locks the route's tier to auth.tier without the unnecessary "or 'free'".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/routes/api_keys.py`:
- Around line 64-65: The assignment "tier = auth.tier or \"free\"" redundantly
falls back to "free" because AuthContext.tier is always set; remove the
unreachable fallback and assign directly from auth.tier by replacing that
expression with a direct use of auth.tier (referencing the variable name tier
and the auth.tier property and AuthContext.tier declaration) so the code simply
locks the route's tier to auth.tier without the unnecessary "or 'free'".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bd78bab-43c4-4e6d-85a7-18fda06c145d
📒 Files selected for processing (3)
backend/routes/api_keys.pybackend/services/rate_limiter.pybackend/tests/test_observability_routes.py
…preview suffix 1. auth.py: 'no linked user' check moved INTO _validate_api_key() where it can actually fire. Previously it was in _authenticate's error block which only ran when _validate_api_key returned None (meaning the key wasn't found at all, so user_id was never checked). 2. api_keys.py: key_id parameter changed from str to UUID. FastAPI now returns 422 for malformed UUIDs before hitting the DB. Added return type annotations (Dict[str, Any]) to list and revoke routes. 3. rate_limiter.py: key_preview now uses persisted key_suffix (last 8 chars of raw key) instead of SHA-256 hash suffix. Users see 'ci_...xYz12345' matching their actual key, not hash gibberish. 4. 004_api_keys.sql: added key_suffix column. Skipped: async conversion of service methods (Supabase client is sync; all other APIKeyManager methods are sync, would be inconsistent). Dev: run ALTER TABLE api_keys ADD COLUMN key_suffix text; in Supabase. 392 tests pass.
AuthContext.tier defaults to 'free' in the dataclass and every construction path sets it explicitly. The 'or free' was dead code.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/routes/api_keys.py (1)
67-68: Minor:auth.tieralways has a default value.Per
AuthContextdefinition inbackend/middleware/auth.py(line 48),tier: str = "free"is non-Optional with a default, soauth.tierwill never beNone. Theor "free"fallback is safe but redundant.This is a nit and doesn't affect correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/api_keys.py` around lines 67 - 68, Remove the redundant fallback when assigning the user's tier: replace the line that sets tier using "tier = auth.tier or \"free\"" with a direct assignment from auth.tier since AuthContext defines tier with a default ("free") and auth.tier is never None; locate the assignment in the API keys route (the statement using auth.tier) and simply use auth.tier, referencing AuthContext and the variable names auth.tier and tier to find the code to update.supabase/migrations/004_api_keys.sql (1)
16-16: Consider addingkey_suffixto the immutable columns trigger.The
key_suffixcolumn is not protected byprotect_api_key_immutable_cols()(lines 30-47), allowing users to modify it via the UPDATE RLS policy. While this doesn't pose a security risk since it's purely for display, it could lead to confusion if a user changes their key preview to something misleading.If you want to keep
key_suffiximmutable after creation, add it to the trigger:🔧 Proposed fix
+ IF NEW.key_suffix IS DISTINCT FROM OLD.key_suffix THEN + RAISE EXCEPTION 'Cannot modify key_suffix'; + END IF; RETURN NEW;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/004_api_keys.sql` at line 16, The key_suffix column isn't included in the immutable-columns check inside the protect_api_key_immutable_cols() trigger, so add "key_suffix" to the list of protected/immutable columns in that trigger/function (the same place where columns like raw_key, key_type, etc. are checked) so UPDATEs cannot change key_suffix after creation; update the protect_api_key_immutable_cols() function to include key_suffix in its immutable columns check and ensure the trigger re-raises or rejects updates that attempt to modify it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/routes/api_keys.py`:
- Around line 67-68: Remove the redundant fallback when assigning the user's
tier: replace the line that sets tier using "tier = auth.tier or \"free\"" with
a direct assignment from auth.tier since AuthContext defines tier with a default
("free") and auth.tier is never None; locate the assignment in the API keys
route (the statement using auth.tier) and simply use auth.tier, referencing
AuthContext and the variable names auth.tier and tier to find the code to
update.
In `@supabase/migrations/004_api_keys.sql`:
- Line 16: The key_suffix column isn't included in the immutable-columns check
inside the protect_api_key_immutable_cols() trigger, so add "key_suffix" to the
list of protected/immutable columns in that trigger/function (the same place
where columns like raw_key, key_type, etc. are checked) so UPDATEs cannot change
key_suffix after creation; update the protect_api_key_immutable_cols() function
to include key_suffix in its immutable columns check and ensure the trigger
re-raises or rejects updates that attempt to modify it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57a5766a-e698-44df-8eb3-f95e8d0835fb
📒 Files selected for processing (4)
backend/middleware/auth.pybackend/routes/api_keys.pybackend/services/rate_limiter.pysupabase/migrations/004_api_keys.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/middleware/auth.py
DevanshuNEU
added a commit
to DevanshuNEU/opencodeintel-fork-new
that referenced
this pull request
Mar 8, 2026
New page at /dashboard/api-keys for managing MCP API keys: - Generate modal: name input, creates key, shows raw key ONCE with copy-to-clipboard and 'will not be shown again' warning - Key list: name, preview (ci_...suffix), tier badge, active/revoked status, last used timestamp, revoke button - Empty state with helpful context about what keys are for - Quick setup hint pointing to Claude Desktop config path - Sidebar: added 'API Keys' nav item with KeyRound icon - Command palette: added API Keys navigation entry - Max 5 active keys enforced (button disabled at limit) Uses existing patterns: useAuth(), API_URL, Bearer token, shadcn/ui Card/Dialog/Badge/Button, toast from sonner. Backend endpoints from PR OpenCodeIntel#288: - POST /api/v1/keys/generate - GET /api/v1/keys - DELETE /api/v1/keys/{key_id} TypeScript clean, build passes.
This was referenced Mar 8, 2026
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.
What
Completes the backend API key management endpoints. We discovered via dogfooding that
POST /keys/generatealready existed -- this PR adds the missing list and revoke.New endpoints
Existing (unchanged)
Service layer
APIKeyManager.list_keys(user_id)-- returns masked keys:ci_...f2e8b3a1APIKeyManager.revoke_key_by_id(key_id, user_id)-- ownership-safe revokeBetter 401 errors
Before:
Invalid token or API key(for ALL failures)After (for ci_ keys):
API key not found. Check that the key is correct.API key has been revoked.API key has no linked user. Contact admin.This would have saved us 2 hours of debugging today.
Dogfooding note
Found the existing
generate_api_keyendpoint by runningcodeintel:search_codeon our own indexed repo. First real dogfood discovery.Testing
392 backend tests pass. 3 files, +93 -3 lines.
Summary by CodeRabbit
New Features
Improvements