feat(api_keys): implement API key management with CRUD operations and permissions#549
feat(api_keys): implement API key management with CRUD operations and permissions#549ImMohammad20000 wants to merge 21 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…eak cryptographic hashing algorithm on sensitive data' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@app/db/crud/api_key.py`:
- Around line 17-24: The APIKey is being created with key_hash computed from
model.raw_key which doesn't exist; change the code to hash the generated raw_key
variable instead (use hash_api_key(raw_key) when constructing the APIKey object
in the APIKey creation flow), ensure the local raw_key (the str(uuid.uuid4())
you create) is used for key_hash in the APIKey constructor (and return or expose
raw_key to the caller if the plain token must be delivered).
In `@app/operation/admin.py`:
- Around line 125-129: The admin update and API key role sync must be atomic:
move the call to update_api_keys_role(db, db_admin.id, modified_admin.role_id)
so it executes inside the same transaction managed by update_admin (i.e., before
the commit that persists the admin change), or ensure update_admin wraps both
operations in one transaction and only commits after update_api_keys_role
succeeds; update any commits so there is a single commit after both operations
and remove the separate commit() call currently after update_api_keys_role to
avoid partial persistence.
In `@app/routers/api_key.py`:
- Line 36: The delete handler remove_api_key currently sets
status_code=status.HTTP_204_NO_CONTENT but returns an empty dict which produces
a response body; change remove_api_key to return nothing or explicitly return
starlette.responses.Response(status_code=status.HTTP_204_NO_CONTENT) and import
Response, ensuring no response body is sent. Also confirm the collection route
decorator `@router.get`("s", response_model=APIKeysResponse) is intentionally
using the "s" suffix (it matches the repo convention) and requires no change.
In `@app/routers/authentication.py`:
- Around line 204-213: The current get_current function only falls back to
X-Api-Key when token is absent, so a present-but-invalid/expired bearer token
prevents the API-key path; change get_current (and the other get_current*
variant around 229-241) to: if token is present, call await get_admin(db, token)
but if that call returns falsy OR raises the usual auth exceptions, then extract
api_key via _extract_api_key(request) and call await get_admin_from_api_key(db,
api_key); i.e., treat a failed/exceptional get_admin result as a trigger to
attempt the API-key fallback. Ensure you reference the same helpers: get_admin,
get_admin_from_api_key, and _extract_api_key.
In `@app/utils/crypto.py`:
- Around line 104-115: hash_api_key currently generates a new random salt every
call (in hash_api_key), so identical API keys yield different stored strings and
cannot be matched via exact DB lookup; instead, change the flow so hash_api_key
returns a stable lookup identifier plus the salted PBKDF2 hash for verification
(e.g., generate a key_id/prefix deterministically from the raw key using a
server-side HMAC or a stable fingerprint and store that as the DB lookup column,
while keeping the salted pbkdf2 output for verification), and implement/ensure a
separate verify_api_key(raw_api_key, stored_hash) that pbkdf2-verifies using the
salt embedded in stored_hash; update any code that does exact equality checks to
query by the deterministic key_id/prefix rather than the full salted hash.
🪄 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: CHILL
Plan: Pro
Run ID: ba79ec72-74bb-42b5-a06f-e2770d0a9ece
📒 Files selected for processing (15)
app/db/crud/api_key.pyapp/db/migrations/versions/c9b48df42f10_add_api_keys_table.pyapp/db/models.pyapp/models/admin_role.pyapp/models/api_key.pyapp/operation/admin.pyapp/operation/api_key.pyapp/operation/user.pyapp/routers/__init__.pyapp/routers/api_key.pyapp/routers/authentication.pyapp/routers/dependencies/__init__.pyapp/routers/dependencies/api_key.pyapp/utils/crypto.pytests/api/test_user.py
| rows = conn.execute(sa.select(admin_roles.c.id, admin_roles.c.permissions)).fetchall() | ||
| for role_id, role_permissions in rows: | ||
| permissions = _normalize_permissions(role_permissions) | ||
| if "api_keys" not in permissions: | ||
| continue | ||
| permissions.pop("api_keys", None) | ||
| conn.execute(admin_roles.update().where(admin_roles.c.id == role_id).values(permissions=permissions)) |
There was a problem hiding this comment.
Preserve pre-existing api_keys permissions on downgrade.
upgrade() explicitly skips roles that already have an "api_keys" entry, but downgrade() removes that entry from every role unconditionally. Rolling this migration back will therefore delete pre-existing custom permission data, not just the defaults introduced here. Please make the downgrade remove only the values added by this migration, or otherwise preserve original role data.
|
✅ Created PR with unit tests: #551 |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Chores