-
Notifications
You must be signed in to change notification settings - Fork 10
Credentials: Add support for org credentials #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
29d219a
Add provider column to credential table and update API for provider-s…
priyanshu6238 b91b3fc
Refactor credential tests to streamline organization and credential c…
priyanshu6238 a7fb888
Add soft delete functionality for credentials and update tests to ver…
priyanshu6238 206fd9b
Refactor credential handling to improve error management and ensure p…
priyanshu6238 e935a15
Enhance credential update handling with organization existence checks…
priyanshu6238 dfd17b4
Enhance credential update handling with validation for provider and c…
priyanshu6238 c1a54e8
Refactor credential
priyanshu6238 6e841cc
Merge branch 'main' into credential_4
priyanshu6238 df539d0
Fix down_revision reference in migration script for provider column a…
priyanshu6238 35329e2
added provider test in core
priyanshu6238 d4fa0cc
refactor test_provider.py
priyanshu6238 ba4d3ad
updates
priyanshu6238 7c49131
cherry picking from other PR
priyanshu6238 7533b1a
using encrypt/decrypt
df8303e
cleanups
668cb9b
decrypting credentials
1ab315e
added support for per project per organization
db02b99
few more steps forward to per org per project per provider
ac7b2a6
encrypting the entire credentials column
0e64dfd
added testcases
2900865
updating threads codes
6c34316
updating time
43080b7
updating time
4fae6db
reverting changes
1322ee7
update migration
636ba91
updated testcases
6258c81
updated credentials testcases
2560882
using langfuse from creds
d25aa89
Merge branch 'main' into feature/add-provider-org-credentials
AkhileshNegi 083e72d
few cleanups
e2d20fa
cleanup & refactoring
dec44e0
refactor security
92002f7
getting rid of redundant str()
a4fd2ca
using now()
dd35f67
cleanup few testcases and code
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
93 changes: 93 additions & 0 deletions
93
backend/app/alembic/versions/904ed70e7dab_added_provider_column_to_the_credential_.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| """Added provider column to the credential table | ||
|
|
||
| Revision ID: 904ed70e7dab | ||
| Revises: 79e47bc3aac6 | ||
| Create Date: 2025-05-10 11:13:17.868238 | ||
|
|
||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| import sqlmodel.sql.sqltypes | ||
|
|
||
|
|
||
| revision = "904ed70e7dab" | ||
| down_revision = "79e47bc3aac6" | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): | ||
| # Add new columns to credential table | ||
| op.add_column( | ||
| "credential", | ||
| sa.Column("provider", sqlmodel.sql.sqltypes.AutoString(), nullable=False), | ||
| ) | ||
| op.add_column("credential", sa.Column("project_id", sa.Integer(), nullable=True)) | ||
|
|
||
| # Create indexes and constraints | ||
| op.create_index( | ||
| op.f("ix_credential_provider"), "credential", ["provider"], unique=False | ||
| ) | ||
|
|
||
| # Drop existing foreign keys | ||
| op.drop_constraint( | ||
| "credential_organization_id_fkey", "credential", type_="foreignkey" | ||
| ) | ||
| op.drop_constraint("project_organization_id_fkey", "project", type_="foreignkey") | ||
|
|
||
| # Create all foreign keys together | ||
| op.create_foreign_key( | ||
| "credential_organization_id_fkey", | ||
| "credential", | ||
| "organization", | ||
| ["organization_id"], | ||
| ["id"], | ||
| ondelete="CASCADE", | ||
| ) | ||
| op.create_foreign_key( | ||
| None, | ||
| "project", | ||
| "organization", | ||
| ["organization_id"], | ||
| ["id"], | ||
| ) | ||
| op.create_foreign_key( | ||
| "credential_project_id_fkey", | ||
| "credential", | ||
| "project", | ||
| ["project_id"], | ||
| ["id"], | ||
| ondelete="SET NULL", | ||
| ) | ||
|
|
||
|
|
||
| def downgrade(): | ||
| # Drop project_id foreign key and column | ||
| op.drop_constraint("credential_project_id_fkey", "credential", type_="foreignkey") | ||
| op.drop_column("credential", "project_id") | ||
|
|
||
| # Drop existing foreign keys | ||
| op.drop_constraint(None, "project", type_="foreignkey") | ||
| op.drop_constraint( | ||
| "credential_organization_id_fkey", "credential", type_="foreignkey" | ||
| ) | ||
|
|
||
| # Create all foreign keys together | ||
| op.create_foreign_key( | ||
| "project_organization_id_fkey", | ||
| "project", | ||
| "organization", | ||
| ["organization_id"], | ||
| ["id"], | ||
| ondelete="CASCADE", | ||
| ) | ||
| op.create_foreign_key( | ||
| "credential_organization_id_fkey", | ||
| "credential", | ||
| "organization", | ||
| ["organization_id"], | ||
| ["id"], | ||
| ) | ||
|
|
||
| op.drop_index(op.f("ix_credential_provider"), table_name="credential") | ||
| op.drop_column("credential", "provider") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,95 +1,147 @@ | ||
| from typing import List | ||
|
|
||
| from fastapi import APIRouter, Depends, HTTPException | ||
| from sqlalchemy.exc import IntegrityError | ||
|
|
||
| from app.api.deps import SessionDep, get_current_active_superuser | ||
| from app.crud.credentials import ( | ||
| get_creds_by_org, | ||
| get_key_by_org, | ||
| get_provider_credential, | ||
| remove_creds_for_org, | ||
| set_creds_for_org, | ||
| update_creds_for_org, | ||
| remove_provider_credential, | ||
| ) | ||
| from app.models import CredsCreate, CredsPublic, CredsUpdate | ||
| from app.models.organization import Organization | ||
| from app.models.project import Project | ||
| from app.utils import APIResponse | ||
| from datetime import datetime | ||
| from app.core.providers import validate_provider | ||
|
|
||
| router = APIRouter(prefix="/credentials", tags=["credentials"]) | ||
|
|
||
|
|
||
| @router.post( | ||
| "/", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[CredsPublic], | ||
| response_model=APIResponse[List[CredsPublic]], | ||
| 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.", | ||
| ) | ||
| def create_new_credential(*, session: SessionDep, creds_in: CredsCreate): | ||
| new_creds = None | ||
| try: | ||
| existing_creds = get_creds_by_org( | ||
| session=session, org_id=creds_in.organization_id | ||
| ) | ||
| if not existing_creds: | ||
| new_creds = set_creds_for_org(session=session, creds_add=creds_in) | ||
| # Check if organization exists | ||
| organization = session.get(Organization, creds_in.organization_id) | ||
| if not organization: | ||
| raise HTTPException(status_code=404, detail="Organization not found") | ||
|
|
||
| # Check if project exists if project_id is provided | ||
| if creds_in.project_id: | ||
| project = session.get(Project, creds_in.project_id) | ||
| if not project: | ||
| raise HTTPException(status_code=404, detail="Project not found") | ||
| if project.organization_id != creds_in.organization_id: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Project does not belong to the specified organization", | ||
| ) | ||
|
|
||
| # Check for existing credentials for each provider | ||
| for provider in creds_in.credential.keys(): | ||
| existing_cred = get_provider_credential( | ||
| session=session, | ||
| org_id=creds_in.organization_id, | ||
| provider=provider, | ||
| project_id=creds_in.project_id, | ||
| ) | ||
| if existing_cred: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Credentials for provider '{provider}' already exist for this organization and project combination", | ||
| ) | ||
|
|
||
| # Create new credentials | ||
| new_creds = set_creds_for_org(session=session, creds_add=creds_in) | ||
| if not new_creds: | ||
| raise HTTPException(status_code=500, detail="Failed to create credentials") | ||
| return APIResponse.success_response([cred.to_public() for cred in new_creds]) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=404, detail=str(e)) | ||
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=500, detail=f"An unexpected error occurred: {str(e)}" | ||
| ) | ||
|
|
||
| # Ensure inserted_at is set during creation | ||
| new_creds.inserted_at = datetime.utcnow() | ||
|
|
||
| return APIResponse.success_response(new_creds) | ||
|
|
||
|
|
||
| @router.get( | ||
| "/{org_id}", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[CredsPublic], | ||
| response_model=APIResponse[List[CredsPublic]], | ||
| 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.", | ||
| ) | ||
| def read_credential(*, session: SessionDep, org_id: int): | ||
| try: | ||
| creds = get_creds_by_org(session=session, org_id=org_id) | ||
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=500, detail=f"An unexpected error occurred: {str(e)}" | ||
| ) | ||
|
|
||
| if creds is None: | ||
| def read_credential(*, session: SessionDep, org_id: int, project_id: int | None = None): | ||
| creds = get_creds_by_org(session=session, org_id=org_id, project_id=project_id) | ||
| if not creds: | ||
| raise HTTPException(status_code=404, detail="Credentials not found") | ||
|
|
||
| return APIResponse.success_response(creds) | ||
| return APIResponse.success_response([cred.to_public() for cred in creds]) | ||
|
|
||
|
|
||
| @router.get( | ||
| "/{org_id}/api-key", | ||
| "/{org_id}/{provider}", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[dict], | ||
| summary="Get specific provider credentials for an organization and project", | ||
| description="Retrieves credentials for a specific provider (e.g., 'openai', 'anthropic') for a given organization and project combination. If project_id is not provided, returns organization-level credentials. This endpoint requires superuser privileges.", | ||
| ) | ||
| def read_api_key(*, session: SessionDep, org_id: int): | ||
| try: | ||
| api_key = get_key_by_org(session=session, org_id=org_id) | ||
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=500, detail=f"An unexpected error occurred: {str(e)}" | ||
| ) | ||
|
|
||
| if api_key is None: | ||
| raise HTTPException(status_code=404, detail="API key not found") | ||
|
|
||
| return APIResponse.success_response({"api_key": api_key}) | ||
| def read_provider_credential( | ||
| *, session: SessionDep, org_id: int, provider: str, project_id: int | None = None | ||
| ): | ||
| provider_enum = validate_provider(provider) | ||
| provider_creds = get_provider_credential( | ||
| session=session, | ||
| org_id=org_id, | ||
| provider=provider_enum, | ||
| project_id=project_id, | ||
| ) | ||
| if provider_creds is None: | ||
| raise HTTPException(status_code=404, detail="Provider credentials not found") | ||
| return APIResponse.success_response(provider_creds) | ||
|
|
||
|
|
||
| @router.patch( | ||
| "/{org_id}", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[CredsPublic], | ||
| response_model=APIResponse[List[CredsPublic]], | ||
| summary="Update organization and project credentials", | ||
| description="Updates credentials for a specific organization and project combination. Can update specific provider credentials or add new providers. If project_id is provided in the update, credentials will be moved to that project. This endpoint requires superuser privileges.", | ||
| ) | ||
| def update_credential(*, session: SessionDep, org_id: int, creds_in: CredsUpdate): | ||
| try: | ||
| if not creds_in or not creds_in.provider or not creds_in.credential: | ||
| raise HTTPException( | ||
| status_code=400, detail="Provider and credential must be provided" | ||
| ) | ||
|
Comment on lines
+121
to
+124
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be in a try-except? |
||
| organization = session.get(Organization, org_id) | ||
| if not organization: | ||
| raise HTTPException(status_code=404, detail="Organization not found") | ||
| updated_creds = update_creds_for_org( | ||
| session=session, org_id=org_id, creds_in=creds_in | ||
| ) | ||
|
|
||
| updated_creds.updated_at = datetime.utcnow() | ||
|
|
||
| return APIResponse.success_response(updated_creds) | ||
| if not updated_creds: | ||
| raise HTTPException(status_code=404, detail="Failed to update credentials") | ||
| return APIResponse.success_response( | ||
| [cred.to_public() for cred in updated_creds] | ||
| ) | ||
| except IntegrityError as e: | ||
| if "ForeignKeyViolation" in str(e): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Invalid organization ID. Ensure the organization exists before updating credentials.", | ||
| ) | ||
| raise HTTPException( | ||
| status_code=500, detail=f"An unexpected database error occurred: {str(e)}" | ||
| ) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=404, detail=str(e)) | ||
| except Exception as e: | ||
|
|
@@ -98,30 +150,61 @@ def update_credential(*, session: SessionDep, org_id: int, creds_in: CredsUpdate | |
| ) | ||
|
|
||
|
|
||
| from fastapi import HTTPException, Depends | ||
| from app.crud.credentials import remove_creds_for_org | ||
| from app.utils import APIResponse | ||
| from app.api.deps import SessionDep, get_current_active_superuser | ||
|
|
||
|
|
||
| @router.delete( | ||
| "/{org_id}/api-key", | ||
| "/{org_id}/{provider}", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[dict], | ||
| summary="Delete specific provider credentials for an organization and project", | ||
| description="Removes credentials for a specific provider while keeping other provider credentials intact. If project_id is provided, only removes credentials for that project. This endpoint requires superuser privileges.", | ||
| ) | ||
| def delete_credential(*, session: SessionDep, org_id: int): | ||
| def delete_provider_credential( | ||
| *, session: SessionDep, org_id: int, provider: str, project_id: int | None = None | ||
| ): | ||
| try: | ||
| creds = remove_creds_for_org(session=session, org_id=org_id) | ||
| provider_enum = validate_provider(provider) | ||
| updated_creds = remove_provider_credential( | ||
| session=session, | ||
| org_id=org_id, | ||
| provider=provider_enum, | ||
| project_id=project_id, | ||
| ) | ||
| if not updated_creds: | ||
| raise HTTPException( | ||
| status_code=404, detail="Provider credentials not found" | ||
| ) | ||
| return APIResponse.success_response( | ||
| {"message": "Provider credentials removed successfully"} | ||
| ) | ||
| except ValueError: | ||
| raise HTTPException(status_code=404, detail="Provider credentials not found") | ||
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=500, detail=f"An unexpected error occurred: {str(e)}" | ||
| ) | ||
|
|
||
| if creds is None: | ||
|
|
||
| @router.delete( | ||
| "/{org_id}", | ||
| dependencies=[Depends(get_current_active_superuser)], | ||
| response_model=APIResponse[dict], | ||
| summary="Delete all credentials for an organization and project", | ||
| description="Removes all credentials for a specific organization and project combination. If project_id is provided, only removes credentials for that project. This is a soft delete operation that marks credentials as inactive. This endpoint requires superuser privileges.", | ||
| ) | ||
| def delete_all_credentials( | ||
| *, session: SessionDep, org_id: int, project_id: int | None = None | ||
| ): | ||
| try: | ||
| creds = remove_creds_for_org( | ||
| session=session, org_id=org_id, project_id=project_id | ||
| ) | ||
| if not creds: | ||
| raise HTTPException( | ||
| status_code=404, detail="Credentials for organization not found" | ||
| ) | ||
| return APIResponse.success_response( | ||
| {"message": "Credentials deleted successfully"} | ||
| ) | ||
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=404, detail="Credentials for organization not found" | ||
| status_code=500, detail=f"An unexpected error occurred: {str(e)}" | ||
| ) | ||
|
|
||
| # No need to manually set deleted_at and is_active if it's done in remove_creds_for_org | ||
| # Simply return the success response | ||
| return APIResponse.success_response({"message": "Credentials deleted successfully"}) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of these outer exception handlers, and most notably because of the base exception handling here, none of the exceptions in the function body will be raised on their own. Notably, all HTTPExceptions raised in the function body will be cause by this parent exception, which will return 500 to the user. Because the body-exceptions raise different status codes, I imagine this -- raising 500 always -- was not the intention.