Fix API Key Validation and Improve Error Handling #396
Conversation
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
…CRET_PREFIX with fallback
Contributor
## Walkthrough
The changes introduce a configurable secret prefix for API keys using the new `API_KEY_SECRET_PREFIX` environment variable, defaulting to "sk-". The `APIKeyService` class now raises custom exceptions (`InvalidAPIKeyFormatError`, `APIKeyNotFoundError`) for different validation failures instead of returning `None`. Exception handling in the `revoke_api_key` method is refined to specifically catch the `NotFound` exception. The `.env.template` file is updated to include the new environment variable. Additionally, the API router now explicitly handles the `InvalidAPIKeyFormatError` to return a 401 HTTP error with a specific message.
## Changes
| File(s) | Change Summary |
|---------------------------------|-----------------------------------------------------------------------------------------------------------------|
| .env.template | Added `API_KEY_SECRET_PREFIX` environment variable placeholder. |
| app/modules/auth/api_key_service.py | Added custom exception classes for API key validation errors, made secret prefix configurable, updated `validate_api_key` to raise exceptions instead of returning `None`, and refined exception handling in `revoke_api_key`. |
| app/api/router.py | Updated `get_api_key_user` function to catch `InvalidAPIKeyFormatError` and raise a 401 HTTP error with a specific message and header. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant APIKeyService
participant DB
participant API_Router
Client->>API_Router: get_api_key_user(api_key)
API_Router->>APIKeyService: validate_api_key(api_key, db)
APIKeyService->>APIKeyService: Check prefix (from env or default)
alt Invalid prefix
APIKeyService-->>API_Router: Raise InvalidAPIKeyFormatError
API_Router-->>Client: 401 HTTP Error "Invalid API key format"
else Valid prefix
APIKeyService->>APIKeyService: Hash API key
APIKeyService->>DB: Query for hashed key
alt Key not found
APIKeyService-->>API_Router: Raise APIKeyNotFoundError
API_Router-->>Client: 401 HTTP Error "Invalid API key or user not found"
else Key found
APIKeyService-->>API_Router: Return user info
API_Router-->>Client: Return user info
end
endAssessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
|
![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: 1
🧹 Nitpick comments (2)
.env.template (1)
34-34: Good addition of configurable API key prefix.Adding the
API_KEY_SECRET_PREFIXenvironment variable improves flexibility by allowing customization of the API key prefix without code changes.Consider adding a brief comment to document the purpose and expected format of this variable, for example:
-API_KEY_SECRET_PREFIX= +# Prefix for API keys (default: "sk-" if not specified) +API_KEY_SECRET_PREFIX=app/modules/auth/api_key_service.py (1)
114-114: Update method signature to reflect exception behavior.The method now raises exceptions instead of returning
Nonein error cases, but the type annotation doesn't reflect this change.- async def validate_api_key(api_key: str, db: Session) -> dict: + async def validate_api_key(api_key: str, db: Session) -> dict: + """Validate an API key and return user info if valid. + + Raises: + InvalidAPIKeyFormatError: If the API key does not start with the expected prefix + APIKeyNotFoundError: If no matching API key is found in the database + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.template(1 hunks)app/modules/auth/api_key_service.py(4 hunks)
🔇 Additional comments (5)
app/modules/auth/api_key_service.py (5)
13-14: Appropriate import for specific exception handling.Good improvement to import the specific
NotFoundexception instead of catching generic exceptions.
16-25: Great addition of custom exception classes.Creating a hierarchy of custom exceptions with a base class and specific error types is an excellent practice that improves error handling clarity.
29-29: Good implementation of configurable secret prefix.Making the secret prefix configurable via environment variable with a sensible default value of "sk-" improves flexibility while maintaining backward compatibility.
133-133: Improved error handling with specific exceptions.Raising
APIKeyNotFoundErrorwith a clear message is more informative than returningNoneand enforces proper error handling.As with the previous exception, ensure all existing callers are updated to handle this exception.
162-162: More precise exception handling.Catching the specific
NotFoundexception rather than a generic exception is a good practice. This avoids masking unexpected errors while still handling the expected case where a secret doesn't exist.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/api/router.py (1)
66-74: Improve exception handling by preserving the exception chainThe implementation correctly catches the
InvalidAPIKeyFormatErrorand raises an appropriate HTTP exception with a clear error message. However, the static analysis tool suggests usingraise ... from errorraise ... from Noneto preserve the exception chain, which is useful for debugging.try: user = await APIKeyService.validate_api_key(x_api_key, db) - except InvalidAPIKeyFormatError: + except InvalidAPIKeyFormatError as err: raise HTTPException( status_code=401, detail="Invalid API key format", headers={"WWW-Authenticate": "ApiKey"}, - ) + ) from errAlso, consider explicitly handling
APIKeyNotFoundErrorif it's raised byvalidate_api_key()as mentioned in the PR summary, instead of relying on the subsequentif not user:check.🧰 Tools
🪛 Ruff (0.8.2)
69-73: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/api/router.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/api/router.py
69-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
app/api/router.py (1)
10-10: LGTM - Good addition of InvalidAPIKeyFormatError importThe import has been correctly updated to include the new custom exception class, which supports the improved error handling implementation.
Author
|
@dhirenmathur hey dhiren raised this PR for a while now , would love to know your thoughts about it . Like did a follow same pattern as you guys or not . Get back to me whenever you get the time . |
dhirenmathur
requested review from
kinshuksinghbist and
nndn
and removed request for
nndn
|
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.
Issue
Fixes #357
What I changed
Summary by CodeRabbit
New Features
Bug Fixes