[I] [Bug]: Dashboard API exception handler returns HTTP 200 for error responses [amoro]

["sivakumar-mahalingam (via GitHub)" <gi...@apache.org>]

sivakumar-mahalingam opened a new issue, #4437:
URL: https://github.com/apache/amoro/issues/4437

   ### What happened?
   
   The dashboard REST API exception handler includes the intended HTTP status in the JSON response body but does not set the actual HTTP response status.
   
   For example, an unauthenticated dashboard API request returns a body similar to:
   
   ```json
   {
     "code": 403,
     "message": "Please login first",
     "requestId": ""
   }
   
   ### Affects Versions
   
   0.9.0-incubating / 0.9.x
   
   ### What table formats are you seeing the problem on?
   
   _No response_
   
   ### What engines are you seeing the problem on?
   
   AMS
   
   ### How to reproduce
   
   1. Start AMS locally.
   
   2. Send a dashboard API request with `X-Request-Source: Web` without an authenticated session:
   
   ```shell
   curl --include \
     --header "X-Request-Source: Web" \
     http://localhost:1630/api/ams/v1/catalogs
   ```
   
   3. Check the HTTP status line and JSON response body.
   
   The response status is:
   
   ```http
   HTTP/1.1 200 OK
   ```
   
   However, the response body reports a forbidden error:
   
   ```json
   {
     "code": 403,
     "message": "Please login first",
     "requestId": ""
   }
   ```
   
   The HTTP status should instead be:
   
   ```http
   HTTP/1.1 403 Forbidden
   ```
   
   The issue can also be reproduced through the existing `TestDashboardAuthentication` integration test by adding an HTTP status assertion to `assertForbidden()`:
   
   ```java
   private static void assertForbidden(
       HttpResponse<String> response, String expectedMessage) throws IOException {
     Assertions.assertEquals(403, response.statusCode());
   
     JsonNode responseBody = OBJECT_MAPPER.readTree(response.body());
     Assertions.assertEquals(403, responseBody.get("code").asInt());
     Assertions.assertEquals(expectedMessage, responseBody.get("message").asText());
   }
   ```
   
   The new `response.statusCode()` assertion fails with the current implementation.
   
   ### Relevant log output
   
   ```shell
   No specific log output is required. The problem is visible in the HTTP response status and JSON body.
   ```
   
   ### Anything else
   
   The issue is in `DashboardServer.handleException()`.
   
   The handler creates an `ErrorResponse` containing the correct error code, but `ctx.json(...)` does not set the HTTP response status.
   
   Current implementation:
   
   ```java
   ctx.json(new ErrorResponse(HttpCode.FORBIDDEN, e.getMessage(), ""));
   ```
   
   Suggested implementation:
   
   ```java
   ctx.status(HttpCode.FORBIDDEN)
       .json(new ErrorResponse(HttpCode.FORBIDDEN, e.getMessage(), ""));
   ```
   
   The same change should be made for the other error branches:
   
   ```java
   public void handleException(Exception e, Context ctx) {
     if (e instanceof ForbiddenException) {
       if (!ctx.req.getRequestURI().startsWith("/api/ams")) {
         ctx.html(getIndexFileContent());
       } else {
         ctx.status(HttpCode.FORBIDDEN)
             .json(new ErrorResponse(HttpCode.FORBIDDEN, e.getMessage(), ""));
       }
     } else if (e instanceof SignatureCheckException) {
       ctx.status(HttpCode.FORBIDDEN)
           .json(new ErrorResponse(HttpCode.FORBIDDEN, "Signature check failed", ""));
     } else {
       ctx.status(HttpCode.INTERNAL_SERVER_ERROR)
           .json(new ErrorResponse(HttpCode.INTERNAL_SERVER_ERROR, e.getMessage(), ""));
     }
   
     LOG.error("An error occurred while processing the url:{}", ctx.url(), e);
   }
   ```
   
   The existing authentication integration tests already verify the error code in the JSON body. They should also verify the actual HTTP response status.
   
   ### Are you willing to submit a PR?
   
   - [x] Yes I am willing to submit a PR!
   
   ### Code of Conduct
   
   - [x] I agree to follow this project's Code of Conduct


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug]: Dashboard API exception handler returns HTTP 200 for error responses [amoro]

["sivakumar-mahalingam (via GitHub)" <gi...@apache.org>]

sivakumar-mahalingam commented on issue #4437:
URL: https://github.com/apache/amoro/issues/4437#issuecomment-6128492906

   take


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Bug]: Dashboard API exception handler returns HTTP 200 for error responses [amoro]

["xxubai (via GitHub)" <gi...@apache.org>]

xxubai closed issue #4437: [Bug]: Dashboard API exception handler returns HTTP 200 for error responses
URL: https://github.com/apache/amoro/issues/4437


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org