Skip to content
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

Basic API key setup with core APIs #226

Merged
merged 5 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions app/api/router.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
from fastapi import Depends, HTTPException, Header
from fastapi.responses import StreamingResponse
from sqlalchemy.orm import Session
from typing import Optional, List
from datetime import datetime
from pydantic import BaseModel

from app.core.database import get_db
from app.modules.auth.api_key_service import APIKeyService
from app.modules.conversations.conversation.conversation_controller import ConversationController
from app.modules.conversations.message.message_schema import MessageRequest
from app.modules.parsing.graph_construction.parsing_controller import ParsingController
from app.modules.parsing.graph_construction.parsing_schema import ParsingRequest
from app.modules.utils.APIRouter import APIRouter
from app.modules.conversations.conversation.conversation_schema import (
CreateConversationRequest,
CreateConversationResponse,
ConversationStatus,
)

router = APIRouter()

class SimpleConversationRequest(BaseModel):
project_ids: List[str]
agent_ids: List[str]

async def get_api_key_user(
x_api_key: Optional[str] = Header(None),
db: Session = Depends(get_db)
Comment on lines +28 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid calling Depends directly in default argument values.

Using Depends directly in the default values of function arguments can lead to unexpected behavior. FastAPI recommends declaring dependencies in the function parameters without default values.

Apply this diff to fix the issue:

-    db: Session = Depends(get_db)
+    db: Session = Depends(get_db),

Adjust the function signatures accordingly:

async def get_api_key_user(
    x_api_key: Optional[str] = Header(None),
-    db: Session = Depends(get_db)
+    db: Session = Depends(get_db),
) -> dict:

async def create_conversation(
    conversation: SimpleConversationRequest,
-    db: Session = Depends(get_db),
-    user=Depends(get_api_key_user),
+    db: Session = Depends(get_db),
+    user=Depends(get_api_key_user),
):

async def parse_directory(
    repo_details: ParsingRequest,
-    db: Session = Depends(get_db),
-    user=Depends(get_api_key_user),
+    db: Session = Depends(get_db),
+    user=Depends(get_api_key_user),
):

async def get_parsing_status(
    project_id: str,
-    db: Session = Depends(get_db),
-    user=Depends(get_api_key_user),
+    db: Session = Depends(get_db),
+    user=Depends(get_api_key_user),
):

async def post_message(
    conversation_id: str,
    message: MessageRequest,
-    db: Session = Depends(get_db),
-    user=Depends(get_api_key_user),
+    db: Session = Depends(get_db),
+    user=Depends(get_api_key_user),
):

Also applies to: 52-53, 71-72, 79-80, 88-89

🧰 Tools
🪛 Ruff (0.8.2)

29-29: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

) -> dict:
"""Dependency to validate API key and get user info."""
if not x_api_key:
raise HTTPException(
status_code=401,
detail="API key is required",
headers={"WWW-Authenticate": "ApiKey"},
)

user = await APIKeyService.validate_api_key(x_api_key, db)
if not user:
Comment on lines +39 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure validate_api_key is an async function or remove await.

The call to APIKeyService.validate_api_key is awaited, implying it's an asynchronous function. Confirm that validate_api_key is defined using async def. If it's a synchronous function, remove the await keyword.

If validate_api_key is synchronous, apply this diff:

-    user = await APIKeyService.validate_api_key(x_api_key, db)
+    user = APIKeyService.validate_api_key(x_api_key, db)

Committable suggestion skipped: line range outside the PR's diff.

raise HTTPException(
status_code=401,
detail="Invalid API key",
headers={"WWW-Authenticate": "ApiKey"},
)

return user

@router.post("/conversations/", response_model=CreateConversationResponse)
async def create_conversation(
conversation: SimpleConversationRequest,
db: Session = Depends(get_db),
user=Depends(get_api_key_user),
):
user_id = user["user_id"]
# Create full conversation request with defaults
full_request = CreateConversationRequest(
user_id=user_id,
title=datetime.now().strftime("%Y-%m-%d %H:%M:%S"),
status=ConversationStatus.ARCHIVED,
project_ids=conversation.project_ids,
agent_ids=conversation.agent_ids
)

controller = ConversationController(db, user_id, None)
return await controller.create_conversation(full_request)

@router.post("/parse")
async def parse_directory(
repo_details: ParsingRequest,
db: Session = Depends(get_db),
user=Depends(get_api_key_user),
):
return await ParsingController.parse_directory(repo_details, db, user)

@router.get("/parsing-status/{project_id}")
async def get_parsing_status(
project_id: str,
db: Session = Depends(get_db),
user=Depends(get_api_key_user),
):
return await ParsingController.fetch_parsing_status(project_id, db, user)

@router.post("/conversations/{conversation_id}/message/")
async def post_message(
conversation_id: str,
message: MessageRequest,
db: Session = Depends(get_db),
user=Depends(get_api_key_user),
):
if message.content == "" or message.content is None or message.content.isspace():
raise HTTPException(
status_code=400,
detail="Message content cannot be empty"
)

