nandorKollar commented on code in PR #3719:
URL: https://github.com/apache/polaris/pull/3719#discussion_r2879066040
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +615,133 @@ public LoadTableResponse createTableStaged(
* @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()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ LOGGER.debug(
+ "registerTable: identifier={}, overwrite={}, request={}", identifier,
overwrite, request);
+ if (overwrite) {
+ LOGGER.debug("registerTable: overwrite requested for {}", identifier);
+ authorizeRegisterTableOverwriteOrCreate(identifier);
+ return registerTableWithOverwrite(identifier, request);
+ }
+
+ // Creating new table requires REGISTER_TABLE privilege
+ PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.REGISTER_TABLE;
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier);
+ LOGGER.debug("registerTable: authorized REGISTER_TABLE for {}",
identifier);
return catalogHandlerUtils().registerTable(baseCatalog, namespace,
request);
}
+ /**
+ * Authorize registerTable with overwrite=true.
+ *
+ * <p>If the table exists, require REGISTER_TABLE_OVERWRITE on the table;
otherwise require
+ * REGISTER_TABLE on the parent namespace.
+ *
+ * <p>Resolve both the namespace and an optional passthrough table path in
one pass because the
+ * standard helpers either assume the table exists or always authorize
against the namespace.
+ * Also, baseCatalog.tableExists() cannot be used here since
initializeCatalog() has not run.
+ */
+ private void authorizeRegisterTableOverwriteOrCreate(TableIdentifier
identifier) {
+ LOGGER.debug("authorizeRegisterTableOverwriteOrCreate: start for {}",
identifier);
+ // Build a resolution manifest that includes the namespace and optional
table path.
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(
+ Arrays.asList(identifier.namespace().levels()),
PolarisEntityType.NAMESPACE),
+ identifier.namespace());
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */),
+ identifier);
+ resolutionManifest.resolveAll();
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
+ identifier, PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE, true);
+
+ if (tableTarget != null) {
+ LOGGER.debug(
+ "authorizeRegisterTableOverwriteOrCreate: found existing table
target for {}, requiring REGISTER_TABLE_OVERWRITE",
+ identifier);
+ // Overwrite on an existing table requires full metadata permissions.
+ authorizer()
+ .authorizeOrThrow(
+ polarisPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE,
+ tableTarget,
+ null /* secondary */);
+ initializeCatalog();
+ LOGGER.debug(
+ "registerTable: overwrite=true, authorized for
REGISTER_TABLE_OVERWRITE on existing table {}",
+ identifier);
+ return;
+ }
+
+ // Table doesn't exist, fall back to standard register-table authorization.
Review Comment:
I'm wondering how important this fallback is. If the table already exists,
then eventually the caller needs `REGISTER_TABLE_OVERWRITE` role, but it turns
out only when they attempted the operation. I'd expect anyone, who calls this
endpoint to have `REGISTER_TABLE_OVERWRITE`, regardless of the 'state of the
world'. What do you think?
--
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]
