sachinnn99 commented on code in PR #10699:
URL: https://github.com/apache/gravitino/pull/10699#discussion_r3049980900
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -130,6 +131,18 @@ public LoadTableResponse loadTable(
return loadTableResponse;
}
+ public LoadTableResponse registerTable(
+ Namespace namespace, RegisterTableRequest request, boolean
requestCredential) {
+ LoadTableResponse loadTableResponse = super.registerTable(namespace,
request);
+ if (shouldGenerateCredential(loadTableResponse, requestCredential)) {
+ return injectCredentialConfig(
+ TableIdentifier.of(namespace, request.name()),
+ loadTableResponse,
+ CredentialPrivilege.WRITE);
Review Comment:
You're right, and after digging in I'm switching to READ.
Specifically:
1. **registerTable writes no files at the storage layer.** Verified
end-to-end: `CatalogHandlers.registerTable` →
`BaseMetastoreCatalog.registerTable` → `ops.commit(null, metadata)` →
`BaseMetastoreTableOperations.writeNewMetadataIfRequired(true, metadata)`
early-returns the existing `metadata.metadataFileLocation()` without writing,
then `JdbcTableOperations` just inserts the catalog row. So the storage-layer
asymmetry you flagged with `createTable` is real — `createTable` proves write
capability by writing a metadata.json file, `registerTable` doesn't.
2. **registerTable also doesn't set ownership.**
`IcebergNamespaceOperationExecutor.registerTable` is a straight delegation to
the wrapper — unlike `IcebergTableOperationExecutor.createTable` and
`IcebergNamespaceOperationExecutor.createNamespace`, which both explicitly set
`IcebergConstants.OWNER` to `context.userName()`. So a user with only
`ANY_CREATE_TABLE` privilege never becomes the table owner via register.
3. **WRITE was therefore a one-off privilege escalation.** That same user
cannot pass `FILTER_MODIFY_TABLE_AUTHORIZATION_EXPRESSION` (`ANY(OWNER,
METALAKE, CATALOG, SCHEMA, TABLE) || ANY_MODIFY_TABLE`) on the registered table
via any other API path. A follow-up `loadTable` with vending would return READ
for them via `getCredentialPrivilege`. Hardcoding WRITE here was vending
storage credentials that let them perform actions gravitino's authz model
explicitly forbids.
Switching to READ. Higher-privileged users (catalog/metalake owners,
`MODIFY_TABLE` holders) who legitimately need to write to a registered table
can call `loadTable` with vending afterwards and get WRITE through the dynamic
helper, or be granted `MODIFY_TABLE` explicitly.
I'll file a separate follow-up issue about the missing ownership assignment
on register — that looks like an oversight (`createTable` and `createNamespace`
both do it). Once that's fixed, we can also consider routing this through
`getCredentialPrivilege` the way `loadTable` does, so higher-privileged users
get WRITE on register without a second round-trip.
I've added a code comment documenting all of this.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java:
##########
Review Comment:
Added an inline comment at the call site explaining the duplication: the
in-memory test catalog cannot perform `registerTable` natively, so the override
synthesizes a mock `LoadTableResponse` instead of delegating to
`super.registerTable`. Because we never go through the parent's
`registerTable`, we must explicitly re-run its vending logic so
credential-vending tests still exercise the same code path as production. (Also
updated this wrapper to use READ to match the production change.)
--
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]
