Feat: add API key management system#9177
Open
Shylesh1640 wants to merge 2 commits intofossasia:developmentfrom
Open
Feat: add API key management system#9177Shylesh1640 wants to merge 2 commits intofossasia:developmentfrom
Shylesh1640 wants to merge 2 commits intofossasia:developmentfrom
Conversation
Reviewer's GuideImplements a full API key management system (model, schema, routes, migration) with per-user JWT-aware rate limiting and configurable default rate limits, plus minor Docker build changes for Poetry installation. Sequence diagram for creating a new API key via POST /api-keyssequenceDiagram
actor Client
participant ApiKeyListPost
participant PermissionManager as PermissionManager
participant ApiKeyModel as ApiKey
participant DB as Database
Client->>ApiKeyListPost: POST /api-keys (user relationship)
ApiKeyListPost->>PermissionManager: has_access is_user_itself(user_id)
PermissionManager-->>ApiKeyListPost: allowed / forbidden
ApiKeyListPost-->>Client: 403 Forbidden
Note over Client,ApiKeyListPost: if forbidden
ApiKeyListPost->>ApiKeyModel: generate_token()
ApiKeyModel-->>ApiKeyListPost: raw_token
ApiKeyListPost->>ApiKeyModel: hash_token(raw_token)
ApiKeyModel-->>ApiKeyListPost: token_hash
ApiKeyListPost->>DB: INSERT api_keys (token_hash, prefix, user_id, name)
DB-->>ApiKeyListPost: created ApiKey
ApiKeyListPost->>ApiKeyModel: set token on instance
ApiKeyListPost-->>Client: 201 Created (token, prefix, metadata)
ER diagram for new api_keys table and relation to userserDiagram
USERS {
int id PK
string email
}
API_KEYS {
int id PK
string name
string token_hash UK
string prefix
datetime last_used_at
datetime revoked_at
datetime created_at
int user_id FK
datetime deleted_at
}
USERS ||--o{ API_KEYS : has
Class diagram for new ApiKey domain and API resourcesclassDiagram
class ApiKey {
<<SQLAlchemyModel>>
+int id
+str name
+str token_hash
+str prefix
+datetime last_used_at
+datetime revoked_at
+datetime created_at
+int user_id
+str token
+static str generate_token()
+static str hash_token(token)
+static str get_service_name()
}
class User {
<<SQLAlchemyModel>>
+int id
+str email
+str password
+List~ApiKey~ api_keys
}
class ApiKeySchema {
<<MarshmallowSchema>>
+int id
+str name
+str prefix
+str token
+str token_hash
+datetime created_at
+datetime last_used_at
+datetime revoked_at
+Relationship user
}
class ApiKeyListPost {
<<ResourceList>>
+before_post(args, kwargs, data)
+before_create_object(data, view_kwargs)
+after_create_object(api_key, data, view_kwargs)
}
class ApiKeyList {
<<ResourceList>>
+query(view_kwargs)
}
class ApiKeyDetail {
<<ResourceDetail>>
+before_get(args, kwargs)
+before_update_object(api_key, data, view_kwargs)
}
class ApiKeyRelationship {
<<ResourceRelationship>>
}
ApiKey --> User : user
User "1" --> "*" ApiKey : api_keys
ApiKeySchema --> ApiKey : maps
ApiKeyListPost --> ApiKeySchema : uses
ApiKeyList --> ApiKeySchema : uses
ApiKeyDetail --> ApiKeySchema : uses
ApiKeyRelationship --> ApiKeySchema : uses
ApiKeyListPost --> ApiKey : creates
ApiKeyList --> ApiKey : queries
ApiKeyDetail --> ApiKey : reads_updates_deletes
ApiKeyRelationship --> ApiKey : manages_relationships
Flow diagram for rate limit key selection with JWT and IP fallbackflowchart TD
A[Incoming request] --> B[Call rate_limit_key]
B --> C[Try get_identity]
C --> D{User object with id available?}
D -- Yes --> E[Return key user:id]
D -- No --> F[Call get_ipaddr]
F --> G[Return client IP as key]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
ApiKeyDetailresource allowsDELETEbut does not perform any ownership or role check before deletion; consider adding abefore_delete_object(orbefore_delete) hook mirroring thebefore_get/before_update_objectaccess controls so users cannot delete keys they do not own. - In
ApiKeyListPost.before_post,data['user']is passed directly asuser_id, but for JSON:API relationships this is typically an object (e.g.{data: {id: ...}}); verify and normalize the payload shape before callinghas_access('is_user_itself', user_id=...)to avoid incorrect authorization decisions. - The
userrelationship inApiKeySchemausesrelated_view_kwargs={'api_key_id': '<id>'}, but the user detail route appears to be keyed byidrather thanapi_key_id; aligning this kwarg with the actual route signature will avoid URL generation errors for the relationship.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ApiKeyDetail` resource allows `DELETE` but does not perform any ownership or role check before deletion; consider adding a `before_delete_object` (or `before_delete`) hook mirroring the `before_get`/`before_update_object` access controls so users cannot delete keys they do not own.
- In `ApiKeyListPost.before_post`, `data['user']` is passed directly as `user_id`, but for JSON:API relationships this is typically an object (e.g. `{data: {id: ...}}`); verify and normalize the payload shape before calling `has_access('is_user_itself', user_id=...)` to avoid incorrect authorization decisions.
- The `user` relationship in `ApiKeySchema` uses `related_view_kwargs={'api_key_id': '<id>'}`, but the user detail route appears to be keyed by `id` rather than `api_key_id`; aligning this kwarg with the actual route signature will avoid URL generation errors for the relationship.
## Individual Comments
### Comment 1
<location path="app/extensions/limiter.py" line_range="9-11" />
<code_context>
+
+def rate_limit_key():
+ user = None
+ try:
+ user = get_identity()
+ except Exception:
+ user = None
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching bare `Exception` in the rate limit key function can hide unexpected errors.
This will hide real bugs in `get_identity()` by silently falling back to IP-based limiting, making diagnosis harder and changing behavior unexpectedly. Prefer catching only the specific exception(s) raised when no/invalid JWT is present and let other exceptions propagate.
Suggested implementation:
```python
from flask_limiter import Limiter
from flask_limiter.util import get_ipaddr
from flask_jwt_extended.exceptions import JWTExtendedException
from app.api.helpers.jwt import get_identity
def rate_limit_key():
user = None
try:
user = get_identity()
except JWTExtendedException:
# No or invalid JWT; fall back to IP-based limiting
user = None
```
If your project uses a different exception type to signal “no/invalid JWT” in `get_identity()`, replace `JWTExtendedException` with that specific exception and adjust the import accordingly. Also ensure that `rate_limit_key` ultimately returns either a user identifier or the IP address (e.g. `return user or get_ipaddr()`), matching how the rest of the file/config is structured.
</issue_to_address>
### Comment 2
<location path="app/api/schema/api_keys.py" line_range="39" />
<code_context>
+ self_view='v1.api_key_user',
+ self_view_kwargs={'id': '<id>'},
+ related_view='v1.user_detail',
+ related_view_kwargs={'api_key_id': '<id>'},
+ schema='UserSchema',
+ type_='user',
</code_context>
<issue_to_address>
**issue (bug_risk):** `related_view_kwargs` for the `user` relationship likely targets the wrong parameter name.
The `user` relationship targets `v1.user_detail`, but `related_view_kwargs` uses `{'api_key_id': '<id>'}`. Unless `user_detail` is defined to accept `api_key_id`, this link won’t resolve. You likely want to pass the user identifier instead (for example `{'id': '<user_id>'}`), aligned with how `user_detail` is wired.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Why
How
Testing
Summary
This PR introduces API key management and enhances rate limiting by shifting from IP-based to user-based identification when available.
Summary by Sourcery
Introduce per-user API rate limiting and a new API key management system, including persistence, schema, and REST endpoints, plus configurable default rate limits and minor Docker build updates.
New Features:
Enhancements:
Build: