This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new f7489c7195 [#11581] fix: Return JSON ErrorResponse for 401/403 auth
failures in AuthenticationFilter (#11614)
f7489c7195 is described below
commit f7489c71953a96531a35ca167f0a488dbda556a2
Author: Akshay Thorat <[email protected]>
AuthorDate: Thu Jun 11 23:11:48 2026 -0700
[#11581] fix: Return JSON ErrorResponse for 401/403 auth failures in
AuthenticationFilter (#11614)
### What changes were proposed in this pull request?
`AuthenticationFilter.sendAuthErrorResponse()` now writes a Gravitino
`ErrorResponse` JSON body with `application/json` content type instead
of calling `response.sendError()`, which triggered Jetty's default HTML
error page.
Changes:
- **ErrorConstants**: Added `UNAUTHORIZED_CODE = 1011`
- **ErrorResponse**: Added `unauthorized(type, message, throwable)`
factory method
- **AuthenticationFilter**: Rewrote `sendAuthErrorResponse()` to produce
JSON for `UnauthorizedException` (401), `ForbiddenException` (403), and
generic exceptions (500)
- **TestAuthenticationFilter**: Updated existing tests to verify JSON
output; added 3 new tests for 401, 403, and 500 JSON responses
### Why are the changes needed?
`AuthenticationFilter` rejected unauthenticated requests with
`response.sendError(status, message)`, which triggers Jetty's default
HTML error page. REST clients expect a JSON error response body, causing
them to fail parsing 401/403 responses and surface opaque errors.
Fix: #11581
### Does this PR introduce _any_ user-facing change?
Yes. 401/403/500 error responses from the `AuthenticationFilter` now
return a JSON body with `Content-Type: application/json` instead of
HTML.
### How was this patch tested?
- Updated existing unit tests to assert JSON body, content type, and
status code
- Added `testUnauthorizedErrorReturnsJsonBody`,
`testForbiddenErrorReturnsJsonBody`, and
`testInternalServerErrorReturnsJsonBody`
- Verified `IcebergAuthenticationFilter` tests still pass (subclass
overrides `sendAuthErrorResponse` independently)
---
.../gravitino/dto/responses/ErrorConstants.java | 3 +
.../gravitino/dto/responses/ErrorResponse.java | 13 +++
.../authentication/AuthenticationFilter.java | 36 +++++--
.../authentication/TestAuthenticationFilter.java | 115 ++++++++++++++++++++-
4 files changed, 154 insertions(+), 13 deletions(-)
diff --git
a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorConstants.java
b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorConstants.java
index 4953463d74..870addfba9 100644
---
a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorConstants.java
+++
b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorConstants.java
@@ -54,6 +54,9 @@ public class ErrorConstants {
/** Error codes for drop an in use entity. */
public static final int IN_USE_CODE = 1010;
+ /** Error codes for unauthorized access. */
+ public static final int UNAUTHORIZED_CODE = 1011;
+
/** Error codes for invalid state. */
public static final int UNKNOWN_ERROR_CODE = 1100;
diff --git
a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java
b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java
index 10b510a581..a47d4cbb80 100644
--- a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java
+++ b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java
@@ -348,6 +348,19 @@ public class ErrorResponse extends BaseResponse {
getStackTrace(throwable));
}
+ /**
+ * Create a new unauthorized error instance of {@link ErrorResponse}.
+ *
+ * @param type The type of the error.
+ * @param message The message of the error.
+ * @param throwable The throwable that caused the error.
+ * @return The new instance.
+ */
+ public static ErrorResponse unauthorized(String type, String message,
Throwable throwable) {
+ return new ErrorResponse(
+ ErrorConstants.UNAUTHORIZED_CODE, type, message,
getStackTrace(throwable));
+ }
+
private static List<String> getStackTrace(Throwable throwable) {
if (throwable == null) {
return null;
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
index f796a107e0..e2a1ac9245 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
@@ -33,7 +33,10 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.UnauthorizedException;
+import org.apache.gravitino.server.web.ObjectMapperProvider;
import org.apache.gravitino.utils.PrincipalUtils;
public class AuthenticationFilter implements Filter {
@@ -114,22 +117,35 @@ public class AuthenticationFilter implements Filter {
}
/**
- * Sends an error response when authentication fails. Subclasses can
override this to customize
- * the error response format (e.g., Iceberg REST server returns JSON error
bodies).
- *
- * <p>TODO: Gravitino server should override this method to return a correct
JSON response
- * following the Gravitino error response spec.
+ * Sends a JSON error response when authentication fails. Subclasses can
override this to
+ * customize the error response format (e.g., Iceberg REST server returns
Iceberg-specific JSON
+ * error bodies).
*
* @param response the HTTP servlet response
* @param exception the authentication exception
*/
protected void sendAuthErrorResponse(HttpServletResponse response, Exception
exception)
throws IOException {
- int status =
- exception instanceof UnauthorizedException
- ? HttpServletResponse.SC_UNAUTHORIZED
- : HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
- response.sendError(status, exception.getMessage());
+ int httpStatus;
+ ErrorResponse errorResponse;
+
+ if (exception instanceof UnauthorizedException) {
+ httpStatus = HttpServletResponse.SC_UNAUTHORIZED;
+ errorResponse =
+ ErrorResponse.unauthorized(
+ exception.getClass().getSimpleName(), exception.getMessage(),
exception);
+ } else if (exception instanceof ForbiddenException) {
+ httpStatus = HttpServletResponse.SC_FORBIDDEN;
+ errorResponse = ErrorResponse.forbidden(exception.getMessage(),
exception);
+ } else {
+ httpStatus = HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+ errorResponse = ErrorResponse.internalError(exception.getMessage(),
exception);
+ }
+
+ response.setStatus(httpStatus);
+ response.setContentType("application/json");
+ response.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ ObjectMapperProvider.objectMapper().writeValue(response.getWriter(),
errorResponse);
}
/**
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationFilter.java
index 8c227727b4..fa8a01bc40 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationFilter.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationFilter.java
@@ -27,8 +27,11 @@ import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.Lists;
import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
import java.util.Collections;
import java.util.Vector;
import javax.servlet.FilterChain;
@@ -37,7 +40,11 @@ import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.UnauthorizedException;
+import org.apache.gravitino.server.web.ObjectMapperProvider;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
public class TestAuthenticationFilter {
@@ -66,6 +73,9 @@ public class TestAuthenticationFilter {
FilterChain mockChain = mock(FilterChain.class);
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
HttpServletResponse mockResponse = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(mockResponse.getWriter()).thenReturn(printWriter);
when(mockRequest.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION))
.thenReturn(new
Vector<>(Collections.singletonList("user")).elements());
when(authenticator.supportsToken(any())).thenReturn(true);
@@ -73,7 +83,17 @@ public class TestAuthenticationFilter {
when(authenticator.authenticateToken(any()))
.thenThrow(new UnauthorizedException("UNAUTHORIZED"));
filter.doFilter(mockRequest, mockResponse, mockChain);
- verify(mockResponse).sendError(HttpServletResponse.SC_UNAUTHORIZED,
"UNAUTHORIZED");
+ verify(mockResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(mockResponse).setContentType("application/json");
+ verify(mockResponse).setCharacterEncoding("UTF-8");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(1011, errorResponse.getCode());
+ Assertions.assertEquals("UnauthorizedException", errorResponse.getType());
+ Assertions.assertEquals("UNAUTHORIZED", errorResponse.getMessage());
}
@Test
@@ -109,6 +129,9 @@ public class TestAuthenticationFilter {
FilterChain mockChain = mock(FilterChain.class);
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
HttpServletResponse mockResponse = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(mockResponse.getWriter()).thenReturn(printWriter);
when(mockRequest.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION))
.thenReturn(new
Vector<>(Collections.singletonList("user")).elements());
when(authenticator1.supportsToken(any())).thenReturn(false);
@@ -120,7 +143,16 @@ public class TestAuthenticationFilter {
.thenThrow(new UnauthorizedException("UNAUTHORIZED"));
filter.doFilter(mockRequest, mockResponse, mockChain);
- verify(mockResponse).sendError(HttpServletResponse.SC_UNAUTHORIZED,
"UNAUTHORIZED");
+ verify(mockResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(mockResponse).setContentType("application/json");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(1011, errorResponse.getCode());
+ Assertions.assertEquals("UnauthorizedException", errorResponse.getType());
+ Assertions.assertEquals("UNAUTHORIZED", errorResponse.getMessage());
}
@Test
@@ -169,6 +201,9 @@ public class TestAuthenticationFilter {
FilterChain mockChain = mock(FilterChain.class);
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
HttpServletResponse mockResponse = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(mockResponse.getWriter()).thenReturn(printWriter);
when(mockRequest.getRequestURI()).thenReturn(path);
when(mockRequest.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION))
.thenReturn(new
Vector<>(Collections.singletonList("user")).elements());
@@ -180,7 +215,81 @@ public class TestAuthenticationFilter {
filter.doFilter(mockRequest, mockResponse, mockChain);
// Auth flow ran and rejected — proves these paths are not exempted.
- verify(mockResponse).sendError(HttpServletResponse.SC_UNAUTHORIZED,
"UNAUTHORIZED");
+ verify(mockResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(mockResponse).setContentType("application/json");
}
}
+
+ @Test
+ public void testUnauthorizedErrorReturnsJsonBody() throws Exception {
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList());
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(
+ response, new UnauthorizedException("The provided credentials did not
support"));
+
+ verify(response).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(response).setContentType("application/json");
+ verify(response).setCharacterEncoding("UTF-8");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(1011, errorResponse.getCode());
+ Assertions.assertEquals("UnauthorizedException", errorResponse.getType());
+ Assertions.assertEquals("The provided credentials did not support",
errorResponse.getMessage());
+ }
+
+ @Test
+ public void testForbiddenErrorReturnsJsonBody() throws Exception {
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList());
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(response, new ForbiddenException("Access
denied"));
+
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ verify(response).setContentType("application/json");
+ verify(response).setCharacterEncoding("UTF-8");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(1008, errorResponse.getCode());
+ Assertions.assertEquals("ForbiddenException", errorResponse.getType());
+ Assertions.assertEquals("Access denied", errorResponse.getMessage());
+ }
+
+ @Test
+ public void testInternalServerErrorReturnsJsonBody() throws Exception {
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList());
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(response, new RuntimeException("Something
went wrong"));
+
+ verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ verify(response).setContentType("application/json");
+ verify(response).setCharacterEncoding("UTF-8");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(1002, errorResponse.getCode());
+ Assertions.assertEquals("RuntimeException", errorResponse.getType());
+ Assertions.assertEquals("Something went wrong",
errorResponse.getMessage());
+ }
}
