This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 652cd31bf7 [#12728] fix(server): make HTTP error stack traces
configurable (#13057)
652cd31bf7 is described below
commit 652cd31bf7f12238ab812c3f59acf84debfc8c46
Author: Nevin Zheng <[email protected]>
AuthorDate: Mon Sep 14 20:56:30 2026 -0700
[#12728] fix(server): make HTTP error stack traces configurable (#13057)
### What changes were proposed in this pull request?
HTTP error responses can now omit server-side stack traces. The main
server, Iceberg REST and Lance REST each read `includeErrorStackTrace`
from their own configuration, and it defaults to `true`, so existing
clients keep today's responses.
- **Main server:** `gravitino.server.webserver.includeErrorStackTrace`
controls the `stack` field in Jersey JSON errors, the authentication and
versioning filters, and Jetty's own error page.
- **Iceberg REST and Lance REST:**
`gravitino.iceberg-rest.includeErrorStackTrace` and
`gravitino.lance-rest.includeErrorStackTrace`. Iceberg adds `stack` only
when enabled; Lance leaves generated `detail` empty when disabled, while
Lance exceptions that carry their own `detail` keep it.
- **Diagnostics stay server-side:** `AuthenticationFilter` logs
unexpected failures once at ERROR (401/403/400 client errors are not
logged), and the authorization path logs the original throwable.
- **Hook contract:** `JettyServer.createAuthenticationFilter` now takes
`includeErrorStackTrace`, and implementations must not include stack
traces when it is `false`.
- **OpenAPI:** the `stack` field is marked deprecated.
Unchanged: the `message` field, and connector and Python client
behavior.
Reviewer notes:
- The Iceberg and Lance flags live in each service module, not in
`server-common`. `IsolatedClassLoader` shares non-catalog classes with
the main server, so a shared static would hold one value for all three
servers.
- `AuthenticationFilter.doFilter` runs authentication and the downstream
chain in one `try` with typed catches, so each failure is handled and
logged once. It keeps `ServerHealth.recordFailure` from #13067 in every
branch.
### Why are the changes needed?
Stack traces in error responses expose internal class names, line
numbers and proxy chains to any caller, including unauthenticated ones
(#12728). Omitting them follows [OWASP REST error-handling
guidance](https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling)
and mitigates
[CWE-209](https://cwe.mitre.org/data/definitions/209.html). The default
stays `true` for compatibility, as agreed in review, and the docs
recommend `false` for new deployments.
Related to: #12728
Follow-ups: #13107 (raw exception text in `message`), #13108 (skip
building stack text when disabled)
### Does this PR introduce _any_ user-facing change?
Yes.
- **New configuration keys**, all defaulting to `true`:
`gravitino.server.webserver.includeErrorStackTrace` (Docker:
`GRAVITINO_SERVER_WEBSERVER_INCLUDE_ERROR_STACK_TRACE`),
`gravitino.iceberg-rest.includeErrorStackTrace`, and
`gravitino.lance-rest.includeErrorStackTrace`. With the defaults,
responses are unchanged.
- **Extension API:** `JettyServer.createAuthenticationFilter()` is
replaced by `createAuthenticationFilter(boolean
includeErrorStackTrace)`. Subclasses that override the old method must
update.
- **Logging:** unexpected authentication failures are now logged at
ERROR.
### How was this patch tested?
- Unit tests cover both settings for Jersey serialization and legacy
deserialization (`TestObjectMapperProvider`, `TestGravitinoServer`),
Jetty's error page (`TestJettyServer`), authentication errors and their
logging (`TestAuthenticationFilter`), Iceberg REST
(`TestIcebergRESTUtils`, `TestIcebergAuthenticationFilter`,
`TestRESTService`) and Lance REST (`TestLanceExceptionMapper`,
`TestLanceMetadataAuthorizationMethodInterceptor`,
`TestLanceAuthenticationFilter`, `TestLanceRESTService`).
- `TestResponses` checks that error DTOs still carry the stack for
server-side use, and `TestGravitinoInterceptionService` covers logging
of the original throwable on the authorization path.
- Those suites pass locally with `-PskipITs`, together with the related
`TestAuthenticationOutOfMemoryHttp`, `TestOutOfMemoryHealthHttp`,
`TestHttpsServerAuthentication` and `TestJettyServerConfig`.
`spotlessCheck` passes for `common`, `server-common`, `server`,
`iceberg-rest-server` and `lance-rest-server` on the current head.
- The main behavioral tests were checked by inverting or removing the
code under test, which makes them fail.
---------
Co-authored-by: roryqi <[email protected]>
Co-authored-by: roryqi <[email protected]>
---
.../gravitino/dto/responses/TestResponses.java | 28 ++
conf/gravitino-iceberg-rest-server.conf.template | 3 +
conf/gravitino-lance-rest-server.conf.template | 3 +
conf/gravitino.conf.template | 3 +
.../gravitino/rewrite_gravitino_server_config.py | 2 +
docs/gravitino-server-config.md | 2 +
docs/iceberg-rest-service.md | 1 +
docs/lance-rest-service.md | 2 +
docs/open-api/catalogs.yaml | 9 +-
docs/open-api/openapi.yaml | 8 +
.../org/apache/gravitino/iceberg/RESTService.java | 7 +-
.../iceberg/service/IcebergRESTUtils.java | 26 +-
.../apache/gravitino/iceberg/TestRESTService.java | 11 +
.../service/TestIcebergAuthenticationFilter.java | 1 +
.../iceberg/service/TestIcebergRESTUtils.java | 21 ++
.../apache/gravitino/lance/LanceJettyServer.java | 4 +-
.../apache/gravitino/lance/LanceRESTService.java | 1 +
.../lance/service/LanceAuthenticationFilter.java | 4 -
.../lance/service/LanceExceptionMapper.java | 36 ++-
.../gravitino/lance/TestLanceRESTService.java | 11 +
.../service/TestLanceAuthenticationFilter.java | 1 +
.../lance/service/TestLanceExceptionMapper.java | 28 ++
...anceMetadataAuthorizationMethodInterceptor.java | 19 ++
.../authentication/AuthenticationFilter.java | 193 +++++++++-----
.../apache/gravitino/server/web/JettyServer.java | 12 +-
.../gravitino/server/web/JettyServerConfig.java | 27 ++
.../gravitino/server/web/ObjectMapperProvider.java | 83 ++++--
.../authentication/TestAuthenticationFilter.java | 287 ++++++++++++++++++++-
.../TestAuthenticationOutOfMemoryHttp.java | 2 +-
.../gravitino/server/web/TestJettyServer.java | 61 +++++
.../server/web/TestJettyServerConfig.java | 11 +
.../apache/gravitino/server/GravitinoServer.java | 10 +-
.../gravitino/server/web/VersioningFilter.java | 22 +-
.../web/filter/GravitinoInterceptionService.java | 3 +-
.../gravitino/server/TestGravitinoServer.java | 37 +++
.../server/web/TestObjectMapperProvider.java | 49 ++++
.../filter/TestGravitinoInterceptionService.java | 149 +++++++----
37 files changed, 1016 insertions(+), 161 deletions(-)
diff --git
a/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
b/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
index de286ac75a..2c5ab6bcb0 100644
--- a/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
+++ b/common/src/test/java/org/apache/gravitino/dto/responses/TestResponses.java
@@ -22,6 +22,7 @@ import static
org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -234,6 +235,33 @@ public class TestResponses {
error.validate(); // No exception thrown
}
+ @Test
+ void testThrowableErrorResponsesRetainStackTrace() throws
IllegalArgumentException {
+ Throwable throwable = new RuntimeException("private error details");
+ String message = "public error message";
+ ErrorResponse[] responses = {
+ ErrorResponse.illegalArguments(message, throwable),
+ ErrorResponse.connectionFailed(message, throwable),
+ ErrorResponse.notFound("error type", message, throwable),
+ ErrorResponse.internalError(message, throwable),
+ ErrorResponse.alreadyExists("error type", message, throwable),
+ ErrorResponse.notInUse("error type", message, throwable),
+ ErrorResponse.inUse("error type", message, throwable),
+ ErrorResponse.nonEmpty("error type", message, throwable),
+ ErrorResponse.unsupportedOperation(message, throwable),
+ ErrorResponse.forbidden(message, throwable),
+ ErrorResponse.unauthorized("error type", message, throwable)
+ };
+
+ for (ErrorResponse response : responses) {
+ response.validate();
+ assertEquals(message, response.getMessage());
+ assertNotNull(response.getStack());
+ assertTrue(
+ response.getStack().stream().anyMatch(line -> line.contains("private
error details")));
+ }
+ }
+
@Test
void testNotFoundErrorResponse() throws IllegalArgumentException {
ErrorResponse error = ErrorResponse.notFound("error type", "not found
error");
diff --git a/conf/gravitino-iceberg-rest-server.conf.template
b/conf/gravitino-iceberg-rest-server.conf.template
index 62df320496..0cd7d1e0b2 100644
--- a/conf/gravitino-iceberg-rest-server.conf.template
+++ b/conf/gravitino-iceberg-rest-server.conf.template
@@ -41,6 +41,9 @@ gravitino.iceberg-rest.threadPoolWorkQueueSize = 100
gravitino.iceberg-rest.requestHeaderSize = 131072
# The response header size of the built-in web server
gravitino.iceberg-rest.responseHeaderSize = 131072
+# Whether to include server-side stack traces in HTTP error responses. Set
this to false in new
+# deployments. It remains true by default only to preserve legacy response
behavior.
+gravitino.iceberg-rest.includeErrorStackTrace = true
# THE CONFIGURATION FOR Iceberg catalog backend
# The Iceberg catalog backend, it's recommended to change to hive or jdbc
diff --git a/conf/gravitino-lance-rest-server.conf.template
b/conf/gravitino-lance-rest-server.conf.template
index b8e1d0b1e1..bb1acbb3a2 100644
--- a/conf/gravitino-lance-rest-server.conf.template
+++ b/conf/gravitino-lance-rest-server.conf.template
@@ -39,6 +39,9 @@ gravitino.lance-rest.threadPoolWorkQueueSize = 100
gravitino.lance-rest.requestHeaderSize = 131072
# The response header size of the built-in web server
gravitino.lance-rest.responseHeaderSize = 131072
+# Whether to include server-side stack traces in HTTP error responses. Set
this to false in new
+# deployments. It remains true by default only to preserve legacy response
behavior.
+gravitino.lance-rest.includeErrorStackTrace = true
# THE CONFIGURATION FOR Lance namespace backend
# The backend Lance namespace for Lance REST service, it's recommended to use
Gravitino
diff --git a/conf/gravitino.conf.template b/conf/gravitino.conf.template
index 86bde7b784..4202d1d174 100644
--- a/conf/gravitino.conf.template
+++ b/conf/gravitino.conf.template
@@ -43,6 +43,9 @@ gravitino.server.webserver.threadPoolWorkQueueSize = 100
gravitino.server.webserver.requestHeaderSize = 131072
# The response header size of the built-in web server
gravitino.server.webserver.responseHeaderSize = 131072
+# Whether to include server-side stack traces in HTTP error responses. Set
this to false in new
+# deployments. It remains true by default only to avoid breaking legacy
clients that expect stack.
+gravitino.server.webserver.includeErrorStackTrace = true
# UI inactivity timeout in milliseconds. The UI environment variable
# NEXT_PUBLIC_IDLE_TIMEOUT_MS is used when this config is not set.
# gravitino.ui.sessionIdleTimeoutMs = 900000
diff --git a/dev/docker/gravitino/rewrite_gravitino_server_config.py
b/dev/docker/gravitino/rewrite_gravitino_server_config.py
index c5aca5d72c..9884b69b86 100755
--- a/dev/docker/gravitino/rewrite_gravitino_server_config.py
+++ b/dev/docker/gravitino/rewrite_gravitino_server_config.py
@@ -29,6 +29,7 @@ env_map = {
"GRAVITINO_SERVER_WEBSERVER_REQUEST_HEADER_SIZE":
"server.webserver.requestHeaderSize",
"GRAVITINO_SERVER_WEBSERVER_RESPONSE_HEADER_SIZE":
"server.webserver.responseHeaderSize",
"GRAVITINO_SERVER_BULK_MAX_ITEMS": "server.bulk.maxItems",
+ "GRAVITINO_SERVER_WEBSERVER_INCLUDE_ERROR_STACK_TRACE":
"server.webserver.includeErrorStackTrace",
"GRAVITINO_ENTITY_STORE": "entity.store",
"GRAVITINO_ENTITY_STORE_RELATIONAL": "entity.store.relational",
"GRAVITINO_ENTITY_STORE_RELATIONAL_JDBC_URL":
"entity.store.relational.jdbcUrl",
@@ -91,6 +92,7 @@ init_config = {
"server.webserver.requestHeaderSize": "131072",
"server.webserver.responseHeaderSize": "131072",
"server.bulk.maxItems": "100",
+ "server.webserver.includeErrorStackTrace": "true",
"entity.store": "relational",
"entity.store.relational": "JDBCBackend",
"entity.store.relational.jdbcUrl": "jdbc:h2",
diff --git a/docs/gravitino-server-config.md b/docs/gravitino-server-config.md
index 9a39b1a613..d8421e6f04 100644
--- a/docs/gravitino-server-config.md
+++ b/docs/gravitino-server-config.md
@@ -173,6 +173,7 @@ empty string or list; `(none)` means it has no default at
all.
| `gravitino.server.rest.extensionPackages` | Comma-separated list
of packages to scan for additional REST resources.
| (empty) |
| `gravitino.server.visibleConfigs` | Comma-separated list
of extra properties to expose on the unauthenticated `GET /configs` endpoint,
on top of the fixed set it always returns. Additive, so each entry widens what
is public. | (empty) |
| `gravitino.server.bulk.maxItems` | Maximum number of
items allowed in a single bulk request.
| `100` |
+| `gravitino.server.webserver.includeErrorStackTrace` | Whether HTTP error
responses include server-side stack traces. Set this to `false` in new
deployments because responses can expose internal implementation details. It
remains `true` by default only to avoid breaking legacy clients that expect the
`stack` field. See [OWASP REST Security: Error
handling](https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling)
and [CWE-209](https://cwe.mitre.org/d [...]
Filters named in `customFilters` must be standard `javax.servlet` filters.
Pass parameters to a
filter with properties of the form
@@ -645,6 +646,7 @@ means the property is left alone.
| `GRAVITINO_SERVER_WEBSERVER_REQUEST_HEADER_SIZE` |
`gravitino.server.webserver.requestHeaderSize` | `131072`
|
| `GRAVITINO_SERVER_WEBSERVER_RESPONSE_HEADER_SIZE` |
`gravitino.server.webserver.responseHeaderSize` | `131072`
|
| `GRAVITINO_SERVER_BULK_MAX_ITEMS` |
`gravitino.server.bulk.maxItems` | `100`
|
+| `GRAVITINO_SERVER_WEBSERVER_INCLUDE_ERROR_STACK_TRACE` |
`gravitino.server.webserver.includeErrorStackTrace` | `true`
|
| `GRAVITINO_ENTITY_STORE` |
`gravitino.entity.store` | `relational`
|
| `GRAVITINO_ENTITY_STORE_RELATIONAL` |
`gravitino.entity.store.relational` | `JDBCBackend`
|
| `GRAVITINO_ENTITY_STORE_RELATIONAL_JDBC_URL` |
`gravitino.entity.store.relational.jdbcUrl` | `jdbc:h2`
|
diff --git a/docs/iceberg-rest-service.md b/docs/iceberg-rest-service.md
index 8eeffbcd8d..76f35650db 100644
--- a/docs/iceberg-rest-service.md
+++ b/docs/iceberg-rest-service.md
@@ -123,6 +123,7 @@ Do not add them to the standalone server configuration.
| `gravitino.iceberg-rest.idleTimeout` | The timeout in ms of idle
connections.
| `30000`
| No |
| `gravitino.iceberg-rest.requestHeaderSize` | The maximum size of an
HTTP request.
| `131072`
| No |
| `gravitino.iceberg-rest.responseHeaderSize` | The maximum size of an
HTTP response.
| `131072`
| No |
+| `gravitino.iceberg-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces. Set this to `false` in new deployments
because responses can expose internal implementation details. | `true`
| No |
| `gravitino.iceberg-rest.customFilters` | Comma-separated list of
filter class names to apply to the APIs.
| (none)
| No |
The filter in `customFilters` should be a standard javax servlet filter.
diff --git a/docs/lance-rest-service.md b/docs/lance-rest-service.md
index ca27ebb7c0..f8c6ed5b89 100644
--- a/docs/lance-rest-service.md
+++ b/docs/lance-rest-service.md
@@ -136,6 +136,7 @@ To enable the Lance REST service within Gravitino server,
configure the followin
| `gravitino.lance-rest.classpath` | Classpath for Lance REST
service, relative to Gravitino home directory | lance-rest-server/libs |
Yes |
| `gravitino.lance-rest.httpPort` | Port number for Lance REST
service | 9101 |
No |
| `gravitino.lance-rest.host` | Hostname for Lance REST service
| 0.0.0.0 | No
|
+| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces in `detail`. Set this to `false` in new
deployments | true | No |
| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend
(currently only `gravitino` is supported) | gravitino |
Yes |
| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI. Not
required in auxiliary mode. | http://localhost:8090 |
No |
| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name
(required when namespace-backend is `gravitino`) | (none)
| Yes |
@@ -189,6 +190,7 @@ Configure the service by editing
`{GRAVITINO_HOME}/conf/gravitino-lance-rest-ser
| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name |
(none) | Yes |
| `gravitino.lance-rest.httpPort` | Service port number |
9101 | No |
| `gravitino.lance-rest.host` | Service hostname |
0.0.0.0 | No |
+| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include stack traces | true | No |
:::tip
In standalone deployments, you only need to configure
`gravitino.lance-rest.gravitino-metalake`,
diff --git a/docs/open-api/catalogs.yaml b/docs/open-api/catalogs.yaml
index cc290f39db..695ef01737 100644
--- a/docs/open-api/catalogs.yaml
+++ b/docs/open-api/catalogs.yaml
@@ -130,9 +130,16 @@ paths:
description: The message of the exception
stack:
type: array
+ deprecated: true
+ description: >-
+ Deprecated diagnostic stack trace. Operators should
disable it with
+ gravitino.server.webserver.includeErrorStackTrace=false
because it can
+ expose internal implementation details. It remains
enabled by default only
+ to avoid breaking legacy clients that expect this field.
See
+ [OWASP REST Security: Error
handling](https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling)
+ and
[CWE-209](https://cwe.mitre.org/data/definitions/209.html).
items:
type: string
- description: The stack trace of the exception
examples:
TestConnectionSuccess:
value: {
diff --git a/docs/open-api/openapi.yaml b/docs/open-api/openapi.yaml
index 23d1429a8a..826dd33aa8 100644
--- a/docs/open-api/openapi.yaml
+++ b/docs/open-api/openapi.yaml
@@ -315,6 +315,14 @@ components:
description: A human-readable message
stack:
type: array
+ deprecated: true
+ description: >-
+ Deprecated diagnostic stack trace. Operators should disable it with
+ gravitino.server.webserver.includeErrorStackTrace=false because it
can expose internal
+ implementation details. It remains enabled by default only to
avoid breaking legacy
+ clients that expect this field. See
+ [OWASP REST Security: Error
handling](https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#error-handling)
+ and [CWE-209](https://cwe.mitre.org/data/definitions/209.html).
items:
type: string
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index 3710708c57..1786ba3e0e 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -34,6 +34,7 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergExceptionMapper;
import org.apache.gravitino.iceberg.service.IcebergHealthCheckPathMatcher;
import org.apache.gravitino.iceberg.service.IcebergObjectMapperProvider;
+import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import org.apache.gravitino.iceberg.service.cleanup.IcebergCleanupJobStore;
import org.apache.gravitino.iceberg.service.cleanup.IcebergCleanupManager;
@@ -91,10 +92,14 @@ public class RESTService implements
GravitinoAuxiliaryService {
private void initServer(IcebergConfig icebergConfig) {
JettyServerConfig serverConfig =
JettyServerConfig.fromConfig(icebergConfig);
+
IcebergRESTUtils.setIncludeErrorStackTrace(serverConfig.isIncludeErrorStackTrace());
server =
new JettyServer() {
@Override
- protected javax.servlet.Filter createAuthenticationFilter() {
+ protected javax.servlet.Filter createAuthenticationFilter(
+ boolean includeErrorStackTrace) {
+ // Iceberg authentication errors never carry a stack trace (see
+ // TestIcebergAuthenticationFilter), so either setting is honored.
return new IcebergAuthenticationFilter();
}
};
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
index a61a92e391..d88f4de9dd 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
@@ -49,6 +49,7 @@ import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.credential.Credential;
import org.apache.gravitino.credential.CredentialPropertyUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
+import org.apache.gravitino.server.web.JettyServerConfig;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
@@ -92,6 +93,11 @@ public class IcebergRESTUtils {
"gcs.oauth2.refresh-credentials-endpoint",
"adls.refresh-credentials-endpoint");
+ // Set once at service startup. Only Iceberg REST reads this flag, and its
classes are packaged
+ // outside the main server's classpath, so each service loads its own copy.
+ private static volatile boolean includeErrorStackTrace =
+ JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue();
+
/** Snapshot modes for the Iceberg loadTable endpoint. */
public enum SnapshotMode {
ALL(SNAPSHOT_ALL),
@@ -425,14 +431,26 @@ public class IcebergRESTUtils {
return Response.status(Status.NOT_FOUND).build();
}
+ /**
+ * Sets whether error responses built by {@link #errorResponse(Throwable,
int)} include the
+ * exception's stack trace. Called once when the service starts.
+ *
+ * @param include whether to include stack traces
+ */
+ public static void setIncludeErrorStackTrace(boolean include) {
+ includeErrorStackTrace = include;
+ }
+
public static Response errorResponse(Throwable ex, int httpStatus) {
- ErrorResponse errorResponse =
+ ErrorResponse.Builder builder =
ErrorResponse.builder()
.responseCode(httpStatus)
.withType(ex.getClass().getSimpleName())
- .withMessage(ex.getMessage())
- .withStackTrace(ex)
- .build();
+ .withMessage(ex.getMessage());
+ if (includeErrorStackTrace) {
+ builder.withStackTrace(ex);
+ }
+ ErrorResponse errorResponse = builder.build();
return Response.status(httpStatus)
.entity(errorResponse)
.type(MediaType.APPLICATION_JSON)
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
index 36b2b5e6bd..51f4a1b8c3 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.iceberg;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Collections;
@@ -74,4 +75,14 @@ public class TestRESTService {
server.stop();
}
}
+
+ /** The documented Iceberg REST key reaches the value the service passes to
its error builder. */
+ @Test
+ public void testIncludeErrorStackTraceReadFromServiceConfig() {
+ assertTrue(JettyServerConfig.fromConfig(new
IcebergConfig()).isIncludeErrorStackTrace());
+ assertFalse(
+ JettyServerConfig.fromConfig(
+ new
IcebergConfig(Collections.singletonMap("includeErrorStackTrace", "false")))
+ .isIncludeErrorStackTrace());
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
index 4dbd5e1292..1abcb6ede0 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
@@ -146,6 +146,7 @@ public class TestIcebergAuthenticationFilter {
Assertions.assertEquals(500, errorResponse.code());
Assertions.assertEquals("ServiceFailureException", errorResponse.type());
Assertions.assertEquals("Something went wrong", errorResponse.message());
+ Assertions.assertFalse(json.contains("\"stack\""), json);
}
@Test
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
index e3e4f3225d..af01018630 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
@@ -48,6 +48,7 @@ import org.apache.iceberg.io.StorageCredential;
import org.apache.iceberg.io.SupportsStorageCredentials;
import org.apache.iceberg.rest.credentials.Credential;
import org.apache.iceberg.rest.requests.CreateTableRequest;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ImmutableLoadCredentialsResponse;
import org.apache.iceberg.rest.responses.LoadCredentialsResponse;
import org.apache.iceberg.rest.responses.LoadTableResponse;
@@ -449,4 +450,24 @@ public class TestIcebergRESTUtils {
"v1/irc1/namespaces/db/tables/tbl/credentials",
rewritten.credentials().get(0).config().get("client.refresh-credentials-endpoint"));
}
+
+ @Test
+ void testErrorResponseStackTraceFollowsSetting() throws Exception {
+ RuntimeException failure = new RuntimeException("failure");
+ try {
+ ErrorResponse withStack =
+ (ErrorResponse) IcebergRESTUtils.errorResponse(failure,
500).getEntity();
+ Assertions.assertTrue(
+
IcebergObjectMapper.getInstance().writeValueAsString(withStack).contains("\"stack\""));
+
+ IcebergRESTUtils.setIncludeErrorStackTrace(false);
+ ErrorResponse withoutStack =
+ (ErrorResponse) IcebergRESTUtils.errorResponse(failure,
500).getEntity();
+ String json =
IcebergObjectMapper.getInstance().writeValueAsString(withoutStack);
+ Assertions.assertFalse(json.contains("\"stack\""), json);
+ Assertions.assertEquals("failure", withoutStack.message());
+ } finally {
+ IcebergRESTUtils.setIncludeErrorStackTrace(true);
+ }
+ }
}
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceJettyServer.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceJettyServer.java
index be8b8dbff5..4261dd15f7 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceJettyServer.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceJettyServer.java
@@ -29,7 +29,9 @@ import org.apache.gravitino.server.web.JettyServer;
class LanceJettyServer extends JettyServer {
@Override
- protected Filter createAuthenticationFilter() {
+ protected Filter createAuthenticationFilter(boolean includeErrorStackTrace) {
+ // Lance authentication errors always send an empty detail (see
+ // TestLanceAuthenticationFilter), so either setting is honored.
return new LanceAuthenticationFilter();
}
}
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
index bfb27e04d6..59b8a3afbd 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
@@ -80,6 +80,7 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
public void serviceInit(Map<String, String> properties, boolean auxMode) {
LanceConfig lanceConfig = new LanceConfig(new HashMap<>(properties));
JettyServerConfig serverConfig = JettyServerConfig.fromConfig(lanceConfig);
+
LanceExceptionMapper.setIncludeErrorStackTrace(serverConfig.isIncludeErrorStackTrace());
server = new LanceJettyServer();
// Get MetricsSystem and EventBus from GravitinoEnv once at init time.
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceAuthenticationFilter.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceAuthenticationFilter.java
index 45cebe3852..235ce41094 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceAuthenticationFilter.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceAuthenticationFilter.java
@@ -28,8 +28,6 @@ import org.apache.gravitino.exceptions.UnauthorizedException;
import org.apache.gravitino.server.authentication.AuthenticationFilter;
import org.apache.gravitino.server.web.ObjectMapperProvider;
import org.lance.namespace.model.ErrorResponse;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* An {@link AuthenticationFilter} subclass for the Lance REST server that:
@@ -43,7 +41,6 @@ import org.slf4j.LoggerFactory;
*/
public class LanceAuthenticationFilter extends AuthenticationFilter {
- private static final Logger LOG =
LoggerFactory.getLogger(LanceAuthenticationFilter.class);
private static final ObjectMapper MAPPER =
ObjectMapperProvider.objectMapper();
public LanceAuthenticationFilter() {
@@ -75,7 +72,6 @@ public class LanceAuthenticationFilter extends
AuthenticationFilter {
}
} else {
status = HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
- LOG.error("Authentication failure", exception);
message = "Authentication failed";
}
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java
index a56ee9bc3b..daad886540 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java
@@ -27,6 +27,7 @@ import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.NoSuchTableException;
import org.apache.gravitino.exceptions.NotFoundException;
import org.apache.gravitino.exceptions.UnauthorizedException;
+import org.apache.gravitino.server.web.JettyServerConfig;
import org.apache.gravitino.server.web.ServerHealth;
import org.lance.namespace.errors.ConcurrentModificationException;
import org.lance.namespace.errors.InternalException;
@@ -49,6 +50,31 @@ public class LanceExceptionMapper implements
ExceptionMapper<Throwable> {
private static final Logger LOG =
LoggerFactory.getLogger(LanceExceptionMapper.class);
+ // Set once at service startup. Only Lance REST reads this flag, and its
classes are packaged
+ // outside the main server's classpath, so each service loads its own copy.
+ private static volatile boolean includeErrorStackTrace =
+ JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue();
+
+ /**
+ * Sets whether error details built by {@link #errorDetail(Throwable)}
include the exception's
+ * stack trace. Called once when the service starts.
+ *
+ * @param include whether to include stack traces
+ */
+ public static void setIncludeErrorStackTrace(boolean include) {
+ includeErrorStackTrace = include;
+ }
+
+ /**
+ * Returns the error detail for a Lance error response.
+ *
+ * @param ex the failure to describe
+ * @return the stack trace of {@code ex}, or an empty string when stack
traces are disabled
+ */
+ public static String errorDetail(Throwable ex) {
+ return includeErrorStackTrace ? getStackTrace(ex) : "";
+ }
+
public static Response toRESTResponse(String instance, Throwable ex) {
ServerHealth.getInstance().recordFailure(ex);
LanceNamespaceException lanceException =
@@ -77,20 +103,20 @@ public class LanceExceptionMapper implements
ExceptionMapper<Throwable> {
return new UnauthenticatedException(ex.getMessage(), "", instance);
} else if (ex instanceof NoSuchTableException) {
- return new TableNotFoundException(ex.getMessage(), getStackTrace(ex),
instance);
+ return new TableNotFoundException(ex.getMessage(), errorDetail(ex),
instance);
} else if (ex instanceof NotFoundException) {
- return new NamespaceNotFoundException(ex.getMessage(),
getStackTrace(ex), instance);
+ return new NamespaceNotFoundException(ex.getMessage(), errorDetail(ex),
instance);
} else if (ex instanceof IllegalArgumentException) {
- return new InvalidInputException(ex.getMessage(), getStackTrace(ex),
instance);
+ return new InvalidInputException(ex.getMessage(), errorDetail(ex),
instance);
} else if (ex instanceof
org.apache.gravitino.exceptions.TableAlreadyExistsException) {
- return new TableAlreadyExistsException(ex.getMessage(),
getStackTrace(ex), instance);
+ return new TableAlreadyExistsException(ex.getMessage(), errorDetail(ex),
instance);
} else if (ex instanceof UnsupportedOperationException) {
return new org.lance.namespace.errors.UnsupportedOperationException(
- ex.getMessage(), getStackTrace(ex), instance);
+ ex.getMessage(), errorDetail(ex), instance);
} else {
LOG.warn("Lance REST server unexpected exception:", ex);
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
index bda74859f0..4798ea1f55 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.lance;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Collections;
@@ -74,4 +75,14 @@ public class TestLanceRESTService {
server.stop();
}
}
+
+ /** The documented Lance REST key reaches the value the service passes to
its error builder. */
+ @Test
+ public void testIncludeErrorStackTraceReadFromServiceConfig() {
+ assertTrue(JettyServerConfig.fromConfig(new
LanceConfig()).isIncludeErrorStackTrace());
+ assertFalse(
+ JettyServerConfig.fromConfig(
+ new
LanceConfig(Collections.singletonMap("includeErrorStackTrace", "false")))
+ .isIncludeErrorStackTrace());
+ }
}
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceAuthenticationFilter.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceAuthenticationFilter.java
index 0b73c7ae7f..69d581597f 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceAuthenticationFilter.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceAuthenticationFilter.java
@@ -153,5 +153,6 @@ public class TestLanceAuthenticationFilter {
ErrorResponse errorResponse = MAPPER.readValue(json, ErrorResponse.class);
Assertions.assertEquals(500, errorResponse.getCode());
Assertions.assertEquals("Authentication failed", errorResponse.getError());
+ Assertions.assertEquals("", errorResponse.getDetail());
}
}
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java
index 8f4513fdf6..42c4faac2f 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java
@@ -135,4 +135,32 @@ public class TestLanceExceptionMapper extends JerseyTest {
Assertions.assertEquals("catalog", error.getInstance());
}
}
+
+ /** Verifies that the error detail omits the stack trace when disabled. */
+ @Test
+ public void testErrorDetailFollowsSetting() {
+ IllegalArgumentException failure = new IllegalArgumentException("failure");
+ try {
+ Assertions.assertTrue(
+ LanceExceptionMapper.errorDetail(failure)
+ .contains("java.lang.IllegalArgumentException: failure"));
+
+ LanceExceptionMapper.setIncludeErrorStackTrace(false);
+ Assertions.assertEquals("", LanceExceptionMapper.errorDetail(failure));
+ Response response = LanceExceptionMapper.toRESTResponse("", failure);
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals("failure", entity.getError());
+ Assertions.assertEquals("", entity.getDetail());
+
+ // Lance exceptions that already carry a detail keep it; only generated
stacks are omitted.
+ Response nativeResponse =
+ LanceExceptionMapper.toRESTResponse(
+ "instance", new InvalidInputException("bad input", "native
detail", "instance"));
+ Assertions.assertEquals(
+ "native detail", ((ErrorResponse)
nativeResponse.getEntity()).getDetail());
+ } finally {
+ LanceExceptionMapper.setIncludeErrorStackTrace(true);
+ }
+ }
}
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
index 517d74d770..5c30b511cd 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/authorization/TestLanceMetadataAuthorizationMethodInterceptor.java
@@ -40,6 +40,7 @@ import org.apache.gravitino.UserPrincipal;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.authorization.GravitinoAuthorizer;
import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.lance.service.LanceExceptionMapper;
import
org.apache.gravitino.lance.service.authorization.annotations.LanceRootNamespace;
import org.apache.gravitino.lance.service.rest.LanceNamespaceOperations;
import org.apache.gravitino.lance.service.rest.LanceTableOperations;
@@ -122,6 +123,24 @@ class TestLanceMetadataAuthorizationMethodInterceptor {
assertErrorResponse(describe, Response.Status.FORBIDDEN);
}
+ @Test
+ void testForbiddenResponseOmitsStackTraceWhenDisabled() throws Throwable {
+ allow(Privilege.Name.USE_CATALOG, Privilege.Name.CREATE_SCHEMA);
+ when(authorizer.deny(any(), any(), any(), any(), any())).thenReturn(true);
+ try {
+ LanceExceptionMapper.setIncludeErrorStackTrace(false);
+ Object result =
+ interceptor.invoke(
+ invocation(namespaceMethod("namespaceExists"), CATALOG + "$" +
SCHEMA, "$"));
+
+ assertErrorResponse(result, Response.Status.FORBIDDEN);
+ ErrorResponse entity = (ErrorResponse) ((Response) result).getEntity();
+ assertEquals("", entity.getDetail());
+ } finally {
+ LanceExceptionMapper.setIncludeErrorStackTrace(true);
+ }
+ }
+
@Test
void testDenyOverridesSchemaProbePrivileges() throws Throwable {
allow(Privilege.Name.USE_CATALOG, Privilege.Name.CREATE_SCHEMA);
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 28024d6075..74eca33ed4 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
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.server.authentication;
+import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
@@ -55,6 +56,8 @@ public class AuthenticationFilter implements Filter {
private final List<Authenticator> filterAuthenticators;
+ private final ObjectMapper objectMapper;
+
/**
* The matcher used to identify health check paths that bypass
authentication. Subclasses may
* replace this with a server-specific matcher (e.g. {@code
IcebergHealthCheckPathMatcher}).
@@ -62,12 +65,32 @@ public class AuthenticationFilter implements Filter {
protected HealthCheckPathMatcher healthCheckMatcher = new
HealthCheckPathMatcher();
public AuthenticationFilter() {
- filterAuthenticators = null;
+ this(null, ObjectMapperProvider.objectMapper());
+ }
+
+ /**
+ * Creates an authentication filter with explicit error stack-trace response
behavior.
+ *
+ * @param includeErrorStackTrace whether authentication error responses
should include diagnostic
+ * stack traces
+ */
+ public AuthenticationFilter(boolean includeErrorStackTrace) {
+ this(null, ObjectMapperProvider.objectMapper(includeErrorStackTrace));
}
@VisibleForTesting
AuthenticationFilter(List<Authenticator> authenticators) {
+ this(authenticators, ObjectMapperProvider.objectMapper());
+ }
+
+ @VisibleForTesting
+ AuthenticationFilter(List<Authenticator> authenticators, boolean
includeErrorStackTrace) {
+ this(authenticators,
ObjectMapperProvider.objectMapper(includeErrorStackTrace));
+ }
+
+ private AuthenticationFilter(List<Authenticator> authenticators,
ObjectMapper objectMapper) {
this.filterAuthenticators = authenticators;
+ this.objectMapper = objectMapper;
}
@Override
@@ -83,74 +106,26 @@ public class AuthenticationFilter implements Filter {
chain.doFilter(request, response);
return;
}
+ HttpServletRequest req = (HttpServletRequest) request;
+ HttpServletResponse resp = (HttpServletResponse) response;
try {
- List<Authenticator> authenticators;
- if (filterAuthenticators == null || filterAuthenticators.isEmpty()) {
- authenticators = ServerAuthenticator.getInstance().authenticators();
- } else {
- authenticators = filterAuthenticators;
- }
- HttpServletRequest req = (HttpServletRequest) request;
- Enumeration<String> headerData =
req.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION);
- byte[] authData = null;
- if (headerData.hasMoreElements()) {
- authData = headerData.nextElement().getBytes(StandardCharsets.UTF_8);
- }
-
- // If token is supported by multiple authenticators, use the first by
default.
- Principal principal = null;
- for (Authenticator authenticator : authenticators) {
- if (authenticator.supportsToken(authData) &&
authenticator.isDataFromToken()) {
- principal = authenticator.authenticateToken(authData);
- if (principal != null) {
- break;
- }
- }
- }
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "uri={} hasAuthHeader={} principal={}",
- req.getRequestURI(),
- authData != null,
- principal == null ? "null" : principal.getName());
- }
- if (principal == null) {
- throw new UnauthorizedException("The provided credentials did not
support");
- }
- // Role assumption: parse the header (syntactic only; malformed -> 400)
and, only when
- // narrowed, attach the roles to the principal. Membership 403 is
checked later.
- ActiveRoles activeRoles =
-
ActiveRolesParser.parse(req.getHeader(AuthConstants.X_GRAVITINO_ACTIVE_ROLES_HEADER));
- if (!activeRoles.isAll() && principal instanceof UserPrincipal) {
- principal = ((UserPrincipal) principal).withActiveRoles(activeRoles);
- }
- // Publish the finalized principal (already carrying any narrowed roles)
so downstream
- // re-binds from the attribute (e.g. Utils.doAs) see the same identity
and roles.
-
request.setAttribute(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME,
principal);
- PrincipalUtils.doAs(
- principal,
- () -> {
- chain.doFilter(request, response);
- return null;
- });
+ Principal principal = authenticate(req);
+ runAsPrincipal(principal, req, resp, chain);
} catch (UnauthorizedException ue) {
health.recordFailure(ue);
- HttpServletResponse resp = (HttpServletResponse) response;
- if (!ue.getChallenges().isEmpty()) {
- // For some authentication, HTTP response can provide some challenge
information
- // to let client to create correct authenticated request.
- // Refer to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate
- for (String challenge : ue.getChallenges()) {
- if (!challenge.toLowerCase().startsWith("basic")) {
- resp.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
- }
- }
- }
- sendAuthErrorResponse(resp, ue);
- } catch (Exception e) {
- health.recordFailure(e);
- HttpServletResponse resp = (HttpServletResponse) response;
- sendAuthErrorResponse(resp, e);
+ sendUnauthorizedResponse(resp, ue);
+ } catch (IllegalActiveRolesException | ForbiddenException clientError) {
+ health.recordFailure(clientError);
+ sendAuthErrorResponse(resp, clientError);
+ } catch (RuntimeException unexpected) {
+ health.recordFailure(unexpected);
+ // The response may omit the stack trace, so keep the cause in the
server log.
+ LOG.error("Unexpected error while processing request to {}",
req.getRequestURI(), unexpected);
+ sendAuthErrorResponse(resp, unexpected);
+ } catch (Exception checked) {
+ health.recordFailure(checked);
+ // Only the downstream chain throws checked exceptions, and
PrincipalUtils.doAs logs them.
+ sendAuthErrorResponse(resp, checked);
}
}
@@ -188,7 +163,7 @@ public class AuthenticationFilter implements Filter {
response.setStatus(httpStatus);
response.setContentType("application/json");
response.setCharacterEncoding(StandardCharsets.UTF_8.name());
- ObjectMapperProvider.objectMapper().writeValue(response.getWriter(),
errorResponse);
+ objectMapper.writeValue(response.getWriter(), errorResponse);
}
/**
@@ -207,4 +182,88 @@ public class AuthenticationFilter implements Filter {
@Override
public void destroy() {}
+
+ private Principal authenticate(HttpServletRequest request) {
+ return applyActiveRoles(request, resolvePrincipal(request));
+ }
+
+ private Principal resolvePrincipal(HttpServletRequest request) {
+ byte[] authData = authorizationData(request);
+ Principal principal = null;
+ // If token is supported by multiple authenticators, use the first by
default.
+ for (Authenticator authenticator : authenticators()) {
+ if (authenticator.supportsToken(authData) &&
authenticator.isDataFromToken()) {
+ principal = authenticator.authenticateToken(authData);
+ if (principal != null) {
+ break;
+ }
+ }
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "uri={} hasAuthHeader={} principal={}",
+ request.getRequestURI(),
+ authData != null,
+ principal == null ? "null" : principal.getName());
+ }
+ if (principal == null) {
+ throw new UnauthorizedException("The provided credentials did not
support");
+ }
+ return principal;
+ }
+
+ private List<Authenticator> authenticators() {
+ if (filterAuthenticators == null || filterAuthenticators.isEmpty()) {
+ return ServerAuthenticator.getInstance().authenticators();
+ }
+ return filterAuthenticators;
+ }
+
+ private static byte[] authorizationData(HttpServletRequest request) {
+ Enumeration<String> headerData =
request.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION);
+ return headerData.hasMoreElements()
+ ? headerData.nextElement().getBytes(StandardCharsets.UTF_8)
+ : null;
+ }
+
+ // Role assumption: parse the header (syntactic only; malformed -> 400) and,
only when narrowed,
+ // attach the roles to the principal. Membership 403 is checked later.
+ private static Principal applyActiveRoles(HttpServletRequest request,
Principal principal) {
+ ActiveRoles activeRoles =
+
ActiveRolesParser.parse(request.getHeader(AuthConstants.X_GRAVITINO_ACTIVE_ROLES_HEADER));
+ if (!activeRoles.isAll() && principal instanceof UserPrincipal) {
+ return ((UserPrincipal) principal).withActiveRoles(activeRoles);
+ }
+ return principal;
+ }
+
+ private static void runAsPrincipal(
+ Principal principal,
+ HttpServletRequest request,
+ HttpServletResponse response,
+ FilterChain chain)
+ throws Exception {
+ // Publish the finalized principal (already carrying any narrowed roles)
so downstream
+ // re-binds from the attribute (e.g. Utils.doAs) see the same identity and
roles.
+ request.setAttribute(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME,
principal);
+ PrincipalUtils.doAs(
+ principal,
+ () -> {
+ chain.doFilter(request, response);
+ return null;
+ });
+ }
+
+ private void sendUnauthorizedResponse(HttpServletResponse response,
UnauthorizedException ue)
+ throws IOException {
+ // For some authentication, HTTP response can provide some challenge
information
+ // to let client to create correct authenticated request.
+ // Refer to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate
+ for (String challenge : ue.getChallenges()) {
+ if (!challenge.toLowerCase().startsWith("basic")) {
+ response.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
+ }
+ }
+ sendAuthErrorResponse(response, ue);
+ }
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
index 7a507eb74c..7f0dea29e1 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
@@ -116,7 +116,7 @@ public class JettyServer {
// Set error handler for Jetty Server
ErrorHandler errorHandler = new ErrorHandler();
- errorHandler.setShowStacks(true);
+ errorHandler.setShowStacks(serverConfig.isIncludeErrorStackTrace());
errorHandler.setServer(server);
server.addBean(errorHandler);
@@ -538,14 +538,18 @@ public class JettyServer {
servletContextHandler.addFilter(
CorsFilterHolder.create(serverConfig), pathSpec,
EnumSet.allOf(DispatcherType.class));
}
- addFilter(createAuthenticationFilter(), pathSpec);
+
addFilter(createAuthenticationFilter(serverConfig.isIncludeErrorStackTrace()),
pathSpec);
}
/**
* Creates the authentication filter for this server. Subclasses can
override this to provide a
* custom authentication filter (e.g., one that returns Iceberg-spec JSON
error responses).
+ *
+ * @param includeErrorStackTrace whether the filter's error responses may
include server-side
+ * stack traces; implementations must not include them when this is
{@code false}
+ * @return the authentication filter
*/
- protected Filter createAuthenticationFilter() {
- return new AuthenticationFilter();
+ protected Filter createAuthenticationFilter(boolean includeErrorStackTrace) {
+ return new AuthenticationFilter(includeErrorStackTrace);
}
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
index cb7a5b27e6..825716faa3 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
@@ -112,6 +112,21 @@ public final class JettyServerConfig {
.checkValue(value -> value > 0,
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
.createWithDefault(128 * 1024);
+ /**
+ * Whether HTTP error responses include server-side stack traces. Defaults
to {@code true} for
+ * compatibility with clients that read the {@code stack} field.
+ */
+ public static final ConfigEntry<Boolean> INCLUDE_ERROR_STACK_TRACE =
+ new ConfigBuilder("includeErrorStackTrace")
+ .doc(
+ "Whether to include server-side stack traces in HTTP error
responses. Set this to "
+ + "false in new deployments because stack traces can expose
internal "
+ + "implementation details. It remains true by default only
to avoid breaking "
+ + "legacy clients that expect the stack field")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .booleanConf()
+ .createWithDefault(true);
+
public static final ConfigEntry<Integer>
WEBSERVER_THREAD_POOL_WORK_QUEUE_SIZE =
new ConfigBuilder("threadPoolWorkQueueSize")
.doc("The size of the queue in the thread pool used by Jetty
webserver")
@@ -313,6 +328,8 @@ public final class JettyServerConfig {
private final int responseHeaderSize;
+ private final boolean includeErrorStackTrace;
+
private final int threadPoolWorkQueueSize;
private final int httpsPort;
@@ -368,6 +385,7 @@ public final class JettyServerConfig {
this.idleTimeout = internalConfig.get(WEBSERVER_IDLE_TIMEOUT);
this.requestHeaderSize = internalConfig.get(WEBSERVER_REQUEST_HEADER_SIZE);
this.responseHeaderSize =
internalConfig.get(WEBSERVER_RESPONSE_HEADER_SIZE);
+ this.includeErrorStackTrace =
internalConfig.get(INCLUDE_ERROR_STACK_TRACE);
this.threadPoolWorkQueueSize =
internalConfig.get(WEBSERVER_THREAD_POOL_WORK_QUEUE_SIZE);
this.enableHttps = internalConfig.get(ENABLE_HTTPS);
@@ -456,6 +474,15 @@ public final class JettyServerConfig {
return responseHeaderSize;
}
+ /**
+ * Returns whether HTTP error responses include server-side stack traces.
+ *
+ * @return {@code true} if HTTP error responses include server-side stack
traces
+ */
+ public boolean isIncludeErrorStackTrace() {
+ return includeErrorStackTrace;
+ }
+
public int getThreadPoolWorkQueueSize() {
return threadPoolWorkQueueSize;
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java
b/server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java
index fdf8ac9115..0430824fd8 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/ObjectMapperProvider.java
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.server.web;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
@@ -28,38 +29,88 @@ import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import javax.ws.rs.ext.ContextResolver;
import javax.ws.rs.ext.Provider;
+import org.apache.gravitino.dto.responses.ErrorResponse;
@Provider
public class ObjectMapperProvider implements ContextResolver<ObjectMapper> {
+ // Keep diagnostic stacks inside the server and accept legacy payloads while
allowing operators
+ // to omit them from responses.
+ @JsonIgnoreProperties(value = "stack", allowSetters = true)
+ private abstract static class ErrorResponseMixin {}
+
private static class ObjectMapperHolder {
- private static final ObjectMapper INSTANCE =
- JsonMapper.builder()
- .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false)
- .configure(EnumFeature.WRITE_ENUMS_TO_LOWERCASE, true)
- .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)
- .build()
- .setSerializationInclusion(JsonInclude.Include.NON_NULL)
- .registerModule(new JavaTimeModule())
- .registerModule(new Jdk8Module());
+ private static final ObjectMapper WITHOUT_ERROR_STACK_TRACE =
createObjectMapper(false);
+ private static final ObjectMapper WITH_ERROR_STACK_TRACE =
createObjectMapper(true);
+ }
+
+ private final ObjectMapper objectMapper;
+
+ /**
+ * Creates a provider using the backward-compatible server default, which
includes error stack
+ * traces.
+ */
+ public ObjectMapperProvider() {
+ this.objectMapper = objectMapper();
+ }
+
+ /**
+ * Creates a provider with explicit error stack-trace serialization behavior.
+ *
+ * @param includeErrorStackTrace whether HTTP error responses should include
diagnostic stack
+ * traces
+ */
+ public ObjectMapperProvider(boolean includeErrorStackTrace) {
+ this.objectMapper = objectMapper(includeErrorStackTrace);
}
/**
- * Retrieves a globally shared {@link ObjectMapper} instance.
+ * Retrieves the shared {@link ObjectMapper} using the backward-compatible
server default.
*
- * <p>Note: This ObjectMapper is a global single instance. If you need to
modify the default
- * serialization/deserialization settings, make changes within the INSTANCE
builder directly.
- * Avoid modifying properties of the returned {@code ObjectMapper} instance
to prevent unintended
- * side effects.
+ * <p>Do not modify the returned mapper. Use {@link #objectMapper(boolean)}
to select explicit
+ * error stack-trace behavior.
*
* @return the globally shared {@link ObjectMapper} instance
*/
public static ObjectMapper objectMapper() {
- return ObjectMapperHolder.INSTANCE;
+ return
objectMapper(JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getDefaultValue());
}
@Override
public ObjectMapper getContext(Class<?> type) {
- return ObjectMapperHolder.INSTANCE;
+ return objectMapper;
+ }
+
+ /**
+ * Retrieves a shared, preconfigured mapper with explicit error stack-trace
serialization
+ * behavior.
+ *
+ * <p>Do not modify the returned mapper.
+ *
+ * @param includeErrorStackTrace whether HTTP error responses should include
diagnostic stack
+ * traces
+ * @return a shared {@link ObjectMapper} with the requested behavior
+ */
+ public static ObjectMapper objectMapper(boolean includeErrorStackTrace) {
+ return includeErrorStackTrace
+ ? ObjectMapperHolder.WITH_ERROR_STACK_TRACE
+ : ObjectMapperHolder.WITHOUT_ERROR_STACK_TRACE;
+ }
+
+ private static ObjectMapper createObjectMapper(boolean
includeErrorStackTrace) {
+ JsonMapper.Builder builder =
+ JsonMapper.builder()
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false)
+ .configure(EnumFeature.WRITE_ENUMS_TO_LOWERCASE, true)
+ .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS);
+ if (!includeErrorStackTrace) {
+ builder.addMixIn(ErrorResponse.class, ErrorResponseMixin.class);
+ }
+
+ return builder
+ .build()
+ .setSerializationInclusion(JsonInclude.Include.NON_NULL)
+ .registerModule(new JavaTimeModule())
+ .registerModule(new Jdk8Module());
}
}
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 8af1b58fed..7aaa9fcc99 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
@@ -22,6 +22,7 @@ package org.apache.gravitino.server.authentication;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@@ -35,7 +36,11 @@ import java.io.StringWriter;
import java.security.Principal;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import java.util.Vector;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
@@ -49,6 +54,14 @@ 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;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.AbstractConfiguration;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.apache.logging.log4j.core.layout.PatternLayout;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -149,7 +162,8 @@ public class TestAuthenticationFilter {
@Test
public void testDoFilterWithException() throws ServletException, IOException
{
Authenticator authenticator = mock(Authenticator.class);
- AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList(authenticator));
+ AuthenticationFilter filter =
+ new AuthenticationFilter(Lists.newArrayList(authenticator), false);
FilterChain mockChain = mock(FilterChain.class);
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
HttpServletResponse mockResponse = mock(HttpServletResponse.class);
@@ -169,13 +183,29 @@ public class TestAuthenticationFilter {
printWriter.flush();
String json = stringWriter.toString();
- ObjectMapper mapper = ObjectMapperProvider.objectMapper();
+ ObjectMapper mapper = ObjectMapperProvider.objectMapper(false);
+ Assertions.assertFalse(mapper.readTree(json).has("stack"));
ErrorResponse errorResponse = mapper.readValue(json, ErrorResponse.class);
Assertions.assertEquals(1011, errorResponse.getCode());
Assertions.assertEquals("UnauthorizedException", errorResponse.getType());
Assertions.assertEquals("UNAUTHORIZED", errorResponse.getMessage());
}
+ @Test
+ public void testAuthErrorIncludesStackWhenEnabled() throws Exception {
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList(), true);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(response, new
UnauthorizedException("UNAUTHORIZED"));
+
+ printWriter.flush();
+ Assertions.assertTrue(
+
ObjectMapperProvider.objectMapper(true).readTree(stringWriter.toString()).has("stack"));
+ }
+
@Test
public void testMultiFilterNormal() throws ServletException, IOException {
@@ -372,4 +402,257 @@ public class TestAuthenticationFilter {
Assertions.assertEquals("RuntimeException", errorResponse.getType());
Assertions.assertEquals("Something went wrong",
errorResponse.getMessage());
}
+
+ @Test
+ public void testUnexpectedAuthenticationErrorIsLogged() throws Exception {
+ RuntimeException failure = new RuntimeException("authenticator bug");
+ Authenticator authenticator = mock(Authenticator.class);
+ when(authenticator.supportsToken(any())).thenReturn(true);
+ when(authenticator.isDataFromToken()).thenReturn(true);
+ when(authenticator.authenticateToken(any())).thenThrow(failure);
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList(authenticator));
+ FilterChain mockChain = mock(FilterChain.class);
+ HttpServletRequest mockRequest = requestWithAuthorizationHeader();
+ HttpServletResponse mockResponse = responseWithWriter();
+
+ List<LogEvent> errors =
+ captureErrorLogs(() -> filter.doFilter(mockRequest, mockResponse,
mockChain));
+
+
verify(mockResponse).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ verify(mockChain, never()).doFilter(any(), any());
+ Assertions.assertEquals(1, errors.size());
+ Assertions.assertSame(failure, errors.get(0).getThrown());
+ }
+
+ @Test
+ public void testClientAuthenticationErrorsAreNotLogged() throws Exception {
+ Authenticator rejectingAuthenticator = mock(Authenticator.class);
+ when(rejectingAuthenticator.supportsToken(any())).thenReturn(true);
+ when(rejectingAuthenticator.isDataFromToken()).thenReturn(true);
+ when(rejectingAuthenticator.authenticateToken(any()))
+ .thenThrow(new UnauthorizedException("UNAUTHORIZED"));
+ AuthenticationFilter rejectingFilter =
+ new AuthenticationFilter(Lists.newArrayList(rejectingAuthenticator));
+ HttpServletResponse unauthorizedResponse = responseWithWriter();
+
+ Authenticator acceptingAuthenticator = mock(Authenticator.class);
+ when(acceptingAuthenticator.supportsToken(any())).thenReturn(true);
+ when(acceptingAuthenticator.isDataFromToken()).thenReturn(true);
+ when(acceptingAuthenticator.authenticateToken(any())).thenReturn(new
UserPrincipal("user"));
+ AuthenticationFilter acceptingFilter =
+ new AuthenticationFilter(Lists.newArrayList(acceptingAuthenticator));
+ HttpServletRequest malformedRolesRequest =
requestWithAuthorizationHeader();
+
when(malformedRolesRequest.getHeader(AuthConstants.X_GRAVITINO_ACTIVE_ROLES_HEADER))
+ .thenReturn("ALL,analyst");
+ HttpServletResponse badRequestResponse = responseWithWriter();
+
+ Authenticator forbiddingAuthenticator = mock(Authenticator.class);
+ when(forbiddingAuthenticator.supportsToken(any())).thenReturn(true);
+ when(forbiddingAuthenticator.isDataFromToken()).thenReturn(true);
+ when(forbiddingAuthenticator.authenticateToken(any()))
+ .thenThrow(new ForbiddenException("Access denied"));
+ AuthenticationFilter forbiddingFilter =
+ new AuthenticationFilter(Lists.newArrayList(forbiddingAuthenticator));
+ HttpServletResponse forbiddenResponse = responseWithWriter();
+
+ List<LogEvent> errors =
+ captureErrorLogs(
+ () -> {
+ rejectingFilter.doFilter(
+ requestWithAuthorizationHeader(), unauthorizedResponse,
mock(FilterChain.class));
+ acceptingFilter.doFilter(
+ malformedRolesRequest, badRequestResponse,
mock(FilterChain.class));
+ forbiddingFilter.doFilter(
+ requestWithAuthorizationHeader(), forbiddenResponse,
mock(FilterChain.class));
+ });
+
+
verify(unauthorizedResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(badRequestResponse).setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ verify(forbiddenResponse).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ Assertions.assertTrue(errors.isEmpty());
+ }
+
+ @Test
+ public void testDownstreamCheckedFailureIsLoggedOnce() throws Exception {
+ ServletException failure = new ServletException("downstream failure");
+ FilterChain failingChain =
+ (req, resp) -> {
+ throw failure;
+ };
+ HttpServletResponse mockResponse = responseWithWriter();
+
+ List<LogEvent> errors =
+ captureErrorLogs(
+ () ->
+ acceptingFilter()
+ .doFilter(requestWithAuthorizationHeader(), mockResponse,
failingChain));
+
+
verify(mockResponse).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ // PrincipalUtils.doAs logs checked failures, so the filter must not log
them again.
+ Assertions.assertEquals(1, errors.size());
+ Assertions.assertEquals(PrincipalUtils.class.getName(),
errors.get(0).getLoggerName());
+ }
+
+ @Test
+ public void testDownstreamUncheckedFailureIsLoggedOnce() throws Exception {
+ IllegalStateException failure = new IllegalStateException("downstream
bug");
+ FilterChain failingChain =
+ (req, resp) -> {
+ throw failure;
+ };
+ HttpServletResponse mockResponse = responseWithWriter();
+
+ List<LogEvent> errors =
+ captureErrorLogs(
+ () ->
+ acceptingFilter()
+ .doFilter(requestWithAuthorizationHeader(), mockResponse,
failingChain));
+
+
verify(mockResponse).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ Assertions.assertEquals(1, errors.size());
+ Assertions.assertSame(failure, errors.get(0).getThrown());
+ }
+
+ @Test
+ public void testDownstreamForbiddenIsNotLogged() throws Exception {
+ FilterChain forbiddingChain =
+ (req, resp) -> {
+ throw new ForbiddenException("Access denied");
+ };
+ HttpServletResponse mockResponse = responseWithWriter();
+
+ List<LogEvent> errors =
+ captureErrorLogs(
+ () ->
+ acceptingFilter()
+ .doFilter(requestWithAuthorizationHeader(), mockResponse,
forbiddingChain));
+
+ verify(mockResponse).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ Assertions.assertTrue(errors.isEmpty());
+ }
+
+ @Test
+ public void testUnauthorizedResponseSetsOnlyNonBasicChallenges() throws
Exception {
+ Authenticator negotiateAuthenticator = mock(Authenticator.class);
+ when(negotiateAuthenticator.supportsToken(any())).thenReturn(true);
+ when(negotiateAuthenticator.isDataFromToken()).thenReturn(true);
+ when(negotiateAuthenticator.authenticateToken(any()))
+ .thenThrow(new UnauthorizedException("Blank token found",
AuthConstants.NEGOTIATE));
+ HttpServletResponse negotiateResponse = responseWithWriter();
+
+ Authenticator basicAuthenticator = mock(Authenticator.class);
+ when(basicAuthenticator.supportsToken(any())).thenReturn(true);
+ when(basicAuthenticator.isDataFromToken()).thenReturn(true);
+ when(basicAuthenticator.authenticateToken(any()))
+ .thenThrow(new UnauthorizedException("Bad credentials", "Basic
realm=\"gravitino\""));
+ HttpServletResponse basicResponse = responseWithWriter();
+
+ new AuthenticationFilter(Lists.newArrayList(negotiateAuthenticator))
+ .doFilter(requestWithAuthorizationHeader(), negotiateResponse,
mock(FilterChain.class));
+ new AuthenticationFilter(Lists.newArrayList(basicAuthenticator))
+ .doFilter(requestWithAuthorizationHeader(), basicResponse,
mock(FilterChain.class));
+
+ verify(negotiateResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(negotiateResponse)
+ .setHeader(AuthConstants.HTTP_CHALLENGE_HEADER,
AuthConstants.NEGOTIATE);
+ verify(basicResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(basicResponse,
never()).setHeader(eq(AuthConstants.HTTP_CHALLENGE_HEADER), anyString());
+ }
+
+ @Test
+ public void testDownstreamUnauthorizedReturns401WithChallenge() throws
Exception {
+ Authenticator authenticator = mock(Authenticator.class);
+ when(authenticator.supportsToken(any())).thenReturn(true);
+ when(authenticator.isDataFromToken()).thenReturn(true);
+ when(authenticator.authenticateToken(any())).thenReturn(new
UserPrincipal("user"));
+ AuthenticationFilter filter = new
AuthenticationFilter(Lists.newArrayList(authenticator));
+ HttpServletRequest mockRequest = requestWithAuthorizationHeader();
+ HttpServletResponse mockResponse = responseWithWriter();
+ FilterChain rejectingChain =
+ (req, resp) -> {
+ throw new UnauthorizedException("Token expired downstream",
AuthConstants.NEGOTIATE);
+ };
+
+ List<LogEvent> errors =
+ captureErrorLogs(() -> filter.doFilter(mockRequest, mockResponse,
rejectingChain));
+
+ verify(mockRequest)
+
.setAttribute(eq(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME), any());
+ verify(mockResponse).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(mockResponse).setHeader(AuthConstants.HTTP_CHALLENGE_HEADER,
AuthConstants.NEGOTIATE);
+ Assertions.assertTrue(errors.isEmpty());
+ }
+
+ private static AuthenticationFilter acceptingFilter() {
+ Authenticator authenticator = mock(Authenticator.class);
+ when(authenticator.supportsToken(any())).thenReturn(true);
+ when(authenticator.isDataFromToken()).thenReturn(true);
+ when(authenticator.authenticateToken(any())).thenReturn(new
UserPrincipal("user"));
+ return new AuthenticationFilter(Lists.newArrayList(authenticator));
+ }
+
+ private static HttpServletRequest requestWithAuthorizationHeader() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION))
+ .thenReturn(new
Vector<>(Collections.singletonList("user")).elements());
+ return request;
+ }
+
+ private static HttpServletResponse responseWithWriter() throws IOException {
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ when(response.getWriter()).thenReturn(new PrintWriter(new StringWriter()));
+ return response;
+ }
+
+ private interface FilterInvocation {
+ void run() throws Exception;
+ }
+
+ /**
+ * Runs the invocation and returns the ERROR events logged by {@link
AuthenticationFilter} and
+ * {@link PrincipalUtils}, the two places a failure on the authentication
path can be logged.
+ */
+ private static List<LogEvent> captureErrorLogs(FilterInvocation invocation)
throws Exception {
+ List<String> loggerNames =
+ Arrays.asList(AuthenticationFilter.class.getName(),
PrincipalUtils.class.getName());
+ LoggerContext loggerContext =
+ (LoggerContext)
LogManager.getContext(AuthenticationFilter.class.getClassLoader(), false);
+ AbstractConfiguration configuration = (AbstractConfiguration)
loggerContext.getConfiguration();
+ Map<String, LoggerConfig> previousLoggerConfigs = new HashMap<>();
+ for (String loggerName : loggerNames) {
+ previousLoggerConfigs.put(loggerName,
configuration.getLoggers().get(loggerName));
+ }
+ List<LogEvent> events = new CopyOnWriteArrayList<>();
+ AbstractAppender appender =
+ new AbstractAppender(
+ "authenticationFilterCapture", null,
PatternLayout.createDefaultLayout(), true, null) {
+ @Override
+ public void append(LogEvent event) {
+ events.add(event.toImmutable());
+ }
+ };
+ try {
+ appender.start();
+ configuration.addAppender(appender);
+ for (String loggerName : loggerNames) {
+ LoggerConfig loggerConfig = new LoggerConfig(loggerName, Level.ERROR,
false);
+ loggerConfig.addAppender(appender, Level.ERROR, null);
+ configuration.addLogger(loggerName, loggerConfig);
+ }
+ loggerContext.updateLoggers();
+ invocation.run();
+ } finally {
+ for (String loggerName : loggerNames) {
+ configuration.removeLogger(loggerName);
+ LoggerConfig previousLoggerConfig =
previousLoggerConfigs.get(loggerName);
+ if (previousLoggerConfig != null) {
+ configuration.addLogger(loggerName, previousLoggerConfig);
+ }
+ }
+ configuration.removeAppender(appender.getName());
+ appender.stop();
+ loggerContext.updateLoggers();
+ }
+ return events;
+ }
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationOutOfMemoryHttp.java
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationOutOfMemoryHttp.java
index af300d029f..87b99cc44e 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationOutOfMemoryHttp.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authentication/TestAuthenticationOutOfMemoryHttp.java
@@ -79,7 +79,7 @@ class TestAuthenticationOutOfMemoryHttp {
new JettyServer() {
/** {@inheritDoc} */
@Override
- protected Filter createAuthenticationFilter() {
+ protected Filter createAuthenticationFilter(boolean
includeErrorStackTrace) {
return new
AuthenticationFilter(Collections.singletonList(authenticator)) {
/** {@inheritDoc} */
@Override
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
index 21b0bfc58d..909666c523 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
@@ -19,14 +19,25 @@
package org.apache.gravitino.server.web;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
+import com.google.common.io.CharStreams;
import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
import javax.servlet.Filter;
import javax.servlet.Servlet;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
import org.apache.gravitino.Config;
import org.apache.gravitino.rest.RESTUtils;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
@@ -37,6 +48,10 @@ import org.mockito.MockedStatic;
public class TestJettyServer {
+ // The error page always names the servlet, so match a stack frame rather
than the class name.
+ private static final String FAILING_SERVLET_STACK_FRAME =
+ FailingServlet.class.getName() + ".doGet(";
+
private JettyServer jettyServer;
@BeforeEach
@@ -90,6 +105,23 @@ public class TestJettyServer {
jettyServer.stop();
}
+ @Test
+ public void testErrorPageIncludesStackTraceByDefault() throws IOException {
+ String errorPage = requestFailingServlet(new Config(false) {});
+
+ assertTrue(errorPage.contains(FAILING_SERVLET_STACK_FRAME), errorPage);
+ }
+
+ @Test
+ public void testErrorPageOmitsStackTraceWhenDisabled() throws IOException {
+ Config config = new Config(false) {};
+ config.set(JettyServerConfig.INCLUDE_ERROR_STACK_TRACE, false);
+
+ String errorPage = requestFailingServlet(config);
+
+ assertFalse(errorPage.contains(FAILING_SERVLET_STACK_FRAME), errorPage);
+ }
+
@Test
public void testStopWithNullServer() {
assertDoesNotThrow(() -> jettyServer.stop());
@@ -114,4 +146,33 @@ public class TestJettyServer {
assertTrue(health.hasOutOfMemoryError());
}
}
+
+ /** Starts the server with a servlet that throws, and returns Jetty's error
page for it. */
+ private String requestFailingServlet(Config config) throws IOException {
+ int port = RESTUtils.findAvailablePort(5000, 6000);
+ config.set(JettyServerConfig.WEBSERVER_HOST, "127.0.0.1");
+ config.set(JettyServerConfig.WEBSERVER_HTTP_PORT, port);
+ jettyServer.initialize(JettyServerConfig.fromConfig(config), "test",
false);
+ jettyServer.addServlet(new FailingServlet(), "/fail");
+ jettyServer.start();
+
+ HttpURLConnection connection =
+ (HttpURLConnection) new URL("http://127.0.0.1:"; + port +
"/fail").openConnection();
+ try {
+ assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
connection.getResponseCode());
+ try (Reader errorBody =
+ new InputStreamReader(connection.getErrorStream(),
StandardCharsets.UTF_8)) {
+ return CharStreams.toString(errorBody);
+ }
+ } finally {
+ connection.disconnect();
+ }
+ }
+
+ private static class FailingServlet extends HttpServlet {
+ @Override
+ protected void doGet(HttpServletRequest request, HttpServletResponse
response) {
+ throw new IllegalStateException("servlet failure");
+ }
+ }
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
index 616d932191..5c33f9d486 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
@@ -31,6 +31,17 @@ import org.junit.jupiter.api.Test;
public class TestJettyServerConfig {
+ @Test
+ public void testIncludeErrorStackTrace() {
+ Config config = new Config() {};
+ JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(config,
"");
+ Assertions.assertTrue(jettyServerConfig.isIncludeErrorStackTrace());
+
+ config.set(JettyServerConfig.INCLUDE_ERROR_STACK_TRACE, false);
+ jettyServerConfig = JettyServerConfig.fromConfig(config, "");
+ Assertions.assertFalse(jettyServerConfig.isIncludeErrorStackTrace());
+ }
+
@Test
public void testCipherAlgorithms() {
Config noIntersectConfig = new Config() {};
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index 0fb4b007b6..8b9a21844a 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -140,14 +140,14 @@ public class GravitinoServer extends ResourceConfig {
new
LineageConfig(serverConfig.getConfigsWithPrefix(LineageConfig.LINEAGE_CONFIG_PREFIX)));
// initialize Jersey REST API resources.
- initializeRestApi();
+ initializeRestApi(jettyServerConfig);
}
public ServerConfig serverConfig() {
return serverConfig;
}
- private void initializeRestApi() {
+ private void initializeRestApi(JettyServerConfig jettyServerConfig) {
HashSet<String> restApiPackagesSet = new HashSet<>();
restApiPackagesSet.add("org.apache.gravitino.server.web.rest");
restApiPackagesSet.addAll(serverConfig.get(Configs.REST_API_EXTENSION_PACKAGES));
@@ -196,7 +196,8 @@ public class GravitinoServer extends ResourceConfig {
register(ParamExceptionMapper.class);
register(NotFoundExceptionMapper.class);
register(WebApplicationExceptionMapper.class);
- register(ObjectMapperProvider.class).register(JacksonFeature.class);
+ register(new
ObjectMapperProvider(jettyServerConfig.isIncludeErrorStackTrace()))
+ .register(JacksonFeature.class);
property(CommonProperties.JSON_JACKSON_DISABLED_MODULES,
"DefaultScalaModule");
if (!enableAuthorization) {
@@ -227,7 +228,8 @@ public class GravitinoServer extends ResourceConfig {
server.addFilter(new RequestContextFilter(gravitinoEnv.eventBus()),
API_ANY_PATH);
server.addFilter(
new HttpAuditFilter(gravitinoEnv.eventBus(),
EventSource.GRAVITINO_SERVER), API_ANY_PATH);
- server.addFilter(new VersioningFilter(), API_ANY_PATH);
+ server.addFilter(
+ new VersioningFilter(jettyServerConfig.isIncludeErrorStackTrace()),
API_ANY_PATH);
// GH-12760: servlets mounted outside API_ANY_PATH used to receive none of
the filters below
// (no request-context tracking, no audit-on-failure, no custom filters),
with nothing in the
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/VersioningFilter.java
b/server/src/main/java/org/apache/gravitino/server/web/VersioningFilter.java
index 26fc3271c0..f6b8977838 100644
--- a/server/src/main/java/org/apache/gravitino/server/web/VersioningFilter.java
+++ b/server/src/main/java/org/apache/gravitino/server/web/VersioningFilter.java
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.server.web;
+import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
@@ -94,6 +95,22 @@ public class VersioningFilter implements Filter {
private static final String ACCEPT_VERSION_HEADER = "Accept";
private static final String CONTENT_TYPE_HEADER = "Content-Type";
+ private final ObjectMapper objectMapper;
+
+ /** Creates a versioning filter with the backward-compatible error response
behavior. */
+ public VersioningFilter() {
+ this.objectMapper = ObjectMapperProvider.objectMapper();
+ }
+
+ /**
+ * Creates a versioning filter with explicit error stack-trace response
behavior.
+ *
+ * @param includeErrorStackTrace whether error responses should include
diagnostic stack traces
+ */
+ public VersioningFilter(boolean includeErrorStackTrace) {
+ this.objectMapper =
ObjectMapperProvider.objectMapper(includeErrorStackTrace);
+ }
+
private static String getAcceptVersion(int version) {
return String.format("application/vnd.gravitino.v%d+json", version);
}
@@ -151,8 +168,7 @@ public class VersioningFilter implements Filter {
return matcher.find() ? Integer.parseInt(matcher.group(1)) : null;
}
- private static boolean isUnsupportedVersion(int version, ServletResponse
response)
- throws IOException {
+ private boolean isUnsupportedVersion(int version, ServletResponse response)
throws IOException {
if (ApiVersion.isSupportedVersion(version)) {
return false;
}
@@ -168,7 +184,7 @@ public class VersioningFilter implements Filter {
resp.setStatus(HttpServletResponse.SC_NOT_ACCEPTABLE);
resp.setContentType("application/json");
resp.setCharacterEncoding(StandardCharsets.UTF_8.name());
- ObjectMapperProvider.objectMapper().writeValue(resp.getWriter(),
errorResponse);
+ objectMapper.writeValue(resp.getWriter(), errorResponse);
return true;
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
index af16c2efd1..1cd0306af4 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java
@@ -323,7 +323,8 @@ public class GravitinoInterceptionService implements
InterceptionService {
"User validation failed - User: {}, Metalake: {}, Reason: {}",
currentUser,
metalakeIdent.name(),
- ex.getMessage());
+ ex.getMessage(),
+ ex);
dispatchAuthzDenialEvent(currentUser, metalakeIdent, method.getName(),
expression);
return Optional.of(Utils.forbidden(ex.getMessage(), ex));
} catch (Exception ex) {
diff --git
a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
index 2ad3f616ef..1288446005 100644
--- a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
+++ b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
@@ -25,6 +25,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import java.io.IOException;
@@ -235,6 +236,42 @@ public class TestGravitinoServer {
assertEquals(404, response.statusCode());
}
+ @Test
+ public void testJerseyErrorOmitsStackWhenDisabled() throws Exception {
+ Map<String, String> configs = new HashMap<>();
+ configs.put(
+ GravitinoServer.WEBSERVER_CONF_PREFIX +
JettyServerConfig.WEBSERVER_HTTP_PORT.getKey(),
+ String.valueOf(RESTUtils.findAvailablePort(5000, 6000)));
+ configs.put(
+ GravitinoServer.WEBSERVER_CONF_PREFIX
+ + JettyServerConfig.INCLUDE_ERROR_STACK_TRACE.getKey(),
+ "false");
+ ServerConfig serverConfig = new ServerConfig();
+ serverConfig.loadFromMap(configs, key -> true);
+ serverConfig = spyServerConfig(serverConfig);
+ gravitinoServer = new GravitinoServer(serverConfig,
GravitinoEnv.getInstance());
+ gravitinoServer.initialize();
+ gravitinoServer.start();
+
+ int port =
+ JettyServerConfig.fromConfig(serverConfig,
GravitinoServer.WEBSERVER_CONF_PREFIX)
+ .getHttpPort();
+ HttpResponse<String> response =
+ HttpClient.newHttpClient()
+ .send(
+ HttpRequest.newBuilder(URI.create("http://127.0.0.1:"; + port +
"/api/metalakes"))
+ .header("Accept", "application/vnd.gravitino.v1+json")
+ .header("Content-Type", "application/json")
+ .POST(HttpRequest.BodyPublishers.ofString("{"))
+ .build(),
+ HttpResponse.BodyHandlers.ofString());
+
+ assertEquals(400, response.statusCode(), response.body());
+ JsonNode responseJson =
ObjectMapperProvider.objectMapper(false).readTree(response.body());
+ assertTrue(responseJson.hasNonNull("message"));
+ assertFalse(responseJson.has("stack"));
+ }
+
@Test
public void testSecretProvidersRequestIsAudited() throws Exception {
// GH-12921 moved discovery under
/api/metalakes/{metalake}/secrets/providers so it inherits
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/TestObjectMapperProvider.java
b/server/src/test/java/org/apache/gravitino/server/web/TestObjectMapperProvider.java
index 935f59f9f8..9ee9a12406 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/TestObjectMapperProvider.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/TestObjectMapperProvider.java
@@ -19,10 +19,15 @@
package org.apache.gravitino.server.web;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.gravitino.dto.responses.ErrorResponse;
import org.junit.jupiter.api.Test;
public class TestObjectMapperProvider {
@@ -39,4 +44,48 @@ public class TestObjectMapperProvider {
JsonInclude.Include.NON_NULL,
objectMapper.getSerializationConfig().getDefaultPropertyInclusion().getValueInclusion());
}
+
+ @Test
+ public void testErrorResponseStackIsRedactedOnSerialization() throws
JsonProcessingException {
+ ObjectMapper objectMapper = new
ObjectMapperProvider(false).getContext(ErrorResponse.class);
+ ErrorResponse errorResponse =
+ ErrorResponse.internalError(
+ "public error message", new RuntimeException("private error
details"));
+
+ assertNotNull(errorResponse.getStack());
+ assertTrue(
+ errorResponse.getStack().stream().anyMatch(line ->
line.contains("private error details")));
+
+ JsonNode responseJson =
objectMapper.readTree(objectMapper.writeValueAsString(errorResponse));
+ assertEquals("public error message", responseJson.get("message").asText());
+ assertFalse(responseJson.has("stack"));
+ }
+
+ @Test
+ public void testErrorResponseStackCanBeIncludedOnSerialization() throws
JsonProcessingException {
+ ObjectMapper objectMapper = new
ObjectMapperProvider().getContext(ErrorResponse.class);
+ ErrorResponse errorResponse =
+ ErrorResponse.internalError(
+ "public error message", new RuntimeException("private error
details"));
+
+ JsonNode responseJson =
objectMapper.readTree(objectMapper.writeValueAsString(errorResponse));
+ assertTrue(responseJson.has("stack"));
+ assertTrue(responseJson.get("stack").get(0).asText().contains("private
error details"));
+ }
+
+ @Test
+ public void testErrorResponseStackIsAcceptedOnDeserialization() throws
JsonProcessingException {
+ ObjectMapper serverObjectMapper = ObjectMapperProvider.objectMapper(false);
+ ObjectMapper clientObjectMapper = new ObjectMapper();
+ ErrorResponse errorResponse =
+ ErrorResponse.internalError(
+ "public error message", new RuntimeException("private error
details"));
+ String legacyResponseJson =
clientObjectMapper.writeValueAsString(errorResponse);
+
+ assertTrue(clientObjectMapper.readTree(legacyResponseJson).has("stack"));
+
+ ErrorResponse deserialized =
+ serverObjectMapper.readValue(legacyResponseJson, ErrorResponse.class);
+ assertEquals(errorResponse.getStack(), deserialized.getStack());
+ }
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
index 572c492d68..35efcd1cfb 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java
@@ -36,6 +36,7 @@ import java.lang.reflect.Method;
import java.security.Principal;
import java.util.Collections;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.core.Response;
import org.aopalliance.intercept.MethodInterceptor;
@@ -81,6 +82,14 @@ import org.apache.gravitino.server.web.rest.ViewOperations;
import org.apache.gravitino.tag.TagDispatcher;
import org.apache.gravitino.utils.PrincipalUtils;
import org.apache.gravitino.utils.RequestContext;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.AbstractConfiguration;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.apache.logging.log4j.core.layout.PatternLayout;
import org.glassfish.hk2.api.Descriptor;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
@@ -719,55 +728,84 @@ public class TestGravitinoInterceptionService {
/**
* When {@code checkCurrentUser} throws {@link ForbiddenException} (user is
not a metalake
* member), the interceptor must dispatch an {@link
AuthorizationDenialFailureEvent} and set
- * {@code operationFailureFired}.
+ * {@code operationFailureFired}, while logging the original exception for
server-side diagnosis.
*/
@Test
- public void testForbiddenExceptionDispatchesEventAndSetsFlag() throws
Throwable {
- try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
- MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
- mockStatic(GravitinoAuthorizerProvider.class);
- MockedStatic<AuthorizationUtils> authUtilsMocked =
mockStatic(AuthorizationUtils.class);
- MockedStatic<GravitinoEnv> envMocked = mockStatic(GravitinoEnv.class))
{
-
- principalUtilsMocked
- .when(PrincipalUtils::getCurrentPrincipal)
- .thenReturn(new UserPrincipal("outsider"));
-
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("outsider");
-
- GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
-
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
- when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
-
- authUtilsMocked
- .when(
- () ->
- AuthorizationUtils.checkCurrentUser(
- ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
- .thenThrow(new ForbiddenException("User outsider is not a member"));
-
- GravitinoEnv mockEnv = mock(GravitinoEnv.class);
- EventBus mockEventBus = spy(new EventBus(Collections.emptyList()));
- envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
- when(mockEnv.eventBus()).thenReturn(mockEventBus);
-
- GravitinoInterceptionService service = new
GravitinoInterceptionService();
- Method testMethod = TestOperations.class.getMethods()[0];
- MethodInterceptor interceptor =
service.getMethodInterceptors(testMethod).get(0);
-
- MethodInvocation invocation = mock(MethodInvocation.class);
- when(invocation.getMethod()).thenReturn(testMethod);
- when(invocation.getArguments()).thenReturn(new Object[]
{"testMetalake"});
-
- Assertions.assertFalse(RequestContext.isOperationFailureFired());
- Response response = (Response) interceptor.invoke(invocation);
-
- assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
- ArgumentCaptor<AuthorizationDenialFailureEvent> captor =
- ArgumentCaptor.forClass(AuthorizationDenialFailureEvent.class);
- verify(mockEventBus).dispatchEvent(captor.capture());
- AuthorizationDenialFailureEvent event = captor.getValue();
- assertEquals("outsider", event.user());
- Assertions.assertTrue(RequestContext.isOperationFailureFired());
+ public void testForbiddenExceptionDispatchesEventSetsFlagAndLogsThrowable()
throws Throwable {
+ String loggerName =
+ GravitinoInterceptionService.class.getName() +
"$MetadataAuthorizationMethodInterceptor";
+ LoggerContext loggerContext =
+ (LoggerContext)
+
LogManager.getContext(GravitinoInterceptionService.class.getClassLoader(),
false);
+ AbstractConfiguration configuration = (AbstractConfiguration)
loggerContext.getConfiguration();
+ LoggerConfig previousLoggerConfig =
configuration.getLoggers().get(loggerName);
+ CaptureAppender captureAppender = new
CaptureAppender("authorizationCapture");
+ ForbiddenException forbiddenException = new ForbiddenException("User
outsider is not a member");
+ try {
+ captureAppender.start();
+ configuration.addAppender(captureAppender);
+ LoggerConfig loggerConfig = new LoggerConfig(loggerName, Level.WARN,
false);
+ loggerConfig.addAppender(captureAppender, Level.WARN, null);
+ configuration.addLogger(loggerName, loggerConfig);
+ loggerContext.updateLoggers();
+
+ try (MockedStatic<PrincipalUtils> principalUtilsMocked =
mockStatic(PrincipalUtils.class);
+ MockedStatic<GravitinoAuthorizerProvider> authorizerMocked =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ MockedStatic<AuthorizationUtils> authUtilsMocked =
mockStatic(AuthorizationUtils.class);
+ MockedStatic<GravitinoEnv> envMocked =
mockStatic(GravitinoEnv.class)) {
+
+ principalUtilsMocked
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(new UserPrincipal("outsider"));
+
principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("outsider");
+
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ authUtilsMocked
+ .when(
+ () ->
+ AuthorizationUtils.checkCurrentUser(
+ ArgumentMatchers.any(), ArgumentMatchers.any(),
ArgumentMatchers.any()))
+ .thenThrow(forbiddenException);
+
+ GravitinoEnv mockEnv = mock(GravitinoEnv.class);
+ EventBus mockEventBus = spy(new EventBus(Collections.emptyList()));
+ envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv);
+ when(mockEnv.eventBus()).thenReturn(mockEventBus);
+
+ GravitinoInterceptionService service = new
GravitinoInterceptionService();
+ Method testMethod = TestOperations.class.getMethods()[0];
+ MethodInterceptor interceptor =
service.getMethodInterceptors(testMethod).get(0);
+
+ MethodInvocation invocation = mock(MethodInvocation.class);
+ when(invocation.getMethod()).thenReturn(testMethod);
+ when(invocation.getArguments()).thenReturn(new Object[]
{"testMetalake"});
+
+ Assertions.assertFalse(RequestContext.isOperationFailureFired());
+ Response response = (Response) interceptor.invoke(invocation);
+
+ assertEquals(Response.Status.FORBIDDEN.getStatusCode(),
response.getStatus());
+ ArgumentCaptor<AuthorizationDenialFailureEvent> captor =
+ ArgumentCaptor.forClass(AuthorizationDenialFailureEvent.class);
+ verify(mockEventBus).dispatchEvent(captor.capture());
+ AuthorizationDenialFailureEvent event = captor.getValue();
+ assertEquals("outsider", event.user());
+ Assertions.assertTrue(RequestContext.isOperationFailureFired());
+ Assertions.assertTrue(
+ captureAppender.getEvents().stream()
+ .anyMatch(logEvent -> logEvent.getThrown() ==
forbiddenException));
+ }
+ } finally {
+ configuration.removeLogger(loggerName);
+ if (previousLoggerConfig != null) {
+ configuration.addLogger(loggerName, previousLoggerConfig);
+ }
+ configuration.removeAppender(captureAppender.getName());
+ captureAppender.stop();
+ loggerContext.updateLoggers();
}
}
@@ -1228,4 +1266,21 @@ public class TestGravitinoInterceptionService {
@Override
public void close() throws IOException {}
}
+
+ private static class CaptureAppender extends AbstractAppender {
+ private final List<LogEvent> events = new CopyOnWriteArrayList<>();
+
+ CaptureAppender(String name) {
+ super(name, null, PatternLayout.createDefaultLayout(), true, null);
+ }
+
+ @Override
+ public void append(LogEvent event) {
+ events.add(event.toImmutable());
+ }
+
+ List<LogEvent> getEvents() {
+ return events;
+ }
+ }
}
