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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
