Authentication: Replace encryption with hashing and add permission-based authorization #396
Conversation
WalkthroughRefactors API key storage and auth: introduces APIKeyManager (generate/verify with bcrypt), APIKeyCrud, AuthContext and permission checks, migrates encrypted keys to prefix+hash with optional UUID PK via Alembic, and updates tests/fixtures to use TestAuthContext. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Route Handler
participant Deps as get_auth_context
participant Mgr as APIKeyManager
participant DB as Database
Client->>Route: Request (Authorization / X-API-KEY)
Route->>Deps: resolve auth dependency
Deps->>Mgr: api_key_manager.verify(session, api_key)
Mgr->>DB: SELECT apikey JOIN user, project, org
DB-->>Mgr: APIKey + relations
Mgr->>Mgr: parse prefix & secret → bcrypt verify
alt verified
Mgr-->>Deps: AuthContext
Deps-->>Route: AuthContext
Route->>Route: require_permission / handler
Route-->>Client: 2xx response
else invalid
Mgr-->>Deps: None
Deps-->>Route: raise HTTPException(401)
Route-->>Client: 401 response
end
sequenceDiagram
participant Test
participant Crud as APIKeyCrud
participant Mgr as APIKeyManager
participant DB as Database
Test->>Crud: create(user_id, project_id)
Crud->>Mgr: api_key_manager.generate()
Mgr->>Mgr: build raw_key ("ApiKey "+prefix+secret)
Mgr->>Mgr: bcrypt.hash(secret)
Mgr-->>Crud: (raw_key, key_prefix, key_hash)
Crud->>DB: INSERT APIKey (uuid id, key_prefix, key_hash, project_id, organization_id)
DB-->>Crud: APIKey
Crud-->>Test: (raw_key, APIKey)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
backend/app/crud/onboarding.py (1)
99-99: Consider usingorganization.iddirectly for consistency.Line 99 uses
project.organization_id, butorganizationis already available in scope (created or retrieved earlier). Usingorganization.idwould be more direct and consistent with Line 110 whereorganization.idis used for credentials.Apply this diff for consistency:
api_key = APIKey( key_prefix=key_prefix, key_hash=key_hash, user_id=user.id, - organization_id=project.organization_id, + organization_id=organization.id, project_id=project.id, )backend/app/tests/utils/collection.py (1)
32-32: Capture or document the ignored return value.
APIKeyCrud.create()returns a tuple(raw_key, api_key)(as shown in backend/app/crud/api_key.py lines 180-185), but the return value is not captured here. If the raw key is not needed for this test utility, consider adding a comment to clarify the intentional discard. Otherwise, if the key should be used, capture and return it.If intentional, add a clarifying comment:
# Step 2: Create API key for user with valid foreign keys + # Note: raw_key is not needed for collection setup APIKeyCrud(session=db, project_id=project.id).create(user_id=owner_id)Or if the return value should be used:
- APIKeyCrud(session=db, project_id=project.id).create(user_id=owner_id) + raw_key, api_key = APIKeyCrud(session=db, project_id=project.id).create(user_id=owner_id)backend/app/tests/core/doctransformer/test_service/conftest.py (1)
55-63: Drop the unuseddbfixture argument
current_userno longer touchesdb, so keeping it triggers Ruff ARG001 and adds noise. Please remove the parameter (theuser_api_keyfixture already depends ondb) or rename it to_dbif you want to keep the dependency wiring.backend/app/tests/utils/auth.py (1)
63-68: Useis_(False)for SQLAlchemy booleans.Ruff flagged this equality check. Switching to
.is_(False)avoids E712 and matches SQLAlchemy style.Apply this diff:
- .where(APIKey.is_deleted == False) + .where(APIKey.is_deleted.is_(False))backend/app/tests/utils/utils.py (1)
85-94: Align boolean filter with SQLAlchemy style.Replace the
== Falsecomparison with.is_(False)to satisfy Ruff’s E712 warning and adhere to SQLAlchemy conventions.Apply this diff:
- filters = [Assistant.is_deleted == False] + filters = [Assistant.is_deleted.is_(False)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
backend/app/alembic/migrate_api_key.py(1 hunks)backend/app/alembic/versions/d209cddac1fa_refactor_api_key_table.py(1 hunks)backend/app/api/deps.py(5 hunks)backend/app/api/permissions.py(1 hunks)backend/app/api/routes/api_keys.py(1 hunks)backend/app/crud/__init__.py(1 hunks)backend/app/crud/api_key.py(1 hunks)backend/app/crud/onboarding.py(2 hunks)backend/app/models/__init__.py(2 hunks)backend/app/models/api_key.py(2 hunks)backend/app/models/organization.py(0 hunks)backend/app/models/project.py(0 hunks)backend/app/models/user.py(1 hunks)backend/app/seed_data/seed_data.json(1 hunks)backend/app/seed_data/seed_data.py(3 hunks)backend/app/tests/api/routes/collections/test_collection_info.py(4 hunks)backend/app/tests/api/routes/collections/test_create_collections.py(4 hunks)backend/app/tests/api/routes/documents/test_route_document_upload.py(2 hunks)backend/app/tests/api/routes/test_api_key.py(0 hunks)backend/app/tests/api/routes/test_assistants.py(16 hunks)backend/app/tests/api/routes/test_creds.py(17 hunks)backend/app/tests/api/routes/test_doc_transformation_job.py(15 hunks)backend/app/tests/api/routes/test_openai_conversation.py(14 hunks)backend/app/tests/conftest.py(2 hunks)backend/app/tests/core/doctransformer/test_service/conftest.py(2 hunks)backend/app/tests/crud/test_api_key.py(0 hunks)backend/app/tests/crud/test_onboarding.py(0 hunks)backend/app/tests/utils/auth.py(1 hunks)backend/app/tests/utils/collection.py(2 hunks)backend/app/tests/utils/document.py(3 hunks)backend/app/tests/utils/test_data.py(2 hunks)backend/app/tests/utils/utils.py(2 hunks)
💤 Files with no reviewable changes (5)
- backend/app/tests/api/routes/test_api_key.py
- backend/app/tests/crud/test_api_key.py
- backend/app/tests/crud/test_onboarding.py
- backend/app/models/organization.py
- backend/app/models/project.py
🧰 Additional context used
🧬 Code graph analysis (24)
backend/app/tests/utils/collection.py (1)
backend/app/crud/api_key.py (1)
APIKeyCrud(139-236)
backend/app/crud/__init__.py (1)
backend/app/crud/api_key.py (1)
APIKeyCrud(139-236)
backend/app/alembic/versions/d209cddac1fa_refactor_api_key_table.py (1)
backend/app/alembic/migrate_api_key.py (2)
migrate_api_keys(34-154)verify_migration(157-198)
backend/app/tests/utils/test_data.py (2)
backend/app/crud/api_key.py (1)
APIKeyCrud(139-236)backend/app/tests/utils/auth.py (2)
AuthContext(9-21)get_auth_context(24-83)
backend/app/api/deps.py (3)
backend/app/models/auth.py (1)
TokenPayload(11-12)backend/app/models/user.py (2)
User(48-56)UserContext(68-71)backend/app/crud/api_key.py (1)
verify(92-133)
backend/app/tests/api/routes/documents/test_route_document_upload.py (2)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (1)
user_api_key(93-95)
backend/app/tests/conftest.py (2)
backend/app/tests/utils/utils.py (1)
get_superuser_token_headers(32-41)backend/app/tests/utils/auth.py (3)
get_superuser_auth_context(86-110)get_user_auth_context(113-137)AuthContext(9-21)
backend/app/tests/utils/utils.py (2)
backend/app/models/project.py (1)
Project(29-60)backend/app/models/assistants.py (1)
Assistant(33-44)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(139-236)create(178-217)read_all(161-176)delete(219-236)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/models/__init__.py (2)
backend/app/models/api_key.py (4)
APIKey(34-45)APIKeyBase(11-18)APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/user.py (1)
UserContext(68-71)
backend/app/alembic/migrate_api_key.py (1)
backend/app/core/security.py (1)
decrypt_api_key(125-141)
backend/app/tests/utils/document.py (2)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (1)
user_api_key(93-95)
backend/app/tests/api/routes/test_creds.py (2)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (3)
user_api_key(93-95)client(56-59)db(28-45)
backend/app/api/permissions.py (1)
backend/app/models/user.py (1)
UserContext(68-71)
backend/app/tests/utils/auth.py (4)
backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/api_key.py (1)
APIKey(34-45)
backend/app/tests/api/routes/test_assistants.py (4)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (3)
user_api_key(93-95)db(28-45)client(56-59)backend/app/tests/utils/utils.py (1)
get_assistant(76-99)backend/app/tests/utils/document.py (2)
get(118-122)delete(124-128)
backend/app/tests/api/routes/collections/test_create_collections.py (3)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (2)
user_api_key(93-95)db(28-45)backend/app/crud/collection.py (1)
read_one(84-93)
backend/app/tests/core/doctransformer/test_service/conftest.py (3)
backend/app/tests/utils/auth.py (2)
get_user_auth_context(113-137)AuthContext(9-21)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(93-95)backend/app/models/user.py (1)
UserProjectOrg(64-65)
backend/app/tests/api/routes/test_openai_conversation.py (2)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(93-95)
backend/app/crud/api_key.py (2)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/crud/project.py (1)
get_project_by_id(37-38)
backend/app/tests/api/routes/test_doc_transformation_job.py (4)
backend/app/models/doc_transformation_job.py (1)
TransformationStatus(9-13)backend/app/tests/utils/document.py (1)
DocumentStore(53-80)backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (4)
client(56-59)db(28-45)user_api_key(93-95)superuser_api_key(87-89)
backend/app/tests/api/routes/collections/test_collection_info.py (3)
backend/app/tests/utils/auth.py (1)
AuthContext(9-21)backend/app/tests/conftest.py (3)
db(28-45)user_api_key(93-95)client(56-59)backend/app/models/collection.py (2)
CollectionStatus(15-18)Collection(21-54)
backend/app/seed_data/seed_data.py (2)
backend/app/core/security.py (2)
get_password_hash(93-103)encrypt_credentials(144-161)backend/app/models/api_key.py (1)
APIKey(34-45)
backend/app/crud/onboarding.py (2)
backend/app/crud/api_key.py (1)
generate(34-56)backend/app/models/api_key.py (1)
APIKey(34-45)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/utils/auth.py
[error] 1-1: Black formatting check failed. 1 file reformatted, 212 files left unchanged. Reformatted backend/app/tests/utils/auth.py. Command: 'pre-commit run --all-files'. Hook: black.
🪛 Ruff (0.13.3)
backend/app/tests/utils/utils.py
85-85: Avoid equality comparisons to False; use not Assistant.is_deleted: for false checks
Replace with not Assistant.is_deleted
(E712)
backend/app/tests/api/routes/test_creds.py
123-123: Unused function argument: db
(ARG001)
171-171: Unused function argument: db
(ARG001)
233-233: Unused function argument: db
(ARG001)
287-287: Unused function argument: db
(ARG001)
342-342: Unused function argument: db
(ARG001)
backend/app/api/permissions.py
21-21: Unused function argument: session
(ARG001)
backend/app/tests/utils/auth.py
67-67: Avoid equality comparisons to False; use not APIKey.is_deleted: for false checks
Replace with not APIKey.is_deleted
(E712)
backend/app/tests/core/doctransformer/test_service/conftest.py
55-55: Unused function argument: db
(ARG001)
backend/app/crud/api_key.py
4-4: typing.Tuple is deprecated, use tuple instead
(UP035)
🔇 Additional comments (10)
backend/app/crud/onboarding.py (1)
93-101: LGTM! API key generation refactored correctly.The transition to
api_key_manager.generate()with separatekey_prefixandkey_hashstorage aligns with the hash-based authentication approach. The raw key is properly returned for the onboarding response.backend/app/models/api_key.py (2)
22-31: LGTM! Public model structure improved.The new
APIKeyPublicandAPIKeyCreateResponsemodels correctly expose the necessary fields. The separation between the persistent model and the one-time response containing the raw key is a good security practice.
34-45: LGTM! APIKey model refactored correctly.The model properly implements the hash-based storage pattern with UUID primary key,
key_prefixfor lookup, andkey_hashfor verification. The field constraints (unique, indexed, nullable) are appropriate.backend/app/seed_data/seed_data.json (2)
51-51: Verify API key format matches the new pattern.Similar to Line 43, ensure this seed API key format aligns with the
APIKeyManager.generate()output.
43-43: Seed API key format is correct and matches theAPIKeyManager.generate()pattern.backend/app/crud/__init__.py (1)
30-30: LGTM! API surface consolidated correctly.The shift from individual functions to
APIKeyCrudandapi_key_managerprovides a cleaner, more maintainable API surface. This aligns well with the PR's refactoring objectives.backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
80-81: LGTM! Test fixture updated for AuthContext pattern.The migration from
APIKeyPublictoAuthContextaligns with the broader test infrastructure refactor. TheAuthContextprovides richer context (user, project, organization) which improves test clarity.backend/app/models/user.py (1)
68-72: LGTM! UserContext model added correctly.The new
UserContextclass provides a clean structure for passing user identity with project and organization context through the authentication/authorization flow. The optional fields are appropriate for handling different authentication scenarios.backend/app/tests/utils/test_data.py (2)
23-23: LGTM! Test utilities updated for new API key management.The import of
APIKeyCrudaligns with the consolidated API key CRUD operations. This is consistent with the broader refactoring effort.
33-33: LGTM! AuthContext integration added correctly.The imports of
AuthContextandget_auth_contextsupport the new test pattern where authentication context is derived from seed data rather than creating API keys on-the-fly. This improves test consistency and reduces setup complexity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/api/routes/api_keys.py (1)
31-33: Fix: useuser_idfrom the user context.
current_useris now aUserContext, which exposesuser_id(seeAuthContextin backend/app/tests/utils/auth.py line 12). Calling.idraisesAttributeError, breaking the POST/api-keysendpoint at runtime.Apply this diff:
raw_key, api_key = api_key_crud.create( - user_id=current_user.id, + user_id=current_user.user_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/api_keys.py(1 hunks)backend/app/tests/utils/auth.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(139-236)create(178-217)read_all(161-176)delete(219-236)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/utils/auth.py (4)
backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/api_key.py (1)
APIKey(34-45)
🪛 Ruff (0.13.3)
backend/app/tests/utils/auth.py
74-74: Avoid equality comparisons to False; use not APIKey.is_deleted: for false checks
Replace with not APIKey.is_deleted
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (7)
backend/app/api/routes/api_keys.py (3)
13-37: LGTM on the create endpoint structure!The endpoint correctly returns
APIKeyCreateResponsewith the raw key only at creation time, uses proper permission checks viaPermission.SUPERUSER, and follows the new APIKeyCrud-based flow. Once theuser_idattribute issue is fixed, this implementation is solid.
40-60: LGTM on the list endpoint!The endpoint correctly uses
current_user.project_idto scope the listing, implements pagination with sensible defaults and constraints, and returns masked keys (APIKeyPublic) for security. The permission check viaPermission.REQUIRE_PROJECTis appropriate.
63-79: LGTM on the delete endpoint!The endpoint correctly uses
current_user.project_idto scope the deletion, takes UUID as the key identifier, and returns a clear success message. The permission check viaPermission.REQUIRE_PROJECTis appropriate.backend/app/tests/utils/auth.py (4)
9-22: LGTM on AuthContext design!The
AuthContextclass provides a clean, comprehensive test authentication context with all necessary IDs and nested objects. This design allows tests to access both scalar identifiers and full ORM objects as needed.
25-92: LGTM on get_auth_context!The helper function correctly retrieves seeded data from the database and provides clear error messages when data is missing. The logic flow (user → project → organization → api_key) is appropriate, and all required validations are in place.
74-74: Keep the equality comparison for SQLModel query building.The static analysis tool suggests using
not APIKey.is_deletedinstead ofAPIKey.is_deleted == False. However, this is a false positive: SQLModel/SQLAlchemy uses comparison operators to build SQL expressions, andnotwould produce incorrect behavior. The current code is correct.
95-119: LGTM on get_superuser_auth_context!The helper correctly retrieves the superuser authentication context using seeded data references from settings and provides clear documentation about the expected seed data structure.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/app/api/permissions.py (1)
34-40: Restore superuser bypass for all permission checks.Superusers authenticating via JWT still fail
REQUIRE_ORGANIZATION/REQUIRE_PROJECTbecause we only match on those branches. Please short-circuit onauth_context.user.is_superuserbefore thematchso admins retain full access, as previously requested.- match permission: + if auth_context.user.is_superuser: + return True + + match permission: case Permission.SUPERUSER: return auth_context.user.is_superuserbackend/app/api/routes/api_keys.py (1)
31-33: Critical: Fix attribute access oncurrent_user.
current_useris anAuthContextobject that exposesuser_id, notid. This code will raiseAttributeErrorat runtime when creating an API key.Apply this diff:
raw_key, api_key = api_key_crud.create( - user_id=current_user.id, + user_id=current_user.user_id, )backend/app/crud/api_key.py (1)
9-9: Critical: Fix circular import.Importing
get_project_by_idfromapp.crudcauses a circular dependency becauseapp.crud.__init__re-exportsAPIKeyCrudfrom this module. This will raiseImportErrorwhen the package initializes.Apply this diff:
-from app.crud import get_project_by_id +from app.crud.project import get_project_by_id
🧹 Nitpick comments (2)
backend/app/core/security.py (1)
15-15: Use built-intupleinstead oftyping.Tuple.Passlib/SQLModel stack has moved to favor built-in generics; Ruff is flagging
typing.Tupleas deprecated (UP035). Switching the return annotation totuple[str, str, str]keeps the linter quiet without behavior changes.-from typing import Any, Tuple +from typing import Any ... - def generate(cls) -> Tuple[str, str, str]: + def generate(cls) -> tuple[str, str, str]:Also applies to: 208-231
backend/app/crud/api_key.py (1)
3-3: Modernize type hint.Use
tupleinstead oftyping.Tuplefor Python 3.9+ compatibility.Apply this diff:
-from typing import TupleThen update line 55:
- def create(self, user_id: int) -> Tuple[str, APIKey]: + def create(self, user_id: int) -> tuple[str, APIKey]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/api/deps.py(5 hunks)backend/app/api/permissions.py(1 hunks)backend/app/api/routes/api_keys.py(1 hunks)backend/app/core/security.py(2 hunks)backend/app/crud/api_key.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/auth.py(2 hunks)backend/app/models/user.py(0 hunks)
💤 Files with no reviewable changes (1)
- backend/app/models/user.py
🧰 Additional context used
🧬 Code graph analysis (7)
backend/app/models/auth.py (3)
backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)
backend/app/api/deps.py (3)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-25)TokenPayload(14-15)backend/app/core/security.py (1)
verify(266-327)
backend/app/models/__init__.py (2)
backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/models/api_key.py (4)
APIKey(34-45)APIKeyBase(11-18)APIKeyPublic(21-25)APIKeyCreateResponse(28-31)
backend/app/core/security.py (5)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/auth.py (1)
AuthContext(18-25)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(16-113)create(55-94)read_all(38-53)delete(96-113)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/api/permissions.py (1)
backend/app/models/auth.py (1)
AuthContext(18-25)
backend/app/crud/api_key.py (3)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/crud/project.py (1)
get_project_by_id(37-38)backend/app/core/security.py (1)
generate(208-230)
🪛 Ruff (0.13.3)
backend/app/core/security.py
15-15: typing.Tuple is deprecated, use tuple instead
(UP035)
backend/app/api/permissions.py
21-21: Unused function argument: session
(ARG001)
backend/app/crud/api_key.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/api/routes/api_keys.py (1)
57-57: AuthContext definesproject_id: Confirmedproject_idexists onAuthContext(backend/app/models/auth.py:20).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/tests/api/routes/test_assistants.py (2)
218-228: Update to TestAuthContext for consistency.This test still uses the old
user_api_key_header: dictparameter while all other tests in this file have been updated to useuser_api_key: TestAuthContext. This inconsistency creates maintenance burden and confusion.Apply this diff to align with the rest of the file:
def test_get_assistant_not_found( client: TestClient, - user_api_key_header: dict, + user_api_key: TestAuthContext, ): """Test failure when fetching a non-existent assistant.""" non_existent_id = str(uuid4()) response = client.get( f"/api/v1/assistant/{non_existent_id}", - headers=user_api_key_header, + headers={"X-API-KEY": user_api_key.key}, )
259-283: Update remaining tests to TestAuthContext for consistency.Two more tests (
test_list_assistants_invalid_paginationandtest_delete_assistant_not_found) still use the olduser_api_key_header: dictparameter. Update these to match the refactored pattern used throughout this file.Apply this diff:
def test_list_assistants_invalid_pagination( client: TestClient, - user_api_key_header: dict, + user_api_key: TestAuthContext, ): """Test assistants list with invalid pagination parameters.""" # Test negative skip response = client.get( "/api/v1/assistant/?skip=-1&limit=10", - headers=user_api_key_header, + headers={"X-API-KEY": user_api_key.key}, ) assert response.status_code == 422 # Test limit too high response = client.get( "/api/v1/assistant/?skip=0&limit=101", - headers=user_api_key_header, + headers={"X-API-KEY": user_api_key.key}, ) assert response.status_code == 422 # Test limit too low response = client.get( "/api/v1/assistant/?skip=0&limit=0", - headers=user_api_key_header, + headers={"X-API-KEY": user_api_key.key}, ) assert response.status_code == 422 def test_delete_assistant_not_found( client: TestClient, - user_api_key_header: dict, + user_api_key: TestAuthContext, ): """Test failure when deleting a non-existent assistant.""" non_existent_id = str(uuid4()) response = client.delete( f"/api/v1/assistant/{non_existent_id}", - headers=user_api_key_header, + headers={"X-API-KEY": user_api_key.key}, )Also applies to: 305-315
♻️ Duplicate comments (1)
backend/app/tests/utils/auth.py (1)
140-146: Fix the raw_key mismatch between code and docstring.Line 144 has
raw_key="ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j"(46 chars after prefix) but the docstring at line 129 documents"ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j9"(47 chars after prefix). The key is missing the final "9".Apply this diff to match the documented seed data:
return get_auth_context( session=session, user_email=settings.EMAIL_TEST_USER, project_name="Dalgo", - raw_key="ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j", + raw_key="ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j9", user_type="User", )
🧹 Nitpick comments (3)
backend/app/tests/core/doctransformer/test_service/conftest.py (1)
54-63: Remove unuseddbparameter.The
dbparameter is no longer used since the fixture now acceptsuser_api_key: TestAuthContextwhich already provides all necessary context. Removing it improves clarity.Apply this diff:
@pytest.fixture -def current_user(db: Session, user_api_key: TestAuthContext) -> UserProjectOrg: +def current_user(user_api_key: TestAuthContext) -> UserProjectOrg: """Create a test user for testing.""" api_key = user_api_key user = api_key.userbackend/app/tests/utils/auth.py (1)
70-75: Usenotinstead of== Falsefor boolean comparison.Per PEP8 and the static analysis hint, boolean comparisons should use
notrather than explicit comparison toFalse.Apply this diff:
# Get API key for this user and project api_key = session.exec( select(APIKey) .where(APIKey.user_id == user.id) .where(APIKey.project_id == project.id) - .where(APIKey.is_deleted == False) + .where(not APIKey.is_deleted) ).first()backend/app/tests/api/routes/test_creds.py (1)
122-124: Consider removing unuseddbparameters.Several test functions declare a
db: Sessionparameter that is no longer used. While harmless, removing these improves clarity:
test_read_credentials_not_found(line 123)test_read_provider_credential_not_found(line 171)test_update_credentials_not_found_for_provider(line 233)test_delete_provider_credential_not_found(line 287)test_delete_all_credentials_not_found(line 342)Example fix for one test:
def test_read_credentials_not_found( - client: TestClient, db: Session, user_api_key: TestAuthContext + client: TestClient, user_api_key: TestAuthContext ):Also applies to: 170-172, 232-234, 286-288, 341-343
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/app/tests/api/routes/collections/test_collection_info.py(4 hunks)backend/app/tests/api/routes/collections/test_create_collections.py(4 hunks)backend/app/tests/api/routes/documents/test_route_document_upload.py(2 hunks)backend/app/tests/api/routes/test_assistants.py(16 hunks)backend/app/tests/api/routes/test_creds.py(17 hunks)backend/app/tests/api/routes/test_doc_transformation_job.py(15 hunks)backend/app/tests/api/routes/test_openai_conversation.py(14 hunks)backend/app/tests/conftest.py(2 hunks)backend/app/tests/core/doctransformer/test_service/conftest.py(2 hunks)backend/app/tests/utils/auth.py(1 hunks)backend/app/tests/utils/document.py(3 hunks)backend/app/tests/utils/test_data.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/app/tests/utils/test_data.py
- backend/app/tests/conftest.py
- backend/app/tests/api/routes/collections/test_collection_info.py
- backend/app/tests/api/routes/test_openai_conversation.py
- backend/app/tests/api/routes/documents/test_route_document_upload.py
- backend/app/tests/utils/document.py
🧰 Additional context used
🧬 Code graph analysis (6)
backend/app/tests/api/routes/collections/test_create_collections.py (3)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (2)
user_api_key(93-95)db(28-45)backend/app/crud/collection.py (1)
read_one(84-93)
backend/app/tests/api/routes/test_doc_transformation_job.py (3)
backend/app/tests/utils/document.py (1)
DocumentStore(53-80)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (3)
db(28-45)user_api_key(93-95)superuser_api_key(87-89)
backend/app/tests/core/doctransformer/test_service/conftest.py (3)
backend/app/tests/utils/auth.py (2)
get_user_auth_context(122-146)TestAuthContext(9-22)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(93-95)backend/app/models/user.py (1)
UserProjectOrg(64-65)
backend/app/tests/api/routes/test_assistants.py (4)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (3)
user_api_key(93-95)db(28-45)client(56-59)backend/app/tests/utils/utils.py (1)
get_assistant(76-99)backend/app/tests/utils/document.py (2)
get(118-122)delete(124-128)
backend/app/tests/api/routes/test_creds.py (2)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (3)
user_api_key(93-95)client(56-59)db(28-45)
backend/app/tests/utils/auth.py (4)
backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/api_key.py (1)
APIKey(34-45)
🪛 Ruff (0.13.3)
backend/app/tests/core/doctransformer/test_service/conftest.py
55-55: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/test_creds.py
123-123: Unused function argument: db
(ARG001)
171-171: Unused function argument: db
(ARG001)
233-233: Unused function argument: db
(ARG001)
287-287: Unused function argument: db
(ARG001)
342-342: Unused function argument: db
(ARG001)
backend/app/tests/utils/auth.py
74-74: Avoid equality comparisons to False; use not APIKey.is_deleted: for false checks
Replace with not APIKey.is_deleted
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/tests/api/routes/collections/test_create_collections.py (1)
14-14: LGTM!The migration to
TestAuthContextis complete and consistent. The test properly usesuser_api_key.project_id,user_api_key.user_id, anduser_api_key.keythroughout, aligning with the new authentication-context-based testing pattern.Also applies to: 51-95
backend/app/tests/api/routes/test_doc_transformation_job.py (1)
6-8: LGTM!All test methods have been consistently updated to use
TestAuthContext. The refactoring is complete and correct, with proper usage ofuser_api_key.key,user_api_key.project_id, andsuperuser_api_keythroughout the test suite.Also applies to: 13-315
backend/app/tests/api/routes/test_creds.py (1)
16-16: LGTM!The migration to
TestAuthContextis consistent across all test functions. The test suite properly usesuser_api_key.project_id,user_api_key.organization_id, anduser_api_key.keythroughout.Also applies to: 24-521
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/crud/api_key.py (1)
55-99: Refine exception handling to preserve HTTPException status codes.The broad
except Exceptionblock (line 90) catchesHTTPExceptioninstances raised at lines 62 and 67, then re-wraps them as 500 errors. This loses the original 404 status codes and detail messages.Apply this diff to fix exception handling:
- except Exception as e: + except HTTPException: + raise + except Exception as e: logger.error( f"[APIKeyCrud.create_api_key] Failed to create API key | " f"{{'project_id': {self.project_id}, 'user_id': {user_id}, 'error': '{str(e)}'}}",
🧹 Nitpick comments (1)
backend/app/crud/api_key.py (1)
55-55: Usetupleinstead of deprecatedtyping.Tuple.Python 3.9+ supports using the built-in
tupletype for type hints.Apply this diff:
- def create(self, user_id: int, project_id: int) -> Tuple[str, APIKey]: + def create(self, user_id: int, project_id: int) -> tuple[str, APIKey]:And remove the import:
-from typing import Tuple
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/api_keys.py(1 hunks)backend/app/crud/api_key.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/crud/api_key.py (4)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-56)backend/app/crud/project.py (1)
get_project_by_id(37-38)backend/app/core/security.py (1)
generate(208-230)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(16-118)create(55-99)read_all(38-53)delete(101-118)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
🪛 Ruff (0.13.3)
backend/app/crud/api_key.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
backend/app/api/routes/api_keys.py
22-22: Unused function argument: current_user
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (4)
backend/app/api/routes/api_keys.py (2)
47-62: LGTM!The list route correctly uses
current_user.project_idfor project-scoped access and includes proper pagination support.
70-81: LGTM!The delete route correctly uses
current_user.project_idto ensure project-scoped deletion and returns an appropriate success message.backend/app/crud/api_key.py (2)
38-53: LGTM!The read_all method correctly implements project-scoped filtering with pagination support.
101-118: LGTM!The delete method correctly implements soft deletion with project-scoped access control via
read_one.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/app/tests/utils/auth.py (1)
144-144: Fix the raw_key to match the documented seed data.Line 144 has the key ending with "...ai8j" but the docstring at line 129 documents it as "...ai8j9" (with a final "9"). This mismatch will cause authentication failures.
Apply this diff:
- raw_key="ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j", + raw_key="ApiKey Px8y47B6roJHin1lWLkR88eiDrFdXSJRZmFQazzai8j9",backend/app/crud/api_key.py (2)
27-27: Fix the docstring to match implementation.The docstring states "by its key_prefix" but the method queries by
key_id(UUID) on line 31.Apply this diff:
- Retrieve a single non-deleted API key by its key_prefix. + Retrieve a single non-deleted API key by its ID.
90-99: Refine exception handling to avoid wrapping HTTPException.The broad
except Exceptionwill catch and re-wrap anyHTTPExceptionraised byget_project_by_idor user validation (lines 59-66) as a 500 error, losing the original status code.Apply this diff:
+ except HTTPException: + raise except Exception as e: logger.error(
🧹 Nitpick comments (2)
backend/app/tests/utils/auth.py (1)
74-74: Use idiomatic comparison for boolean check.The static analysis hint is valid: prefer
APIKey.is_deleted.is_(False)over== Falsefor SQLModel/SQLAlchemy boolean comparisons to ensure proper SQL generation.Apply this diff:
- .where(APIKey.is_deleted == False) + .where(APIKey.is_deleted.is_(False))backend/app/crud/api_key.py (1)
3-3: Use modern type annotation.The static analysis hint is valid:
typing.Tupleis deprecated in favor of the built-intupletype (Python 3.9+).Apply this diff:
-from typing import Tuple +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Tuple +else: + Tuple = tupleOr simply:
-from typing import TupleAnd use
tuple[str, APIKey]at line 55.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/app/api/deps.py(5 hunks)backend/app/crud/api_key.py(1 hunks)backend/app/tests/api/routes/test_api_key.py(1 hunks)backend/app/tests/conftest.py(2 hunks)backend/app/tests/core/doctransformer/test_service/conftest.py(2 hunks)backend/app/tests/core/test_security.py(2 hunks)backend/app/tests/crud/test_api_key.py(1 hunks)backend/app/tests/utils/auth.py(1 hunks)backend/app/tests/utils/test_data.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/utils/test_data.py
🧰 Additional context used
🧬 Code graph analysis (8)
backend/app/tests/core/test_security.py (5)
backend/app/core/security.py (4)
APIKeyManager(191-327)generate(208-230)_extract_key_parts(233-263)verify(266-327)backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/tests/utils/test_data.py (1)
create_test_api_key(87-107)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/api/routes/test_api_key.py (5)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/utils/test_data.py (2)
create_test_api_key(87-107)create_test_project(47-63)backend/app/tests/conftest.py (5)
db(28-45)client(56-59)superuser_token_headers(63-64)normal_user_token_headers(68-71)user_api_key(93-95)backend/app/tests/crud/test_api_key.py (1)
test_delete_api_key(208-222)backend/app/crud/api_key.py (1)
delete(101-118)
backend/app/crud/api_key.py (4)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-56)backend/app/crud/project.py (1)
get_project_by_id(37-38)backend/app/core/security.py (1)
generate(208-230)
backend/app/api/deps.py (7)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-25)TokenPayload(14-15)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/project_user.py (1)
ProjectUser(27-35)backend/app/models/user.py (3)
User(48-56)UserOrganization(59-61)UserProjectOrg(64-65)backend/app/core/security.py (1)
verify(266-327)
backend/app/tests/conftest.py (2)
backend/app/tests/utils/utils.py (1)
get_superuser_token_headers(32-41)backend/app/tests/utils/auth.py (3)
get_superuser_test_auth_context(95-119)get_user_test_auth_context(122-146)TestAuthContext(9-22)
backend/app/tests/utils/auth.py (4)
backend/app/models/user.py (1)
User(48-56)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/api_key.py (1)
APIKey(34-45)
backend/app/tests/crud/test_api_key.py (4)
backend/app/crud/api_key.py (5)
APIKeyCrud(16-118)create(55-99)read_one(25-36)delete(101-118)read_all(38-53)backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/tests/utils/test_data.py (2)
create_test_project(47-63)create_test_api_key(87-107)backend/app/tests/utils/utils.py (1)
get_non_existent_id(49-51)
backend/app/tests/core/doctransformer/test_service/conftest.py (3)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (2)
db(28-45)user_api_key(93-95)backend/app/models/user.py (1)
UserProjectOrg(64-65)
🪛 Ruff (0.13.3)
backend/app/crud/api_key.py
3-3: typing.Tuple is deprecated, use tuple instead
(UP035)
backend/app/tests/utils/auth.py
74-74: Avoid equality comparisons to False; use not APIKey.is_deleted: for false checks
Replace with not APIKey.is_deleted
(E712)
backend/app/tests/core/doctransformer/test_service/conftest.py
55-55: Unused function argument: db
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (24)
backend/app/api/deps.py (4)
51-53: LGTM!The API key verification logic correctly uses the new
api_key_manager.verifyand handles the None case appropriately.
97-100: LGTM!The API key verification and organization validation are correct. The function properly handles the optional API key scenario.
118-122: LGTM!The API key verification and validation logic is correct for the project-organization context.
153-208: LGTM!The new
get_auth_contextfunction provides a clean separation between authentication and authorization. The error handling is comprehensive, covering invalid credentials, inactive entities, and missing authentication.backend/app/tests/crud/test_api_key.py (5)
13-30: LGTM!The test properly validates all fields of the created API key, including the new key_prefix, key_hash, and is_deleted fields.
33-58: LGTM!Good coverage of error cases for nonexistent project and user. The tests correctly verify the 404 status codes and error messages.
61-106: LGTM!Comprehensive coverage of read_one scenarios: successful read, nonexistent key, wrong project scope, and deleted keys. All edge cases are handled correctly.
109-205: LGTM!Excellent test coverage for read_all: basic listing, pagination, exclusion of deleted keys, and project scoping. The tests validate that project isolation works correctly.
208-267: LGTM!Thorough testing of delete operations covering successful deletion, nonexistent keys, wrong project scope, and double deletion attempts. All error paths are validated.
backend/app/tests/core/doctransformer/test_service/conftest.py (1)
55-63: LGTM!The fixture correctly uses TestAuthContext to derive the user and organization/project IDs. The static analysis hint about unused
dbis a false positive—the parameter is required for pytest's dependency injection even though it's not directly used in the function body.backend/app/tests/core/test_security.py (1)
129-308: LGTM!Excellent comprehensive test coverage for APIKeyManager:
- Key generation (format, length, uniqueness, bcrypt hash)
- Key parsing (new/old formats, invalid inputs)
- Verification (valid keys, invalid keys, wrong secrets, deleted keys, malformed inputs)
- Constants validation
- Integration test confirming generated keys can be verified
The test suite validates all critical paths and edge cases.
backend/app/tests/conftest.py (1)
74-95: LGTM!The fixture updates correctly use the new TestAuthContext pattern, simplifying authentication setup by leveraging seeded data instead of creating new API keys per test.
backend/app/tests/api/routes/test_api_key.py (4)
12-40: LGTM!The test correctly validates the new response structure with expanded fields (id, key_prefix, organization_id) and confirms the key format starts with "ApiKey ".
43-57: LGTM!Good authorization test confirming normal users receive 403 when attempting to create API keys (superuser-only operation).
60-84: LGTM!The list test creates multiple keys and validates the response includes at least the expected count. The >= 4 assertion accounts for both created keys and the fixture key.
87-114: LGTM!Good coverage of delete operations: successful deletion with message validation and 404 for nonexistent keys.
backend/app/tests/utils/auth.py (3)
9-22: LGTM!The TestAuthContext model provides a clean structure for test authentication with all necessary IDs and nested objects.
25-92: LGTM!The helper function properly assembles authentication context from seeded data with comprehensive error handling for missing entities.
95-119: LGTM!The superuser context helper correctly references the seeded data for Glific project and documents the expected values.
backend/app/crud/api_key.py (5)
16-23: LGTM!The class initialization properly stores the session and project_id for scoped operations.
38-53: LGTM!The read_all method correctly filters by project_id, excludes deleted keys, and supports pagination.
59-67: LGTM!The project and user validation with appropriate 404 errors is correct (per past review).
68-88: LGTM!The API key generation and persistence logic correctly uses the new api_key_manager and stores key_prefix and key_hash.
101-118: LGTM!The soft delete implementation correctly marks the key as deleted, updates the timestamp, and raises 404 for missing keys.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/api/deps.py (3)
50-61: Fix misleading variable name and eliminate redundant database query.The variable
api_key_recordholds anAuthContextobject (which includes aUserobject), not an APIKey record. Additionally, line 55 performs a redundant database query sinceapi_key_manager.verify()already eagerly loads theUserobject.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if not api_key_record: + auth_context = api_key_manager.verify(session, api_key) + if not auth_context: raise HTTPException(status_code=401, detail="Invalid API Key") - user = session.get(User, api_key_record.user_id) - if not user: - raise HTTPException( - status_code=404, detail="User linked to API Key not found" - ) - - return user # Return only User object + return auth_context.user # Return only User object
50-61: Add inactive entity checks for API key authentication.The API key authentication path doesn't validate that the user, organization, and project are active, while the token path does check for inactive users. This inconsistency could allow inactive entities to authenticate via API key. The new
get_auth_contextfunction properly validates all inactive states—consider using similar checks here or migrating toget_auth_context.Apply this diff to add the missing checks:
if api_key: auth_context = api_key_manager.verify(session, api_key) if not auth_context: raise HTTPException(status_code=401, detail="Invalid API Key") + if not auth_context.user.is_active: + raise HTTPException(status_code=403, detail="Inactive user") + + if auth_context.organization and not auth_context.organization.is_active: + raise HTTPException(status_code=403, detail="Inactive Organization") + + if auth_context.project and not auth_context.project.is_active: + raise HTTPException(status_code=403, detail="Inactive Project") + return auth_context.user # Return only User object
117-122: Remove redundant organization validation.Similar to
get_current_user_org, theapi_key_manager.verify()method already validates and loads the organization, making thevalidate_organization()call redundant.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization_id) - organization_id = api_key_record.organization_id - project_id = api_key_record.project_id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + if not auth_context.organization.is_active: + raise HTTPException(status_code=400, detail="Organization is not active") + organization_id = auth_context.organization_id + project_id = auth_context.project_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/deps.py(5 hunks)backend/app/tests/api/test_deps.py(2 hunks)backend/app/tests/utils/collection.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/api/deps.py (3)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-25)TokenPayload(14-15)backend/app/core/security.py (1)
verify(266-327)
backend/app/tests/utils/collection.py (2)
backend/app/tests/utils/test_data.py (1)
create_test_api_key(87-107)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/api/test_deps.py (4)
backend/app/api/deps.py (1)
get_auth_context(153-204)backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/utils/test_data.py (2)
create_test_api_key(87-107)create_test_project(47-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (4)
backend/app/tests/utils/collection.py (2)
11-11: LGTM! Import correctly migrated to test utilities.The switch from
create_api_key(CRUD) tocreate_test_api_key(test helper) appropriately aligns with the PR's migration to hash-based keys where raw keys can't be retrieved post-creation.
32-32: Verify that the unused return value is intentional.The migration to
create_test_api_keyis correct. However, the function returns anAPIKeyCreateResponse(containing the raw key and API key details) that is not captured or used.Given that line 35 uses a hardcoded
"test_api_key"for the OpenAI client and theCollectionobject referencesproject.organization_iddirectly, this appears to be intentional—the API key creation is solely for establishing database relationships/foreign keys.Please confirm this is the intended behavior and the raw key isn't needed elsewhere in the test flow.
backend/app/tests/api/test_deps.py (1)
169-321: LGTM! Comprehensive test coverage for authentication.The test suite thoroughly validates all authentication paths including valid/invalid API keys, valid/invalid tokens, missing credentials, and inactive entity handling. The tests properly verify both success and error scenarios with appropriate status codes and error messages.
backend/app/api/deps.py (1)
153-204: LGTM! Well-structured authentication context function.The new
get_auth_contextfunction properly centralizes authentication logic with comprehensive validation for both API key and JWT token flows. It correctly handles:
- API key path: validates and returns full context (user, project, organization)
- Token path: validates and returns user-only context
- Proper inactive entity checks for all paths
- Appropriate HTTP status codes (401 for auth failures, 403 for inactive entities)
The separation of authentication concerns into this dedicated function improves code organization and maintainability.
…KeyManager class, add delete API key route, and update related CRUD operations
…anager, update key prefix extraction logic, and adjust seed data creation for API keys.
…y, and update comments for clarity.
…ormat and update database schema
- Removed direct usage of APIKeyPublic in tests and replaced with AuthContext for better encapsulation of authentication details. - Updated test cases across various modules (e.g., test_api_key, test_assistants, test_creds, etc.) to utilize the new AuthContext structure. - Simplified the retrieval of API keys and user context in test setups, enhancing readability and maintainability. - Removed redundant API key creation logic from test utilities and centralized it within the AuthContext. - Ensured all tests still pass with the new structure, maintaining functionality while improving code organization.
…erification logic for API key authentication
… user-specific key management.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/crud/__init__.py(1 hunks)backend/app/models/__init__.py(2 hunks)backend/app/models/user.py(0 hunks)backend/app/tests/utils/collection.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/app/models/user.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/utils/collection.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/__init__.pybackend/app/models/__init__.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/__init__.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/__init__.py
🧬 Code graph analysis (2)
backend/app/crud/__init__.py (1)
backend/app/crud/api_key.py (1)
APIKeyCrud(16-118)
backend/app/models/__init__.py (2)
backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/models/api_key.py (3)
APIKey(34-45)APIKeyPublic(21-25)APIKeyCreateResponse(28-31)
🪛 Ruff (0.13.3)
backend/app/models/__init__.py
113-113: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (3)
backend/app/models/__init__.py (2)
3-3: LGTM!The addition of
AuthContextto the public exports aligns with the PR's introduction of AuthContext-based authentication flows.
5-5: LGTM!The addition of
APIKeyCreateResponsesupports the new API key creation flow where the raw key is exposed only once at creation, aligning with the hash-based storage approach.backend/app/crud/__init__.py (1)
29-29: LGTM!The consolidation from individual API key functions to the class-based
APIKeyCrudand centralizedapi_key_manageraligns with the PR's objective to introduce an APIKeyManager class for centralized authentication logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py (2)
22-64: Add return annotations to comply with project typing rulesPer the project guideline to use type hints in Python files, please add explicit
-> Noneannotations to bothupgradeanddowngrade.As per coding guidelines
Apply this diff:-def upgrade(): +def upgrade() -> None: @@ -def downgrade(): - pass +def downgrade() -> None: + pass
38-45: Close the Alembic session via context managerWrap the migration session in a
with Session(bind=bind)block so the connection is deterministically closed/rolled back on failure before the subsequent DDL runs.Apply this diff:
- bind = op.get_bind() - session = Session(bind=bind) - migrate_api_keys(session, generate_uuid=True) - - # Step 4: Verify migration was successful - if not verify_migration(session): - raise Exception("API key migration verification failed. Please check the logs.") + bind = op.get_bind() + with Session(bind=bind) as session: + migrate_api_keys(session, generate_uuid=True) + + # Step 4: Verify migration was successful + if not verify_migration(session): + raise Exception( + "API key migration verification failed. Please check the logs." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py(1 hunks)backend/app/models/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/models/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py
🧬 Code graph analysis (1)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py (1)
backend/app/alembic/migrate_api_key.py (2)
migrate_api_keys(34-154)verify_migration(157-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py (1)
51-55: Confirm and handle foreign key dependencies before replacing the primary key
Automated searches found noForeignKey('apikey.id')in models orop.create_foreign_keyin migrations; please manually audit for any remaining constraints (e.g. raw SQL, views, external services) and update or remove them in this migration to avoid orphaned references.
| # ### end Alembic commands ### | ||
|
|
||
|
|
||
| def downgrade(): |
There was a problem hiding this comment.
should there be something over here which does the reverse of migrate_api_keys?
what would be the repurcussions if we had to rollback and did nothing on downgrade?
There was a problem hiding this comment.
instead of migration we will use db snapshot for downgrade.
Downgrade through script not possible due to hashing.
Added a comment.
backend/app/models/auth.py
Outdated
| user_id: int | ||
| project_id: int | None = None | ||
| organization_id: int | None = None | ||
|
|
||
| user: User | ||
| organization: Organization | None = None | ||
| project: Project | None = None |
There was a problem hiding this comment.
any reason to have redundant fields here?
if user.id == user_id, why have both user and user_id fields?
similar questions for project and organization
There was a problem hiding this comment.
Since we’re already fetching the user, organization and project when validating the API key, we could just pass them along to the route instead of fetching again.
Reduce redundant lookup.
Well not sure how much this optimize,
What's your opinion we can remove this also.
| The raw API key is returned only once during creation. | ||
| Store it securely as it cannot be retrieved again. |
There was a problem hiding this comment.
The raw API key is returned only once during creation.
Store it securely as it cannot be retrieved again.
i guess this message should also be sent in the metadata of the response; this is important info that should be known to the consumer of this endpoint
right now it is only superuser who can create this, so might be less relevant in the current scheme of things, but given that we already have a metadata field in our response payload, including this should be straightforward
There was a problem hiding this comment.
added a message.
|
|
||
|
|
||
| def downgrade(): | ||
| pass |
There was a problem hiding this comment.
why is the downgrade here empty
| from app.api.deps import AuthContextDep, SessionDep | ||
|
|
||
|
|
||
| class Permission(str, Enum): |
There was a problem hiding this comment.
where is this permisson being set for the check
There was a problem hiding this comment.
Each routes set a permission.
Check api_key routes.
| created_at: Optional[str] = None | ||
|
|
||
|
|
||
| class CredentialData(BaseModel): |
There was a problem hiding this comment.
possible to confuse this with our byok credential module, maybe should be named AuthData
There was a problem hiding this comment.
thats creds data only.
| project_id: int | None = None | ||
| organization_id: int | None = None | ||
|
|
||
| user: User |
There was a problem hiding this comment.
what are these fields exactly
There was a problem hiding this comment.
this is User Project Organization object.
more context
| response_model=APIResponse[Message], | ||
| dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], | ||
| ) | ||
| def delete_api_key_route( |
There was a problem hiding this comment.
understandable that redundant logging would not be needed but there is no info log in crud delete as well as delete router, i feel like there should be one at least at one place, tracebility should be there for happy instances as well
There was a problem hiding this comment.
there is already a log in delete method in crud.
- Updated downgrade function to clarify snapshot restoration. - Changed conditional logic in get_current_user function for clarity. - Enhanced create_api_key_route response with security message about raw API key. - Updated APIKeyManager docstring - removed unnecessary comments
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/deps.py (1)
117-122: Remove redundant organization validation (duplicate issue).Similar to
get_current_user_org, this function performs redundant validation sinceapi_key_manager.verify()already loads and validates the organization.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization_id) - organization_id = api_key_record.organization_id - project_id = api_key_record.project_id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + if not auth_context.organization.is_active: + raise HTTPException(status_code=400, detail="Organization is not active") + organization_id = auth_context.organization_id + project_id = auth_context.project_id
♻️ Duplicate comments (2)
backend/app/core/security.py (1)
15-15: Update to use modern type annotation.The code uses the deprecated
typing.Tupleinstead of the built-intupletype hint.Apply this diff:
-from typing import Any, Tuple +from typing import AnyThen update the return type annotations to use lowercase
tuple:
- Line 218:
def generate(cls) -> tuple[str, str, str]:- Line 243:
def _extract_key_parts(cls, raw_key: str) -> tuple[str, str] | None:backend/app/api/deps.py (1)
96-100: Remove redundant organization validation.The
api_key_manager.verify()method already validates that the organization exists and eagerly loads it in a single query. The subsequentvalidate_organization()call on line 99 performs a redundant database query.Apply this diff:
api_key = request.headers.get("X-API-KEY") if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization_id) - organization_id = api_key_record.organization_id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + if not auth_context.organization.is_active: + raise HTTPException(status_code=400, detail="Organization is not active") + organization_id = auth_context.organization_id
🧹 Nitpick comments (1)
backend/app/core/security.py (1)
226-233: Consider the implications of padding for fixed-length keys.The code uses
.ljust(length, "A")to pad tokens to exact lengths. While this ensures predictable key sizes, padding with a fixed character slightly reduces entropy ifsecrets.token_urlsafe()occasionally produces shorter output than expected (though rare). Sincetoken_urlsafe(n)typically generates consistent-length output, the padding serves primarily as a defensive safeguard.If you prefer to avoid any entropy reduction, consider regenerating until exact length is achieved:
def _generate_exact_length_token(byte_count: int, target_length: int) -> str: """Generate a token of exact target length.""" while True: token = secrets.token_urlsafe(byte_count) if len(token) >= target_length: return token[:target_length]However, the current padding approach is simpler and the entropy reduction is negligible for cryptographic purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py(1 hunks)backend/app/api/deps.py(6 hunks)backend/app/api/routes/api_keys.py(1 hunks)backend/app/core/security.py(2 hunks)backend/app/tests/core/test_security.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/core/security.pybackend/app/api/deps.pybackend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.pybackend/app/api/routes/api_keys.pybackend/app/tests/core/test_security.py
backend/app/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place core functionality (config, DB session, security, exceptions, middleware) in backend/app/core/
Files:
backend/app/core/security.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/deps.pybackend/app/api/routes/api_keys.py
🧬 Code graph analysis (5)
backend/app/core/security.py (5)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-54)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-60)backend/app/models/auth.py (1)
AuthContext(18-25)
backend/app/api/deps.py (3)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-25)TokenPayload(14-15)backend/app/core/security.py (1)
verify(276-337)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py (1)
backend/app/alembic/migrate_api_key.py (2)
migrate_api_keys(34-154)verify_migration(157-198)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(16-118)create(55-98)read_all(38-53)delete(100-118)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/core/test_security.py (5)
backend/app/core/security.py (4)
APIKeyManager(191-337)generate(218-240)_extract_key_parts(243-273)verify(276-337)backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/tests/utils/test_data.py (1)
create_test_api_key(87-107)backend/app/tests/conftest.py (1)
db(28-45)
🪛 Ruff (0.14.0)
backend/app/core/security.py
15-15: typing.Tuple is deprecated, use tuple instead
(UP035)
backend/app/api/routes/api_keys.py
22-22: Unused function argument: current_user
(ARG001)
🔇 Additional comments (10)
backend/app/alembic/versions/a06c34a6d730_refactor_api_key_table.py (1)
42-60: Migration logic and verification are well-structured.The step-by-step approach is clear:
- Add nullable columns for transition
- Migrate data with verification
- Enforce constraints after successful migration
- Update primary key and indexes
The verification step correctly raises an exception on failure, preventing partial migrations.
backend/app/core/security.py (1)
191-340: LGTM! Well-designed API key management system.The
APIKeyManagerclass provides a secure and well-structured approach to API key handling:
- Uses
secrets.token_urlsafe()for cryptographically secure random generation- Employs bcrypt hashing (one-way, not reversible)
- Maintains backward compatibility with old key format
- Efficiently loads related entities (User, Organization, Project) in a single query
- Returns an
AuthContextfor streamlined authentication flow- Includes proper error handling and logging
backend/app/tests/core/test_security.py (1)
129-298: LGTM! Comprehensive test coverage for APIKeyManager.The test suite thoroughly validates the
APIKeyManagerfunctionality:
- Key generation correctness (format, length, uniqueness)
- Bcrypt hash format validation
- Key part extraction for both old (43-char) and new (65-char) formats
- Verification scenarios (valid keys, invalid keys, wrong secrets, deleted keys, malformed keys)
- Integration test ensuring generated keys can be verified
- Edge cases and error conditions
The tests are well-organized, clearly documented, and use proper fixtures.
backend/app/api/deps.py (2)
50-62: LGTM! Properly structured authentication flow.The if/elif structure correctly prioritizes API key authentication over JWT token authentication, as requested in previous reviews. The use of
api_key_manager.verify()is appropriate.
153-207: LGTM! Well-structured authentication context provider.The
get_auth_contextfunction provides a clean authentication flow:
- Prioritizes API key over JWT token (consistent with other auth functions)
- Validates all related entities (user, organization, project) are active for API keys
- Returns a streamlined
AuthContextfor downstream use- Proper error handling with appropriate HTTP status codes (401 for invalid auth, 403 for inactive entities, 404 for missing user)
- Clear separation of authentication (this function) from authorization (route-level permissions)
This aligns well with the broader refactoring to permission-based authorization.
backend/app/api/routes/api_keys.py (5)
19-35: Verify: Is accepting arbitraryuser_idintentional for admin functionality?The route accepts
user_idas a parameter (line 21), allowing superusers to create API keys for any user. This was flagged in a previous review as a security concern. While the route is restricted to SUPERUSER permission (line 17), accepting arbitraryuser_idis broader than necessary if the intent is for users to create their own keys.If the intention is for superusers to create keys only for themselves, apply this diff:
def create_api_key_route( project_id: int, - user_id: int, current_user: AuthContextDep, session: SessionDep, ): """ - Create a new API key for the project and user, Restricted to Superuser. + Create a new API key for the current user on the project, Restricted to Superuser. The raw API key is returned only once during creation. Store it securely as it cannot be retrieved again. """ api_key_crud = APIKeyCrud(session=session, project_id=project_id) raw_key, api_key = api_key_crud.create( - user_id=user_id, + user_id=current_user.user_id, project_id=project_id, )However, if the design intent is for superusers to create API keys on behalf of other users (e.g., for administrative or onboarding purposes), the current implementation is acceptable. Please confirm whether this broader permission is intentional.
22-22: Note:current_useris not actually unused.Static analysis flagged
current_useras unused, but this is a false positive. The parameter is used indirectly through theAuthContextDepdependency, which enforces authentication and provides the necessary context for the permission check viarequire_permission(Permission.SUPERUSER).
37-43: LGTM! Clear communication about one-time key disclosure.The response properly includes the raw key and a clear metadata message warning users that the key will only be shown once, addressing the concern from previous reviews.
46-66: LGTM! Well-designed list endpoint.The list endpoint:
- Uses project-scoped
APIKeyCrudfor proper isolation- Returns only
APIKeyPublic(excludes sensitive hash data)- Implements pagination with sensible defaults and validation
- Requires project-level permission
- Clear documentation about returning masked keys
69-85: LGTM! Secure deletion endpoint.The delete endpoint:
- Performs project-scoped deletion (prevents cross-project access)
- Requires project-level permission
- Returns clear success message
- Delegates to CRUD layer for soft-delete implementation
backend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/deps.py (1)
114-119: Remove redundant organization validation.Similar to the previous occurrence,
api_key_manager.verify()already validates the organization, making thevalidate_organization()call redundant.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization_id) - organization_id = api_key_record.organization_id - project_id = api_key_record.project_id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization_id + project_id = auth_context.project_id
♻️ Duplicate comments (2)
backend/app/seed_data/seed_data.py (1)
196-201: Prefix length mismatch breaks authentication.The code extracts only 12 characters for
key_prefixand hashes from index 12 onward, butAPIKeyManager.parse_api_key()expects a 16-character prefix. This leaks the last 4 prefix characters into the hash, causing all seeded keys to fail verification.Apply this diff to fix the prefix length:
- key_prefix = key_portion[:12] # First 12 characters as prefix + key_prefix = key_portion[:16] # First 16 characters as prefix (matches APIKeyManager) from passlib.context import CryptContext pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") - key_hash = pwd_context.hash(key_portion[12:]) + secret = key_portion[16:] + if not secret: + raise ValueError("API key secret segment is empty") + key_hash = pwd_context.hash(secret)backend/app/api/deps.py (1)
93-97: Remove redundant organization validation.
api_key_manager.verify()already validates that the organization exists and is active, and eagerly loads it. The subsequentvalidate_organization()call performs a redundant database query.Apply this diff:
api_key = request.headers.get("X-API-KEY") if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization_id) - organization_id = api_key_record.organization_id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization_id
🧹 Nitpick comments (1)
backend/app/api/deps.py (1)
47-58: Renameapi_key_recordtoauth_contextfor clarity.The variable
api_key_recordholds anAuthContext(returned byapi_key_manager.verify()), not an API key record. Renaming improves code clarity and aligns with the type being returned.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if not api_key_record: + auth_context = api_key_manager.verify(session, api_key) + if not auth_context: raise HTTPException(status_code=401, detail="Invalid API Key") - user = session.get(User, api_key_record.user_id) + user = session.get(User, auth_context.user_id) if not user: raise HTTPException( status_code=404, detail="User linked to API Key not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/app/api/deps.py(6 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/project.py(0 hunks)backend/app/models/user.py(0 hunks)backend/app/seed_data/seed_data.json(1 hunks)backend/app/seed_data/seed_data.py(3 hunks)backend/app/tests/api/routes/test_creds.py(16 hunks)backend/app/tests/api/test_deps.py(1 hunks)backend/app/tests/conftest.py(2 hunks)backend/app/tests/utils/test_data.py(3 hunks)
💤 Files with no reviewable changes (2)
- backend/app/models/project.py
- backend/app/models/user.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/seed_data/seed_data.json
- backend/app/tests/api/test_deps.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/seed_data/seed_data.pybackend/app/tests/conftest.pybackend/app/api/deps.pybackend/app/tests/utils/test_data.pybackend/app/tests/api/routes/test_creds.pybackend/app/models/__init__.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/deps.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/__init__.py
🧬 Code graph analysis (6)
backend/app/seed_data/seed_data.py (2)
backend/app/core/security.py (2)
get_password_hash(100-110)encrypt_credentials(151-168)backend/app/models/api_key.py (1)
APIKey(34-45)
backend/app/tests/conftest.py (2)
backend/app/tests/utils/utils.py (1)
get_superuser_token_headers(32-41)backend/app/tests/utils/auth.py (3)
get_superuser_test_auth_context(95-119)get_user_test_auth_context(122-146)TestAuthContext(9-22)
backend/app/api/deps.py (3)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-25)TokenPayload(14-15)backend/app/core/security.py (1)
verify(276-337)
backend/app/tests/utils/test_data.py (2)
backend/app/models/api_key.py (1)
APIKeyCreateResponse(28-31)backend/app/crud/api_key.py (2)
APIKeyCrud(16-118)create(55-98)
backend/app/tests/api/routes/test_creds.py (2)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (2)
client(65-68)user_api_key(102-104)
backend/app/models/__init__.py (2)
backend/app/models/auth.py (1)
AuthContext(18-25)backend/app/models/api_key.py (3)
APIKey(34-45)APIKeyPublic(21-25)APIKeyCreateResponse(28-31)
🔇 Additional comments (8)
backend/app/models/__init__.py (1)
3-5: LGTM!The new imports for
AuthContextandAPIKeyCreateResponsecorrectly expose the authentication context and API key response types needed by the refactored authentication flow.backend/app/seed_data/seed_data.py (1)
244-246: LGTM!The credential encryption logic correctly parses the JSON string into a dictionary before encrypting, aligning with the new
encrypt_credentialsfunction signature.backend/app/api/deps.py (1)
150-204: LGTM!The new
get_auth_contextfunction correctly:
- Verifies API keys or JWT tokens
- Returns
AuthContextwith user, organization, and project info (when available)- Validates that all linked resources (user, organization, project) are active
- Properly handles both authentication methods with appropriate error responses
backend/app/tests/conftest.py (4)
83-86: LGTM!The fixture correctly retrieves the superuser authentication context and returns the API key header. The removal of explicit
scope="function"is acceptable as pytest fixtures default to function scope.
89-92: LGTM!The fixture correctly retrieves the user authentication context and returns the API key header. The implementation is consistent with the
superuser_api_key_headerfixture.
95-98: LGTM!The fixture now returns a complete
TestAuthContextinstead of justAPIKeyPublic, providing tests with richer authentication context including user, project, organization, and API key details. This aligns well with the new authentication model.
101-104: LGTM!The fixture now returns a complete
TestAuthContext, consistent with thesuperuser_api_keyfixture. This provides tests with comprehensive authentication context.backend/app/tests/api/routes/test_creds.py (1)
15-15: LGTM! Type annotations properly updated.All test function signatures have been correctly updated to use
TestAuthContextinstead ofAPIKeyPublic. The type annotation changes are consistent with the new authentication model, and all attribute accesses (.key,.project_id,.organization_id, etc.) remain valid sinceTestAuthContextprovides all required attributes. The changes adhere to the coding guidelines requiring type hints in Python code.Also applies to: 20-20, 64-64, 90-90, 119-119, 134-134, 150-150, 167-167, 195-195, 222-222, 249-249, 266-266, 292-292, 313-313, 337-337, 403-403, 428-428
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/app/core/security.py (4)
15-15: Duplicate: Update to modern type annotation.This issue was already flagged in a previous review. The code still uses the deprecated
typing.Tupleinstead of the built-intupletype hint.
218-218: Duplicate: Update to modern type annotation.This method signature still uses
Tuplefrom typing instead of the built-intuple. This was already flagged in a previous review.
243-243: Duplicate: Update to modern type annotation.This method signature still uses
Tuplefrom typing instead of the built-intuple. This was already flagged in a previous review.
276-289: Duplicate: Fix incorrect docstring for return type.The docstring's "Returns" section was already flagged in a previous review as incorrect—it claims the method returns a tuple while it actually returns
AuthContext | None.
🧹 Nitpick comments (1)
backend/app/core/security.py (1)
227-233: Consider generating sufficient bytes to avoid padding.The current approach slices and pads tokens, which slightly reduces entropy when padding occurs. Padding with "A" is deterministic and predictable.
Apply this diff to generate sufficient bytes upfront:
- PREFIX_BYTES = 16 # Generates 22 chars in urlsafe base64 - SECRET_BYTES = 32 # Generates 43 chars in urlsafe base64 + PREFIX_BYTES = 17 # Generates 23 chars in urlsafe base64, slice to 22 + SECRET_BYTES = 33 # Generates 44 chars in urlsafe base64, slice to 43Then update the generate method:
# Generate tokens and ensure exact length secret_length = cls.KEY_LENGTH - cls.PREFIX_LENGTH - key_prefix = secrets.token_urlsafe(cls.PREFIX_BYTES)[: cls.PREFIX_LENGTH].ljust( - cls.PREFIX_LENGTH, "A" - ) - secret_key = secrets.token_urlsafe(cls.SECRET_BYTES)[:secret_length].ljust( - secret_length, "A" - ) + key_prefix = secrets.token_urlsafe(cls.PREFIX_BYTES)[: cls.PREFIX_LENGTH] + secret_key = secrets.token_urlsafe(cls.SECRET_BYTES)[:secret_length]This eliminates the need for padding while maintaining exact lengths through slicing only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/core/security.py(2 hunks)backend/app/seed_data/seed_data.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/seed_data/seed_data.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/core/security.py
backend/app/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place core functionality (config, DB session, security, exceptions, middleware) in backend/app/core/
Files:
backend/app/core/security.py
🧬 Code graph analysis (1)
backend/app/core/security.py (5)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-50)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-57)backend/app/models/auth.py (1)
AuthContext(18-25)
🪛 Ruff (0.14.0)
backend/app/core/security.py
15-15: typing.Tuple is deprecated, use tuple instead
(UP035)
🔇 Additional comments (2)
backend/app/core/security.py (2)
299-324: LGTM: Efficient single-query approach with eager loading.The verification logic efficiently fetches the APIKey and all related entities (User, Organization, Project) in a single joined query, avoiding N+1 query issues. The query correctly filters by key_prefix and is_deleted status, and the explicit joins ensure that all required relationships are validated at the database level.
326-337: LGTM: Secure verification with proper error handling.The bcrypt verification is correctly implemented and resistant to timing attacks. The error handling with logging provides good observability without exposing sensitive information. Returning
Noneon any exception maintains a consistent interface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py (1)
22-64: Add missing return type hints.Both
upgrade()anddowngrade()need-> Noneannotations to comply with our Python guidelines. Apply the following diff (line numbers refer to this file):-def upgrade(): +def upgrade() -> None: @@ -def downgrade(): +def downgrade() -> None:As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py(1 hunks)backend/app/tests/utils/test_data.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/tests/utils/test_data.pybackend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py
🧬 Code graph analysis (2)
backend/app/tests/utils/test_data.py (3)
backend/app/models/api_key.py (1)
APIKeyCreateResponse(28-31)backend/app/crud/api_key.py (2)
APIKeyCrud(16-118)create(55-98)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py (1)
backend/app/alembic/migrate_api_key.py (2)
migrate_api_keys(34-154)verify_migration(157-198)
🪛 GitHub Actions: AI Platform CI
backend/app/alembic/versions/e7c68e43ce6f_refactor_api_key_table.py
[error] 1-1: Black formatting: reformatted by Black. Run 'pre-commit run --all-files' or 'black' to fix formatting issues.
🔇 Additional comments (2)
backend/app/tests/utils/test_data.py (2)
7-7: LGTM! Import additions support the refactored API key creation.The new imports for
APIKeyCreateResponseandAPIKeyCrudare correctly placed and necessary for the updatedcreate_test_api_keyfunction.Also applies to: 24-24
86-106: LGTM! Refactored test helper correctly uses the new CRUD pattern and Pydantic v2 API.The function properly:
- Uses type hints throughout (satisfies coding guideline for Python 3.11+)
- Creates missing dependencies (user/project) for test flexibility
- Delegates API key creation to
APIKeyCrud- Uses
model_dump()at line 106, which is the correct Pydantic v2 method (the past review concern aboutdict()has been addressed)- Returns the appropriate
APIKeyCreateResponsecontaining the raw key and API key metadata
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/app/core/security.py (1)
15-15: Update to use modern type annotation.The code uses the deprecated
typing.Tupleinstead of the built-intupletype hint.Apply this diff:
-from typing import Any, Tuple +from typing import AnyThen update the return type annotations to use lowercase
tuple:
- Line 218:
def generate(cls) -> tuple[str, str, str]:- Line 243:
def _extract_key_parts(cls, raw_key: str) -> tuple[str, str] | None:backend/app/api/deps.py (1)
93-97: Remove redundant organization validation.The
api_key_manager.verify()already validates that the organization exists and eagerly loads it. Thevalidate_organization()call performs a redundant database query.Apply this diff:
api_key = request.headers.get("X-API-KEY") if api_key: api_key_record = api_key_manager.verify(session, api_key) if api_key_record: - validate_organization(session, api_key_record.organization.id) organization_id = api_key_record.organization.idbackend/app/api/permissions.py (1)
18-42: Allow superusers to bypass all permission checks.Currently, superusers authenticating via JWT (without organization_id/project_id) will fail
REQUIRE_ORGANIZATIONandREQUIRE_PROJECTchecks, regressing superuser access on routes guarded by those permissions.Apply this diff to short-circuit superusers:
def has_permission( auth_context: AuthContext, permission: Permission, session: Session | None = None, ) -> bool: """ Check if the auth_context has the specified permission. Args: user_context: The authenticated user context permission: The permission to check (Permission enum) session: Database session (optional) Returns: bool: True if user has permission, False otherwise """ + # Superusers bypass all permission checks + if auth_context.user.is_superuser: + return True + match permission: case Permission.SUPERUSER: return auth_context.user.is_superuser case Permission.REQUIRE_ORGANIZATION: return auth_context.organization is not None case Permission.REQUIRE_PROJECT: return auth_context.project is not None case _: return Falsebackend/app/api/routes/api_keys.py (1)
19-43: Security: Use authenticated user's ID instead of accepting arbitrary user_id.The route accepts
user_idas a parameter, allowing callers to create API keys for any user, even with SUPERUSER permission. This violates the principle of least privilege.Apply this diff to fix the security issue:
def create_api_key_route( project_id: int, - user_id: int, current_user: AuthContextDep, session: SessionDep, ): """ Create a new API key for the project and user, Restricted to Superuser. The raw API key is returned only once during creation. Store it securely as it cannot be retrieved again. """ api_key_crud = APIKeyCrud(session=session, project_id=project_id) raw_key, api_key = api_key_crud.create( - user_id=user_id, + user_id=current_user.user.id, project_id=project_id, )
🧹 Nitpick comments (1)
backend/app/api/permissions.py (1)
21-21: Remove unused session parameter.The
sessionparameter is declared but never used inhas_permission. If it's intended for future use, add a comment explaining its purpose; otherwise, remove it.Apply this diff if the parameter is not needed:
def has_permission( auth_context: AuthContext, permission: Permission, - session: Session | None = None, ) -> bool:And update the docstring and
permission_checkercall accordingly (line 59).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/api/deps.py(5 hunks)backend/app/api/permissions.py(1 hunks)backend/app/api/routes/api_keys.py(1 hunks)backend/app/core/security.py(2 hunks)backend/app/models/auth.py(2 hunks)backend/app/tests/api/test_permissions.py(1 hunks)backend/app/tests/core/test_security.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/api/test_permissions.py
- backend/app/models/auth.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/core/security.pybackend/app/tests/core/test_security.pybackend/app/api/deps.pybackend/app/api/routes/api_keys.pybackend/app/api/permissions.py
backend/app/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place core functionality (config, DB session, security, exceptions, middleware) in backend/app/core/
Files:
backend/app/core/security.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/deps.pybackend/app/api/routes/api_keys.pybackend/app/api/permissions.py
🧬 Code graph analysis (5)
backend/app/core/security.py (5)
backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/user.py (1)
User(48-50)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(29-57)backend/app/models/auth.py (1)
AuthContext(18-21)
backend/app/tests/core/test_security.py (5)
backend/app/core/security.py (4)
APIKeyManager(191-334)generate(218-240)_extract_key_parts(243-273)verify(276-334)backend/app/models/api_key.py (1)
APIKey(34-45)backend/app/models/auth.py (1)
AuthContext(18-21)backend/app/tests/utils/test_data.py (1)
create_test_api_key(86-106)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/api/deps.py (4)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-21)TokenPayload(14-15)backend/app/models/user.py (3)
User(48-50)UserOrganization(53-55)UserProjectOrg(58-59)backend/app/core/security.py (1)
verify(276-334)
backend/app/api/routes/api_keys.py (5)
backend/app/crud/api_key.py (4)
APIKeyCrud(16-118)create(55-98)read_all(38-53)delete(100-118)backend/app/models/api_key.py (2)
APIKeyPublic(21-25)APIKeyCreateResponse(28-31)backend/app/models/message.py (1)
Message(5-6)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/api/permissions.py (1)
backend/app/models/auth.py (1)
AuthContext(18-21)
🪛 Ruff (0.14.0)
backend/app/core/security.py
15-15: typing.Tuple is deprecated, use tuple instead
(UP035)
backend/app/api/routes/api_keys.py
22-22: Unused function argument: current_user
(ARG001)
backend/app/api/permissions.py
21-21: Unused function argument: session
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (10)
backend/app/core/security.py (4)
265-270: LGTM! Backward compatibility for old key format.The old key format support (43 chars with 12-char prefix) is properly implemented, ensuring smooth migration from the legacy system.
299-310: LGTM! Efficient single-query authentication.The join strategy efficiently loads all related entities (User, Organization, Project) in a single query, avoiding N+1 problems.
329-334: LGTM! Proper error handling with logging.The try-except block properly catches exceptions, logs them with context, and returns None, preventing information leakage while maintaining observability.
226-233: Padding never triggers—the code is secure as written.The constants are perfectly aligned with
token_urlsafeoutput lengths:
token_urlsafe(16)produces exactly 22 chars; slicing toPREFIX_LENGTH (22)yields the full string, soljust(22, "A")is a no-op.token_urlsafe(32)produces exactly 43 chars; slicing tosecret_length (43)yields the full string, soljust(43, "A")is a no-op.The padding with
'A'is defensive but will never execute, so there is no entropy loss in practice. The code is correct.backend/app/tests/core/test_security.py (1)
129-298: LGTM! Comprehensive test coverage for APIKeyManager.The test suite thoroughly covers:
- Key generation (format, length, uniqueness, bcrypt hashing)
- Key extraction for both new (65-char) and old (43-char) formats
- Verification with valid keys, invalid keys, wrong secrets, deleted keys, and malformed inputs
- Constants validation
- Integration testing
The tests are well-structured and provide strong confidence in the APIKeyManager implementation.
backend/app/api/deps.py (3)
47-58: LGTM! Proper API key authentication flow.The function correctly uses
api_key_manager.verifyto authenticate and retrieve the user from the AuthContext.
60-78: LGTM! JWT authentication fallback.The elif branch properly handles JWT token authentication with appropriate error handling for invalid tokens and inactive users.
150-174: LGTM! Comprehensive authentication with proper validation.The function properly:
- Authenticates via API key or JWT token
- Validates user, organization, and project are active
- Returns a complete AuthContext for downstream use
The error handling is appropriate with specific status codes for different failure scenarios.
backend/app/api/routes/api_keys.py (2)
46-66: LGTM! Proper pagination and security for listing API keys.The route correctly:
- Enforces project-scoped permission
- Uses pagination with sensible defaults
- Returns only public key information (prefix, not secret)
69-85: LGTM! Proper project-scoped deletion.The route correctly:
- Enforces project-scoped permission
- Uses the authenticated user's project ID
- Returns a clear success message
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/api/deps.py (2)
94-98: Remove redundant organization validation and avoid extra query.api_key_manager.verify already joins and returns AuthContext with organization; drop validate_organization() and use the loaded org.
- api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization.id) - organization_id = api_key_record.organization.id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization.id
115-119: Remove redundant organization validation and avoid extra query.- api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization.id) - organization_id = api_key_record.organization.id - project_id = api_key_record.project.id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization.id + project_id = auth_context.project.id
🧹 Nitpick comments (4)
backend/app/api/deps.py (4)
36-37: Make token and api_key parameters Optional in type hints.With auto_error=False, both may be None. Use str | None for accuracy and mypy friendliness.
As per coding guidelines.
SessionDep = Annotated[Session, Depends(get_db)] -TokenDep = Annotated[str, Depends(reusable_oauth2)] +TokenDep = Annotated[str | None, Depends(reusable_oauth2)] def get_current_user( session: SessionDep, token: TokenDep, - api_key: Annotated[str, Depends(api_key_header)], + api_key: Annotated[str | None, Depends(api_key_header)], ) -> User: @@ def get_auth_context( session: SessionDep, token: TokenDep, - api_key: Annotated[str, Depends(api_key_header)], + api_key: Annotated[str | None, Depends(api_key_header)], ) -> AuthContext:Also applies to: 43-44, 153-154
75-76: Use 403 for inactive user (consistency with get_auth_context).- if not user.is_active: - raise HTTPException(status_code=400, detail="Inactive user") + if not user.is_active: + raise HTTPException(status_code=403, detail="Inactive user")
155-159: *Fix docstring: AuthContext contains objects, not _id fields.- Verify valid authentication (API Key or JWT token) and return authenticated user context. - Returns AuthContext with user info, project_id, and organization_id. - Authorization logic should be handled in routes. + Verify valid authentication (API Key or JWT token) and return authenticated user context. + Returns AuthContext with user, organization, and project objects. + Authorization should be handled in routes.
168-173: Defensive None checks for org/project before is_active access.AuthContext types are Optional; guard against None to prevent AttributeError if future changes return partial contexts.
- if not auth_context.organization.is_active: + if not auth_context.organization or not auth_context.organization.is_active: raise HTTPException(status_code=403, detail="Inactive Organization") - if not auth_context.project.is_active: + if not auth_context.project or not auth_context.project.is_active: raise HTTPException(status_code=403, detail="Inactive Project")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/deps.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/api/deps.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/deps.py
🧬 Code graph analysis (1)
backend/app/api/deps.py (4)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-21)TokenPayload(14-15)backend/app/models/user.py (3)
User(48-50)UserOrganization(53-55)UserProjectOrg(58-59)backend/app/core/security.py (1)
verify(276-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/api/deps.py (1)
121-123: Confirm API key requirement for org/project path.JWT branch is not allowed here. Is this intentional? If JWT users should access org/project-scoped routes via membership, consider a JWT fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/app/api/deps.py (3)
91-94: Fix variable name and remove redundant organization validation.Two issues:
- The variable
api_key_recordis misleading—api_key_manager.verify()returnsAuthContext | None, not an APIKey record.- The
validate_organization()call on line 93 is redundant. Per the relevant code snippets,api_key_manager.verify()already validates that the organization exists and is active in a single query, then eagerly loads it into the returnedAuthContext.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization.id) - organization_id = api_key_record.organization.id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization.id
112-116: Fix variable name and remove redundant organization validation.Two issues:
- The variable
api_key_recordis misleading—api_key_manager.verify()returnsAuthContext | None, not an APIKey record.- The
validate_organization()call on line 114 is redundant. Per the relevant code snippets,api_key_manager.verify()already validates that the organization exists and is active in a single query, then eagerly loads it into the returnedAuthContext.Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if api_key_record: - validate_organization(session, api_key_record.organization.id) - organization_id = api_key_record.organization.id - project_id = api_key_record.project.id + auth_context = api_key_manager.verify(session, api_key) + if auth_context: + organization_id = auth_context.organization.id + project_id = auth_context.project.id
48-55: Fix misleading variable name.The variable
api_key_recordis misleading sinceapi_key_manager.verify()returnsAuthContext | None, not an APIKey record. This creates confusion and is inconsistent with the naming inget_auth_context(line 158).Apply this diff:
if api_key: - api_key_record = api_key_manager.verify(session, api_key) - if not api_key_record: + auth_context = api_key_manager.verify(session, api_key) + if not auth_context: raise HTTPException(status_code=401, detail="Invalid API Key") - if not api_key_record.user.is_active: + if not auth_context.user.is_active: raise HTTPException(status_code=403, detail="Inactive user") - return api_key_record.user # Return only User object + return auth_context.user # Return only User object
🧹 Nitpick comments (1)
backend/app/api/deps.py (1)
165-169: Consider adding defensive None checks.While
api_key_manager.verify()guarantees thatorganizationandprojectare populated when it returns a non-NoneAuthContext(per the join logic in the relevant code snippet), theAuthContextmodel defines these fields as optional (Organization | NoneandProject | None). Adding explicit None checks before accessing.is_activewould be defensive and guard against future refactoring.Apply this diff:
if not auth_context.user.is_active: raise HTTPException(status_code=403, detail="Inactive user") - if not auth_context.organization.is_active: + if auth_context.organization and not auth_context.organization.is_active: raise HTTPException(status_code=403, detail="Inactive Organization") - if not auth_context.project.is_active: + if auth_context.project and not auth_context.project.is_active: raise HTTPException(status_code=403, detail="Inactive Project")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/deps.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/api/deps.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/deps.py
🧬 Code graph analysis (1)
backend/app/api/deps.py (4)
backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/auth.py (2)
AuthContext(18-21)TokenPayload(14-15)backend/app/models/user.py (3)
User(48-50)UserOrganization(53-55)UserProjectOrg(58-59)backend/app/core/security.py (1)
verify(276-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
3 participants
Summary
This PR refactors the API key authentication system to improve security by replacing encryption-based key storage with hash-based verification. The new implementation uses a prefix-based lookup strategy combined with secure hashing, while maintaining backward compatibility with existing encrypted keys.
Key Changes
Security Improvements
Architecture Updates
verify_api_keyfunction with a comprehensiveAPIKeyManagerclass for better code organizationDatabase & Migration
Testing Infrastructure
AuthContextfrom seed data instead of generating keys (since hashed keys cannot be retrieved)Summary by CodeRabbit
New Features
Improvements
Tests