dimas-b commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3236478419
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -244,6 +282,27 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
PolarisEntitySubType subType,
TableIdentifier identifier) {
ensureResolutionManifestForTable(identifier);
+ if (resolutionManifest.getPrimaryResolverStatus() == null) {
Review Comment:
How can we have a `resolutionManifest` here that has already been
resolved?.. Just for my education 😅
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1019,13 +1023,27 @@ private UpdateTableRequest
applyUpdateFilters(UpdateTableRequest request) {
public LoadTableResponse updateTable(
TableIdentifier tableIdentifier, UpdateTableRequest request) {
-
- // Ensure resolution manifest is initialized so we can determine whether
- // fine grained authz model is enabled at the catalog level
ensureResolutionManifestForTable(tableIdentifier);
+ // Intentionally pre-resolve once using coarse UPDATE_TABLE so we can read
catalog-scoped
+ // config from the shared manifest before authorizing the final per-update
operation set.
+ // This is a temporary misuse of the current SPI shape: operation is part
of the
+ // resolveAuthorizationInputs(...) request, but built-in authorizers do
not currently vary
+ // resolution by operation. Once the SPI supports multiple resolution
passes cleanly, this flow
+ // should stop relying on a representative operation for planning-time
config lookup.
+ AuthorizationState authzState = new AuthorizationState();
Review Comment:
Javadoc on `AuthorizationState` says it's "request-scoped", but creating
multiple local instances kind of contradicts that 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -244,6 +282,27 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
PolarisEntitySubType subType,
TableIdentifier identifier) {
ensureResolutionManifestForTable(identifier);
+ if (resolutionManifest.getPrimaryResolverStatus() == null) {
+ if (!ops.isEmpty()) {
Review Comment:
Should it be a runtime error is `ops` is empty? 🤔
--
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]
