This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-77eae699-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 4235fe567e30d0fc708f4f46d80d8be17fbf9f64 Author: Yuhui <[email protected]> AuthorDate: Sat Sep 12 02:08:23 2026 +0800 [#13096] fix(trino-connector): Reuse service metadata when the user token is missing (#13097) ### What changes were proposed in this pull request? For OAuth2 user forwarding, reuse the configured service metadata when the session's token credential is absent or blank. Keep user forwarding for supplied tokens and preserve SIMPLE username forwarding. Add regression tests and document the fallback behavior. ### Why are the changes needed? Password-authenticated sessions, including internal JDBC catalog-management sessions, do not carry an OAuth2 token. They currently fail when Trino invokes the connector's metadata entry point despite having a configured service identity. Fix: #13096 ### Does this PR introduce _any_ user-facing change? Yes. With OAuth2 and `gravitino.client.session.forwardUser=true`, sessions without a token access Gravitino metadata under the configured service identity and its permissions. Failures using supplied tokens still propagate. No configuration keys are added. Downstream catalog/IRC authentication is unchanged. ### How was this patch tested? Added regression tests for missing-token fallback and credential forwarding. Trino connector unit tests and formatting checks passed. --- docs/trino-connector/authentication.md | 34 +++---- .../trino/connector/GravitinoConnector.java | 6 ++ .../TestGravitinoConnectorForwardUser.java | 102 ++++++++++++++++++++- 3 files changed, 125 insertions(+), 17 deletions(-) diff --git a/docs/trino-connector/authentication.md b/docs/trino-connector/authentication.md index e7166b3542..d95c926c1b 100644 --- a/docs/trino-connector/authentication.md +++ b/docs/trino-connector/authentication.md @@ -60,8 +60,8 @@ gravitino.client.basic.password=YourSecureGravitinoPassword | Property | Description | Default value | Required | |-----------------------------------|-----------------------------------------------------------------|---------------|----------------------------| | `gravitino.client.authType` | Authentication type: `simple`, `basic`, `oauth2`, or `kerberos` | (none) | Yes (to enable Basic) | -| `gravitino.client.basic.username` | Local user store username | (none) | Yes if authType is `basic` | -| `gravitino.client.basic.password` | Local user store password | (none) | Yes if authType is `basic` | +| `gravitino.client.basic.username` | Local user store username | (none) | Yes if authType is `basic` | +| `gravitino.client.basic.password` | Local user store password | (none) | Yes if authType is `basic` | ### OAuth2 Authentication @@ -154,7 +154,7 @@ gravitino.client.kerberos.keytabFilePath=/path/to/user.keytab ## Session Credential Forwarding -Setting `gravitino.client.session.forwardUser=true` creates a dedicated Gravitino client per Trino session user, so each user is visible in the Gravitino audit log instead of the shared `gravitino.user` or service identity. It is supported with `authType=simple` and `authType=oauth2`. +Setting `gravitino.client.session.forwardUser=true` creates a dedicated Gravitino client per Trino session user, so each user is visible in the Gravitino audit log instead of the shared `gravitino.user` or service identity. It is supported with `authType=simple` and `authType=oauth2`. For OAuth2 sessions without a forwarded token, the connector reuses the shared service metadata instead. **Configuration (`authType=simple`):** @@ -184,7 +184,7 @@ gravitino.client.oauth2.scope=gravitino gravitino.client.session.forwardUser=true ``` -With `authType=oauth2`, the end user's IdP access token is presented to Gravitino directly instead of the shared client-credentials identity. This requires the Trino coordinator to populate the session's extra-credentials with the caller's access token under the key `token`; the connector reads it from there, and `buildForSession` fails with a clear error if it's missing. +With `authType=oauth2`, the end user's IdP access token is presented to Gravitino directly when the Trino coordinator populates the session's extra-credentials with the caller's access token under the key `token` (or the configured `gravitino.client.session.userTokenCredentialKey`). If that credential is absent, empty, or whitespace-only, the connector reuses the shared service metadata. This allows password-authenticated sessions, including internal catalog-management JDBC sessions, to [...] Whether the coordinator can populate this extra-credential depends on the Trino distribution: @@ -215,11 +215,11 @@ which the connector does not re-route), the connector does not set `iceberg.rest **Configuration properties:** -| Property | Description | Default value | Required | Since version | -|--------------------------------------------------------------|--------------------------------------------------------------------------------------------------|-----------------|------------|-----------------| -| `gravitino.client.session.forwardUser` | When `true` with `authType=simple` or `authType=oauth2`, forwards the Trino session user/token to Gravitino per-query | `false` | No | 1.3.0 | -| `gravitino.client.session.cache.maxSize` | Maximum number of per-user sessions to keep in the cache | `500` | No | 1.3.0 | -| `gravitino.client.session.cache.expireAfterAccessSeconds` | Seconds before an idle per-user session is evicted from the cache | `3600` | No | 1.3.0 | +| Property | Description | Default value | Required | Since version | +|-----------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------| +| `gravitino.client.session.forwardUser` | When `true` with `authType=simple` or `authType=oauth2`, forwards the Trino session user/token to Gravitino per-query; OAuth2 sessions without a token use the shared service metadata | `false` | No | 1.3.0 | +| `gravitino.client.session.cache.maxSize` | Maximum number of per-user sessions to keep in the cache | `500` | No | 1.3.0 | +| `gravitino.client.session.cache.expireAfterAccessSeconds` | Seconds before an idle per-user session is evicted from the cache | `3600` | No | 1.3.0 | ### Example: OAuth2 Per-User Token Forwarding @@ -244,8 +244,8 @@ populate the session's extra-credentials with the caller's access token under th - **Open-source Trino**: there is currently no equivalent coordinator setting. Track [trinodb/trino discussion #24403](https://github.com/trinodb/trino/discussions/24403) and [issue #27917](https://github.com/trinodb/trino/issues/27917) for this feature request. Until - it lands upstream, this connector's `authType=oauth2` forwardUser path requires a Trino - distribution that provides this extra-credential itself. + it lands upstream, forwarding OAuth2 user tokens requires a Trino distribution that + provides this extra-credential itself. Sessions without it use the shared service metadata. **2. Gravitino server: enable OAuth2** (in `conf/gravitino.conf`): @@ -273,8 +273,9 @@ gravitino.client.session.forwardUser=true ``` The `gravitino.client.oauth2.*` properties configure the shared service identity used for catalog -discovery; the per-user forwarded token (from step 1) is what each query actually authenticates -with once `forwardUser=true`. +discovery and for metadata access by sessions without a forwarded token. With +`forwardUser=true`, sessions carrying a token from step 1 authenticate metadata requests with +that token instead. **4. Create the metalake and catalog.** Create the metalake `my_metalake` first (via the Gravitino REST API, SDK, or CLI — see @@ -304,9 +305,10 @@ call gravitino.system.create_catalog( ); ``` -This call itself runs with the connector's own shared service identity, not any forwarded user -token — `forwardUser` only affects `SELECT`/`SHOW`-style queries against the catalog afterward, -not catalog registration itself. `create_catalog` both creates the catalog in Gravitino and loads +The procedure uses the connector's shared service client to create the catalog in Gravitino. +Catalog registration can also invoke the connector's metadata entry point; an internal +password-authenticated JDBC session without a forwarded token uses the shared service metadata +there. `create_catalog` both creates the catalog in Gravitino and loads it into Trino as its own top-level catalog — not as a schema nested under a single `gravitino` catalog. If the two `trino.bypass.iceberg.rest-catalog.*` properties above are omitted, the REST catalog keeps its own default security setting, independent of diff --git a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java index 66f7b78036..57dc9913c6 100644 --- a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java +++ b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java @@ -242,6 +242,12 @@ public class GravitinoConnector implements Connector { GravitinoAuthProvider.USER_TOKEN_CREDENTIAL_KEY, GravitinoAuthProvider.DEFAULT_USER_TOKEN_CREDENTIAL_KEY); String token = session.getIdentity().getExtraCredentials().get(credentialKey); + // Password-authenticated sessions have no OAuth token. Reuse the configured service + // identity in that case; failures with a supplied token must still propagate. + if (GravitinoAuthProvider.parseAuthType(authType) == GravitinoAuthProvider.AuthType.OAUTH2 + && StringUtils.isBlank(token)) { + return connectorMetadata; + } String credKey = sessionCacheKey(authType, session.getUser(), token); try { return perUserSessionCache.get( diff --git a/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorForwardUser.java b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorForwardUser.java index 29b91f8ca3..82e73441e2 100644 --- a/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorForwardUser.java +++ b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorForwardUser.java @@ -28,12 +28,18 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; import io.trino.spi.TrinoException; +import io.trino.spi.connector.Connector; +import io.trino.spi.connector.ConnectorMetadata; import io.trino.spi.connector.ConnectorSession; +import io.trino.spi.connector.ConnectorTransactionHandle; import io.trino.spi.security.ConnectorIdentity; +import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import org.apache.gravitino.Catalog; @@ -44,6 +50,7 @@ import org.apache.gravitino.client.GravitinoMetalake; import org.apache.gravitino.rel.TableCatalog; import org.apache.gravitino.trino.connector.catalog.CatalogConnectorContext; import org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadata; +import org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadataAdapter; import org.apache.gravitino.trino.connector.metadata.GravitinoCatalog; import org.apache.gravitino.trino.connector.security.GravitinoAuthProvider; import org.junit.jupiter.api.Test; @@ -51,6 +58,98 @@ import org.junit.jupiter.api.Test; /** Tests for forwardUser startup validation in {@link GravitinoConnector}. */ class TestGravitinoConnectorForwardUser { + @Test + void testPasswordSessionUsesServiceMetadataForCatalogLifecycleAndQueries() { + CatalogConnectorContext ctx = + mockContextWithConfig( + ImmutableMap.of( + GravitinoAuthProvider.FORWARD_SESSION_USER_KEY, "true", + GravitinoAuthProvider.AUTH_TYPE_KEY, "oauth2")); + Connector internal = mock(Connector.class); + ConnectorMetadata internalMetadata = mock(ConnectorMetadata.class); + when(ctx.getInternalConnector()).thenReturn(internal); + when(internal.getMetadata(any(), any())).thenReturn(internalMetadata); + GravitinoConnector connector = + new GravitinoConnector(ctx) { + @Override + protected GravitinoMetadata createGravitinoMetadata( + CatalogConnectorMetadata metadata, + CatalogConnectorMetadataAdapter adapter, + ConnectorMetadata delegate) { + return new GravitinoMetadata(metadata, adapter, delegate) {}; + } + }; + ConnectorSession session = mockSession("gravitino_catalog_manager", ""); + GravitinoTransactionHandle transaction = + new GravitinoTransactionHandle(mock(ConnectorTransactionHandle.class)); + ConnectorMetadata metadata = + assertDoesNotThrow(() -> connector.getMetadata(session, transaction)); + assertDoesNotThrow(() -> metadata.beginQuery(session)); + assertDoesNotThrow(() -> metadata.cleanupQuery(session)); + verify(internalMetadata).beginQuery(session); + verify(internalMetadata).cleanupQuery(session); + SupportsSchemas schemas = ctx.getMetalake().loadCatalog("catalog").asSchemas(); + when(schemas.listSchemas()).thenReturn(new String[] {"test_schema"}); + assertEquals(List.of("test_schema"), metadata.listSchemaNames(session)); + verify(schemas).listSchemas(); + verify(internalMetadata, never()).listSchemaNames(any()); + } + + @Test + void testMissingOAuthTokenReusesServiceMetadataWithoutBuildingUserClient() { + CatalogConnectorContext ctx = + mockContextWithConfig( + ImmutableMap.of( + GravitinoAuthProvider.FORWARD_SESSION_USER_KEY, "true", + GravitinoAuthProvider.AUTH_TYPE_KEY, "oauth2")); + GravitinoConnector connector = + newConnectorWithAuthClient( + ctx, + session -> { + throw new AssertionError("A missing token must not create a user client"); + }); + CatalogConnectorMetadata service = + connector.resolveSessionMetadata(mockSession("manager", null)); + assertSame(service, connector.resolveSessionMetadata(mockSession("alice", ""))); + assertSame(service, connector.resolveSessionMetadata(mockSession("bob", " "))); + } + + @Test + void testCustomTokenKeyControlsForwardingAndFallback() { + CatalogConnectorContext ctx = + mockContextWithConfig( + ImmutableMap.of( + GravitinoAuthProvider.FORWARD_SESSION_USER_KEY, "true", + GravitinoAuthProvider.AUTH_TYPE_KEY, "oauth2", + GravitinoAuthProvider.USER_TOKEN_CREDENTIAL_KEY, "custom-token")); + AtomicInteger count = new AtomicInteger(); + GravitinoConnector connector = + newConnectorWithAuthClient(ctx, session -> mockAdminClient(ctx.getMetalake(), count)); + ConnectorSession session = mockSession("alice", "ignored-default-token"); + CatalogConnectorMetadata service = connector.resolveSessionMetadata(session); + assertEquals(0, count.get()); + when(session.getIdentity().getExtraCredentials()) + .thenReturn(ImmutableMap.of("custom-token", "user-token")); + assertNotSame(service, connector.resolveSessionMetadata(session)); + assertEquals(1, count.get()); + } + + @Test + void testSimpleAuthStillForwardsUsernameWithoutToken() { + CatalogConnectorContext ctx = + mockContextWithConfig( + ImmutableMap.of( + GravitinoAuthProvider.FORWARD_SESSION_USER_KEY, "true", + GravitinoAuthProvider.AUTH_TYPE_KEY, "simple")); + AtomicInteger count = new AtomicInteger(); + GravitinoConnector connector = + newConnectorWithAuthClient(ctx, session -> mockAdminClient(ctx.getMetalake(), count)); + assertNotSame( + connector.resolveSessionMetadata(mockSession("alice", null)), + connector.resolveSessionMetadata(mockSession("bob", null))); + assertEquals(2, count.get()); + } + @Test void testForwardUserWithoutAuthTypeThrowsAtConstruction() { CatalogConnectorContext ctx = @@ -249,7 +348,8 @@ class TestGravitinoConnectorForwardUser { private static ConnectorSession mockSession(String user, String token) { ConnectorIdentity identity = mock(ConnectorIdentity.class); - when(identity.getExtraCredentials()).thenReturn(ImmutableMap.of("token", token)); + when(identity.getExtraCredentials()) + .thenReturn(token == null ? ImmutableMap.of() : ImmutableMap.of("token", token)); ConnectorSession session = mock(ConnectorSession.class); when(session.getUser()).thenReturn(user); when(session.getIdentity()).thenReturn(identity);
