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. |
Contributor
Author
|
@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
Comment on lines
+108
to
+114
| 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 |
- Add admin_id field to APIKeyCreate to allow API key assignment to other admins - Implement ownership validation ensuring only owners can assign keys to other admins - Add target admin permission validation to prevent permission escalation - Refactor API key creation to use target admin instead of current admin - Update notification to reflect actual key owner and creator - Simplify and improve api-key-filters component with better accessibility - Remove unused Popover component and streamline filter button UI - Add aria-labels and titles for better screen reader support - Fix rtl text directionality handling in filter input - Refactor search placeholder and improve styling consistency - Add admin selection capability to API key modal and forms - Enhance API key action dialogs with new assignment workflow - Update advance search modal to support admin filtering - Add admin_id parameter to API key list endpoint - Improve test coverage for admin assignment validation
- Add admin_id field to APIKeyUpdate model to allow reassigning keys to other admins - Implement admin assignment validation in update operation with owner-only restriction - Add permission checking for target admin when assigning keys with custom permissions - Update permission inheritance logic to validate target admin's permissions - Add duplicate name checking across target admin's keys during updates - Update API key form to include admin selection dropdown - Add admin column to API keys table display - Expand advance search modal with admin filter capability - Add new localization strings for admin selection and API key identifiers - Add "noKeys" and "noSearchResults" empty state messages - Add global localization entries for "clear", "resources", and "disabled" states - Update API documentation URL utility for key assignment features
- Remove "create" from inherited permission action list in migration - Update OWNER_ADMIN_API_KEY_PERMS to use boolean value for create action - Define APIKeysPermissions class with create as boolean-only field - Update frontend permission form to mark api_keys create as non-scoped - Add SCOPED_PERMISSION_ACTIONS map to validate and enforce scoped permissions - Implement permission sanitization to reject scoped values for boolean-only actions - Add tests to verify create permission accepts boolean and rejects scope values - Enforce stricter permission model where API key creation is not scope-dependent
- Add get_api_keys_by_ids CRUD operation to fetch multiple API keys efficiently - Create BulkAPIKeySelection and RemoveAPIKeysResponse models for bulk operations - Implement bulk_delete_api_keys operation with permission validation and audit logging - Add POST /api-keys/bulk/delete endpoint for bulk deletion - Add bulk delete confirmation dialog and success/error messages - Update localization strings (en, fa, ru, zh) with bulk delete UI text - Update API service to support bulk delete requests - Add comprehensive test coverage for bulk delete functionality - Enables users to delete multiple API keys in a single operation with proper authorization checks
- Add `_create_owner_admin()` helper to create admin users with role_id=1 - Add `_delete_admin_row()` helper to clean up admin records after tests - Import `Admin` model and `hash_password` function for owner creation - Import `strong_password` helper for consistent password generation - Update `test_owner_can_assign_api_key_to_admin()` to use owner credentials - Update `test_bulk_delete_api_keys_removes_selected_keys()` to use owner credentials and add try/finally cleanup - Wrap bulk delete test in try/finally block to ensure owner admin cleanup - Improve test isolation by having API key owners use dedicated admin accounts
- Replace SHA256 hashing with bcrypt for improved security - Implement HMAC-based lookup ID derivation using JWT secret key - Update API_KEY_HASH_VERSION from v1 to v2 - Modify hash_api_key, verify_api_key, and api_key_lookup_id to accept secret_key parameter - Update create_api_key and revoke_api_key to fetch and use JWT secret key - Refactor get_api_key_by_raw_key to use constant API_KEY_HASH_VERSION - Add bcrypt dependency and remove hashlib direct usage - Update all tests to use TEST_SECRET_KEY fixture and verify bcrypt behavior - Add error handling for invalid bcrypt hash format in verify_api_key
Closed
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
4 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.
Summary by CodeRabbit
Release Notes
New Features
Chores