-
Notifications
You must be signed in to change notification settings - Fork 58
Role-based authorization layer #356
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ dependencies = [ | |
| "openai==1.99.9", | ||
| "sqlalchemy>=2.0.42", | ||
| "semver<4.0.0", | ||
| "jsonpath-ng>=1.6.1", | ||
| ] | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,19 +5,21 @@ | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| from llama_stack_client import APIConnectionError, NotFoundError | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| from fastapi import APIRouter, HTTPException, status, Depends | ||||||||||||||||||||||||||||||||
| from fastapi import APIRouter, HTTPException, Request, status, Depends | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| from client import AsyncLlamaStackClientHolder | ||||||||||||||||||||||||||||||||
| from configuration import configuration | ||||||||||||||||||||||||||||||||
| from app.database import get_session | ||||||||||||||||||||||||||||||||
| from auth import get_auth_dependency | ||||||||||||||||||||||||||||||||
| from authorization.middleware import authorize | ||||||||||||||||||||||||||||||||
| from models.config import Action | ||||||||||||||||||||||||||||||||
| from models.database.conversations import UserConversation | ||||||||||||||||||||||||||||||||
| from models.responses import ( | ||||||||||||||||||||||||||||||||
| ConversationResponse, | ||||||||||||||||||||||||||||||||
| ConversationDeleteResponse, | ||||||||||||||||||||||||||||||||
| ConversationsListResponse, | ||||||||||||||||||||||||||||||||
| ConversationDetails, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| from models.database.conversations import UserConversation | ||||||||||||||||||||||||||||||||
| from auth import get_auth_dependency | ||||||||||||||||||||||||||||||||
| from app.database import get_session | ||||||||||||||||||||||||||||||||
| from utils.endpoints import check_configuration_loaded, validate_conversation_ownership | ||||||||||||||||||||||||||||||||
| from utils.suid import check_suid | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -146,7 +148,9 @@ def simplify_session_data(session_data: dict) -> list[dict[str, Any]]: | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| @router.get("/conversations", responses=conversations_list_responses) | ||||||||||||||||||||||||||||||||
| def get_conversations_list_endpoint_handler( | ||||||||||||||||||||||||||||||||
| @authorize(Action.LIST_CONVERSATIONS) | ||||||||||||||||||||||||||||||||
| async def get_conversations_list_endpoint_handler( | ||||||||||||||||||||||||||||||||
| request: Request, | ||||||||||||||||||||||||||||||||
| auth: Any = Depends(auth_dependency), | ||||||||||||||||||||||||||||||||
| ) -> ConversationsListResponse: | ||||||||||||||||||||||||||||||||
| """Handle request to retrieve all conversations for the authenticated user.""" | ||||||||||||||||||||||||||||||||
|
|
@@ -158,11 +162,16 @@ def get_conversations_list_endpoint_handler( | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| with get_session() as session: | ||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| # Get all conversations for this user | ||||||||||||||||||||||||||||||||
| user_conversations = ( | ||||||||||||||||||||||||||||||||
| session.query(UserConversation).filter_by(user_id=user_id).all() | ||||||||||||||||||||||||||||||||
| query = session.query(UserConversation) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| filtered_query = ( | ||||||||||||||||||||||||||||||||
| query | ||||||||||||||||||||||||||||||||
| if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||||||||||||||||||||||||||||||||
| else query.filter_by(user_id=user_id) | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| user_conversations = filtered_query.all() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
Comment on lines
+167
to
+174
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. 🛠️ Refactor suggestion Guard against missing request.state.authorized_actions If the decorator/middleware doesn’t populate - filtered_query = (
- query
- if Action.LIST_OTHERS_CONVERSATIONS in request.state.authorized_actions
- else query.filter_by(user_id=user_id)
- )
+ allowed = getattr(request.state, "authorized_actions", set())
+ filtered_query = (
+ query
+ if Action.LIST_OTHERS_CONVERSATIONS in allowed
+ else query.filter_by(user_id=user_id)
+ )📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| # Return conversation summaries with metadata | ||||||||||||||||||||||||||||||||
| conversations = [ | ||||||||||||||||||||||||||||||||
| ConversationDetails( | ||||||||||||||||||||||||||||||||
|
|
@@ -200,7 +209,9 @@ def get_conversations_list_endpoint_handler( | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| @router.get("/conversations/{conversation_id}", responses=conversation_responses) | ||||||||||||||||||||||||||||||||
| @authorize(Action.GET_CONVERSATION) | ||||||||||||||||||||||||||||||||
| async def get_conversation_endpoint_handler( | ||||||||||||||||||||||||||||||||
| request: Request, | ||||||||||||||||||||||||||||||||
| conversation_id: str, | ||||||||||||||||||||||||||||||||
| auth: Any = Depends(auth_dependency), | ||||||||||||||||||||||||||||||||
| ) -> ConversationResponse: | ||||||||||||||||||||||||||||||||
|
|
@@ -239,6 +250,9 @@ async def get_conversation_endpoint_handler( | |||||||||||||||||||||||||||||||
| validate_conversation_ownership( | ||||||||||||||||||||||||||||||||
| user_id=user_id, | ||||||||||||||||||||||||||||||||
| conversation_id=conversation_id, | ||||||||||||||||||||||||||||||||
| others_allowed=( | ||||||||||||||||||||||||||||||||
| Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+253
to
256
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. 🛠️ Refactor suggestion Same defensive default when checking READ_OTHERS_CONVERSATIONS - others_allowed=(
- Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions
- ),
+ others_allowed=(
+ Action.READ_OTHERS_CONVERSATIONS
+ in getattr(request.state, "authorized_actions", set())
+ ),📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| agent_id = conversation_id | ||||||||||||||||||||||||||||||||
|
|
@@ -309,7 +323,9 @@ async def get_conversation_endpoint_handler( | |||||||||||||||||||||||||||||||
| @router.delete( | ||||||||||||||||||||||||||||||||
| "/conversations/{conversation_id}", responses=conversation_delete_responses | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| @authorize(Action.DELETE_CONVERSATION) | ||||||||||||||||||||||||||||||||
| async def delete_conversation_endpoint_handler( | ||||||||||||||||||||||||||||||||
| request: Request, | ||||||||||||||||||||||||||||||||
| conversation_id: str, | ||||||||||||||||||||||||||||||||
| auth: Any = Depends(auth_dependency), | ||||||||||||||||||||||||||||||||
| ) -> ConversationDeleteResponse: | ||||||||||||||||||||||||||||||||
|
|
@@ -342,6 +358,9 @@ async def delete_conversation_endpoint_handler( | |||||||||||||||||||||||||||||||
| validate_conversation_ownership( | ||||||||||||||||||||||||||||||||
| user_id=user_id, | ||||||||||||||||||||||||||||||||
| conversation_id=conversation_id, | ||||||||||||||||||||||||||||||||
| others_allowed=( | ||||||||||||||||||||||||||||||||
| Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions | ||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+361
to
364
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. 🛠️ Refactor suggestion Same defensive default when checking DELETE_OTHERS_CONVERSATIONS - others_allowed=(
- Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions
- ),
+ others_allowed=(
+ Action.DELETE_OTHERS_CONVERSATIONS
+ in getattr(request.state, "authorized_actions", set())
+ ),📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| agent_id = conversation_id | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,19 +5,21 @@ | |
| from pathlib import Path | ||
| import json | ||
| from datetime import datetime, UTC | ||
| from fastapi import APIRouter, Request, HTTPException, Depends, status | ||
| from fastapi import APIRouter, HTTPException, Depends, Request, status | ||
|
|
||
| from auth import get_auth_dependency | ||
| from auth.interface import AuthTuple | ||
| from authorization.middleware import authorize | ||
| from configuration import configuration | ||
| from models.config import Action | ||
| from models.requests import FeedbackRequest | ||
| from models.responses import ( | ||
| ErrorResponse, | ||
| FeedbackResponse, | ||
| StatusResponse, | ||
| UnauthorizedResponse, | ||
| ForbiddenResponse, | ||
| ) | ||
| from models.requests import FeedbackRequest | ||
| from utils.suid import get_suid | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -79,7 +81,8 @@ async def assert_feedback_enabled(_request: Request) -> None: | |
|
|
||
|
|
||
| @router.post("", responses=feedback_response) | ||
| def feedback_endpoint_handler( | ||
| @authorize(Action.FEEDBACK) | ||
| async def feedback_endpoint_handler( | ||
|
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. 👍 |
||
| feedback_request: FeedbackRequest, | ||
| auth: Annotated[AuthTuple, Depends(auth_dependency)], | ||
| _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,24 @@ | ||
| """Handler for REST API call to provide info.""" | ||
|
|
||
| import logging | ||
| from typing import Any | ||
| from typing import Annotated, Any | ||
|
|
||
| from fastapi import APIRouter, Request | ||
| from fastapi import Depends | ||
|
|
||
| from auth.interface import AuthTuple | ||
| from auth import get_auth_dependency | ||
| from authorization.middleware import authorize | ||
| from configuration import configuration | ||
| from version import __version__ | ||
| from models.config import Action | ||
| from models.responses import InfoResponse | ||
| from version import __version__ | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| router = APIRouter(tags=["info"]) | ||
|
|
||
| auth_dependency = get_auth_dependency() | ||
|
|
||
|
|
||
| get_info_responses: dict[int | str, dict[str, Any]] = { | ||
| 200: { | ||
|
|
@@ -22,7 +29,11 @@ | |
|
|
||
|
|
||
| @router.get("/info", responses=get_info_responses) | ||
| def info_endpoint_handler(_request: Request) -> InfoResponse: | ||
| @authorize(Action.INFO) | ||
| async def info_endpoint_handler( | ||
| auth: Annotated[AuthTuple, Depends(auth_dependency)], | ||
| request: Request, | ||
| ) -> InfoResponse: | ||
| """ | ||
| Handle request to the /info endpoint. | ||
|
|
||
|
|
@@ -32,4 +43,10 @@ def info_endpoint_handler(_request: Request) -> InfoResponse: | |
| Returns: | ||
| InfoResponse: An object containing the service's name and version. | ||
| """ | ||
| # Used only for authorization | ||
|
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. dtto
Contributor
Author
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. |
||
| _ = auth | ||
|
|
||
| # Nothing interesting in the request | ||
| _ = request | ||
|
|
||
| return InfoResponse(name=configuration.configuration.name, version=__version__) | ||
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.
why it's used like this? Can't you just rename the parameter to
_authand_request?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.
No, see my commit message remark under "Technical notes"