Copilot commented on code in PR #12022:
URL: https://github.com/apache/gravitino/pull/12022#discussion_r3586009765
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergCatalogPropertyConverter.java:
##########
@@ -161,9 +161,19 @@ private Map<String, String>
buildRestBackendProperties(Map<String, String> prope
"Missing required property for Rest backend: " + missingProperty);
}
- Map<String, String> jdbcProperties = new HashMap<>();
- jdbcProperties.put("iceberg.catalog.type", "rest");
- jdbcProperties.put("iceberg.rest-catalog.uri",
properties.get(IcebergConstants.URI));
- return jdbcProperties;
+ Map<String, String> restProperties = new HashMap<>();
+ restProperties.put("iceberg.catalog.type", "rest");
+ restProperties.put("iceberg.rest-catalog.uri",
properties.get(IcebergConstants.URI));
+ if (properties.containsKey(IcebergConstants.WAREHOUSE)) {
+ restProperties.put(
+ "iceberg.rest-catalog.warehouse",
properties.get(IcebergConstants.WAREHOUSE));
+ }
+ // Forward the end-user IdP token to the IRC for per-user authorization.
The patched
+ // Trino captures the user token into the session and TrinoRestCatalog
forwards it as-is
+ // when session security is USER. Bootstrap credential, server-uri, scope,
and S3 config
+ // are supplied through the catalog's trino.bypass.* properties.
+ restProperties.put("iceberg.rest-catalog.security", "OAUTH2");
+ restProperties.put("iceberg.rest-catalog.session", "USER");
+ return restProperties;
Review Comment:
These properties unconditionally force the Iceberg REST catalog into OAuth2
per-user mode (`security=OAUTH2`, `session=USER`) for all REST-backed catalogs,
regardless of whether the connector is configured for `authType=oauth2` and
`forwardUser=true`. This can break non-OAuth2 deployments and also prevents
users from explicitly configuring other IRC security/session modes. Please gate
these settings behind the connector’s forwarding configuration (and ideally
only when `authType=oauth2`), and/or only set them when the user hasn’t already
provided explicit `iceberg.rest-catalog.*` values.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java:
##########
@@ -226,7 +226,13 @@ public void shutdown() {
}
private CatalogConnectorMetadata resolveSessionMetadata(ConnectorSession
session) {
- String credKey = "simple:" + session.getUser();
+ String credKey =
+ catalogConnectorContext
+ .getConfig()
+ .getClientConfig()
+ .getOrDefault(GravitinoAuthProvider.AUTH_TYPE_KEY, "simple")
+ + ":"
+ + session.getUser();
Review Comment:
For `authType=oauth2`, keying the per-user session cache by only
`authType:user` can cause cross-session token mixups: the same username may
have different/rotated access tokens, but will reuse the cached client/session
created with an earlier token. This can lead to authorization failures (stale
token) or, worse, incorrect identity propagation if usernames collide across
IdP realms. Consider incorporating a stable token identifier into the cache key
(e.g., a hash of the forwarded token, or token `jti` if available), or
disabling/shortening caching for oauth2-forwarded sessions.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/security/GravitinoAuthProvider.java:
##########
@@ -196,13 +198,29 @@ public static GravitinoAdminClient buildForSession(
GravitinoAdminClient.AdminClientBuilder builder =
GravitinoAdminClient.builder(uri);
- if (authType != AuthType.SIMPLE) {
- throw new UnsupportedOperationException(
- "Auth type "
- + authType
- + " does not support session forwarding. Only simple is
supported.");
+ switch (authType) {
+ case SIMPLE:
+ builder.withSimpleAuth(session.getUser());
+ break;
+ case OAUTH2:
+ {
+ String userToken =
session.getIdentity().getExtraCredentials().get("token");
Review Comment:
The extra-credential key `\"token\"` is a hard-coded magic string here (and
is also repeated in tests/docs). To avoid drift and make future changes safer,
consider introducing a shared constant (e.g.,
`FORWARDED_OAUTH2_TOKEN_CREDENTIAL_KEY`) and using it everywhere this key is
referenced.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/security/StaticUserTokenProvider.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.trino.connector.security;
+
+import org.apache.gravitino.client.OAuth2TokenProvider;
+
+/**
+ * An {@link OAuth2TokenProvider} that returns a pre-fetched, already-valid
access token rather than
+ * minting one via client credentials. Used for per-user session forwarding:
the end user's IdP
+ * access token, forwarded by Trino into the connector session, is presented
directly to Gravitino
+ * so the server authorizes against the end user's identity instead of a
shared service identity.
+ *
+ * <p>The raw token is returned from {@link #getAccessToken()}. The {@code
Bearer } prefix is added
+ * by {@link OAuth2TokenProvider#getTokenData()}, so the token held here must
not include it.
+ */
+public final class StaticUserTokenProvider extends OAuth2TokenProvider {
+
+ private final String accessToken;
+
+ /**
+ * Constructs a provider that always returns the given access token.
+ *
+ * @param accessToken the raw bearer token (without the {@code Bearer }
prefix) to present to
+ * Gravitino
+ */
+ public StaticUserTokenProvider(String accessToken) {
+ this.accessToken = accessToken;
+ }
Review Comment:
Since the forwarded token ultimately becomes an `Authorization: Bearer ...`
value, consider normalizing/validating the input here (or where it’s read) to
prevent accidentally double-prefixing (e.g., if the extra credential comes
through as `Bearer <token>`). A small guard to strip a leading `Bearer `
(case-insensitive) or to reject prefixed tokens would make failures clearer and
avoid malformed auth headers.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/security/GravitinoAuthProvider.java:
##########
@@ -196,13 +198,29 @@ public static GravitinoAdminClient buildForSession(
GravitinoAdminClient.AdminClientBuilder builder =
GravitinoAdminClient.builder(uri);
- if (authType != AuthType.SIMPLE) {
- throw new UnsupportedOperationException(
- "Auth type "
- + authType
- + " does not support session forwarding. Only simple is
supported.");
+ switch (authType) {
+ case SIMPLE:
+ builder.withSimpleAuth(session.getUser());
+ break;
+ case OAUTH2:
+ {
+ String userToken =
session.getIdentity().getExtraCredentials().get("token");
+ if (StringUtils.isBlank(userToken)) {
+ throw new IllegalArgumentException(
+ "No forwarded user token found in session extra-credentials
under key 'token'. "
+ + "Ensure Trino is configured with "
+ +
"http-server.authentication.oauth2.forward-token-to-connectors=true and the "
+ + "patched OAuth2Authenticator that injects the user token
is in use.");
Review Comment:
The exception message is actionable, but it hard-depends on internal wording
like “patched OAuth2Authenticator”. Since this is a connector library, it would
be better to describe the requirement generically (e.g., ‘an OAuth2
authenticator that forwards the caller token into connector extra-credentials’)
and/or point to the connector docs section rather than referencing a patch.
This reduces confusion for users on different Trino distributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
