Kaapi v1.0: Permissions Review and Authorization Cleanup #501
Conversation
…zation and project attributes
…lasses; update tests to use AuthContext for user-related operations
📝 WalkthroughWalkthroughConsolidated authentication into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI_Route
participant get_auth_context
participant DB
participant AuthContext
participant require_permission
participant Route_Handler
Client->>FastAPI_Route: HTTP request (Authorization header or API key)
FastAPI_Route->>get_auth_context: call(session, token, api_key)
get_auth_context->>DB: validate API key or decode token & fetch user/org/project
DB-->>get_auth_context: user, organization, project (or error)
get_auth_context->>AuthContext: construct AuthContext(user, organization, project)
AuthContext-->>FastAPI_Route: return AuthContext
FastAPI_Route->>require_permission: evaluate permission against AuthContext
alt permission granted
FastAPI_Route->>Route_Handler: invoke with AuthContextDep, SessionDep
Route_Handler->>DB: CRUD / storage using organization_.id / project_.id
Route_Handler-->>Client: 200/201 response
else permission denied
FastAPI_Route-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/api/routes/threads.py (3)
289-359: Critical: Authorization bypass vulnerability with unvalidated project_id.The endpoint accepts
project_idfrom the request body (lines 304, 319) and uses it directly to fetch credentials without validating that the current user has access to that project. TheREQUIRE_ORGANIZATIONpermission check only verifies organization membership, not project-level access.Attack scenario: A malicious user could specify any
project_id(including projects they shouldn't access) and retrieve OpenAI/Langfuse credentials for that project.Fix: Either:
- Change the permission check to
REQUIRE_PROJECTand use_current_user.project_.idinstead of accepting it from the request, OR- Add explicit validation that the user has access to the requested project_id.
Apply this diff for option 1 (recommended):
@router.post( "/threads", - dependencies=[Depends(require_permission(Permission.REQUIRE_ORGANIZATION))], + dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], ) async def threads( request: dict, background_tasks: BackgroundTasks, _session: SessionDep, _current_user: AuthContextDep, ): """Asynchronous endpoint that processes requests in background.""" credentials = get_provider_credential( session=_session, org_id=_current_user.organization_.id, provider="openai", - project_id=request.get("project_id"), + project_id=_current_user.project_.id, ) client, success = configure_openai(credentials) if not success: logger.error( - f"[threads] OpenAI API key not configured for this organization. | organization_id: {_current_user.organization_.id}, project_id: {request.get('project_id')}" + f"[threads] OpenAI API key not configured for this organization. | organization_id: {_current_user.organization_.id}, project_id: {_current_user.project_.id}" ) return APIResponse.failure_response( error="OpenAI API key not configured for this organization." ) langfuse_credentials = get_provider_credential( session=_session, org_id=_current_user.organization_.id, provider="langfuse", - project_id=request.get("project_id"), + project_id=_current_user.project_.id, ) # ... rest of function logger.info( - f"[threads] Background task scheduled for thread ID: {mask_string(request.get('thread_id'))} | organization_id: {_current_user.organization_.id}, project_id: {request.get('project_id')}" + f"[threads] Background task scheduled for thread ID: {mask_string(request.get('thread_id'))} | organization_id: {_current_user.organization_.id}, project_id: {_current_user.project_.id}" )
362-422: Critical: Same authorization bypass vulnerability.This endpoint has the identical authorization bypass issue as the
/threadsendpoint. It acceptsproject_idfrom the request body (lines 376, 394) without validating user access, while only checkingREQUIRE_ORGANIZATIONpermission.Apply the same fix pattern:
@router.post( "/threads/sync", - dependencies=[Depends(require_permission(Permission.REQUIRE_ORGANIZATION))], + dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], ) async def threads_sync( request: dict, _session: SessionDep, _current_user: AuthContextDep, ): """Synchronous endpoint that processes requests immediately.""" credentials = get_provider_credential( session=_session, org_id=_current_user.organization_.id, provider="openai", - project_id=request.get("project_id"), + project_id=_current_user.project_.id, ) # Configure OpenAI client client, success = configure_openai(credentials) if not success: logger.error( - f"[threads_sync] OpenAI API key not configured for this organization. | organization_id: {_current_user.organization_.id}, project_id: {request.get('project_id')}" + f"[threads_sync] OpenAI API key not configured for this organization. | organization_id: {_current_user.organization_.id}, project_id: {_current_user.project_.id}" ) return APIResponse.failure_response( error="OpenAI API key not configured for this organization." ) # Get Langfuse credentials langfuse_credentials = get_provider_credential( session=_session, org_id=_current_user.organization_.id, provider="langfuse", - project_id=request.get("project_id"), + project_id=_current_user.project_.id, )
489-519: Add thread ownership validation to prevent unauthorized access.The endpoint retrieves thread results without verifying thread ownership. The
get_thread_resultfunction only filters bythread_idwith no organization or project validation. Unlike other CRUD operations in the codebase (e.g.,get_conversation_by_id), theOpenAI_Threadmodel lacksorganization_idorproject_idfields. Any authenticated user with organization access can retrieve any thread by guessing its ID, creating a data access control vulnerability.To fix this, either:
- Add
organization_id/project_idfields to theOpenAI_Threadmodel and filter by them inget_thread_result- Validate thread ownership at the endpoint level before returning results
backend/app/api/routes/fine_tuning.py (1)
66-90: Pass scalar IDs to background task instead of AuthContext object.The
process_fine_tuning_jobfunction is scheduled as a background task (lines 260-262) and receives the fullcurrent_user: AuthContextDepobject. While theorganizationandprojectare explicitly loaded in the current flow, passing ORM objects to background tasks that execute outside the request context is an anti-pattern that can cause issues with session management and object lifecycle.Extract the required IDs before scheduling the task:
background_tasks.add_task( process_fine_tuning_job, job.id, ratio, current_user.organization_.id, current_user.project_.id, request )Then update the function signature:
def process_fine_tuning_job( job_id: int, ratio: float, organization_id: int, project_id: int, request: FineTuningJobCreate, ):This same pattern is used in
model_evaluation.py(line 182) and should be applied consistently across all background tasks.
🧹 Nitpick comments (6)
backend/app/models/auth.py (2)
5-5: Unused import.The
TYPE_CHECKINGimport is added but not used in the visible code. If it's intended for future type annotations, consider adding a comment explaining its purpose, or remove it if not needed.-from typing import TYPE_CHECKING
24-36: Consider more specific exception types for API context.The non-optional accessors
organization_andproject_raiseValueErrorwhen the underlying optional fields areNone. While this is functional, consider whether a more API-specific exception (likeHTTPException) or a custom exception type would provide better error handling in route contexts.However, if routes are protected by permission dependencies (like
Permission.REQUIRE_PROJECT), theseValueErrorexceptions should never be reached in normal operation, making this a defensive programming measure rather than a user-facing error path.Verify that all usages of these accessors are protected by appropriate permission checks:
#!/bin/bash # Find all usages of organization_ and project_ accessors echo "=== Usages of organization_ accessor ===" rg -n "\.organization_\." --type=py -g 'backend/app/**/*.py' -B 3 echo "" echo "=== Usages of project_ accessor ===" rg -n "\.project_\." --type=py -g 'backend/app/**/*.py' -B 3 echo "" echo "=== Routes with REQUIRE_PROJECT permission ===" rg -n "Permission\.REQUIRE_PROJECT" --type=py -g 'backend/app/api/routes/*.py' echo "" echo "=== Routes with REQUIRE_ORGANIZATION permission ===" rg -n "Permission\.REQUIRE_ORGANIZATION" --type=py -g 'backend/app/api/routes/*.py'backend/app/tests/services/doctransformer/test_job/test_start_job.py (1)
45-54: Inconsistent property access:.project.idvs.project_.id.The tests use
current_user.project.id(direct attribute access), while the production route code (e.g.,doc_transformation_job.py) usescurrent_user.project_.id(the non-optional property accessor). While functionally equivalent whenprojectis guaranteed to be set by the fixture, using.project_.idconsistently would align with production code patterns and catch potential fixture misconfigurations earlier.Consider aligning with production code:
- job = self._create_job(db, current_user.project.id, document.id) + job = self._create_job(db, current_user.project_.id, document.id) ... - project_id=current_user.project.id, + project_id=current_user.project_.id, ... - assert kwargs["project_id"] == current_user.project.id + assert kwargs["project_id"] == current_user.project_.idThis pattern should be applied to all occurrences in this file (lines 45, 54, 73, 98, 124, 128, 147, 166, 175, 190).
backend/app/tests/services/doctransformer/test_job/conftest.py (1)
86-93: Consider usingproject_.idfor consistency with route handlers.The test uses
current_user.project.iddirectly, while route handlers use the safe accessorcurrent_user.project_.id. For consistency across the codebase, consider using the same pattern.def test_document( db: Session, current_user: AuthContext ) -> Tuple[Document, Project]: """Create a test document for the current user's project.""" - store = DocumentStore(db, current_user.project.id) - project = get_project_by_id(session=db, project_id=current_user.project.id) + store = DocumentStore(db, current_user.project_.id) + project = get_project_by_id(session=db, project_id=current_user.project_.id) return store.put(), projectbackend/app/api/routes/users.py (2)
162-176: Inconsistent parameter naming:current_uservscurrent_user_dep.This function uses
current_user: AuthContextDepwhile other functions in this file usecurrent_user_dep: AuthContextDep. Consider renaming for consistency.def read_user_by_id( - user_id: int, session: SessionDep, current_user: AuthContextDep + user_id: int, session: SessionDep, current_user_dep: AuthContextDep ) -> Any: user = session.get(User, user_id) - if user == current_user.user: + if user == current_user_dep.user: return user - if not current_user.user.is_superuser: + if not current_user_dep.user.is_superuser: raise HTTPException( status_code=403, detail="The user doesn't have enough privileges", )
212-236: Same inconsistent parameter naming asread_user_by_id.Uses
current_user: AuthContextDepinstead ofcurrent_user_dep: AuthContextDep.def delete_user( - session: SessionDep, current_user: AuthContextDep, user_id: int + session: SessionDep, current_user_dep: AuthContextDep, user_id: int ) -> Message: user = session.get(User, user_id) if not user: logger.error(f"[delete_user] User not found | user_id: {user_id}") raise HTTPException(status_code=404, detail="User not found") - if user == current_user.user: + if user == current_user_dep.user: logger.error( - f"[delete_user] Attempting to delete self by superuser | user_id: {current_user.user.id}" + f"[delete_user] Attempting to delete self by superuser | user_id: {current_user_dep.user.id}" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
backend/app/api/deps.py(0 hunks)backend/app/api/routes/assistants.py(6 hunks)backend/app/api/routes/collection_job.py(3 hunks)backend/app/api/routes/collections.py(8 hunks)backend/app/api/routes/credentials.py(9 hunks)backend/app/api/routes/doc_transformation_job.py(4 hunks)backend/app/api/routes/documents.py(9 hunks)backend/app/api/routes/evaluation.py(24 hunks)backend/app/api/routes/fine_tuning.py(11 hunks)backend/app/api/routes/llm.py(1 hunks)backend/app/api/routes/login.py(3 hunks)backend/app/api/routes/model_evaluation.py(10 hunks)backend/app/api/routes/onboarding.py(2 hunks)backend/app/api/routes/openai_conversation.py(7 hunks)backend/app/api/routes/organization.py(6 hunks)backend/app/api/routes/project.py(6 hunks)backend/app/api/routes/responses.py(3 hunks)backend/app/api/routes/threads.py(11 hunks)backend/app/api/routes/users.py(10 hunks)backend/app/api/routes/utils.py(1 hunks)backend/app/core/cloud/storage.py(0 hunks)backend/app/models/__init__.py(0 hunks)backend/app/models/auth.py(2 hunks)backend/app/models/user.py(0 hunks)backend/app/services/doctransform/job.py(1 hunks)backend/app/services/documents/helpers.py(1 hunks)backend/app/tests/api/routes/test_users.py(1 hunks)backend/app/tests/services/doctransformer/test_job/conftest.py(3 hunks)backend/app/tests/services/doctransformer/test_job/test_integration.py(1 hunks)backend/app/tests/services/doctransformer/test_job/test_start_job.py(13 hunks)
💤 Files with no reviewable changes (4)
- backend/app/models/user.py
- backend/app/models/init.py
- backend/app/api/deps.py
- backend/app/core/cloud/storage.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/api/routes/llm.pybackend/app/services/documents/helpers.pybackend/app/models/auth.pybackend/app/api/routes/doc_transformation_job.pybackend/app/services/doctransform/job.pybackend/app/api/routes/project.pybackend/app/tests/api/routes/test_users.pybackend/app/tests/services/doctransformer/test_job/test_start_job.pybackend/app/api/routes/responses.pybackend/app/api/routes/login.pybackend/app/api/routes/documents.pybackend/app/api/routes/collections.pybackend/app/api/routes/credentials.pybackend/app/api/routes/evaluation.pybackend/app/api/routes/utils.pybackend/app/api/routes/organization.pybackend/app/tests/services/doctransformer/test_job/test_integration.pybackend/app/api/routes/assistants.pybackend/app/api/routes/collection_job.pybackend/app/api/routes/model_evaluation.pybackend/app/api/routes/users.pybackend/app/api/routes/threads.pybackend/app/api/routes/onboarding.pybackend/app/api/routes/fine_tuning.pybackend/app/api/routes/openai_conversation.pybackend/app/tests/services/doctransformer/test_job/conftest.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/routes/llm.pybackend/app/api/routes/doc_transformation_job.pybackend/app/api/routes/project.pybackend/app/api/routes/responses.pybackend/app/api/routes/login.pybackend/app/api/routes/documents.pybackend/app/api/routes/collections.pybackend/app/api/routes/credentials.pybackend/app/api/routes/evaluation.pybackend/app/api/routes/utils.pybackend/app/api/routes/organization.pybackend/app/api/routes/assistants.pybackend/app/api/routes/collection_job.pybackend/app/api/routes/model_evaluation.pybackend/app/api/routes/users.pybackend/app/api/routes/threads.pybackend/app/api/routes/onboarding.pybackend/app/api/routes/fine_tuning.pybackend/app/api/routes/openai_conversation.py
backend/app/services/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement business logic services under backend/app/services/
Files:
backend/app/services/documents/helpers.pybackend/app/services/doctransform/job.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/auth.py
🧬 Code graph analysis (15)
backend/app/api/routes/llm.py (1)
backend/app/models/auth.py (2)
project_(32-36)organization_(25-29)
backend/app/api/routes/project.py (1)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/services/doctransformer/test_job/test_start_job.py (1)
backend/app/models/auth.py (1)
AuthContext(19-36)
backend/app/api/routes/login.py (2)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/tests/services/doctransformer/test_job/conftest.py (1)
current_user(71-77)
backend/app/api/routes/documents.py (3)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/models/auth.py (2)
project_(32-36)organization_(25-29)backend/app/core/cloud/storage.py (1)
get_cloud_storage(261-278)
backend/app/api/routes/credentials.py (4)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/models/credentials.py (3)
CredsCreate(38-48)CredsPublic(134-141)CredsUpdate(51-64)backend/app/models/auth.py (2)
organization_(25-29)project_(32-36)backend/app/core/providers.py (1)
validate_provider(36-57)
backend/app/api/routes/evaluation.py (4)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/models/auth.py (2)
organization_(25-29)project_(32-36)backend/app/crud/evaluations/dataset.py (1)
get_dataset_by_id(107-140)backend/app/utils.py (1)
get_langfuse_client(212-248)
backend/app/api/routes/utils.py (1)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/api/routes/organization.py (1)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/tests/services/doctransformer/test_job/test_integration.py (1)
backend/app/tests/utils/document.py (1)
project(65-66)
backend/app/api/routes/assistants.py (8)
backend/app/models/assistants.py (3)
AssistantCreate(119-148)AssistantUpdate(151-173)Assistant(34-116)backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/tests/api/routes/test_assistants.py (1)
assistant_id(25-26)backend/app/tests/services/doctransformer/test_job/conftest.py (1)
current_user(71-77)backend/app/utils.py (1)
get_openai_client(179-209)backend/app/models/auth.py (2)
organization_(25-29)project_(32-36)backend/app/crud/assistants.py (5)
fetch_assistant_from_openai(55-73)sync_assistant(99-165)create_assistant(168-200)update_assistant(203-257)delete_assistant(260-284)backend/app/seed_data/seed_data.py (1)
create_assistant(264-304)
backend/app/api/routes/users.py (2)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/tests/services/doctransformer/test_job/conftest.py (1)
current_user(71-77)
backend/app/api/routes/threads.py (5)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/crud/credentials.py (1)
get_provider_credential(121-159)backend/app/models/auth.py (2)
organization_(25-29)project_(32-36)backend/app/core/util.py (1)
configure_openai(25-44)backend/app/utils.py (3)
APIResponse(33-57)failure_response(46-57)mask_string(167-176)
backend/app/api/routes/onboarding.py (1)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)
backend/app/api/routes/fine_tuning.py (3)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/tests/services/doctransformer/test_job/conftest.py (2)
current_user(71-77)background_tasks(81-83)backend/app/models/auth.py (2)
project_(32-36)organization_(25-29)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/services/doctransformer/test_job/conftest.py
[error] 1-1: Black formatting failed. The hook reformatted the file. Run 'pre-commit run --all-files' again or run 'black' to apply formatting changes. Exit code 1 from pre-commit.
🪛 Ruff (0.14.8)
backend/app/tests/services/doctransformer/test_job/conftest.py
71-71: Unused function argument: db
(ARG001)
🔇 Additional comments (73)
backend/app/api/routes/utils.py (2)
6-6: LGTM: Clean migration to permission-based imports.The import correctly brings in the new permission infrastructure (
Permissionenum andrequire_permissionfactory) that replaces the legacy superuser dependency.
11-27: LGTM: Permission check correctly replaces legacy superuser dependency.The new
require_permission(Permission.SUPERUSER)dependency correctly enforces superuser-only access while maintaining the same authorization semantics. FastAPI will automatically inject the requiredAuthContextDepandSessionDepinto the permission checker through its hierarchical dependency injection system. The implementation returns a 403 status code with the error message "Insufficient permissions - require superuser access." for unauthorized access attempts, and the function includes proper type hints as required.backend/app/tests/api/routes/test_users.py (1)
473-473: LGTM! Test assertion correctly updated.The error message assertion now matches the new permission system's standardized message format from
require_permission(Permission.SUPERUSER).backend/app/api/routes/collection_job.py (2)
33-33: LGTM! Proper permission dependency added.The endpoint correctly enforces project-level access via
require_permission(Permission.REQUIRE_PROJECT), ensuring thatprojectis guaranteed to be non-None before the handler executes.
40-40: LGTM! Safe usage of non-optional accessor.Using
current_user.project_.idis safe here because the permission dependency at line 33 guarantees the project is present.Also applies to: 50-50
backend/app/api/routes/project.py (1)
24-24: LGTM! Consistent migration to permission-based authorization.All project management endpoints have been consistently updated to use
require_permission(Permission.SUPERUSER)instead of the legacyget_current_active_superuserdependency, maintaining the same superuser-only access control.Also applies to: 45-45, 56-56, 74-74, 99-99
backend/app/api/routes/organization.py (1)
26-26: LGTM! Consistent migration to permission-based authorization.All organization management endpoints have been consistently updated to use
require_permission(Permission.SUPERUSER)instead of the legacyget_current_active_superuserdependency, maintaining the same superuser-only access control.Also applies to: 43-43, 54-54, 72-72, 101-101
backend/app/tests/services/doctransformer/test_job/test_integration.py (1)
52-59: LGTM! Test correctly updated for new signature.The test properly passes
project_id=project.idinstead of a user context object, aligning with the refactoredstart_jobsignature that accepts project identifiers directly rather than user context.backend/app/services/documents/helpers.py (1)
91-98: LGTM! Clean refactor to pass project_id directly.The service correctly passes
project_id(already available as a function parameter) totransformation_job.start_job, eliminating the need to pass user context objects through the service layer.backend/app/api/routes/doc_transformation_job.py (2)
22-52: LGTM! Clean migration to the new permission-based authorization.The route correctly applies
Permission.REQUIRE_PROJECTvia dependencies and consistently accessescurrent_user.project_.idfor all project-scoped operations (CRUD instantiation and storage retrieval).
55-99: Consistent authorization pattern applied.The batch endpoint mirrors the single-job endpoint's authorization approach correctly. All three usages of
current_user.project_.id(lines 74, 75, 82) are consistent.backend/app/api/routes/login.py (2)
54-59: Correct extraction of user from AuthContext.The
test_tokenendpoint properly returnscurrent_user.userto match theUserPublicresponse model, sinceAuthContextDepnow provides the fullAuthContextwrapper rather than just the user object.
109-114: Appropriate SUPERUSER permission for sensitive endpoint.The password recovery HTML content endpoint correctly requires superuser access via the new permission system, maintaining the same security posture as the previous
get_current_active_superuserdependency.backend/app/api/routes/onboarding.py (1)
12-22: Clean removal of unusedcurrent_userparameter.The route correctly enforces SUPERUSER permission via the dependency while removing the unused
current_userparameter from the function signature. The authentication/authorization check still occurs throughrequire_permission, which internally validatesAuthContextDep.backend/app/services/doctransform/job.py (1)
33-60: Good refactoring: decoupling service layer from auth context.Replacing
current_user: CurrentUserOrgProjectwithproject_id: intproperly separates the service layer from authentication concerns. This makesstart_jobmore testable, reusable, and aligns with the principle that business logic services should operate on primitive domain identifiers rather than auth wrapper objects.backend/app/tests/services/doctransformer/test_job/test_start_job.py (1)
36-41: Test type annotations correctly updated to AuthContext.The migration from
UserProjectOrgtoAuthContextis complete and consistent across all test methods.backend/app/api/routes/evaluation.py (3)
7-19: LGTM!The imports are correctly updated to include
Dependsand the permission utilities. The structure follows FastAPI conventions.
122-127: LGTM!Permission enforcement is correctly applied using
require_permission(Permission.REQUIRE_PROJECT)in the route dependencies.
155-159: LGTM!Consistent use of safe accessors
organization_.idandproject_.idthroughout the file ensures proper validation that these values are non-null.backend/app/api/routes/model_evaluation.py (3)
115-126: LGTM!The endpoint correctly applies project-level permission enforcement and uses
AuthContextDepfor the current user context.
200-228: LGTM!Consistent permission enforcement and correct usage of
current_user.project_.idfor project-scoped queries.
49-62: Clarify AuthContext scope in background tasks for safer context passing.
run_model_evaluationreceivescurrent_user: AuthContextDepfrom the route handler and is invoked as a background task. Background tasks execute after the response is sent and after dependencies are finalized, at which point session-bound ORM objects become detached. IfAuthContextcontains lazy-loaded relationships (e.g.,organization_orproject_as SQLModel objects), accessing them in the background task will raise aDetachedInstanceError.Either:
- Pass only primitive values (IDs) instead of the full AuthContext object to
run_model_evaluation- Or ensure all required ORM relationships are eagerly loaded before the request context ends
- Or open a new session within the background task to re-fetch any needed relationships
backend/app/api/routes/collections.py (5)
5-9: LGTM!Imports correctly updated with
Dependsand permission utilities.
59-72: LGTM!The
list_collectionsendpoint correctly applies permission enforcement and initializes the CRUD withcurrent_user.project_.id.
75-124: LGTM!The
create_collectionendpoint correctly passes bothproject_.idandorganization_.idto the service layer.
127-170: LGTM!The
delete_collectionendpoint correctly verifies collection access and passes auth context IDs to the service.
173-202: LGTM!The
collection_infoendpoint correctly scopes the collection lookup to the current user's project.backend/app/api/routes/responses.py (3)
7-8: LGTM!Imports correctly updated with the new dependency types and permission utilities.
33-65: LGTM!The async
responsesendpoint correctly applies permission enforcement and extracts IDs using safe accessors.
68-99: LGTM!The
responses_syncendpoint correctly applies permission enforcement and handles OpenAI client initialization errors gracefully.backend/app/api/routes/credentials.py (7)
5-6: LGTM on imports and dependency setup.The migration to
AuthContextDepandrequire_permissionis correctly imported and aligns with the new authorization pattern.Also applies to: 17-17
24-51: LGTM on create_new_credential.The permission dependency ensures
project_is available before the handler executes, and the access pattern via.organization_.idand.project_.idis correct.
54-73: LGTM on read_credential.Consistent use of the new auth context pattern for retrieving credentials.
76-98: LGTM on read_provider_credential.Properly validates provider and uses the new auth context accessors.
101-131: LGTM on update_credential.Input validation and auth context usage are correct.
134-156: LGTM on delete_provider_credential.Proper permission enforcement and auth context access.
159-178: LGTM on delete_all_credentials.Consistent pattern with other routes in this file.
backend/app/api/routes/documents.py (6)
6-18: LGTM on imports.Correct import of
Depends,AuthContextDep,SessionDep, and permission utilities.
69-98: LGTM on list_docs.Correctly uses
current_user.project_.idfor bothDocumentCrudinitialization andget_cloud_storage.
101-166: LGTM on upload_doc.Consistent use of the new auth context pattern throughout the document upload flow.
169-198: LGTM on remove_doc.Properly uses both
organization_.idandproject_.idforget_openai_clientand CRUD initialization.
201-233: LGTM on permanent_delete_doc.Consistent pattern with
remove_doc, correctly handles both organization and project context.
236-264: LGTM on doc_info.Proper permission enforcement and auth context usage for document retrieval.
backend/app/api/routes/assistants.py (7)
5-5: LGTM on imports.Correct import of dependencies and permission utilities.
Also applies to: 15-16
22-49: LGTM on ingest_assistant_route.Proper use of auth context for OpenAI client and sync_assistant call.
52-76: LGTM on create_assistant_route.Correctly passes
organization_idandproject_idfrom auth context tocreate_assistant.
79-103: LGTM on update_assistant_route.Proper permission enforcement and auth context usage for updating assistants.
106-125: LGTM on get_assistant_route.Correctly scopes assistant lookup to the current project.
128-147: LGTM on list_assistants_route.Pagination parameters and project scoping are correctly implemented.
150-170: LGTM on delete_assistant_route.Soft delete correctly scoped to project.
backend/app/api/routes/users.py (9)
7-11: LGTM on imports.Correct imports for the new auth and permission system.
33-42: LGTM on read_users.SUPERUSER permission correctly applied; no user context needed in handler body.
45-72: LGTM on create_user_endpoint.SUPERUSER permission correctly applied for user creation.
75-95: LGTM on update_user_me.Clean extraction of
current_userfromcurrent_user_dep.userwith proper validation logic.
98-116: LGTM on update_password_me.Proper password verification and update flow using extracted user.
119-121: LGTM on read_user_me.Concise implementation returning the user from auth context.
124-137: LGTM on delete_user_me.Correct superuser self-deletion prevention check.
140-159: LGTM on register_user.SUPERUSER permission correctly applied; docstring accurately describes the access control.
179-209: LGTM on update_user_endpoint.SUPERUSER permission correctly applied with proper validation.
backend/app/api/routes/openai_conversation.py (6)
5-6: LGTM on imports.Correct import of dependencies and permission utilities.
23-45: LGTM on get_conversation_route.Proper permission enforcement, typed path parameter, and project-scoped lookup.
48-71: LGTM on get_conversation_by_response_id_route.Consistent pattern with proper project scoping.
74-99: LGTM on get_conversation_by_ancestor_id_route.Correct multi-line annotation for the path parameter and proper project scoping.
102-133: LGTM on list_conversations_route.Pagination metadata correctly includes total count with project-scoped queries.
136-163: LGTM on delete_conversation_route.Soft delete correctly scoped to project with proper error handling.
backend/app/api/routes/fine_tuning.py (4)
9-17: LGTM! Clean import reorganization.The imports align with the new authentication model, introducing
AuthContextDep,SessionDep, and permission enforcement utilities.Also applies to: 46-47
192-292: LGTM! Authorization and ID access properly aligned.The endpoint correctly enforces
REQUIRE_PROJECTpermission and consistently uses the property accessors (organization_.id,project_.id) throughout the function. The permission check at line 196 guarantees that the properties won't raiseValueError.
405-446: LGTM! Consistent authorization pattern.The endpoint properly enforces
REQUIRE_PROJECTpermission and uses the property accessor pattern consistently.
295-402: Thecurrent_userpassed torun_model_evaluationis safe to use in a background task.AuthContextis a Pydantic model that eagerly loadsuser,organization, andprojectdata during request processing. Unlike SQLAlchemy ORM objects, these are detached, fully-serialized copies with no session dependencies. This pattern is consistently used throughout the codebase for background tasks (e.g.,process_fine_tuning_jobat line 254).Likely an incorrect or invalid review comment.
backend/app/api/routes/threads.py (4)
10-13: LGTM! Clean import updates.The imports are correctly reorganized to use the new authentication and permission infrastructure.
245-245: LGTM! Dependency type updated.The function signature correctly uses
SessionDepfor database session injection.
425-486: LGTM! This endpoint demonstrates the correct authorization pattern.Unlike the other thread endpoints, this one correctly:
- Uses
REQUIRE_PROJECTpermission check (line 427)- Accesses
project_.idfrom the authenticated context (lines 442, 444, 451, 477) rather than accepting it from the request bodyThis is the secure pattern that should be applied to the other thread endpoints.
354-355: Tracer safely supports background task execution.Langfuse is explicitly designed to handle async background operations with built-in background thread buffering and context propagation across async workloads. Passing the tracer to a background task is a supported pattern with no lifecycle issues.
Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/services/doctransformer/test_job/conftest.py (1)
70-77: Remove unuseddbparameter from fixture.The
dbparameter is declared but never used in this fixture. This was correctly flagged by static analysis (ARG001) and in a previous review.Apply this diff to remove the unused parameter:
@pytest.fixture -def current_user(db: Session, user_api_key: TestAuthContext) -> AuthContext: +def current_user(user_api_key: TestAuthContext) -> AuthContext: """Create a test user for testing.""" return AuthContext( user=user_api_key.user, organization=user_api_key.organization, project=user_api_key.project, )
🧹 Nitpick comments (1)
backend/app/tests/services/doctransformer/test_job/conftest.py (1)
87-90: Consider usingproject_.idfor consistency with codebase patterns.The fixture currently accesses
current_user.project.iddirectly. Per the PR's introduction oforganization_andproject_accessors onAuthContext, consider usingcurrent_user.project_.idfor explicit non-None access and consistency with the updated codebase pattern.Apply this diff for consistency:
@pytest.fixture def test_document(db: Session, current_user: AuthContext) -> Tuple[Document, Project]: """Create a test document for the current user's project.""" - store = DocumentStore(db, current_user.project.id) - project = get_project_by_id(session=db, project_id=current_user.project.id) + store = DocumentStore(db, current_user.project_.id) + project = get_project_by_id(session=db, project_id=current_user.project_.id) return store.put(), projectBased on learnings, the codebase is migrating to use the
project_property accessor for explicit non-None guarantees.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/services/doctransformer/test_job/conftest.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/services/doctransformer/test_job/conftest.py
🧬 Code graph analysis (1)
backend/app/tests/services/doctransformer/test_job/conftest.py (4)
backend/app/models/auth.py (1)
AuthContext(19-36)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/utils/document.py (2)
project(65-66)DocumentStore(53-80)backend/app/crud/project.py (1)
get_project_by_id(37-38)
🪛 Ruff (0.14.8)
backend/app/tests/services/doctransformer/test_job/conftest.py
71-71: Unused function argument: db
(ARG001)
🔇 Additional comments (1)
backend/app/tests/services/doctransformer/test_job/conftest.py (1)
16-16: LGTM: Import correctly updated to AuthContext.The import change aligns with the PR objectives of removing legacy authorization models and migrating to the new
AuthContextmodel.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/api/routes/llm.py (1)
41-43: Add return type hint for consistency with coding guidelines.The function signature is missing a return type annotation. Based on the response_model on line 37 and the return statement on line 60, the return type should be
APIResponse[Message].Apply this diff:
def llm_call( _current_user: AuthContextDep, _session: SessionDep, request: LLMCallRequest -): +) -> APIResponse[Message]:As per coding guidelines: "Use type hints in Python code (Python 3.11+ project)".
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/llm.py(2 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/routes/llm.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/routes/llm.py
🧬 Code graph analysis (1)
backend/app/api/routes/llm.py (3)
backend/app/api/permissions.py (2)
Permission(10-15)require_permission(45-70)backend/app/models/llm/request.py (1)
LLMCallRequest(129-163)backend/app/models/auth.py (2)
project_(32-36)organization_(25-29)
⏰ 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 (2)
backend/app/api/routes/llm.py (2)
3-3: LGTM: Imports added correctly.The new imports support the permission dependency pattern applied on line 39.
Also applies to: 6-6
47-48: LGTM: Property access correctly uses new auth context accessors.The code correctly uses
project_.idandorganization_.idaccessors from the updatedAuthContextmodel. With the permission dependency on line 39, these accesses should be safe (assuming the verification in the previous comment confirms thatREQUIRE_PROJECTensures both contexts are present).
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 (6)
backend/app/api/routes/config/config.py (1)
28-43: Add return type hints to all endpoint functions.All five endpoint functions are missing return type hints on their signatures, which violates the coding guideline requiring type hints on all function parameters and return values.
🔎 Proposed fix: Add return type hints
def create_config( config_create: ConfigCreate, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[ConfigWithVersion]:def list_configs( current_user: AuthContextDep, session: SessionDep, skip: int = Query(0, ge=0, description="Number of records to skip"), limit: int = Query(100, ge=1, le=100, description="Maximum records to return"), -): +) -> APIResponse[list[ConfigPublic]]:def get_config( config_id: UUID, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[ConfigPublic]:def update_config( config_id: UUID, config_update: ConfigUpdate, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[ConfigPublic]:def delete_config( config_id: UUID, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[Message]:As per coding guidelines, all function parameters and return values require type hints.
Also applies to: 53-67, 77-89, 99-115, 125-138
backend/app/api/routes/config/version.py (1)
25-42: Add return type hints to all endpoint functions.All four endpoint functions are missing return type hints on their signatures, which violates the coding guideline requiring type hints on all function parameters and return values.
🔎 Proposed fix: Add return type hints
def create_version( config_id: UUID, version_create: ConfigVersionCreate, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[ConfigVersionPublic]:def list_versions( config_id: UUID, current_user: AuthContextDep, session: SessionDep, skip: int = Query(0, ge=0, description="Number of records to skip"), limit: int = Query(100, ge=1, le=100, description="Maximum records to return"), -): +) -> APIResponse[list[ConfigVersionItems]]:def get_version( config_id: UUID, current_user: AuthContextDep, session: SessionDep, version_number: int = Path( ..., ge=1, description="The version number of the config" ), -): +) -> APIResponse[ConfigVersionPublic]:def delete_version( config_id: UUID, current_user: AuthContextDep, session: SessionDep, version_number: int = Path( ..., ge=1, description="The version number of the config" ), -): +) -> APIResponse[Message]:As per coding guidelines, all function parameters and return values require type hints.
Also applies to: 52-72, 82-99, 109-127
backend/app/api/routes/api_keys.py (4)
20-25: Add return type hint to the function signature.The function is missing a return type hint. According to the coding guidelines, all function parameters and return values must have type hints.
🔎 Proposed fix
def create_api_key_route( project_id: int, user_id: int, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[APIKeyCreateResponse]:As per coding guidelines, type hints are required for all function parameters and return values.
47-52: Add return type hint to the function signature.The function is missing a return type hint. According to the coding guidelines, all function parameters and return values must have type hints.
🔎 Proposed fix
def list_api_keys_route( current_user: AuthContextDep, session: SessionDep, skip: int = Query(0, ge=0, description="Number of records to skip"), limit: int = Query(100, ge=1, le=100, description="Maximum records to return"), -): +) -> APIResponse[list[APIKeyPublic]]:As per coding guidelines, type hints are required for all function parameters and return values.
65-69: Add return type hint to the function signature.The function is missing a return type hint. According to the coding guidelines, all function parameters and return values must have type hints.
🔎 Proposed fix
def delete_api_key_route( key_id: UUID, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[Message]:As per coding guidelines, type hints are required for all function parameters and return values.
20-30: Add authorization scope checks for superuser API key creation.This route allows creating API keys for any
project_idanduser_idwithout validating that the superuser has access to those resources. While the CRUD layer validates entity existence, it performs no authorization checks. Thecurrent_userparameter exists but is unused. Other routes (list, delete) scope operations tocurrent_user.project_.id. Add checks to ensure the superuser can only create keys within their accessible organization scope.
🧹 Nitpick comments (2)
backend/app/api/routes/config/config.py (1)
2-2: Remove unused import.
HTTPExceptionis imported but not used in this file.🔎 Proposed fix
-from fastapi import APIRouter, Depends, Query, HTTPException +from fastapi import APIRouter, Depends, Querybackend/app/api/routes/config/version.py (1)
2-2: Remove unused import.
HTTPExceptionis imported but not used in this file.🔎 Proposed fix
-from fastapi import APIRouter, Depends, Query, HTTPException, Path +from fastapi import APIRouter, Depends, Query, Path
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/routes/api_keys.pybackend/app/api/routes/config/config.pybackend/app/api/routes/config/version.py
🧰 Additional context used
📓 Path-based instructions (2)
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/app/api/**/*.py: Define FastAPI REST endpoints inbackend/app/api/organized by domain
Load Swagger endpoint descriptions from external markdown files instead of inline strings usingload_description("domain/action.md")
Files:
backend/app/api/routes/config/version.pybackend/app/api/routes/config/config.pybackend/app/api/routes/api_keys.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/api/routes/config/version.pybackend/app/api/routes/config/config.pybackend/app/api/routes/api_keys.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Implement organization and project-level permissions
🧬 Code graph analysis (2)
backend/app/api/routes/config/version.py (2)
backend/app/tests/services/doctransformer/test_job/conftest.py (1)
current_user(71-77)backend/app/models/auth.py (1)
project_(32-36)
backend/app/api/routes/api_keys.py (3)
backend/app/crud/api_key.py (1)
APIKeyCrud(16-118)backend/app/tests/services/doctransformer/test_job/conftest.py (1)
current_user(71-77)backend/app/models/auth.py (1)
project_(32-36)
⏰ 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.12, 6)
🔇 Additional comments (4)
backend/app/api/routes/config/config.py (1)
36-36: LGTM! Project reference migration is correct.The migration from
current_user.project.idtocurrent_user.project_.idis consistent across all endpoints and aligns with the newAuthContextpattern. Theproject_()accessor provides defensive error handling, though theREQUIRE_PROJECTpermission dependency ensures the project is never None in practice.Also applies to: 63-63, 85-85, 108-108, 133-133
backend/app/api/routes/config/version.py (1)
36-36: LGTM! Project reference migration is correct.The migration from
current_user.project.idtocurrent_user.project_.idis consistent across all version endpoints and correctly aligns with the newAuthContextpattern.Also applies to: 64-64, 94-94, 121-121
backend/app/api/routes/api_keys.py (2)
53-53: Correct use of the new project accessor.The change to
current_user.project_.idcorrectly aligns with the broader authentication refactor, which introduces non-optional project accessors. Since the route has thePermission.REQUIRE_PROJECTdependency, the project is guaranteed to be present.
70-70: Correct use of the new project accessor.The change to
current_user.project_.idcorrectly aligns with the broader authentication refactor. Since the route has thePermission.REQUIRE_PROJECTdependency, the project is guaranteed to be present and the accessor is safe to use.
Summary
This PR performs a comprehensive review of the permissions and authorization layer for Kaapi v1.0 following recent authentication changes.
What’s Changed
Reviewed all FastAPI routes to ensure permissions are applied consistently and correctly
Removed outdated authorization logic tied to the previous auth system
Cleaned up unused dependencies and permission-related code
Aligned route-level permissions with the current auth and role model
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.