[LCORE 756] Before storing feedback, validate that convo exists & belongs to user

src/app/endpoints/feedback.py

@@ -24,7 +24,7 @@
     StatusResponse,
     UnauthorizedResponse,
 )
-from utils.endpoints import check_configuration_loaded
+from utils.endpoints import check_configuration_loaded, retrieve_conversation
 from utils.suid import get_suid
 
 logger = logging.getLogger(__name__)
@@ -112,12 +112,30 @@ 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 and belongs to the user
+    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())
+
+    if conversation.user_id != user_id:
+        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")

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,36 @@ 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 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
+        """
+        {
+            "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

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.",

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"]