From 719977e952cb4a133d1b7a64ce10636324b26372 Mon Sep 17 00:00:00 2001 From: Maxim Svistunov Date: Thu, 15 Jan 2026 15:44:13 +0100 Subject: [PATCH 1/3] Before storing feedback, validate that convo exists & belongs to user --- src/app/endpoints/feedback.py | 26 ++++++++++++++- tests/e2e/features/feedback.feature | 29 +++++++++++++---- tests/e2e/features/steps/feedback.py | 47 ++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/app/endpoints/feedback.py b/src/app/endpoints/feedback.py index e411698ad..174818d84 100644 --- a/src/app/endpoints/feedback.py +++ b/src/app/endpoints/feedback.py @@ -24,7 +24,11 @@ StatusResponse, UnauthorizedResponse, ) -from utils.endpoints import check_configuration_loaded +from utils.endpoints import ( + check_configuration_loaded, + retrieve_conversation, + validate_conversation_ownership, +) from utils.suid import get_suid logger = logging.getLogger(__name__) @@ -112,12 +116,32 @@ async def feedback_endpoint_handler( Response indicating the status of the feedback storage request. Raises: + HTTPException: Returns HTTP 404 if conversation does not exist. + HTTPException: Returns HTTP 403 if conversation belongs to a different user. HTTPException: Returns HTTP 500 if feedback storage fails. """ logger.debug("Feedback received %s", str(feedback_request)) user_id, _, _, _ = auth check_configuration_loaded(configuration) + + # Validate conversation exists + conversation_id = feedback_request.conversation_id + conversation = retrieve_conversation(conversation_id) + if conversation is None: + response = NotFoundResponse( + resource="conversation", resource_id=conversation_id + ) + raise HTTPException(**response.model_dump()) + + # Validate conversation belongs to the user + owned_conversation = validate_conversation_ownership(user_id, conversation_id) + if owned_conversation is None: + response = ForbiddenResponse.conversation( + action="submit feedback for", resource_id=conversation_id, user_id=user_id + ) + raise HTTPException(**response.model_dump()) + store_feedback(user_id, feedback_request.model_dump(exclude={"model_config"})) return FeedbackResponse(response="feedback received") diff --git a/tests/e2e/features/feedback.feature b/tests/e2e/features/feedback.feature index 3c1a13541..93b7bda77 100644 --- a/tests/e2e/features/feedback.feature +++ b/tests/e2e/features/feedback.feature @@ -224,10 +224,8 @@ Feature: feedback endpoint API tests } """ - @skip - Scenario: Check if feedback submittion fails when nonexisting conversation ID is passed + Scenario: Check if feedback submission fails when nonexisting conversation ID is passed Given The system is in default state - And A new conversation is initialized And The feedback is enabled When I submit the following feedback for nonexisting conversation "12345678-abcd-0000-0123-456789abcdef" """ @@ -238,14 +236,33 @@ Feature: feedback endpoint API tests "user_question": "Sample Question" } """ - Then The status code of the response is 422 + Then The status code of the response is 404 And The body of the response is the following """ { - "response": "User has no access to this conversation" + "detail": { + "response": "Conversation not found", + "cause": "Conversation with ID 12345678-abcd-0000-0123-456789abcdef does not exist" + } } """ - + + Scenario: Check if feedback submission fails when conversation belongs to a different user + Given The system is in default state + And A conversation owned by a different user is initialized + And The feedback is enabled + When I submit the following feedback for the conversation created before + """ + { + "llm_response": "Sample Response", + "sentiment": -1, + "user_feedback": "Not satisfied with the response quality", + "user_question": "Sample Question" + } + """ + Then The status code of the response is 403 + And The body of the response contains User does not have permission to perform this action + Scenario: Check if feedback endpoint is not working when not authorized Given The system is in default state And A new conversation is initialized diff --git a/tests/e2e/features/steps/feedback.py b/tests/e2e/features/steps/feedback.py index ce2d968cb..95639be12 100644 --- a/tests/e2e/features/steps/feedback.py +++ b/tests/e2e/features/steps/feedback.py @@ -125,6 +125,53 @@ def initialize_conversation(context: Context) -> None: context.response = response +@given("A conversation owned by a different user is initialized") # type: ignore +def initialize_conversation_different_user(context: Context) -> None: + """Create a conversation owned by a different user for testing access control. + + This step temporarily switches to a different auth token to create a conversation + that will be owned by a different user, then restores the original auth header. + """ + # Save original auth headers + original_auth_headers = ( + context.auth_headers.copy() if hasattr(context, "auth_headers") else {} + ) + + # Set a different auth token (different user_id in the sub claim) + # This token has sub: "different_user_id" instead of "1234567890" + different_user_token = ( + "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9." + "eyJzdWIiOiJkaWZmZXJlbnRfdXNlcl9pZCIsIm5hbWUiOiJPdGhlclVzZXIifQ." + "placeholder_signature" + ) + context.auth_headers = {"Authorization": different_user_token} + + # Create a conversation as the different user + endpoint = "query" + base = f"http://{context.hostname}:{context.port}" + path = f"{context.api_prefix}/{endpoint}".replace("//", "/") + url = base + path + payload = { + "query": "Say Hello.", + "system_prompt": "You are a helpful assistant", + "model": context.default_model, + "provider": context.default_provider, + } + + response = requests.post(url, headers=context.auth_headers, json=payload) + assert ( + response.status_code == 200 + ), f"Failed to create conversation as different user: {response.text}" + + body = response.json() + context.conversation_id = body["conversation_id"] + assert context.conversation_id, "Conversation was not created." + context.feedback_conversations.append(context.conversation_id) + + # Restore original auth headers + context.auth_headers = original_auth_headers + + @given("An invalid feedback storage path is configured") # type: ignore def configure_invalid_feedback_storage_path(context: Context) -> None: """Set an invalid feedback storage path and restart the container.""" From e1b0b50729e820c280d07dbf25a3b3685d8ad134 Mon Sep 17 00:00:00 2001 From: Maxim Svistunov Date: Thu, 15 Jan 2026 16:02:50 +0100 Subject: [PATCH 2/3] Fix unit tests, apply reviewer feedback --- src/app/endpoints/feedback.py | 12 +--- tests/e2e/features/feedback.feature | 6 +- tests/e2e/features/steps/feedback.py | 47 -------------- tests/unit/app/endpoints/test_feedback.py | 78 +++++++++++++++++++++++ 4 files changed, 86 insertions(+), 57 deletions(-) diff --git a/src/app/endpoints/feedback.py b/src/app/endpoints/feedback.py index 174818d84..1127e52bd 100644 --- a/src/app/endpoints/feedback.py +++ b/src/app/endpoints/feedback.py @@ -24,11 +24,7 @@ StatusResponse, UnauthorizedResponse, ) -from utils.endpoints import ( - check_configuration_loaded, - retrieve_conversation, - validate_conversation_ownership, -) +from utils.endpoints import check_configuration_loaded, retrieve_conversation from utils.suid import get_suid logger = logging.getLogger(__name__) @@ -125,7 +121,7 @@ async def feedback_endpoint_handler( user_id, _, _, _ = auth check_configuration_loaded(configuration) - # Validate conversation exists + # Validate conversation exists and belongs to the user conversation_id = feedback_request.conversation_id conversation = retrieve_conversation(conversation_id) if conversation is None: @@ -134,9 +130,7 @@ async def feedback_endpoint_handler( ) raise HTTPException(**response.model_dump()) - # Validate conversation belongs to the user - owned_conversation = validate_conversation_ownership(user_id, conversation_id) - if owned_conversation is None: + if conversation.user_id != user_id: response = ForbiddenResponse.conversation( action="submit feedback for", resource_id=conversation_id, user_id=user_id ) diff --git a/tests/e2e/features/feedback.feature b/tests/e2e/features/feedback.feature index 93b7bda77..dea58e8f3 100644 --- a/tests/e2e/features/feedback.feature +++ b/tests/e2e/features/feedback.feature @@ -249,7 +249,11 @@ Feature: feedback endpoint API tests Scenario: Check if feedback submission fails when conversation belongs to a different user Given The system is in default state - And A conversation owned by a different user is initialized + # Create a conversation as a different user (sub: "different_user_id") + And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJkaWZmZXJlbnRfdXNlcl9pZCIsIm5hbWUiOiJPdGhlclVzZXIifQ.placeholder + And A new conversation is initialized + # Switch back to original user (sub: "1234567890") + And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva And The feedback is enabled When I submit the following feedback for the conversation created before """ diff --git a/tests/e2e/features/steps/feedback.py b/tests/e2e/features/steps/feedback.py index 95639be12..ce2d968cb 100644 --- a/tests/e2e/features/steps/feedback.py +++ b/tests/e2e/features/steps/feedback.py @@ -125,53 +125,6 @@ def initialize_conversation(context: Context) -> None: context.response = response -@given("A conversation owned by a different user is initialized") # type: ignore -def initialize_conversation_different_user(context: Context) -> None: - """Create a conversation owned by a different user for testing access control. - - This step temporarily switches to a different auth token to create a conversation - that will be owned by a different user, then restores the original auth header. - """ - # Save original auth headers - original_auth_headers = ( - context.auth_headers.copy() if hasattr(context, "auth_headers") else {} - ) - - # Set a different auth token (different user_id in the sub claim) - # This token has sub: "different_user_id" instead of "1234567890" - different_user_token = ( - "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9." - "eyJzdWIiOiJkaWZmZXJlbnRfdXNlcl9pZCIsIm5hbWUiOiJPdGhlclVzZXIifQ." - "placeholder_signature" - ) - context.auth_headers = {"Authorization": different_user_token} - - # Create a conversation as the different user - endpoint = "query" - base = f"http://{context.hostname}:{context.port}" - path = f"{context.api_prefix}/{endpoint}".replace("//", "/") - url = base + path - payload = { - "query": "Say Hello.", - "system_prompt": "You are a helpful assistant", - "model": context.default_model, - "provider": context.default_provider, - } - - response = requests.post(url, headers=context.auth_headers, json=payload) - assert ( - response.status_code == 200 - ), f"Failed to create conversation as different user: {response.text}" - - body = response.json() - context.conversation_id = body["conversation_id"] - assert context.conversation_id, "Conversation was not created." - context.feedback_conversations.append(context.conversation_id) - - # Restore original auth headers - context.auth_headers = original_auth_headers - - @given("An invalid feedback storage path is configured") # type: ignore def configure_invalid_feedback_storage_path(context: Context) -> None: """Set an invalid feedback storage path and restart the container.""" diff --git a/tests/unit/app/endpoints/test_feedback.py b/tests/unit/app/endpoints/test_feedback.py index 9f1b0cad7..10d05e96e 100644 --- a/tests/unit/app/endpoints/test_feedback.py +++ b/tests/unit/app/endpoints/test_feedback.py @@ -102,9 +102,17 @@ async def test_feedback_endpoint_handler( mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) mocker.patch("app.endpoints.feedback.store_feedback", return_value=None) + # Mock retrieve_conversation to return a conversation owned by test_user_id + mock_conversation = mocker.Mock() + mock_conversation.user_id = "test_user_id" + mocker.patch( + "app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation + ) + # Prepare the feedback request mock feedback_request = mocker.Mock() feedback_request.model_dump.return_value = feedback_request_data + feedback_request.conversation_id = "12345678-abcd-0000-0123-456789abcdef" # Authorization tuple required by URL endpoint handler auth: AuthTuple = ("test_user_id", "test_user", True, "test_token") @@ -126,6 +134,14 @@ async def test_feedback_endpoint_handler_error(mocker: MockerFixture) -> None: mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) mocker.patch("app.endpoints.feedback.check_configuration_loaded", return_value=None) + + # Mock retrieve_conversation to return a conversation owned by test_user_id + mock_conversation = mocker.Mock() + mock_conversation.user_id = "test_user_id" + mocker.patch( + "app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation + ) + # Mock Path.mkdir to raise OSError so the try block in store_feedback catches it mocker.patch( "app.endpoints.feedback.Path.mkdir", side_effect=OSError("Permission denied") @@ -300,6 +316,14 @@ async def test_feedback_endpoint_valid_requests( """Test endpoint with valid feedback payloads.""" mock_authorization_resolvers(mocker) mocker.patch("app.endpoints.feedback.store_feedback") + + # Mock retrieve_conversation to return a conversation owned by mock_user_id + mock_conversation = mocker.Mock() + mock_conversation.user_id = "mock_user_id" + mocker.patch( + "app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation + ) + request = FeedbackRequest(**{**VALID_BASE, **payload}) response = await feedback_endpoint_handler( feedback_request=request, @@ -337,3 +361,57 @@ def test_feedback_status_disabled(mocker: MockerFixture) -> None: assert response.functionality == "feedback" assert response.status == {"enabled": False} + + +@pytest.mark.asyncio +async def test_feedback_endpoint_handler_conversation_not_found( + mocker: MockerFixture, +) -> None: + """Test that feedback_endpoint_handler returns 404 when conversation doesn't exist.""" + mock_authorization_resolvers(mocker) + mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) + mocker.patch("app.endpoints.feedback.retrieve_conversation", return_value=None) + + feedback_request = FeedbackRequest(**{**VALID_BASE, "sentiment": 1}) + auth: AuthTuple = ("test_user_id", "test_user", True, "test_token") + + with pytest.raises(HTTPException) as exc_info: + await feedback_endpoint_handler( + feedback_request=feedback_request, + _ensure_feedback_enabled=assert_feedback_enabled, + auth=auth, + ) + assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND + detail = exc_info.value.detail + assert isinstance(detail, dict) + assert detail["response"] == "Conversation not found" + + +@pytest.mark.asyncio +async def test_feedback_endpoint_handler_conversation_wrong_owner( + mocker: MockerFixture, +) -> None: + """Test feedback_endpoint_handler returns 403 for conversation owned by different user.""" + mock_authorization_resolvers(mocker) + mocker.patch("app.endpoints.feedback.assert_feedback_enabled", return_value=None) + + # Mock retrieve_conversation to return a conversation owned by a different user + mock_conversation = mocker.Mock() + mock_conversation.user_id = "different_user_id" + mocker.patch( + "app.endpoints.feedback.retrieve_conversation", return_value=mock_conversation + ) + + feedback_request = FeedbackRequest(**{**VALID_BASE, "sentiment": 1}) + auth: AuthTuple = ("test_user_id", "test_user", True, "test_token") + + with pytest.raises(HTTPException) as exc_info: + await feedback_endpoint_handler( + feedback_request=feedback_request, + _ensure_feedback_enabled=assert_feedback_enabled, + auth=auth, + ) + assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN + detail = exc_info.value.detail + assert isinstance(detail, dict) + assert "does not have permission" in detail["response"] From af1ca2a56c8e93d17c191f15f55a3612accff4e1 Mon Sep 17 00:00:00 2001 From: Maxim Svistunov Date: Fri, 16 Jan 2026 09:47:59 +0100 Subject: [PATCH 3/3] Fix e2e test for conversation ownership validation --- tests/e2e/features/feedback.feature | 7 +++---- tests/e2e/features/steps/feedback.py | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/e2e/features/feedback.feature b/tests/e2e/features/feedback.feature index dea58e8f3..5c66f0bb6 100644 --- a/tests/e2e/features/feedback.feature +++ b/tests/e2e/features/feedback.feature @@ -249,11 +249,10 @@ Feature: feedback endpoint API tests Scenario: Check if feedback submission fails when conversation belongs to a different user Given The system is in default state - # Create a conversation as a different user (sub: "different_user_id") - And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJkaWZmZXJlbnRfdXNlcl9pZCIsIm5hbWUiOiJPdGhlclVzZXIifQ.placeholder - And A new conversation is initialized - # Switch back to original user (sub: "1234567890") And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva + # Create a conversation as a different user (via user_id query param for noop_with_token) + And A new conversation is initialized with user_id "different_user_id" + # Feedback submission will use the default user from the auth header And The feedback is enabled When I submit the following feedback for the conversation created before """ diff --git a/tests/e2e/features/steps/feedback.py b/tests/e2e/features/steps/feedback.py index ce2d968cb..1913431c7 100644 --- a/tests/e2e/features/steps/feedback.py +++ b/tests/e2e/features/steps/feedback.py @@ -101,10 +101,25 @@ def access_feedback_get_endpoint(context: Context) -> None: @given("A new conversation is initialized") # type: ignore def initialize_conversation(context: Context) -> None: """Create a conversation for submitting feedback.""" + create_conversation_with_user_id(context, user_id=None) + + +@given('A new conversation is initialized with user_id "{user_id}"') # type: ignore +def initialize_conversation_with_user_id(context: Context, user_id: str) -> None: + """Create a conversation for submitting feedback with a specific user_id.""" + create_conversation_with_user_id(context, user_id=user_id) + + +def create_conversation_with_user_id( + context: Context, user_id: Optional[str] = None +) -> None: + """Create a conversation, optionally with a specific user_id query parameter.""" endpoint = "query" base = f"http://{context.hostname}:{context.port}" path = f"{context.api_prefix}/{endpoint}".replace("//", "/") url = base + path + if user_id is not None: + url = f"{url}?user_id={user_id}" headers = context.auth_headers if hasattr(context, "auth_headers") else {} payload = { "query": "Say Hello.",