Copilot commented on code in PR #3734:
URL: https://github.com/apache/polaris/pull/3734#discussion_r3274452823
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java:
##########
@@ -935,6 +1020,62 @@ public void testRegisterAndLoadTableWithReturnedETag() {
}
}
+ /**
+ * Build an Iceberg table, then invoke a subsequent register table call that
overwrites the
+ * existing table metadata. Verifies that the overwrite is applied and the
catalog reflects the
+ * updated metadata.
+ */
+ @Test
+ public void testRegisterTableOverwriteViaRest() {
+ Namespace ns = Namespace.of("ns_overwrite_rest");
+ restCatalog.createNamespace(ns);
+
+ // Create a source table and capture its metadata location
+ Table source = restCatalog.buildTable(TableIdentifier.of(ns,
"source_table"), SCHEMA).create();
Review Comment:
The PR adds an explicit rejection path for registerTable overwrite on
federated/external catalogs (BadRequestException in IcebergCatalogHandler when
overwrite=true and baseCatalog is not IcebergCatalog). Please check whether an
integration test should be added to cover this user-visible behavior (e.g.,
EXTERNAL catalog + overwrite=true returns HTTP 400 with the expected message),
since current integration coverage only exercises the internal-catalog
overwrite success path.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -853,6 +884,42 @@ private Set<PolarisStorageActions> authorizeLoadTable(
return actionsRequested;
}
+ private Set<PolarisStorageActions> authorizeRegisterTable(
+ TableIdentifier tableIdentifier,
+ EnumSet<AccessDelegationMode> delegationModes,
+ boolean overwrite) {
+
+ if (delegationModes.isEmpty()) {
+ PolarisAuthorizableOperation op =
+ overwrite
+ ? PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE
+ : PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op,
tableIdentifier);
Review Comment:
Authorization for overwrite register currently uses
authorizeCreateTableLikeUnderNamespaceOperationOrThrow (namespace-targeted).
For overwrite=true on an *existing* table, this likely bypasses table-scoped
grants (e.g., a principal with TABLE_FULL_METADATA granted on the specific
table but not on the namespace/catalog could be incorrectly denied) and makes
the privilege boundary less precise. Consider authorizing overwrite against the
table entity when it exists (authorizeBasicTableLikeOperationOrThrow with
ICEBERG_TABLE) and only falling back to namespace-targeted authorization when
the table is missing (to support the overwrite-creates-if-missing behavior).
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -609,14 +609,45 @@ public LoadTableResponse createTableStaged(
*
* @param namespace The namespace to register the table in
* @param request the register table request
+ * @param delegationModes the access delegation modes to use
+ * @param refreshCredentialsEndpoint the refresh credentials endpoint to use
* @return ETagged {@link LoadTableResponse} to uniquely identify the table
metadata
*/
- public LoadTableResponse registerTable(Namespace namespace,
RegisterTableRequest request) {
- PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ public LoadTableResponse registerTable(
+ Namespace namespace,
+ RegisterTableRequest request,
+ EnumSet<AccessDelegationMode> delegationModes,
+ Optional<String> refreshCredentialsEndpoint) {
+
+ request.validate();
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ Set<PolarisStorageActions> actionsRequested =
Review Comment:
registerTable resolves/validates the requested access delegation mode
(resolveAccessDelegationModes) only *after* calling
baseCatalog.registerTable(...). Since resolveAccessDelegationModes can throw
(e.g., REMOTE_SIGNING currently rejected, or vended-credentials disallowed for
an external/federated catalog), an invalid access-delegation request could
still register/overwrite the table and then fail the HTTP call. Consider
resolving/validating the delegation mode (and any feature-flag gating) before
performing the register/overwrite side effect, so error responses don’t leave
partially-applied state.
--
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]
