bharos commented on code in PR #10586:
URL: https://github.com/apache/gravitino/pull/10586#discussion_r3031231548
##########
core/src/main/java/org/apache/gravitino/UserPrincipal.java:
##########
@@ -77,7 +115,7 @@ public List<UserGroup> getGroups() {
@Override
public int hashCode() {
- return Objects.hash(username, groups);
+ return Objects.hash(username, groups, accessToken);
Review Comment:
Including `accessToken` (a transient credential) in `equals()`/`hashCode()`
means two principals for the same user with different JWT tokens are now
**unequal**. If `UserPrincipal` is used in `Set`s or `Map` keys, token rotation
creates distinct identities for the same user. The access token is a transport
concern, not identity — `equals`/`hashCode` should stay based on `username` +
`groups` only.
##########
core/src/main/java/org/apache/gravitino/utils/PrincipalUtils.java:
##########
@@ -31,11 +33,18 @@
@SuppressWarnings("removal")
public class PrincipalUtils {
-
private static final Logger LOG =
LoggerFactory.getLogger(PrincipalUtils.class);
private PrincipalUtils() {}
+ public static final Principal ANONYMOUS_PRINCIPAL =
+ new UserPrincipal(
+ AuthConstants.ANONYMOUS_USER,
+ AuthConstants.AUTHORIZATION_BASIC_HEADER
Review Comment:
`ANONYMOUS_PRINCIPAL` now carries `Basic YW5vbnltb3Vz` as its access token.
When the REST backend forwards this via `UserPrincipalForwardingAuthManager`,
anonymous requests send a fabricated auth header to downstream services.
Consider giving anonymous a `null` token and having the auth manager skip the
Authorization header when none is available (rather than throwing
`IllegalStateException`).
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -171,10 +197,10 @@ public void reloadHadoopConf() {
public LoadTableResponse createTable(Namespace namespace, CreateTableRequest
request) {
request.validate();
if (request.stageCreate()) {
- return CatalogHandlers.stageTableCreate(catalog, namespace, request);
+ return stageTableCreateInternal(namespace, request);
Review Comment:
The replacement of
`CatalogHandlers.loadTable()`/`CatalogHandlers.createTable()` with
`loadTableInternal()`/`createTableInternal()` is in the **base class**, so it
changes behavior for JDBC, Hive, and Memory backends too — not just REST.
Previously load-table responses had an empty `config` map; now they include
FileIO properties via `retrieveFileIOProperties()` + credential filtering. For
non-REST backends, `table.io().properties()` can include catalog-init
credentials/URIs that were never exposed before.
Suggestion: Keep the base class using `CatalogHandlers` and only override in
a REST-specific path.
--
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]