user_id = user["user_id"]
# Note: email is no longer available with API key auth
controller = ConversationController(db, user_id, None)
message_stream = controller.post_message(conversation_id, message, stream=False)
return StreamingResponse(message_stream, media_type="text/event-stream")
Comment on lines +100 to +101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent streaming configuration in post_message.

You're setting stream=False when calling controller.post_message, but wrapping the result in a StreamingResponse. To properly stream the response, set stream=True.

Apply this diff:

-    message_stream = controller.post_message(conversation_id, message, stream=False)
+    message_stream = controller.post_message(conversation_id, message, stream=True)
📝 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.

Suggested change
message_stream = controller.post_message(conversation_id, message, stream=False)
return StreamingResponse(message_stream, media_type="text/event-stream")
message_stream = controller.post_message(conversation_id, message, stream=True)
return StreamingResponse(message_stream, media_type="text/event-stream")

4 changes: 4 additions & 0 deletions app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from app.modules.intelligence.provider.provider_router import router as provider_router
from app.modules.intelligence.tools.tool_router import router as tool_router
from app.modules.key_management.secret_manager import router as secret_manager_router
from app.api.router import router as potpie_api_router
from app.modules.parsing.graph_construction.parsing_router import (
router as parsing_router,
)
Expand Down Expand Up @@ -113,6 +114,9 @@ def include_routers(self):
self.app.include_router(provider_router, prefix="/api/v1", tags=["Providers"])
self.app.include_router(tool_router, prefix="/api/v1", tags=["Tools"])
self.app.include_router(usage_router, prefix="/api/v1/usage", tags=["Usage"])
self.app.include_router(
potpie_api_router, prefix="/api/v2", tags=["Potpie API"]
)
if os.getenv("isDevelopmentMode") != "enabled":
self.app.include_router(
secret_manager_router, prefix="/api/v1", tags=["Secret Manager"]
Expand Down
162 changes: 162 additions & 0 deletions app/modules/auth/api_key_service.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import os
import secrets
import hashlib
from typing import Optional
from fastapi import HTTPException
from sqlalchemy.orm import Session
from google.cloud import secretmanager
from app.modules.users.user_preferences_model import UserPreferences
from sqlalchemy import text

class APIKeyService:
SECRET_PREFIX = "sk-"
KEY_LENGTH = 32

@staticmethod
def get_client_and_project():
"""Get Secret Manager client and project ID based on environment."""
is_dev_mode = os.getenv("isDevelopmentMode", "enabled") == "enabled"
if is_dev_mode:
return None, None

project_id = os.environ.get("GCP_PROJECT")
if not project_id:
raise HTTPException(
status_code=500,
detail="GCP_PROJECT environment variable is not set"
)

try:
client = secretmanager.SecretManagerServiceClient()
return client, project_id
except Exception as e:
raise HTTPException(
status_code=500,
detail=f"Failed to initialize Secret Manager client: {str(e)}"
)

@staticmethod
def generate_api_key() -> str:
"""Generate a new API key with prefix."""
random_key = secrets.token_hex(APIKeyService.KEY_LENGTH)
return f"{APIKeyService.SECRET_PREFIX}{random_key}"

@staticmethod
def hash_api_key(api_key: str) -> str:
"""Hash the API key for storage and comparison."""
return hashlib.sha256(api_key.encode()).hexdigest()

@staticmethod
async def create_api_key(user_id: str, db: Session) -> str:
"""Create a new API key for a user."""
api_key = APIKeyService.generate_api_key()
hashed_key = APIKeyService.hash_api_key(api_key)

# Store hashed key in user preferences
user_pref = db.query(UserPreferences).filter(UserPreferences.user_id == user_id).first()
if not user_pref :
user_pref = UserPreferences(user_id=user_id, preferences={})
db.add(user_pref)
if "api_key_hash" not in user_pref.preferences:
pref = user_pref.preferences.copy()
pref["api_key_hash"] = hashed_key
user_pref.preferences = pref
db.commit()
db.refresh(user_pref)
# Store actual key in Secret Manager
if os.getenv("isDevelopmentMode") != "enabled":
client, project_id = APIKeyService.get_client_and_project()
secret_id = f"user-api-key-{user_id}"
parent = f"projects/{project_id}"

try:
# Create secret
secret = {"replication": {"automatic": {}}}
response = client.create_secret(
request={"parent": parent, "secret_id": secret_id, "secret": secret}
)

# Add secret version
version = {"payload": {"data": api_key.encode("UTF-8")}}
client.add_secret_version(
request={"parent": response.name, "payload": version["payload"]}
)
except Exception as e:
# Rollback database changes if secret manager fails
if "api_key_hash" in user_pref.preferences:
del user_pref.preferences["api_key_hash"]
db.commit()
raise HTTPException(status_code=500, detail=f"Failed to store API key: {str(e)}")

return api_key

@staticmethod
async def validate_api_key(api_key: str, db: Session) -> Optional[dict]:
"""Validate an API key and return user info if valid."""
if not api_key.startswith(APIKeyService.SECRET_PREFIX):
return None

hashed_key = APIKeyService.hash_api_key(api_key)

# Find user with matching hashed key using PostgreSQL JSONB operator
user_pref = db.query(UserPreferences).filter(
text("preferences->>'api_key_hash' = :hashed_key")
).params(hashed_key=hashed_key).first()
Comment on lines +102 to +104
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid raw SQL queries when possible for better security and maintainability.

Using SQLAlchemy's ORM features is safer and more maintainable than executing raw SQL strings.

Consider refactoring the query to use SQLAlchemy's filtering capabilities:

-            user_pref = db.query(UserPreferences).filter(
-                text("preferences->>'api_key_hash' = :hashed_key")
-            ).params(hashed_key=hashed_key).first()
+            from sqlalchemy.dialects.postgresql import JSONB
+            user_pref = db.query(UserPreferences).filter(
+                UserPreferences.preferences['api_key_hash'].astext == hashed_key
+            ).first()

Ensure that you have imported the necessary modules:

+from sqlalchemy.dialects.postgresql import JSONB
📝 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.

Suggested change
user_pref = db.query(UserPreferences).filter(
text("preferences->>'api_key_hash' = :hashed_key")
).params(hashed_key=hashed_key).first()
from sqlalchemy.dialects.postgresql import JSONB
user_pref = db.query(UserPreferences).filter(
UserPreferences.preferences['api_key_hash'].astext == hashed_key
).first()
🧰 Tools
🪛 GitHub Actions: Pre-commit

[warning] Trailing whitespace needs to be removed


if not user_pref:
return None

return {
"user_id": user_pref.user_id,
"auth_type": "api_key"
}

@staticmethod
async def revoke_api_key(user_id: str, db: Session) -> bool:
"""Revoke a user's API key."""
user_pref = db.query(UserPreferences).filter(UserPreferences.user_id == user_id).first()
if not user_pref:
return False

if "api_key_hash" in user_pref.preferences:
# Create a new dictionary without the api_key_hash
updated_preferences = user_pref.preferences.copy()
del updated_preferences["api_key_hash"]
user_pref.preferences = updated_preferences
db.commit()

# Delete from Secret Manager if not in dev mode
if os.getenv("isDevelopmentMode") != "enabled":
client, project_id = APIKeyService.get_client_and_project()
secret_id = f"user-api-key-{user_id}"
name = f"projects/{project_id}/secrets/{secret_id}"

try:
client.delete_secret(request={"name": name})
except Exception:
pass # Ignore if secret doesn't exist

return True

@staticmethod
async def get_api_key(user_id: str, db: Session) -> Optional[str]:
"""Retrieve the existing API key for a user."""
user_pref = db.query(UserPreferences).filter(UserPreferences.user_id == user_id).first()
if not user_pref or "api_key_hash" not in user_pref.preferences:
return None

if os.getenv("isDevelopmentMode") == "enabled":
return None # In dev mode, we can't retrieve the actual key for security

client, project_id = APIKeyService.get_client_and_project()
secret_id = f"user-api-key-{user_id}"
name = f"projects/{project_id}/secrets/{secret_id}/versions/latest"

try:
response = client.access_secret_version(request={"name": name})
return response.payload.data.decode("UTF-8")
except Exception as e:
raise HTTPException(
status_code=500,
detail=f"Failed to retrieve API key: {str(e)}"
)
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ async def get_conversation_messages(
raise HTTPException(status_code=500, detail=str(e))

async def post_message(
self, conversation_id: str, message: MessageRequest
self, conversation_id: str, message: MessageRequest, stream: bool = True
) -> AsyncGenerator[str, None]:
try:
async for chunk in self.service.store_message(
conversation_id, message, MessageType.HUMAN, self.user_id
conversation_id, message, MessageType.HUMAN, self.user_id, stream
):
yield chunk
except ConversationNotFoundError as e:
Expand All @@ -96,11 +96,11 @@ async def post_message(
raise HTTPException(status_code=500, detail=str(e))

async def regenerate_last_message(
self, conversation_id: str, node_ids: List[NodeContext] = []
self, conversation_id: str, node_ids: List[NodeContext] = [], stream: bool = True
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix mutable default argument.

Using an empty list [] as a default argument is a Python gotcha as it's mutable and shared between function calls.

-    async def regenerate_last_message(self, conversation_id: str, node_ids: List[NodeContext] = [], stream: bool = True)
+    async def regenerate_last_message(self, conversation_id: str, node_ids: List[NodeContext] | None = None, stream: bool = True)

Then initialize the list inside the function:

async def regenerate_last_message(...):
    if node_ids is None:
        node_ids = []
    # rest of the function
🧰 Tools
🪛 Ruff (0.8.2)

99-99: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)

) -> AsyncGenerator[str, None]:
try:
async for chunk in self.service.regenerate_last_message(
conversation_id, self.user_id, node_ids
conversation_id, self.user_id, node_ids, stream
):
yield chunk
except ConversationNotFoundError as e:
Expand Down
Loading
Loading