HonahX commented on code in PR #2697:
URL: https://github.com/apache/polaris/pull/2697#discussion_r2383690057
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -259,12 +267,15 @@ protected void authorizeBasicTableLikeOperationOrThrow(
if (target == null) {
throwNotFoundExceptionForTableLikeEntity(identifier, List.of(subType));
}
- authorizer.authorizeOrThrow(
- polarisPrincipal,
- resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
- op,
- target,
- null /* secondary */);
+
+ for (PolarisAuthorizableOperation op : ops) {
+ authorizer.authorizeOrThrow(
+ polarisPrincipal,
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ op,
+ target,
+ null /* secondary */);
+ }
Review Comment:
WDYT about having an `authorizeOrThrow` that could take multiple
authorizableOperations? That way we could reduce the call to authorizer and
potentially save some time from not repeating common logic inside that method.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -843,9 +843,74 @@ private UpdateTableRequest
applyUpdateFilters(UpdateTableRequest request) {
public LoadTableResponse updateTable(
TableIdentifier tableIdentifier, UpdateTableRequest request) {
- PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.UPDATE_TABLE;
- authorizeBasicTableLikeOperationOrThrow(
- op, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ boolean useFineGrainedOperations =
+
realmConfig.getConfig(FeatureConfiguration.ENABLE_FINE_GRAINED_UPDATE_TABLE_PRIVILEGES);
+
+ if (useFineGrainedOperations) {
+ EnumSet<PolarisAuthorizableOperation> actions =
+ EnumSet.noneOf(PolarisAuthorizableOperation.class);
+ request
+ .updates()
+ .forEach(
+ update -> {
+ PolarisAuthorizableOperation operation =
+ switch (update) {
+ case MetadataUpdate.AssignUUID assignUuid ->
+ PolarisAuthorizableOperation.ASSIGN_TABLE_UUID;
+ case MetadataUpdate.UpgradeFormatVersion upgradeFormat ->
+
PolarisAuthorizableOperation.UPGRADE_TABLE_FORMAT_VERSION;
+ case MetadataUpdate.AddSchema addSchema ->
+ PolarisAuthorizableOperation.ADD_TABLE_SCHEMA;
+ case MetadataUpdate.SetCurrentSchema setCurrentSchema ->
+
PolarisAuthorizableOperation.SET_TABLE_CURRENT_SCHEMA;
+ case MetadataUpdate.AddPartitionSpec addPartitionSpec ->
+
PolarisAuthorizableOperation.ADD_TABLE_PARTITION_SPEC;
+ case MetadataUpdate.AddSortOrder addSortOrder ->
+ PolarisAuthorizableOperation.ADD_TABLE_SORT_ORDER;
+ case MetadataUpdate.SetDefaultSortOrder
setDefaultSortOrder ->
+
PolarisAuthorizableOperation.SET_TABLE_DEFAULT_SORT_ORDER;
+ case MetadataUpdate.AddSnapshot addSnapshot ->
+ PolarisAuthorizableOperation.ADD_TABLE_SNAPSHOT;
+ case MetadataUpdate.SetSnapshotRef setSnapshotRef ->
+ PolarisAuthorizableOperation.SET_TABLE_SNAPSHOT_REF;
+ case MetadataUpdate.RemoveSnapshots removeSnapshots ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_SNAPSHOTS;
+ case MetadataUpdate.RemoveSnapshotRef removeSnapshotRef
->
+
PolarisAuthorizableOperation.REMOVE_TABLE_SNAPSHOT_REF;
+ case MetadataUpdate.SetLocation setLocation ->
+ PolarisAuthorizableOperation.SET_TABLE_LOCATION;
+ case MetadataUpdate.SetProperties setProperties ->
+ PolarisAuthorizableOperation.SET_TABLE_PROPERTIES;
+ case MetadataUpdate.RemoveProperties removeProperties ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_PROPERTIES;
+ case MetadataUpdate.SetStatistics setStatistics ->
+ PolarisAuthorizableOperation.SET_TABLE_STATISTICS;
+ case MetadataUpdate.RemoveStatistics removeStatistics ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_STATISTICS;
+ case MetadataUpdate.RemovePartitionSpecs
removePartitionSpecs ->
+
PolarisAuthorizableOperation.REMOVE_TABLE_PARTITION_SPECS;
+ default ->
+ PolarisAuthorizableOperation
+ .UPDATE_TABLE; // Fallback for unknown update
types
+ };
+ actions.add(operation);
+ });
+
+ // If there are no MetadataUpdates, then default to the UPDATE_TABLE
operation.
+ if (actions.isEmpty()) {
+ actions.add(PolarisAuthorizableOperation.UPDATE_TABLE);
+ }
+
+ // Authorize all collected operations
+ for (PolarisAuthorizableOperation action : actions) {
+ authorizeBasicTableLikeOperationOrThrow(
+ action, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ }
Review Comment:
I think we want the `EnumSet``authorizeBasicTableLikeOperationOrThrow` here
instead of doing a for-loop?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -843,9 +843,74 @@ private UpdateTableRequest
applyUpdateFilters(UpdateTableRequest request) {
public LoadTableResponse updateTable(
TableIdentifier tableIdentifier, UpdateTableRequest request) {
- PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.UPDATE_TABLE;
- authorizeBasicTableLikeOperationOrThrow(
- op, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
+ boolean useFineGrainedOperations =
+
realmConfig.getConfig(FeatureConfiguration.ENABLE_FINE_GRAINED_UPDATE_TABLE_PRIVILEGES);
+
+ if (useFineGrainedOperations) {
+ EnumSet<PolarisAuthorizableOperation> actions =
+ EnumSet.noneOf(PolarisAuthorizableOperation.class);
+ request
+ .updates()
+ .forEach(
+ update -> {
+ PolarisAuthorizableOperation operation =
+ switch (update) {
+ case MetadataUpdate.AssignUUID assignUuid ->
+ PolarisAuthorizableOperation.ASSIGN_TABLE_UUID;
+ case MetadataUpdate.UpgradeFormatVersion upgradeFormat ->
+
PolarisAuthorizableOperation.UPGRADE_TABLE_FORMAT_VERSION;
+ case MetadataUpdate.AddSchema addSchema ->
+ PolarisAuthorizableOperation.ADD_TABLE_SCHEMA;
+ case MetadataUpdate.SetCurrentSchema setCurrentSchema ->
+
PolarisAuthorizableOperation.SET_TABLE_CURRENT_SCHEMA;
+ case MetadataUpdate.AddPartitionSpec addPartitionSpec ->
+
PolarisAuthorizableOperation.ADD_TABLE_PARTITION_SPEC;
+ case MetadataUpdate.AddSortOrder addSortOrder ->
+ PolarisAuthorizableOperation.ADD_TABLE_SORT_ORDER;
+ case MetadataUpdate.SetDefaultSortOrder
setDefaultSortOrder ->
+
PolarisAuthorizableOperation.SET_TABLE_DEFAULT_SORT_ORDER;
+ case MetadataUpdate.AddSnapshot addSnapshot ->
+ PolarisAuthorizableOperation.ADD_TABLE_SNAPSHOT;
+ case MetadataUpdate.SetSnapshotRef setSnapshotRef ->
+ PolarisAuthorizableOperation.SET_TABLE_SNAPSHOT_REF;
+ case MetadataUpdate.RemoveSnapshots removeSnapshots ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_SNAPSHOTS;
+ case MetadataUpdate.RemoveSnapshotRef removeSnapshotRef
->
+
PolarisAuthorizableOperation.REMOVE_TABLE_SNAPSHOT_REF;
+ case MetadataUpdate.SetLocation setLocation ->
+ PolarisAuthorizableOperation.SET_TABLE_LOCATION;
+ case MetadataUpdate.SetProperties setProperties ->
+ PolarisAuthorizableOperation.SET_TABLE_PROPERTIES;
+ case MetadataUpdate.RemoveProperties removeProperties ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_PROPERTIES;
+ case MetadataUpdate.SetStatistics setStatistics ->
+ PolarisAuthorizableOperation.SET_TABLE_STATISTICS;
+ case MetadataUpdate.RemoveStatistics removeStatistics ->
+ PolarisAuthorizableOperation.REMOVE_TABLE_STATISTICS;
+ case MetadataUpdate.RemovePartitionSpecs
removePartitionSpecs ->
+
PolarisAuthorizableOperation.REMOVE_TABLE_PARTITION_SPECS;
+ default ->
+ PolarisAuthorizableOperation
+ .UPDATE_TABLE; // Fallback for unknown update
types
+ };
+ actions.add(operation);
Review Comment:
We may consider move this to a helper. Just want to prevent the method
length from increasing when iceberg side add more and more update types :)
--
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]
