Credentials: Fetching ID from API key #354
Conversation
|
Warning Rate limit exceeded@AkhileshNegi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRoutes, CRUD, models, and tests for credentials were refactored to use API-key-derived organization/project context via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant API as Credentials API
participant Auth as get_current_user_org_project
participant CRUD as credentials CRUD / DB
Note over Client,API: Requests include X-API-KEY header
Client->>API: POST/GET/PUT/DELETE /credentials(/provider/{p}) (X-API-KEY, payload)
API->>Auth: Resolve API key -> UserProjectOrg
Auth-->>API: { organization_id, project_id, ... }
API->>CRUD: perform scoped operation using org_id & project_id
CRUD-->>API: Result / NotFound / Error
API-->>Client: JSON response (resource / 404 / 400 / 500)
rect rgb(245,250,245)
Note over API: Provider-specific paths at /credentials/provider/{provider}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
backend/app/api/routes/credentials.py (1)
60-66: Use FastAPI HTTPException instead of bare ExceptionRaising
Exception(status_code=...)won’t be handled as an HTTP error; switch toHTTPException.- if not new_creds: - raise Exception(status_code=500, detail="Failed to create credentials") + if not new_creds: + raise HTTPException(status_code=500, detail="Failed to create credentials")
🧹 Nitpick comments (11)
backend/app/api/routes/credentials.py (7)
35-42: Avoid duplicate org validation (already done in dependency)
get_current_user_org_projectvalidates the organization; this is a redundant DB hit.- # Validate organization - validate_organization(session, creds_in.organization_id) - - # Project comes from API key context; no cross-org check needed here + # Organization/project already validated/sourced by get_current_user_org_project
14-14: Import validate_organization from its module, not the package rootPrevents reliance on package re-exports and avoids circular-import surprises.
-from app.crud import validate_organization +from app.crud.organization import validate_organization
26-26: Prefer built-in generics (PEP 585) for response modelsUse
list[...]instead ofList[...]. Afterwards, remove the now-unusedListimport.- response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],Also applies to: 117-117
91-113: Returning decrypted provider secrets — confirm intent and scopeThe endpoint returns full decrypted credentials. If this is intentional, ensure RBAC and audit logging; otherwise consider masking or a privileged query flag (e.g.,
?include_secret=true) and role checks.
121-138: Ignore/forbid inbound project_id in updatesYou overwrite
creds_in.project_idwith the API-key project. Consider removingproject_idfromCredsUpdateor validating/rejecting if it’s provided to avoid confusion.
154-157: Redundant provider check
validate_provider(provider)should raise on invalid input; the subsequentif not provider_enumis unreachable.- if not provider_enum: - raise HTTPException(status_code=400, detail="Invalid provider")
166-175: Remove unused variable
updated_credsis never used. Drop the assignment.- updated_creds = remove_provider_credential( + remove_provider_credential( session=session, org_id=_current_user.organization_id, provider=provider_enum, project_id=_current_user.project_id, )backend/app/tests/api/routes/test_creds.py (4)
292-295: Add assertions for DELETE provider testThe test currently doesn’t validate the outcome.
response = client.delete( f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", headers={"X-API-KEY": user_api_key.key}, ) +assert response.status_code == 200 +assert response.json()["data"]["message"] == "Provider credentials removed successfully"
494-497: Avoid equality comparison to True in SQLAlchemy filtersUse
.is_(True)for boolean columns.- Credential.is_active == True, + Credential.is_active.is_(True),
24-31: Remove unused fixtures (Ruff ARG001) and use the header fixture consistentlyMany tests accept
dbanduser_api_key_headerbut don’t use them; also headers are rebuilt inline. Remove unused fixtures and preferuser_api_key_headerfor consistency.Example (apply similarly across tests):
-def test_set_credential( - client: TestClient, - db: Session, - user_api_key_header: dict[str, str], - user_api_key: APIKeyPublic, -): +def test_set_credential(client: TestClient, user_api_key: APIKeyPublic): @@ - client.delete( - f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", - headers={"X-API-KEY": user_api_key.key}, - ) + client.delete( + f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", + headers={"X-API-KEY": user_api_key.key}, + ) @@ - response = client.post( - f"{settings.API_V1_STR}/credentials/", - json=credential_data, - headers={"X-API-KEY": user_api_key.key}, - ) + response = client.post( + f"{settings.API_V1_STR}/credentials/", + json=credential_data, + headers={"X-API-KEY": user_api_key.key}, # or: headers=user_api_key_header + )Also applies to: 68-73, 95-100, 144-148, 195-199, 242-244, 269-273, 315-319, 372-376, 403-407, 470-475, 512-513
470-475: Minor: favor the provided header fixture to DRY up setupOptional cleanup to reduce duplication and improve readability.
-def test_credential_encryption( - client: TestClient, - db: Session, - user_api_key_header: dict[str, str], - user_api_key: APIKeyPublic, -): +def test_credential_encryption(client: TestClient, user_api_key_header: dict[str, str], user_api_key: APIKeyPublic): @@ - client.delete( - f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", - headers={"X-API-KEY": user_api_key.key}, - ) + client.delete( + f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", + headers=user_api_key_header, + ) @@ - headers={"X-API-KEY": user_api_key.key}, + headers=user_api_key_header,Also applies to: 480-486, 487-488
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/app/api/routes/credentials.py(6 hunks)backend/app/tests/api/routes/test_creds.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/api/routes/credentials.py (6)
backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/credentials.py (4)
CredsCreate(19-28)CredsPublic(97-105)CredsUpdate(31-47)to_public(78-94)backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)backend/app/crud/credentials.py (5)
get_creds_by_org(77-87)get_provider_credential(90-118)update_creds_for_org(129-162)remove_provider_credential(165-186)remove_creds_for_org(189-206)
backend/app/tests/api/routes/test_creds.py (7)
backend/app/models/api_key.py (1)
APIKeyPublic(23-25)backend/app/core/providers.py (1)
Provider(6-11)backend/app/models/credentials.py (1)
Credential(50-94)backend/app/core/security.py (1)
decrypt_credentials(164-181)backend/app/tests/utils/utils.py (1)
generate_random_string(25-26)backend/app/tests/utils/test_data.py (2)
create_test_credential(93-114)test_credential_data(70-90)backend/app/tests/conftest.py (4)
db(24-41)client(52-55)user_api_key_header(77-79)user_api_key(89-91)
🪛 Ruff (0.12.2)
backend/app/api/routes/credentials.py
70-70: Use list instead of List for type annotation
Replace with list
(UP006)
117-117: Use list instead of List for type annotation
Replace with list
(UP006)
166-166: Local variable updated_creds is assigned to but never used
Remove assignment to unused variable updated_creds
(F841)
backend/app/tests/api/routes/test_creds.py
26-26: Unused function argument: db
(ARG001)
27-27: Unused function argument: user_api_key_header
(ARG001)
70-70: Unused function argument: db
(ARG001)
71-71: Unused function argument: user_api_key_header
(ARG001)
97-97: Unused function argument: db
(ARG001)
98-98: Unused function argument: user_api_key_header
(ARG001)
129-129: Unused function argument: db
(ARG001)
145-145: Unused function argument: db
(ARG001)
146-146: Unused function argument: user_api_key_header
(ARG001)
179-179: Unused function argument: db
(ARG001)
196-196: Unused function argument: db
(ARG001)
197-197: Unused function argument: user_api_key_header
(ARG001)
243-243: Unused function argument: db
(ARG001)
270-270: Unused function argument: db
(ARG001)
271-271: Unused function argument: user_api_key_header
(ARG001)
292-292: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
299-299: Unused function argument: db
(ARG001)
316-316: Unused function argument: db
(ARG001)
317-317: Unused function argument: user_api_key_header
(ARG001)
356-356: Unused function argument: db
(ARG001)
374-374: Unused function argument: user_api_key_header
(ARG001)
404-404: Unused function argument: db
(ARG001)
405-405: Unused function argument: user_api_key_header
(ARG001)
473-473: Unused function argument: user_api_key_header
(ARG001)
496-496: Avoid equality comparisons to True; use Credential.is_active: for truth checks
Replace with Credential.is_active
(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 (1)
backend/app/api/routes/credentials.py (1)
69-83: LGTM: org/project scoping derived from API keyEndpoint shape and context resolution are consistent and clear. Good alignment with tests.
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/models/credentials.py (1)
13-16: Fix schema inconsistency: project_id default None while nullable=False; make is_active an explicit column.Defaulting to None with nullable=False can cause runtime insert/validation issues. Also, define is_active via Field for consistent column metadata.
Apply this diff:
- project_id: int = Field( - default=None, foreign_key="project.id", nullable=False, ondelete="CASCADE" - ) - is_active: bool = True + project_id: int = Field( + foreign_key="project.id", nullable=False, ondelete="CASCADE" + ) + is_active: bool = Field(default=True)backend/app/tests/crud/test_credentials.py (1)
141-143: Pass project_id when removing provider credentials to avoid cross-project deletes.Current call can remove an arbitrary org-level credential if multiple projects exist for the same provider.
Apply this diff:
- removed = remove_provider_credential( - session=db, org_id=credential.organization_id, provider="openai" - ) + removed = remove_provider_credential( + session=db, + org_id=credential.organization_id, + provider="openai", + project_id=project.id, + )backend/app/crud/credentials.py (2)
71-74: Bug: get_key_by_org treats encrypted string as a dict. Decrypt before access.This will currently raise on string indexing or return false positives on substring checks.
Apply this diff:
- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] + if creds and creds.credential: + decrypted = decrypt_credentials(creds.credential) + return decrypted.get("api_key")
173-181: Handle not-found and restrict to active records in remove_provider_credential.Avoid NPE on creds None and ensure soft deletes only target active creds.
Apply this diff:
- statement = select(Credential).where( - Credential.organization_id == org_id, - Credential.provider == provider, - Credential.project_id == project_id if project_id is not None else True, - ) + statement = select(Credential).where( + Credential.organization_id == org_id, + Credential.provider == provider, + Credential.is_active == True, + Credential.project_id == project_id if project_id is not None else True, + ) creds = session.exec(statement).first() + if creds is None: + raise HTTPException(status_code=404, detail="Credentials not found for this provider") + # Soft delete creds.is_active = False creds.updated_at = now()backend/app/api/routes/credentials.py (2)
43-59: Validate provider names up front and map errors to 400.Prevents ValueError from bubbling as 500 from deeper layers.
Apply this diff:
- for provider in creds_in.credential.keys(): + for provider in creds_in.credential.keys(): + # Validate provider early for better error messages + try: + validate_provider(provider) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) existing_cred = get_provider_credential( session=session, org_id=creds_in.organization_id, provider=provider, project_id=creds_in.project_id, )
60-65: Return proper HTTPException on creation failure.Raising bare Exception won’t generate a correct HTTP response.
Apply this diff:
- if not new_creds: - raise Exception(status_code=500, detail="Failed to create credentials") + if not new_creds: + raise HTTPException(status_code=500, detail="Failed to create credentials")
🧹 Nitpick comments (9)
backend/app/models/credentials.py (1)
28-37: Hide org_id/project_id from the request schema and adopt modern union types.These are server-derived; avoid exposing them in OpenAPI and responses. Also address Ruff UP045.
Apply this diff:
- organization_id: Optional[int] = Field( - default=None, - description="Organization ID (automatically derived from API key, do not provide)", - ) - project_id: Optional[int] = Field( - default=None, - description="Project ID (automatically derived from API key, do not provide)", - ) + organization_id: int | None = Field( + default=None, + exclude=True, + description="Derived from API key; ignored if provided.", + ) + project_id: int | None = Field( + default=None, + exclude=True, + description="Derived from API key; ignored if provided.", + )backend/app/tests/crud/test_credentials.py (1)
20-20: Add a unit test for get_key_by_org once fixed in CRUD.Covers API-key extraction path and prevents regressions.
Proposed test (add near other CRUD tests):
def test_get_key_by_org_returns_api_key(db: Session) -> None: project = create_test_project(db) creds_create = CredsCreate( organization_id=project.organization_id, project_id=project.id, credential={"openai": {"api_key": "sk-test-key"}}, ) set_creds_for_org(session=db, creds_add=creds_create) from app.crud.credentials import get_key_by_org key = get_key_by_org(session=db, org_id=project.organization_id, provider="openai", project_id=project.id) assert key == "sk-test-key"backend/app/crud/credentials.py (3)
130-135: Modernize type hint per Ruff (UP045).Use PEP 604 unions.
Apply this diff:
- project_id: Optional[int] = None, + project_id: int | None = None,
146-151: Optional: avoid “else True” in WHERE for readability.Building conditions dynamically makes intent clearer.
Refactor sketch:
conditions = [ Credential.organization_id == org_id, Credential.provider == creds_in.provider, Credential.is_active == True, ] if project_id is not None: conditions.append(Credential.project_id == project_id) statement = select(Credential).where(*conditions)
32-51: Consider surfacing IntegrityError as 409 Conflict and enforcing uniqueness.Duplicate provider creds per (org, project) should be prevented at the DB layer; also return a 409 instead of ValueError for a better API contract.
Suggested DB constraint: UNIQUE(organization_id, project_id, provider, is_active) plus an index on (organization_id, project_id, provider, is_active).
backend/app/api/routes/credentials.py (4)
127-127: Redundant validation: get_current_user_org_project already validates org.You can remove this call for a tiny perf win.
Apply this diff:
- validate_organization(session, _current_user.organization_id) + # Organization was validated in dependency resolution.
167-172: Remove unused variable to satisfy Ruff (F841).Assignment isn’t needed; side effects are sufficient.
Apply this diff:
- updated_creds = remove_provider_credential( + remove_provider_credential( session=session, org_id=_current_user.organization_id, provider=provider_enum, project_id=_current_user.project_id, )
25-26: Style: prefer built-in generics over typing.List (UP006).No behavior change; keeps annotations modern.
Apply this diff:
- response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],- response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],- response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],Also applies to: 70-73, 117-121
196-200: Nit: clarify not-found message to include project scope.More precise in multi-project orgs.
Apply this diff:
- raise HTTPException( - status_code=404, detail="Credentials for organization not found" - ) + raise HTTPException( + status_code=404, detail="No credentials found for current organization/project" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/app/api/routes/credentials.py(6 hunks)backend/app/crud/credentials.py(2 hunks)backend/app/models/credentials.py(2 hunks)backend/app/tests/crud/test_credentials.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
backend/app/tests/crud/test_credentials.py (2)
backend/app/models/credentials.py (1)
CredsUpdate(43-58)backend/app/crud/credentials.py (1)
update_creds_for_org(129-164)
backend/app/crud/credentials.py (1)
backend/app/models/credentials.py (2)
CredsUpdate(43-58)Credential(61-105)
backend/app/models/credentials.py (1)
backend/app/tests/api/routes/test_creds.py (1)
test_update_credentials(183-217)
backend/app/api/routes/credentials.py (6)
backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/credentials.py (3)
CredsCreate(19-40)CredsPublic(108-116)CredsUpdate(43-58)backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)backend/app/crud/credentials.py (5)
get_creds_by_org(77-87)get_provider_credential(90-118)update_creds_for_org(129-164)remove_provider_credential(167-188)remove_creds_for_org(191-208)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
134-134: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/models/credentials.py
28-28: Use X | None for type annotations
Convert to X | None
(UP045)
32-32: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/api/routes/credentials.py
70-70: Use list instead of List for type annotation
Replace with list
(UP006)
117-117: Use list instead of List for type annotation
Replace with list
(UP006)
167-167: Local variable updated_creds is assigned to but never used
Remove assignment to unused variable updated_creds
(F841)
⏰ 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/models/credentials.py (2)
19-26: CredsCreate request model change looks good.Deriving org/project from API key and accepting a provider->credential mapping is aligned with the PR’s goals.
47-48: CredsUpdate doc note aligns with API-key–scoped behavior.No issues.
backend/app/tests/crud/test_credentials.py (2)
111-111: LGTM: CredsUpdate now excludes project_id.Matches the new model.
114-118: LGTM: update_creds_for_org now receives project_id separately.Consistent with the CRUD signature change.
backend/app/api/routes/credentials.py (3)
35-38: LGTM: derive org_id/project_id from API key context.Keeps client payloads clean and prevents scope mismatch.
69-88: LGTM: list credentials for the current org/project.404 on empty matches the PR’s stated behavior.
96-112: LGTM: provider-specific fetch scoped by API key.Validation + project scoping is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/app/models/credentials.py (2)
10-15: Fix model/DB mismatch for project_id (nullable and ondelete).Model declares
project_idwithdefault=Nonebutnullable=Falseandondelete="CASCADE", while migration sets it nullable withondelete=SET NULL. Align to Optional + nullable=True + SET NULL to prevent integrity errors and unexpected deletes.Apply:
- project_id: int = Field( - default=None, foreign_key="project.id", nullable=False, ondelete="CASCADE" - ) + project_id: Optional[int] = Field( + default=None, foreign_key="project.id", nullable=True, ondelete="SET NULL" + )
19-29: Correct CredsCreate.credential typing (None default vs non-Optional).
credentialhasdefault=Nonebut type isDict[str, Any]. Make it Optional for correctness and clearer validation.-class CredsCreate(SQLModel): +class CredsCreate(SQLModel): @@ - is_active: bool = True - credential: Dict[str, Any] = Field( - default=None, + is_active: bool = True + credential: Optional[Dict[str, Any]] = Field( + default=None, description="Dictionary mapping provider names to their credentials", )Optionally add a root validator ensuring at least one provider is supplied if you want schema-level enforcement.
backend/app/crud/credentials.py (3)
17-38: Good API-key scoping; add uniqueness enforcement and consistent error.Creation now uses explicit org/project args. Add a DB-level unique constraint to prevent duplicate provider per (org, project) under concurrency, and surface duplicates as 409/400 consistently.
- except IntegrityError as e: + except IntegrityError as e: session.rollback() - raise ValueError( - f"Error while adding credentials for provider {provider}: {str(e)}" - ) + raise HTTPException(409, f"Credentials already exist for provider '{provider}'")Migration suggestion (separate PR/commit):
op.create_unique_constraint( "uq_credential_org_project_provider", "credential", ["organization_id", "project_id", "provider"], )
65-76: Bug: decrypt before reading api_key.
credentialis stored encrypted (str).get_key_by_orgchecks it like a dict, so it always fails. Decrypt first.- creds = session.exec(statement).first() - - if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] + creds = session.exec(statement).first() + if creds and creds.credential: + data = decrypt_credentials(creds.credential) + return data.get("api_key")
169-191: Harden delete: filter active row, handle missing, set deleted_at.Currently may pick an inactive history row and doesn’t 404 when missing; also doesn’t set
deleted_at.- statement = select(Credential).where( - Credential.organization_id == org_id, - Credential.provider == provider, - Credential.project_id == project_id if project_id is not None else True, - ) + conditions = [ + Credential.organization_id == org_id, + Credential.provider == provider, + Credential.is_active == True, + ] + if project_id is not None: + conditions.append(Credential.project_id == project_id) + statement = select(Credential).where(*conditions) creds = session.exec(statement).first() + if creds is None: + raise HTTPException(404, "Credentials not found for this provider") # Soft delete creds.is_active = False creds.updated_at = now() + creds.deleted_at = now()backend/app/tests/crud/test_credentials.py (1)
154-158: Pass project_id when removing provider creds to avoid cross-project deletions.Without
project_id, the removal could target a different project’s credentials if they exist.- removed = remove_provider_credential( - session=db, org_id=credential.organization_id, provider="openai" - ) + removed = remove_provider_credential( + session=db, + org_id=credential.organization_id, + provider="openai", + project_id=project.id, + )
🧹 Nitpick comments (9)
backend/app/models/credentials.py (1)
57-60: Use Text for encrypted payload column.Encrypted JSON can exceed typical VARCHAR limits; prefer
sa.Textfor portability.- credential: str = Field( - sa_column=sa.Column(sa.String, nullable=False), + credential: str = Field( + sa_column=sa.Column(sa.Text, nullable=False), description="Encrypted provider-specific credentials", )backend/app/crud/credentials.py (3)
1-1: Adopt builtin generics and modern Optional syntax.Ruff hints (UP006/UP045) suggest switching to
list[...]andX | None. This also removes the need to importList.-from typing import Optional, Dict, Any, List, Union +from typing import Optional, Dict, Any, UnionThen update signatures/returns in this file, e.g.:
-) -> List[Credential]: +) -> list[Credential]: @@ -def get_creds_by_org(..., project_id: Optional[int] = None) -> List[Credential]: +def get_creds_by_org(..., project_id: int | None = None) -> list[Credential]:
83-88: Query condition pattern is brittle.
Credential.project_id == project_id if project_id is not None else Trueis awkward and may generate non-sargable SQL. Prefer conditional composition.- statement = select(Credential).where( - Credential.organization_id == org_id, - Credential.is_active == True, - Credential.project_id == project_id if project_id is not None else True, - ) + conditions = [ + Credential.organization_id == org_id, + Credential.is_active == True, + ] + if project_id is not None: + conditions.append(Credential.project_id == project_id) + statement = select(Credential).where(*conditions)Apply similarly in other queries in this module.
193-211: Soft delete all: also set deleted_at.Keep historical audit trail consistent.
for cred in creds: cred.is_active = False cred.updated_at = now() + cred.deleted_at = cred.deleted_at or now() session.add(cred)backend/app/tests/crud/test_credentials.py (1)
69-79: Scope retrieval by project for stricter tests (optional).Consider passing
project_idtoget_creds_by_orgto ensure behavior remains correct when multiple projects exist under the same org.- retrieved_creds = get_creds_by_org(session=db, org_id=project.organization_id) + retrieved_creds = get_creds_by_org( + session=db, org_id=project.organization_id, project_id=project.id + )backend/app/api/routes/credentials.py (4)
25-35: Use builtin generics in response models (ruff UP006).Swap
List[...]tolist[...]and you can dropListfrom imports.- response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],Apply similarly to other endpoints returning lists.
41-56: Guard against missing payload before duplicate check.If
creds_in.credentialis None,.keys()will fail. Early-return a 400; aligns with CRUD behavior.- # Prevent duplicate credentials + # Validate payload + if not creds_in.credential: + raise HTTPException(status_code=400, detail="No credentials provided") + # Prevent duplicate credentials
171-176: Remove unused variable per ruff F841.No need to store the result; you return a fixed message.
- updated_creds = remove_provider_credential( + remove_provider_credential( session=session, org_id=_current_user.organization_id, provider=provider_enum, project_id=_current_user.project_id, )
184-205: Consistent 404 wording (optional).Message says "for organization" but scope is org+project. Consider clarifying to reduce confusion.
- raise HTTPException( - status_code=404, detail="Credentials for organization not found" - ) + raise HTTPException( + status_code=404, + detail="Credentials not found for this organization and project", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/app/api/routes/credentials.py(5 hunks)backend/app/crud/credentials.py(4 hunks)backend/app/models/credentials.py(1 hunks)backend/app/tests/crud/test_credentials.py(9 hunks)backend/app/tests/utils/test_data.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
backend/app/models/credentials.py (2)
backend/app/tests/api/routes/test_creds.py (2)
test_update_credentials(183-217)test_set_credential(32-62)backend/app/alembic/versions/904ed70e7dab_added_provider_column_to_the_credential_.py (1)
upgrade(19-61)
backend/app/tests/utils/test_data.py (1)
backend/app/crud/credentials.py (1)
set_creds_for_org(17-54)
backend/app/crud/credentials.py (2)
backend/app/models/credentials.py (3)
CredsCreate(19-29)Credential(48-92)CredsUpdate(32-45)backend/app/tests/api/routes/test_creds.py (1)
test_set_credentials_for_invalid_project_org_relationship(65-87)
backend/app/tests/crud/test_credentials.py (3)
backend/app/crud/credentials.py (3)
set_creds_for_org(17-54)get_provider_credential(92-120)update_creds_for_org(131-166)backend/app/tests/utils/test_data.py (1)
create_test_project(34-50)backend/app/models/credentials.py (1)
CredsUpdate(32-45)
backend/app/api/routes/credentials.py (6)
backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/credentials.py (4)
CredsCreate(19-29)CredsPublic(95-103)CredsUpdate(32-45)to_public(76-92)backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/crud/credentials.py (6)
get_provider_credential(92-120)set_creds_for_org(17-54)get_creds_by_org(79-89)update_creds_for_org(131-166)remove_provider_credential(169-190)remove_creds_for_org(193-210)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
19-19: Use list instead of List for type annotation
Replace with list
(UP006)
136-136: Use X | None for type annotations
Convert to X | None
(UP045)
backend/app/api/routes/credentials.py
67-67: Undefined name new_creds
(F821)
72-72: Use list instead of List for type annotation
Replace with list
(UP006)
119-119: Use list instead of List for type annotation
Replace with list
(UP006)
171-171: Local variable updated_creds is assigned to but never used
Remove assignment to unused variable updated_creds
(F841)
🪛 GitHub Actions: AI Platform CI
backend/app/api/routes/credentials.py
[error] 67-67: NameError: name 'new_creds' is not defined in create_new_credential. The function returns 'new_creds' which is not defined; should return the created credentials (likely the 'credential' variable or a properly named local).
🔇 Additional comments (3)
backend/app/tests/utils/test_data.py (1)
110-118: Looks good: updated helper passes org/project explicitly.The refactor matches new CRUD signatures and keeps return type unchanged.
backend/app/crud/credentials.py (1)
131-166: The verification script will extract theCredsUpdateclass definition and its fields to confirm whetheris_activeis present and optional. Once we have that, we can determine if the suggested refactor is valid.backend/app/api/routes/credentials.py (1)
71-90: LGTM: listing creds from API-key scope.Reads are correctly constrained to the current org/project, returning public models.
| # Create credentials | ||
| new_creds = set_creds_for_org(session=session, creds_add=creds_in) | ||
| if not new_creds: | ||
| credential = set_creds_for_org( | ||
| session=session, | ||
| creds_add=creds_in, | ||
| organization_id=_current_user.organization_id, | ||
| project_id=_current_user.project_id, | ||
| ) | ||
| if not credential: | ||
| raise Exception(status_code=500, detail="Failed to create credentials") | ||
|
|
||
| return APIResponse.success_response([cred.to_public() for cred in new_creds]) |
There was a problem hiding this comment.
Fix NameError and exception type; return created creds.
new_creds is undefined, and Exception(status_code=500, ...) is not an HTTP error. Use HTTPException and the actual variable. Also rename for clarity.
- credential = set_creds_for_org(
+ created_creds = set_creds_for_org(
session=session,
creds_add=creds_in,
organization_id=_current_user.organization_id,
project_id=_current_user.project_id,
)
- if not credential:
- raise Exception(status_code=500, detail="Failed to create credentials")
-
- return APIResponse.success_response([cred.to_public() for cred in new_creds])
+ if not created_creds:
+ raise HTTPException(status_code=500, detail="Failed to create credentials")
+ return APIResponse.success_response([cred.to_public() for cred in created_creds])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Create credentials | |
| new_creds = set_creds_for_org(session=session, creds_add=creds_in) | |
| if not new_creds: | |
| credential = set_creds_for_org( | |
| session=session, | |
| creds_add=creds_in, | |
| organization_id=_current_user.organization_id, | |
| project_id=_current_user.project_id, | |
| ) | |
| if not credential: | |
| raise Exception(status_code=500, detail="Failed to create credentials") | |
| return APIResponse.success_response([cred.to_public() for cred in new_creds]) | |
| # Create credentials | |
| created_creds = set_creds_for_org( | |
| session=session, | |
| creds_add=creds_in, | |
| organization_id=_current_user.organization_id, | |
| project_id=_current_user.project_id, | |
| ) | |
| if not created_creds: | |
| raise HTTPException(status_code=500, detail="Failed to create credentials") | |
| return APIResponse.success_response([cred.to_public() for cred in created_creds]) |
🧰 Tools
🪛 Ruff (0.12.2)
67-67: Undefined name new_creds
(F821)
🪛 GitHub Actions: AI Platform CI
[error] 67-67: NameError: name 'new_creds' is not defined in create_new_credential. The function returns 'new_creds' which is not defined; should return the created credentials (likely the 'credential' variable or a properly named local).
🤖 Prompt for AI Agents
In backend/app/api/routes/credentials.py around lines 57 to 67, replace the
undefined new_creds and the generic Exception with a proper HTTPException and
the actual returned credential variable: rename the result of set_creds_for_org
to created_creds (or use credential consistently), raise
fastapi.HTTPException(status_code=500, detail="Failed to create credentials")
when creation fails, and return APIResponse.success_response([cred.to_public()
for cred in created_creds]); ensure fastapi.HTTPException is imported if not
already.
| summary="Create new credentials for an organization and project", | ||
| description="Creates new credentials for a specific organization and project combination. This endpoint requires superuser privileges. Each organization can have different credentials for different providers and projects. Only one credential per provider is allowed per organization-project combination.", | ||
| summary="Create new credentials for the current organization and project", | ||
| description="Creates new credentials for the caller's organization and project as derived from the API key. Each organization can have different credentials for different providers and projects. Only one credential per provider is allowed per organization-project combination.", |
There was a problem hiding this comment.
Please remove this part "as derived from the API key." from description.
There was a problem hiding this comment.
didn't understand this, it is anyway picked from API key now.
| summary="Get all credentials for an organization and project", | ||
| description="Retrieves all provider credentials associated with a specific organization and project combination. If project_id is not provided, returns credentials for the organization level. This endpoint requires superuser privileges.", | ||
| summary="Get all credentials for current org and project", | ||
| description="Retrieves all provider credentials associated with the caller's organization and project derived from the API key.", |
There was a problem hiding this comment.
Again remove "derived from the API key".
There was a problem hiding this comment.
same, now we are taking project_id and organization_id from API key so this description is correct
| client: TestClient, | ||
| db: Session, | ||
| user_api_key_header: dict[str, str], | ||
| user_api_key: APIKeyPublic, |
There was a problem hiding this comment.
Same goes for all the test cases,
Use one of user_api_key or user_api_key_header
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/api/routes/credentials.py (2)
27-28: Remove “derived from the API key” from descriptions (requested earlier).Mirror the reviewer’s direction.
- summary="Create new credentials for the current organization and project", - description="Creates new credentials for the caller's organization and project as derived from the API key. Each organization can have different credentials for different providers and projects. Only one credential per provider is allowed per organization-project combination.", + summary="Create new credentials for the current organization and project", + description="Creates new credentials for the caller's organization and project. Each organization can have different credentials for different providers and projects. Only one credential per provider is allowed per organization-project combination.", @@ - summary="Get all credentials for current org and project", - description="Retrieves all provider credentials associated with the caller's organization and project derived from the API key.", + summary="Get all credentials for current org and project", + description="Retrieves all provider credentials associated with the caller's organization and project.",Also applies to: 73-75
58-67: Use HTTPException and return the correct variable; fix naming.Aligns with prior feedback; also avoids generic Exception.
- credential = set_creds_for_org( + created_creds = set_creds_for_org( session=session, creds_add=creds_in, organization_id=_current_user.organization_id, project_id=_current_user.project_id, ) - if not credential: - raise Exception(status_code=500, detail="Failed to create credentials") - - return APIResponse.success_response([cred.to_public() for cred in credential]) + if not created_creds: + raise HTTPException(status_code=500, detail="Failed to create credentials") + return APIResponse.success_response([cred.to_public() for cred in created_creds])
🧹 Nitpick comments (6)
backend/app/api/routes/credentials.py (6)
96-97: Consistency: remove “derived from the API key” in remaining endpoint docs.Keeps docs uniform with prior edits.
- summary="Get specific provider credentials for current org and project", - description="Retrieves credentials for a specific provider (e.g., 'openai', 'anthropic') for the caller's organization and project derived from the API key.", + summary="Get specific provider credentials for current org and project", + description="Retrieves credentials for a specific provider (e.g., 'openai', 'anthropic') for the caller's organization and project.", @@ - summary="Update credentials for current org and project", - description="Updates credentials for a specific provider of the caller's organization and project derived from the API key.", + summary="Update credentials for current org and project", + description="Updates credentials for a specific provider of the caller's organization and project.", @@ - summary="Delete all credentials for current org and project", - description="Removes all credentials for the caller's organization and project derived from the API key. This is a soft delete operation that marks credentials as inactive.", + summary="Delete all credentials for current org and project", + description="Removes all credentials for the caller's organization and project. This is a soft delete operation that marks credentials as inactive.",Also applies to: 121-122, 187-188
1-1: Use built-in generics; drop unused typing import.Satisfies Ruff UP006 and removes dead import.
-from typing import List @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]], @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]], @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],Also applies to: 26-26, 72-72, 119-119
160-162: Remove redundant provider validation check.validate_provider() already enforces validity and raises on error.
- if not provider_enum: - raise HTTPException(status_code=400, detail="Invalid provider")
171-176: Drop unused variable assignment.Cleans up Ruff F841.
- updated_creds = remove_provider_credential( + remove_provider_credential( session=session, org_id=_current_user.organization_id, provider=provider_enum, project_id=_current_user.project_id, )
36-36: Avoid redundant organization validation; dependency already validates.get_current_user_org_project() calls validate_organization; duplicate checks add noise.
-from app.crud import validate_organization @@ - validate_organization(session, _current_user.organization_id) @@ - validate_organization(session, _current_user.organization_id)Also applies to: 129-129, 14-14
201-205: Clarify 404 and success messages.Mention project in 404 and make success explicit.
- raise HTTPException( - status_code=404, detail="Credentials for organization not found" - ) + raise HTTPException( + status_code=404, detail="Credentials for organization/project not found" + ) @@ - return APIResponse.success_response({"message": "Credentials deleted successfully"}) + return APIResponse.success_response({"message": "All credentials deleted successfully"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/api/routes/credentials.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/routes/credentials.py (6)
backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/organization.py (1)
validate_organization(40-57)backend/app/models/credentials.py (4)
CredsCreate(19-29)CredsPublic(95-103)CredsUpdate(32-45)to_public(76-92)backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/crud/credentials.py (6)
get_provider_credential(92-120)set_creds_for_org(17-54)get_creds_by_org(79-89)update_creds_for_org(131-166)remove_provider_credential(169-190)remove_creds_for_org(193-210)backend/app/utils.py (2)
APIResponse(27-48)success_response(34-37)
🪛 Ruff (0.12.2)
backend/app/api/routes/credentials.py
72-72: Use list instead of List for type annotation
Replace with list
(UP006)
119-119: Use list instead of List for type annotation
Replace with list
(UP006)
171-171: Local variable updated_creds is assigned to but never used
Remove assignment to unused variable updated_creds
(F841)
⏰ 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/credentials.py (1)
89-89: Confirm intent to return decrypted credentials to clients.Both list and provider reads expose decrypted credential blobs via to_public()/dict. Confirm this is acceptable for your threat model; otherwise mask/seal sensitive fields.
Also applies to: 114-114
| for provider in creds_in.credential.keys(): | ||
| existing_cred = get_provider_credential( | ||
| session=session, | ||
| org_id=creds_in.organization_id, | ||
| org_id=_current_user.organization_id, | ||
| provider=provider, | ||
| project_id=creds_in.project_id, | ||
| project_id=_current_user.project_id, | ||
| ) |
There was a problem hiding this comment.
Prevent NoneType error when creds_in.credential is missing.
Accessing keys on None will raise at runtime. Validate before the loop.
Apply:
# Prevent duplicate credentials
- for provider in creds_in.credential.keys():
+ if not creds_in.credential:
+ raise HTTPException(status_code=400, detail="No credentials provided")
+ for provider in creds_in.credential.keys():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for provider in creds_in.credential.keys(): | |
| existing_cred = get_provider_credential( | |
| session=session, | |
| org_id=creds_in.organization_id, | |
| org_id=_current_user.organization_id, | |
| provider=provider, | |
| project_id=creds_in.project_id, | |
| project_id=_current_user.project_id, | |
| ) | |
| # Prevent duplicate credentials | |
| if not creds_in.credential: | |
| raise HTTPException(status_code=400, detail="No credentials provided") | |
| for provider in creds_in.credential.keys(): | |
| existing_cred = get_provider_credential( | |
| session=session, | |
| org_id=_current_user.organization_id, | |
| provider=provider, | |
| project_id=_current_user.project_id, | |
| ) |
🤖 Prompt for AI Agents
In backend/app/api/routes/credentials.py around lines 41 to 47, the code loops
over creds_in.credential without validating it, which will raise an
AttributeError if creds_in.credential is None; add a guard before the loop to
verify creds_in.credential is present and is a mapping (e.g. if not
creds_in.credential or not isinstance(creds_in.credential, dict): raise
HTTPException(status_code=400, detail="Missing or invalid credential payload")
or alternatively treat missing as empty dict and skip processing), then proceed
with the for loop; ensure any raised HTTPException is imported from fastapi and
tests/clients expect a 400 for missing credentials.
2 participants
Summary
Target issue is #355
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.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Summary by CodeRabbit
New Features
Refactor
Tests