Add API key scopes, expiry, and production storage guards#176
Add API key scopes, expiry, and production storage guards#176webhop123 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the API key system by introducing support for scopes, expiration timestamps, and organization/project bindings across the API routes and database layers. It also implements a security measure to disable in-memory storage fallbacks in production environments. Feedback highlights a potential runtime error due to naive and aware datetime comparisons, a logic flaw in how bound keys are validated against request contexts, and the addition of unused database indexes that increase write overhead.
|
|
||
| def _is_expired(self, key_doc: Dict[str, Any]) -> bool: | ||
| expires_at = key_doc.get("expires_at") | ||
| return bool(expires_at and datetime.utcnow() > expires_at) |
There was a problem hiding this comment.
The comparison between datetime.utcnow() (which is offset-naive) and expires_at will raise a TypeError if expires_at is an offset-aware datetime. Pydantic parses ISO 8601 strings with timezone offsets (like 'Z' or '+00:00') as aware datetimes, making this a likely runtime error when validating keys created via the API. Additionally, datetime.utcnow() is deprecated in Python 3.12. It is recommended to use aware datetimes consistently throughout the application (e.g., datetime.now(timezone.utc)).
| if org_id is not None and bound_org not in (None, org_id): | ||
| return False | ||
| if project_id is not None and bound_project not in (None, project_id): | ||
| return False |
There was a problem hiding this comment.
The current binding logic allows a restricted API key (where org_id or project_id is set) to be used if the request context does not provide these identifiers (i.e., they are None). This bypasses the intended restriction. If a key is bound to a specific organization or project, it should be rejected if used in a context where that binding cannot be verified.
| if org_id is not None and bound_org not in (None, org_id): | |
| return False | |
| if project_id is not None and bound_project not in (None, project_id): | |
| return False | |
| if bound_org is not None and bound_org != org_id: | |
| return False | |
| if bound_project is not None and bound_project != project_id: | |
| return False |
| self.api_keys.create_index([("expires_at", ASCENDING)]) | ||
| self.api_keys.create_index([("org_id", ASCENDING), ("project_id", ASCENDING)]) |
There was a problem hiding this comment.
These indexes on expires_at, org_id, and project_id are not currently utilized by any queries in this class. The validate_api_key method performs a lookup by key_hash and filters the other fields in Python. Unless these indexes are intended for external tools or future query patterns, they add unnecessary overhead to write operations and should be removed.
|
Bound keys now require a matching org/project context, expiry checks normalize datetimes to UTC, and the unused indexes were removed. I also added coverage for the binding and expiry cases. |
2 participants
Summary
Refs #161
Tests
.\.venv\Scripts\python -m ruff check src\database\api_key_store.py src\database\user_store.py src\api\routes\api_keys.py src\api\dependencies.py tests\unit\test_database_stores.py.\.venv\Scripts\python -m pytest tests\unit\test_database_stores.py tests\api\test_dependencies_and_routes.py -q