From f25bb713fddc91febbbc488e72eb40677f9b7789 Mon Sep 17 00:00:00 2001 From: Andrej Simurka Date: Wed, 15 Oct 2025 09:56:33 +0200 Subject: [PATCH] Refactored status codes, updated tests --- docs/openapi.json | 280 ++++++++++++------ src/app/endpoints/conversations.py | 224 +++++++------- src/models/responses.py | 212 ++++++++++--- src/utils/endpoints.py | 43 +++ tests/e2e/features/conversations.feature | 5 +- tests/e2e/features/query.feature | 4 +- tests/integration/test_openapi_json.py | 2 +- .../unit/app/endpoints/test_conversations.py | 218 +++++++++++++- .../responses/test_unauthorized_response.py | 28 +- 9 files changed, 735 insertions(+), 281 deletions(-) diff --git a/docs/openapi.json b/docs/openapi.json index 4f887ca73..d036fa3f2 100644 --- a/docs/openapi.json +++ b/docs/openapi.json @@ -693,35 +693,17 @@ "operationId": "get_conversations_list_endpoint_handler_v1_conversations_get", "responses": { "200": { - "description": "Successful Response", + "description": "List of conversations retrieved successfully", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ConversationsListResponse" } } - }, - "conversations": [ - { - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "created_at": "2024-01-01T00:00:00Z", - "last_message_at": "2024-01-01T00:05:00Z", - "last_used_model": "gemini/gemini-1.5-flash", - "last_used_provider": "gemini", - "message_count": 5 - }, - { - "conversation_id": "456e7890-e12b-34d5-a678-901234567890", - "created_at": "2024-01-01T01:00:00Z", - "last_message_at": "2024-01-01T01:02:00Z", - "last_used_model": "gemini/gemini-2.0-flash", - "last_used_provider": "gemini", - "message_count": 2 - } - ] + } }, - "400": { - "description": "Missing or invalid credentials provided by client", + "401": { + "description": "Unauthorized: Invalid or missing Bearer token", "content": { "application/json": { "schema": { @@ -730,22 +712,15 @@ } } }, - "401": { - "description": "Unauthorized: Invalid or missing Bearer token", + "503": { + "description": "Service unavailable", "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/UnauthorizedResponse" + "$ref": "#/components/schemas/ServiceUnavailableResponse" } } } - }, - "503": { - "description": "Service Unavailable", - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error." - } } } } @@ -771,38 +746,21 @@ ], "responses": { "200": { - "description": "Successful Response", + "description": "Conversation retrieved successfully", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ConversationResponse" } } - }, - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "chat_history": [ - { - "messages": [ - { - "content": "Hi", - "type": "user" - }, - { - "content": "Hello!", - "type": "assistant" - } - ], - "started_at": "2024-01-01T00:00:00Z", - "completed_at": "2024-01-01T00:00:05Z" - } - ] + } }, "400": { - "description": "Missing or invalid credentials provided by client", + "description": "Invalid request", "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/UnauthorizedResponse" + "$ref": "#/components/schemas/BadRequestResponse" } } } @@ -817,19 +775,35 @@ } } }, + "403": { + "description": "Client does not have permission to access conversation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AccessDeniedResponse" + } + } + } + }, "404": { - "detail": { - "response": "Conversation not found", - "cause": "The specified conversation ID does not exist." - }, - "description": "Not Found" + "description": "Conversation not found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NotFoundResponse" + } + } + } }, "503": { - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error." - }, - "description": "Service Unavailable" + "description": "Service unavailable", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ServiceUnavailableResponse" + } + } + } }, "422": { "description": "Validation Error", @@ -863,24 +837,21 @@ ], "responses": { "200": { - "description": "Successful Response", + "description": "Conversation deleted successfully", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ConversationDeleteResponse" } } - }, - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "success": true, - "message": "Conversation deleted successfully" + } }, "400": { - "description": "Missing or invalid credentials provided by client", + "description": "Invalid request", "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/UnauthorizedResponse" + "$ref": "#/components/schemas/BadRequestResponse" } } } @@ -895,19 +866,35 @@ } } }, + "403": { + "description": "Client does not have permission to access conversation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AccessDeniedResponse" + } + } + } + }, "404": { - "detail": { - "response": "Conversation not found", - "cause": "The specified conversation ID does not exist." - }, - "description": "Not Found" + "description": "Conversation not found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NotFoundResponse" + } + } + } }, "503": { - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error." - }, - "description": "Service Unavailable" + "description": "Service unavailable", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ServiceUnavailableResponse" + } + } + } }, "422": { "description": "Validation Error", @@ -1323,6 +1310,27 @@ }, "components": { "schemas": { + "AccessDeniedResponse": { + "properties": { + "detail": { + "$ref": "#/components/schemas/DetailModel" + } + }, + "type": "object", + "required": [ + "detail" + ], + "title": "AccessDeniedResponse", + "description": "403 Access Denied - User does not have permission to perform the action.", + "examples": [ + { + "detail": { + "cause": "User 6789 does not have permission to access conversation with ID 123e4567-e89b-12d3-a456-426614174000.", + "response": "Access denied" + } + } + ] + }, "AccessRule": { "properties": { "role": { @@ -1609,6 +1617,27 @@ } ] }, + "BadRequestResponse": { + "properties": { + "detail": { + "$ref": "#/components/schemas/DetailModel" + } + }, + "type": "object", + "required": [ + "detail" + ], + "title": "BadRequestResponse", + "description": "400 Bad Request - Invalid resource identifier.", + "examples": [ + { + "detail": { + "cause": "Conversation ID 123e4567-e89b-12d3-a456-426614174000 has invalid format", + "response": "Invalid conversation ID format" + } + } + ] + }, "ByokRag": { "properties": { "rag_id": { @@ -2284,6 +2313,27 @@ "title": "DatabaseConfiguration", "description": "Database configuration." }, + "DetailModel": { + "properties": { + "response": { + "type": "string", + "title": "Response", + "description": "Short summary of the error" + }, + "cause": { + "type": "string", + "title": "Cause", + "description": "Detailed explanation of what caused the error" + } + }, + "type": "object", + "required": [ + "response", + "cause" + ], + "title": "DetailModel", + "description": "Nested detail model for error responses." + }, "ErrorResponse": { "properties": { "detail": { @@ -2527,12 +2577,7 @@ "ForbiddenResponse": { "properties": { "detail": { - "type": "string", - "title": "Detail", - "description": "Details about the authorization issue", - "examples": [ - "Missing or invalid credentials provided by client" - ] + "$ref": "#/components/schemas/DetailModel" } }, "type": "object", @@ -2540,10 +2585,13 @@ "detail" ], "title": "ForbiddenResponse", - "description": "Model representing response for forbidden access.", + "description": "403 Forbidden - User does not have access to this resource.", "examples": [ { - "detail": "Forbidden: User is not authorized to access this resource" + "detail": { + "cause": "User 42 is not allowed to access conversation with ID 123e4567-e89b-12d3-a456-426614174000.", + "response": "Access denied" + } } ] }, @@ -2934,6 +2982,27 @@ "title": "ModelsResponse", "description": "Model representing a response to models request." }, + "NotFoundResponse": { + "properties": { + "detail": { + "$ref": "#/components/schemas/DetailModel" + } + }, + "type": "object", + "required": [ + "detail" + ], + "title": "NotFoundResponse", + "description": "404 Not Found - Resource does not exist.", + "examples": [ + { + "detail": { + "cause": "Conversation with ID 123e4567-e89b-12d3-a456-426614174000 does not exist.", + "response": "Conversation not found" + } + } + ] + }, "PostgreSQLDatabaseConfiguration": { "properties": { "host": { @@ -3695,6 +3764,27 @@ "title": "ServiceConfiguration", "description": "Service configuration." }, + "ServiceUnavailableResponse": { + "properties": { + "detail": { + "$ref": "#/components/schemas/DetailModel" + } + }, + "type": "object", + "required": [ + "detail" + ], + "title": "ServiceUnavailableResponse", + "description": "503 Backend Unavailable - Unable to reach backend service.", + "examples": [ + { + "detail": { + "cause": "Connection error while trying to reach Llama Stack API.", + "response": "Unable to connect to Llama Stack" + } + } + ] + }, "ShieldsResponse": { "properties": { "shields": { @@ -3882,12 +3972,7 @@ "UnauthorizedResponse": { "properties": { "detail": { - "type": "string", - "title": "Detail", - "description": "Details about the authorization issue", - "examples": [ - "Missing or invalid credentials provided by client" - ] + "$ref": "#/components/schemas/DetailModel" } }, "type": "object", @@ -3895,10 +3980,13 @@ "detail" ], "title": "UnauthorizedResponse", - "description": "Model representing response for missing or invalid credentials.", + "description": "401 Unauthorized - Missing or invalid credentials.", "examples": [ { - "detail": "Unauthorized: No auth header found" + "detail": { + "cause": "Missing or invalid credentials provided by client", + "response": "Unauthorized" + } } ] }, diff --git a/src/app/endpoints/conversations.py b/src/app/endpoints/conversations.py index 0d2eece2a..0b894bef3 100644 --- a/src/app/endpoints/conversations.py +++ b/src/app/endpoints/conversations.py @@ -19,11 +19,16 @@ ConversationResponse, ConversationsListResponse, UnauthorizedResponse, + NotFoundResponse, + AccessDeniedResponse, + BadRequestResponse, + ServiceUnavailableResponse, ) from utils.endpoints import ( check_configuration_loaded, delete_conversation, - validate_conversation_ownership, + can_access_conversation, + retrieve_conversation, ) from utils.suid import check_suid @@ -32,102 +37,70 @@ conversation_responses: dict[int | str, dict[str, Any]] = { 200: { - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "chat_history": [ - { - "messages": [ - {"content": "Hi", "type": "user"}, - {"content": "Hello!", "type": "assistant"}, - ], - "started_at": "2024-01-01T00:00:00Z", - "completed_at": "2024-01-01T00:00:05Z", - } - ], + "model": ConversationResponse, + "description": "Conversation retrieved successfully", }, 400: { - "description": "Missing or invalid credentials provided by client", - "model": UnauthorizedResponse, + "model": BadRequestResponse, + "description": "Invalid request", }, 401: { - "description": "Unauthorized: Invalid or missing Bearer token", "model": UnauthorizedResponse, + "description": "Unauthorized: Invalid or missing Bearer token", + }, + 403: { + "model": AccessDeniedResponse, + "description": "Client does not have permission to access conversation", }, 404: { - "detail": { - "response": "Conversation not found", - "cause": "The specified conversation ID does not exist.", - } + "model": NotFoundResponse, + "description": "Conversation not found", }, 503: { - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error.", - } + "model": ServiceUnavailableResponse, + "description": "Service unavailable", }, } conversation_delete_responses: dict[int | str, dict[str, Any]] = { 200: { - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "success": True, - "message": "Conversation deleted successfully", + "model": ConversationDeleteResponse, + "description": "Conversation deleted successfully", }, 400: { - "description": "Missing or invalid credentials provided by client", - "model": UnauthorizedResponse, + "model": BadRequestResponse, + "description": "Invalid request", }, 401: { - "description": "Unauthorized: Invalid or missing Bearer token", "model": UnauthorizedResponse, + "description": "Unauthorized: Invalid or missing Bearer token", + }, + 403: { + "model": AccessDeniedResponse, + "description": "Client does not have permission to access conversation", }, 404: { - "detail": { - "response": "Conversation not found", - "cause": "The specified conversation ID does not exist.", - } + "model": NotFoundResponse, + "description": "Conversation not found", }, 503: { - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error.", - } + "model": ServiceUnavailableResponse, + "description": "Service unavailable", }, } conversations_list_responses: dict[int | str, dict[str, Any]] = { 200: { - "conversations": [ - { - "conversation_id": "123e4567-e89b-12d3-a456-426614174000", - "created_at": "2024-01-01T00:00:00Z", - "last_message_at": "2024-01-01T00:05:00Z", - "last_used_model": "gemini/gemini-1.5-flash", - "last_used_provider": "gemini", - "message_count": 5, - }, - { - "conversation_id": "456e7890-e12b-34d5-a678-901234567890", - "created_at": "2024-01-01T01:00:00Z", - "last_message_at": "2024-01-01T01:02:00Z", - "last_used_model": "gemini/gemini-2.0-flash", - "last_used_provider": "gemini", - "message_count": 2, - }, - ] - }, - 400: { - "description": "Missing or invalid credentials provided by client", - "model": UnauthorizedResponse, + "model": ConversationsListResponse, + "description": "List of conversations retrieved successfully", }, 401: { - "description": "Unauthorized: Invalid or missing Bearer token", "model": UnauthorizedResponse, + "description": "Unauthorized: Invalid or missing Bearer token", }, 503: { - "detail": { - "response": "Unable to connect to Llama Stack", - "cause": "Connection error.", - } + "model": ServiceUnavailableResponse, + "description": "Service unavailable", }, } @@ -267,34 +240,42 @@ async def get_conversation_endpoint_handler( logger.error("Invalid conversation ID format: %s", conversation_id) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "response": "Invalid conversation ID format", - "cause": f"Conversation ID {conversation_id} is not a valid UUID", - }, + detail=BadRequestResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) user_id = auth[0] - - user_conversation = validate_conversation_ownership( - user_id=user_id, - conversation_id=conversation_id, + if not can_access_conversation( + conversation_id, + user_id, others_allowed=( Action.READ_OTHERS_CONVERSATIONS in request.state.authorized_actions ), - ) - - if user_conversation is None: + ): logger.warning( - "User %s attempted to read conversation %s they don't own", + "User %s attempted to read conversation %s they don't have access to", user_id, conversation_id, ) raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, - detail={ - "response": "Access denied", - "cause": "You do not have permission to read this conversation", - }, + detail=AccessDeniedResponse( + user_id=user_id, + resource="conversation", + resource_id=conversation_id, + action="read", + ).dump_detail(), + ) + + # If reached this, user is authorized to retreive this conversation + conversation = retrieve_conversation(conversation_id) + if conversation is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=NotFoundResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) agent_id = conversation_id @@ -308,10 +289,9 @@ async def get_conversation_endpoint_handler( logger.error("No sessions found for conversation %s", conversation_id) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, - detail={ - "response": "Conversation not found", - "cause": f"Conversation {conversation_id} could not be retrieved.", - }, + detail=NotFoundResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) session_id = str(agent_sessions[0].get("session_id")) @@ -334,22 +314,23 @@ async def get_conversation_endpoint_handler( logger.error("Unable to connect to Llama Stack: %s", e) raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, - detail={ - "response": "Unable to connect to Llama Stack", - "cause": str(e), - }, + detail=ServiceUnavailableResponse( + backend_name="Llama Stack", cause=str(e) + ).dump_detail(), ) from e + except NotFoundError as e: logger.error("Conversation not found: %s", e) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, - detail={ - "response": "Conversation not found", - "cause": f"Conversation {conversation_id} could not be retrieved: {str(e)}", - }, + detail=NotFoundResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) from e + except HTTPException: raise + except Exception as e: # Handle case where session doesn't exist or other errors logger.exception("Error retrieving conversation %s: %s", conversation_id, e) @@ -389,34 +370,42 @@ async def delete_conversation_endpoint_handler( logger.error("Invalid conversation ID format: %s", conversation_id) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail={ - "response": "Invalid conversation ID format", - "cause": f"Conversation ID {conversation_id} is not a valid UUID", - }, + detail=BadRequestResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) user_id = auth[0] - - user_conversation = validate_conversation_ownership( - user_id=user_id, - conversation_id=conversation_id, + if not can_access_conversation( + conversation_id, + user_id, others_allowed=( Action.DELETE_OTHERS_CONVERSATIONS in request.state.authorized_actions ), - ) - - if user_conversation is None: + ): logger.warning( - "User %s attempted to delete conversation %s they don't own", + "User %s attempted to delete conversation %s they don't have access to", user_id, conversation_id, ) raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, - detail={ - "response": "Access denied", - "cause": "You do not have permission to delete this conversation", - }, + detail=AccessDeniedResponse( + user_id=user_id, + resource="conversation", + resource_id=conversation_id, + action="delete", + ).dump_detail(), + ) + + # If reached this, user is authorized to retreive this conversation + conversation = retrieve_conversation(conversation_id) + if conversation is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=NotFoundResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) agent_id = conversation_id @@ -452,25 +441,24 @@ async def delete_conversation_endpoint_handler( ) except APIConnectionError as e: - logger.error("Unable to connect to Llama Stack: %s", e) raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, - detail={ - "response": "Unable to connect to Llama Stack", - "cause": str(e), - }, + detail=ServiceUnavailableResponse( + backend_name="Llama Stack", cause=str(e) + ).dump_detail(), ) from e + except NotFoundError as e: - logger.error("Conversation not found: %s", e) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, - detail={ - "response": "Conversation not found", - "cause": f"Conversation {conversation_id} could not be deleted: {str(e)}", - }, + detail=NotFoundResponse( + resource="conversation", resource_id=conversation_id + ).dump_detail(), ) from e + except HTTPException: raise + except Exception as e: # Handle case where session doesn't exist or other errors logger.exception("Error deleting conversation %s: %s", conversation_id, e) diff --git a/src/models/responses.py b/src/models/responses.py index bd2efe16a..fdf324d76 100644 --- a/src/models/responses.py +++ b/src/models/responses.py @@ -1,3 +1,5 @@ +# pylint: disable=too-many-lines + """Models for REST API responses.""" from typing import Any, Optional, Union @@ -595,42 +597,6 @@ class AuthorizedResponse(BaseModel): } -class UnauthorizedResponse(BaseModel): - """Model representing response for missing or invalid credentials.""" - - detail: str = Field( - ..., - description="Details about the authorization issue", - examples=["Missing or invalid credentials provided by client"], - ) - - # provides examples for /docs endpoint - model_config = { - "json_schema_extra": { - "examples": [ - { - "detail": "Unauthorized: No auth header found", - }, - ] - } - } - - -class ForbiddenResponse(UnauthorizedResponse): - """Model representing response for forbidden access.""" - - # provides examples for /docs endpoint - model_config = { - "json_schema_extra": { - "examples": [ - { - "detail": "Forbidden: User is not authorized to access this resource", - }, - ] - } - } - - class ConversationResponse(BaseModel): """Model representing a response for retrieving a conversation. @@ -987,3 +953,177 @@ class ConversationUpdateResponse(BaseModel): description="A message about the update result", examples=["Topic summary updated successfully"], ) + + +class DetailModel(BaseModel): + """Nested detail model for error responses.""" + + response: str = Field(..., description="Short summary of the error") + cause: str = Field(..., description="Detailed explanation of what caused the error") + + +class AbstractErrorResponse(BaseModel): + """Base class for all error responses. + + Contains a nested `detail` field. + """ + + detail: DetailModel + + def dump_detail(self) -> dict: + """Return dict in FastAPI HTTPException format.""" + return self.detail.model_dump() + + +class BadRequestResponse(AbstractErrorResponse): + """400 Bad Request - Invalid resource identifier.""" + + def __init__(self, resource: str, resource_id: str): + """Initialize a BadRequestResponse for invalid resource identifiers.""" + super().__init__( + detail=DetailModel( + response="Invalid conversation ID format", + cause=f"{resource.title()} ID {resource_id} has invalid format", + ) + ) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Invalid conversation ID format", + "cause": "Conversation ID 123e4567-e89b-12d3-a456-426614174000 has invalid format", # pylint: disable=line-too-long + } + } + ] + } + } + + +class AccessDeniedResponse(AbstractErrorResponse): + """403 Access Denied - User does not have permission to perform the action.""" + + def __init__(self, user_id: str, resource: str, resource_id: str, action: str): + """Initialize an AccessDeniedResponse when user lacks permission for an action.""" + super().__init__( + detail=DetailModel( + response="Access denied", + cause=f"User {user_id} does not have permission to {action} {resource} with ID {resource_id}.", # pylint: disable=line-too-long + ) + ) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Access denied", + "cause": "User 6789 does not have permission to access conversation with ID 123e4567-e89b-12d3-a456-426614174000.", # pylint: disable=line-too-long + } + } + ] + } + } + + +class NotFoundResponse(AbstractErrorResponse): + """404 Not Found - Resource does not exist.""" + + def __init__(self, resource: str, resource_id: str): + """Initialize a NotFoundResponse when a resource cannot be located.""" + super().__init__( + detail=DetailModel( + response=f"{resource.title()} not found", + cause=f"{resource.title()} with ID {resource_id} does not exist.", + ) + ) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Conversation not found", + "cause": "Conversation with ID 123e4567-e89b-12d3-a456-426614174000 does not exist.", # pylint: disable=line-too-long + } + } + ] + } + } + + +class ServiceUnavailableResponse(AbstractErrorResponse): + """503 Backend Unavailable - Unable to reach backend service.""" + + def __init__(self, backend_name: str, cause: str): + """Initialize a ServiceUnavailableResponse when a backend service is unreachable.""" + super().__init__( + detail=DetailModel( + response=f"Unable to connect to {backend_name}", cause=cause + ) + ) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Unable to connect to Llama Stack", + "cause": "Connection error while trying to reach Llama Stack API.", + } + } + ] + } + } + + +class UnauthorizedResponse(AbstractErrorResponse): + """401 Unauthorized - Missing or invalid credentials.""" + + def __init__(self, user_id: str | None = None): + """Initialize an UnauthorizedResponse when authentication fails.""" + cause_msg = ( + f"User {user_id} is unauthorized" + if user_id + else "Missing or invalid credentials provided by client" + ) + super().__init__(detail=DetailModel(response="Unauthorized", cause=cause_msg)) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Unauthorized", + "cause": "Missing or invalid credentials provided by client", + } + } + ] + } + } + + +class ForbiddenResponse(UnauthorizedResponse): + """403 Forbidden - User does not have access to this resource.""" + + def __init__(self, user_id: str, resource: str, resource_id: str): + """Initialize a ForbiddenResponse when user is authenticated but lacks resource access.""" + super().__init__(user_id=user_id) + self.detail = DetailModel( + response="Access denied", + cause=f"User {user_id} is not allowed to access {resource} with ID {resource_id}.", + ) + + model_config = { + "json_schema_extra": { + "examples": [ + { + "detail": { + "response": "Access denied", + "cause": "User 42 is not allowed to access conversation with ID 123e4567-e89b-12d3-a456-426614174000.", # pylint: disable=line-too-long + } + } + ] + } + } diff --git a/src/utils/endpoints.py b/src/utils/endpoints.py index 44eed52f8..e9246b714 100644 --- a/src/utils/endpoints.py +++ b/src/utils/endpoints.py @@ -42,6 +42,19 @@ def delete_conversation(conversation_id: str) -> None: ) +def retrieve_conversation(conversation_id: str) -> UserConversation | None: + """Retrieve a conversation from the database by its ID. + + Args: + conversation_id (str): The unique identifier of the conversation to retrieve. + + Returns: + UserConversation | None: The conversation object if found, otherwise None. + """ + with get_session() as session: + return session.query(UserConversation).filter_by(id=conversation_id).first() + + def validate_conversation_ownership( user_id: str, conversation_id: str, others_allowed: bool = False ) -> UserConversation | None: @@ -65,6 +78,36 @@ def validate_conversation_ownership( return conversation +def can_access_conversation( + conversation_id: str, user_id: str, others_allowed: bool +) -> bool: + """Check only whether a user is allowed to access a conversation. + + Args: + conversation_id (str): The ID of the conversation to check. + user_id (str): The ID of the user requesting access. + others_allowed (bool): Whether the user can access conversations owned by others. + + Returns: + bool: True if the user is allowed to access the conversation, False otherwise. + """ + if others_allowed: + return True + + with get_session() as session: + owner_user_id = ( + session.query(UserConversation.user_id) + .filter(UserConversation.id == conversation_id) + .scalar() + ) + # If conversation does not exist, permissions check returns True + if owner_user_id is None: + return True + + # If conversation exists, user_id must match + return owner_user_id == user_id + + def check_configuration_loaded(config: AppConfig) -> None: """ Ensure the application configuration object is present. diff --git a/tests/e2e/features/conversations.feature b/tests/e2e/features/conversations.feature index 8119771f8..391831617 100644 --- a/tests/e2e/features/conversations.feature +++ b/tests/e2e/features/conversations.feature @@ -114,7 +114,7 @@ Feature: conversations endpoint API tests Given The system is in default state And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva When I use REST API conversation endpoint with conversation_id "abcdef" using HTTP GET method - Then The status code of the response is 422 + Then The status code of the response is 400 Scenario: Check if conversations/{conversation_id} GET endpoint fails when llama-stack is unavailable Given The system is in default state @@ -153,10 +153,11 @@ Feature: conversations endpoint API tests Given The system is in default state And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva When I use REST API conversation endpoint with conversation_id "abcdef" using HTTP DELETE method - Then The status code of the response is 422 + Then The status code of the response is 400 Scenario: Check if conversations DELETE endpoint fails when the conversation does not exist Given The system is in default state + And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva When I use REST API conversation endpoint with conversation_id "12345678-abcd-0000-0123-456789abcdef" using HTTP DELETE method Then The status code of the response is 404 diff --git a/tests/e2e/features/query.feature b/tests/e2e/features/query.feature index fb89a04ee..2fe375696 100644 --- a/tests/e2e/features/query.feature +++ b/tests/e2e/features/query.feature @@ -68,8 +68,8 @@ Feature: Query endpoint API tests """ {"conversation_id": "123e4567-e89b-12d3-a456-426614174000", "query": "Write a simple code for reversing string"} """ - Then The status code of the response is 403 - And The body of the response contains User is not authorized to access this resource + Then The status code of the response is 404 + And The body of the response contains Conversation not found Scenario: Check if LLM responds for query request with error for missing query Given The system is in default state diff --git a/tests/integration/test_openapi_json.py b/tests/integration/test_openapi_json.py index 9930b2fd5..166fc00c0 100644 --- a/tests/integration/test_openapi_json.py +++ b/tests/integration/test_openapi_json.py @@ -66,7 +66,7 @@ def test_servers_section_present(spec: dict): ("/v1/feedback", "post", {"200", "401", "403", "500", "422"}), ("/v1/feedback/status", "get", {"200"}), ("/v1/feedback/status", "put", {"200", "422"}), - ("/v1/conversations", "get", {"200", "400", "401", "503"}), + ("/v1/conversations", "get", {"200", "401", "503"}), ( "/v1/conversations/{conversation_id}", "get", diff --git a/tests/unit/app/endpoints/test_conversations.py b/tests/unit/app/endpoints/test_conversations.py index 89548782a..42cfc7d7d 100644 --- a/tests/unit/app/endpoints/test_conversations.py +++ b/tests/unit/app/endpoints/test_conversations.py @@ -1,4 +1,4 @@ -# pylint: disable=redefined-outer-name +# pylint: disable=redefined-outer-name, too-many-lines """Unit tests for the /conversations REST API endpoints.""" @@ -13,6 +13,7 @@ simplify_session_data, ) from models.config import Action +from models.database.conversations import UserConversation from models.responses import ( ConversationResponse, ConversationDeleteResponse, @@ -178,6 +179,19 @@ def expected_chat_history_fixture(): ] +@pytest.fixture(name="mock_conversation") +def mock_conversation_fixture(): + """Create a mock UserConversation object for testing.""" + mock_conv = UserConversation() + mock_conv.id = VALID_CONVERSATION_ID + mock_conv.user_id = "another_user" # Different from test auth + mock_conv.message_count = 2 + mock_conv.last_used_model = "mock-model" + mock_conv.last_used_provider = "mock-provider" + mock_conv.topic_summary = "Mock topic" + return mock_conv + + class TestSimplifySessionData: """Test cases for the simplify_session_data function.""" @@ -295,7 +309,8 @@ async def test_llama_stack_connection_error( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise APIConnectionError mock_client = mocker.AsyncMock() @@ -324,7 +339,8 @@ async def test_llama_stack_not_found_error( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise NotFoundError mock_client = mocker.AsyncMock() @@ -345,7 +361,7 @@ async def test_llama_stack_not_found_error( assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND assert "Conversation not found" in exc_info.value.detail["response"] - assert "could not be retrieved" in exc_info.value.detail["cause"] + assert "does not exist" in exc_info.value.detail["cause"] assert VALID_CONVERSATION_ID in exc_info.value.detail["cause"] @pytest.mark.asyncio @@ -356,7 +372,8 @@ async def test_session_retrieve_exception( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise a general exception mock_client = mocker.AsyncMock() @@ -379,6 +396,93 @@ async def test_session_retrieve_exception( "Unknown error while getting conversation" in exc_info.value.detail["cause"] ) + @pytest.mark.asyncio + async def test_get_conversation_forbidden( + self, mocker, setup_configuration, dummy_request, mock_conversation + ): + """Test forbidden access when user lacks permission to read conversation.""" + mocker.patch("app.endpoints.conversations.configuration", setup_configuration) + mocker.patch("app.endpoints.conversations.check_suid", return_value=True) + mocker.patch( + "app.endpoints.conversations.retrieve_conversation", + return_value=mock_conversation, + ) + mocker.patch( + "authorization.resolvers.NoopAccessResolver.get_actions", + return_value=set(Action.GET_CONVERSATION), + ) # Reduce user's permissions to access only their conversations + + mock_row = mocker.Mock() + mock_row.user_id = "different_user_id" + + # Mock the SQLAlchemy-like session + mock_session = mocker.MagicMock() + mock_session.query.return_value.filter.return_value.first.return_value = ( + mock_row + ) + + mock_session.__enter__.return_value = mock_session + mock_session.__exit__.return_value = None + + mocker.patch("utils.endpoints.get_session", return_value=mock_session) + + with pytest.raises(HTTPException) as exc_info: + await get_conversation_endpoint_handler( + request=dummy_request, + conversation_id=VALID_CONVERSATION_ID, + auth=MOCK_AUTH, + ) + + assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN + expected = ( + f"User {MOCK_AUTH[0]} does not have permission " + f"to read conversation with ID {VALID_CONVERSATION_ID}." + ) + assert expected in exc_info.value.detail["cause"] + + @pytest.mark.asyncio + async def test_get_others_conversations_allowed_for_authorized_user( + self, + mocker, + setup_configuration, + mock_conversation, + dummy_request, + mock_session_data, + ): # pylint: disable=too-many-arguments, too-many-positional-arguments + """Test allowed access to another user's conversation for authorized user.""" + mocker.patch( + "authorization.resolvers.NoopAccessResolver.get_actions", + return_value={Action.GET_CONVERSATION, Action.READ_OTHERS_CONVERSATIONS}, + ) # Allow user to access other users' conversations + mocker.patch("app.endpoints.conversations.configuration", setup_configuration) + mocker.patch("app.endpoints.conversations.check_suid", return_value=True) + mocker.patch( + "app.endpoints.conversations.retrieve_conversation", + return_value=mock_conversation, + ) + + mock_client = mocker.AsyncMock() + mock_client.agents.session.list.return_value = mocker.Mock( + data=[mock_session_data] + ) + + mock_session_retrieve_result = mocker.Mock() + mock_session_retrieve_result.model_dump.return_value = mock_session_data + mock_client.agents.session.retrieve.return_value = mock_session_retrieve_result + + mock_client_holder = mocker.patch( + "app.endpoints.conversations.AsyncLlamaStackClientHolder" + ) + mock_client_holder.return_value.get_client.return_value = mock_client + response = await get_conversation_endpoint_handler( + request=dummy_request, + conversation_id=VALID_CONVERSATION_ID, + auth=MOCK_AUTH, + ) + + assert response.conversation_id == VALID_CONVERSATION_ID + assert hasattr(response, "chat_history") + @pytest.mark.asyncio async def test_successful_conversation_retrieval( self, @@ -392,7 +496,8 @@ async def test_successful_conversation_retrieval( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder mock_client = mocker.AsyncMock() @@ -469,7 +574,8 @@ async def test_llama_stack_connection_error( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise APIConnectionError mock_client = mocker.AsyncMock() @@ -497,7 +603,8 @@ async def test_llama_stack_not_found_error( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise NotFoundError mock_client = mocker.AsyncMock() @@ -518,7 +625,7 @@ async def test_llama_stack_not_found_error( assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND assert "Conversation not found" in exc_info.value.detail["response"] - assert "could not be deleted" in exc_info.value.detail["cause"] + assert "does not exist" in exc_info.value.detail["cause"] assert VALID_CONVERSATION_ID in exc_info.value.detail["cause"] @pytest.mark.asyncio @@ -529,7 +636,8 @@ async def test_session_deletion_exception( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock AsyncLlamaStackClientHolder to raise a general exception mock_client = mocker.AsyncMock() @@ -555,6 +663,93 @@ async def test_session_deletion_exception( in exc_info.value.detail["cause"] ) + @pytest.mark.asyncio + async def test_delete_conversation_forbidden( + self, mocker, setup_configuration, dummy_request, mock_conversation + ): + """Test forbidden deletion when user lacks permission to delete conversation.""" + mocker.patch("app.endpoints.conversations.configuration", setup_configuration) + mocker.patch("app.endpoints.conversations.check_suid", return_value=True) + mocker.patch( + "app.endpoints.conversations.retrieve_conversation", + return_value=mock_conversation, + ) + mocker.patch( + "authorization.resolvers.NoopAccessResolver.get_actions", + return_value=set(Action.DELETE_CONVERSATION), + ) # Reduce user's permissions to delete only their conversations + + mock_row = mocker.Mock() + mock_row.user_id = "different_user_id" + + # Mock the SQLAlchemy-like session + mock_session = mocker.MagicMock() + mock_session.query.return_value.filter.return_value.first.return_value = ( + mock_row + ) + + mock_session.__enter__.return_value = mock_session + mock_session.__exit__.return_value = None + + mocker.patch("utils.endpoints.get_session", return_value=mock_session) + + with pytest.raises(HTTPException) as exc_info: + await delete_conversation_endpoint_handler( + request=dummy_request, + conversation_id=VALID_CONVERSATION_ID, + auth=MOCK_AUTH, + ) + + assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN + expected = ( + f"User {MOCK_AUTH[0]} does not have permission " + f"to delete conversation with ID {VALID_CONVERSATION_ID}." + ) + assert expected in exc_info.value.detail["cause"] + + @pytest.mark.asyncio + async def test_delete_others_conversations_allowed_for_authorized_user( + self, mocker, setup_configuration, mock_conversation, dummy_request + ): + """Test allowed deletion of another user's conversation for authorized user.""" + mocker.patch( + "authorization.resolvers.NoopAccessResolver.get_actions", + return_value={ + Action.DELETE_OTHERS_CONVERSATIONS, + Action.DELETE_CONVERSATION, + }, + ) # Allow user to detele other users' conversations + + mocker.patch("app.endpoints.conversations.configuration", setup_configuration) + mocker.patch("app.endpoints.conversations.check_suid", return_value=True) + mocker.patch( + "app.endpoints.conversations.retrieve_conversation", + return_value=mock_conversation, + ) + + mock_client = mocker.AsyncMock() + mock_client.agents.session.list.return_value.data = [ + {"session_id": VALID_CONVERSATION_ID} + ] + mock_client.agents.session.delete.return_value = None + mocker.patch( + "app.endpoints.conversations.AsyncLlamaStackClientHolder.get_client", + return_value=mock_client, + ) + + mocker.patch( + "app.endpoints.conversations.delete_conversation", return_value=None + ) + response = await delete_conversation_endpoint_handler( + request=dummy_request, + conversation_id=VALID_CONVERSATION_ID, + auth=MOCK_AUTH, + ) + + assert response.success is True + assert response.conversation_id == VALID_CONVERSATION_ID + assert "deleted successfully" in response.response + @pytest.mark.asyncio async def test_successful_conversation_deletion( self, mocker, setup_configuration, dummy_request @@ -563,7 +758,8 @@ async def test_successful_conversation_deletion( mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.conversations.configuration", setup_configuration) mocker.patch("app.endpoints.conversations.check_suid", return_value=True) - mocker.patch("app.endpoints.conversations.validate_conversation_ownership") + mocker.patch("app.endpoints.conversations.can_access_conversation") + mocker.patch("app.endpoints.conversations.retrieve_conversation") # Mock the delete_conversation function mocker.patch("app.endpoints.conversations.delete_conversation") diff --git a/tests/unit/models/responses/test_unauthorized_response.py b/tests/unit/models/responses/test_unauthorized_response.py index 242d68bf7..278f46ca8 100644 --- a/tests/unit/models/responses/test_unauthorized_response.py +++ b/tests/unit/models/responses/test_unauthorized_response.py @@ -1,23 +1,21 @@ """Unit tests for UnauthorizedResponse model.""" -import pytest - -from pydantic import ValidationError - -from models.responses import UnauthorizedResponse +from models.responses import UnauthorizedResponse, DetailModel class TestUnauthorizedResponse: """Test cases for the UnauthorizedResponse model.""" - def test_constructor(self) -> None: - """Test the UnauthorizedResponse constructor.""" - ur = UnauthorizedResponse( - detail="Missing or invalid credentials provided by client" - ) - assert ur.detail == "Missing or invalid credentials provided by client" + def test_constructor_without_user_id(self) -> None: + """Test UnauthorizedResponse when user_id is not provided.""" + ur = UnauthorizedResponse() + assert isinstance(ur.detail, DetailModel) + assert ur.detail.response == "Unauthorized" + assert ur.detail.cause == "Missing or invalid credentials provided by client" - def test_constructor_fields_required(self) -> None: - """Test the UnauthorizedResponse constructor.""" - with pytest.raises(ValidationError): - _ = UnauthorizedResponse() # pyright: ignore + def test_constructor_with_user_id(self) -> None: + """Test UnauthorizedResponse when user_id is provided.""" + ur = UnauthorizedResponse(user_id="user_123") + assert isinstance(ur.detail, DetailModel) + assert ur.detail.response == "Unauthorized" + assert ur.detail.cause == "User user_123 is unauthorized"