You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sungwy (via GitHub)" <gi...@apache.org> on 2026/05/02 22:22:41 UTC
[PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
sungwy opened a new pull request, #4356:
URL: https://github.com/apache/polaris/pull/4356
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
Phase 4 of https://github.com/apache/polaris/issues/3779
Use `resolveAuthorizationInputs()` at handler and admin call sites so authorization-driven entity resolution is owned by the `PolarisAuthorizer` SPI rather than being hardcoded in service code.
Before this change, `CatalogHandler`s and `PolarisAdminService` eagerly performed broad Polaris resolution before authorization, including the target entity and other RBAC-related entities such as the reference catalog, caller principal, principal roles, and catalog roles. That meant resolution scope was effectively fixed by the handlers, even for authorizers that would not need Polaris RBAC state.
After this change, handlers build authorization inputs and pass them to `PolarisAuthorizer.resolveAuthorizationInputs()`. The authorizer then decides what Polaris entities, if any, need to be resolved in order to authorize the request.
Important call outs:
- `updateTable()` now resolves the reference catalog separately before auth so it can read catalog-scoped config for fine-grained update privileges, then runs normal authorizer-controlled pre-resolution for authorization. This removes the old unconditional full RBAC-style pre-auth resolution, but introduces a less-atomic two-resolution window for catalog config.
- Empty-namespace table-like identifiers are no longer tolerated in the migrated paths. Table-like targets are now expected to have a valid namespace. This removes legacy compatibility for invalid admin grant inputs like `TableIdentifier.of(Namespace.empty(), ...)`
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3438887556
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
Hi @flyrain thanks for the detailed review.
I see why it reads that way, but I think the intended contract is different.
`AuthorizationState` is meant to be request-scoped state that the `PolarisAuthorizer` can manipulate throughout the request. This is basically a renamed version of the โauth call contextโ idea we discussed in the [Polaris Community Sync 1/27/2026](https://docs.google.com/document/d/1C_SSaZH1i83UUGXrnVBur1fR_FHKYWZ75ISFfcb3kns/edit?tab=t.0), which allows us to avoid having to do a large refactoring of the Persistence/Resolution layer to reach the same outcome today of skipping RBAC resolution for external authorizers.
The write-once behavior per request is intentional to me. For a given request, we should know the full path of the resource we want to resolve up front. If we try to swap the manifest halfway through the request, Iโd rather fail loudly than silently authorize/execute against a different resolved view.
I agree the current duplicate storage is awkward: the handler field and `AuthorizationState` point to the same thing. But I think that becomes a supporting case for a refactoring of the Resolver/Persistence layer, rather than making `AuthorizationState` a disposable per-call wrapper.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3456663914
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -253,7 +269,15 @@ private PolarisResolutionManifest authorizeBasicTopLevelEntityOperationOrThrow(
@Nullable String referenceCatalogName) {
PolarisResolutionManifest resolutionManifest = newResolutionManifest(referenceCatalogName);
resolutionManifest.addTopLevelName(topLevelEntityName, entityType, false /* isOptional */);
- ResolverStatus status = resolutionManifest.resolveAll();
+ authorizationState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authorizationState,
+ new AuthorizationRequest(
+ polarisPrincipal,
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ op, PolarisSecurable.of(new PathSegment(entityType, topLevelEntityName))))));
+ ResolverStatus status = resolutionManifest.getPrimaryResolverStatus();
Review Comment:
That's a good call out. I've changed the public method to `getPrimaryResolverStatusOrThrow()` which throws if the `PolarisResolutionManifest` had not yet resolved.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3351755511
##########
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:
This is a good call out. I broke down resolution and authorization into two separate methods so we can have the caller ensure that resolution is being called once
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -288,20 +328,61 @@ protected void ensureResolutionManifestForTable(TableIdentifier identifier) {
PolarisCatalogHelpers.tableIdentifierToList(identifier),
PolarisEntityType.TABLE_LIKE,
true /* optional */));
- resolutionManifest.resolveAll();
}
}
- protected void authorizeBasicTableLikeOperationOrThrow(
+ protected void resolveBasicTableLikeTargetOrThrow(
Review Comment:
note to reviewer: we have a few cases where we resolve only once, and then authorize using the same manifest in `IcebergCatalogHandler` like in `updateTable` and `loadTable`.
Having two separate methods for resolution and authorization ensures that we are resolving just once, without relying on a logic that checks if we have yet to resolve - like `if (resolutionManifest.getPrimaryResolverStatus() == null)` - which is more errorprone.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3195300699
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -244,6 +276,25 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
PolarisEntitySubType subType,
TableIdentifier identifier) {
ensureResolutionManifestForTable(identifier);
+ if (resolutionManifest.getPrimaryResolverStatus() == null) {
+ if (!ops.isEmpty()) {
+ // Pre-resolution runs once for the shared target set. Current built-in authorizers
+ // resolve broadly and do not vary resolution scope by operation, so any op in the set is
+ // sufficient here. If a future authorizer makes pre-resolution operation-specific, this
+ // path may need to resolve for each op or accept a multi-op request.
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer()
+ .resolveAuthorizationInputs(
+ authzState,
+ AuthorizationRequest.of(
+ polarisPrincipal(),
+ ops.iterator().next(),
+ List.of(
+ AuthorizationTargetBinding.of(
+ PolarisSecurableMapper.tableLike(catalogName(), identifier), null))));
Review Comment:
updated to only resolve once for updateTable
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3195304784
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.auth.ImmutablePolarisSecurable;
+import org.apache.polaris.core.auth.PathSegment;
+import org.apache.polaris.core.auth.PolarisSecurable;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.service.types.PolicyAttachmentTarget;
+import org.apache.polaris.service.types.PolicyIdentifier;
+
+/**
+ * Utility mapper for translating Polaris domain identifiers into canonical {@link
+ * org.apache.polaris.core.auth.PolarisSecurable} paths.
+ */
+public final class PolarisSecurableMapper {
+ private PolarisSecurableMapper() {}
+
+ public static PolarisSecurable catalog(String catalogName) {
+ return PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ }
+
+ public static PolarisSecurable catalogRole(String catalogName, String catalogRoleName) {
+ return ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName))
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG_ROLE, catalogRoleName))
+ .build();
+ }
+
+ public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+ ImmutablePolarisSecurable.Builder builder =
+ ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ Arrays.stream(namespace.levels())
+ .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+ .forEach(builder::addPathSegment);
+ return builder.build();
Review Comment:
updated `PolarisSecurableMapper` methods to explicitly validate the inputs: `.namespace()` expects non-empty `Namespace`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3461031660
##########
CHANGELOG.md:
##########
@@ -36,6 +36,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
### Breaking changes
- The `MaintenanceService.performMaintenance()` signature now requires an explicit `OptionalLong overrideRunId` argument to supersede the latest unfinished maintenance run.
+- Admin grant APIs now reject table-like privilege targets with an empty namespace. A table-like target without a namespace is considered invalid input.
Review Comment:
nit: I'm not sure this is actually a breaking change from the end user perspective, because IRC catalogs do not allow creating tables/views without a namespace, AFAIK... at least not in the Iceberg java client ๐ค
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #4356:
URL: https://github.com/apache/polaris/pull/4356#issuecomment-4635981028
> LGTM ๐ Thanks, @sungwy !
Thanks @dimas-b !
My apologies, but looking at the CI, I think there's some additional changes I'd need to make in order to update the existing test suite to conform to the new pattern.
My hunch is that a CatalogHander instance is being reused across requests, which doesn't mirror how we do things in production today.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3351686825
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -288,20 +328,61 @@ protected void ensureResolutionManifestForTable(TableIdentifier identifier) {
PolarisCatalogHelpers.tableIdentifierToList(identifier),
PolarisEntityType.TABLE_LIKE,
true /* optional */));
- resolutionManifest.resolveAll();
}
}
- protected void authorizeBasicTableLikeOperationOrThrow(
+ protected void resolveBasicTableLikeTargetOrThrow(
Review Comment:
note to reviewer: we have a few cases where we resolve only once, and then authorize using the same manifest in `IcebergCatalogHandler` like in `updateTable` and `loadTable`.
Having two separate APIs for resolution and authorization ensures that we are resolving just once, without relying on a logic that checks if we have yet to resolve - like `if (resolutionManifest.getPrimaryResolverStatus() == null)` - which is more errorprone.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3188099647
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -244,6 +276,25 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
PolarisEntitySubType subType,
TableIdentifier identifier) {
ensureResolutionManifestForTable(identifier);
+ if (resolutionManifest.getPrimaryResolverStatus() == null) {
+ if (!ops.isEmpty()) {
+ // Pre-resolution runs once for the shared target set. Current built-in authorizers
+ // resolve broadly and do not vary resolution scope by operation, so any op in the set is
+ // sufficient here. If a future authorizer makes pre-resolution operation-specific, this
+ // path may need to resolve for each op or accept a multi-op request.
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer()
+ .resolveAuthorizationInputs(
+ authzState,
+ AuthorizationRequest.of(
+ polarisPrincipal(),
+ ops.iterator().next(),
+ List.of(
+ AuthorizationTargetBinding.of(
+ PolarisSecurableMapper.tableLike(catalogName(), identifier), null))));
Review Comment:
Known edge case, and currently a non-issue given that operation does not affect resolution.
In the future, we should consider sharing a single RequestResolutionState that can be generated additively based with needed entities at different call sites that can validate the existing resolution based on different validation rules (consistent version, consistent version of related entities, etc)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4356:
URL: https://github.com/apache/polaris/pull/4356#issuecomment-4443922662
Should we hold this until after 1.5.0 is branched ?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #4356:
URL: https://github.com/apache/polaris/pull/4356#issuecomment-4904750405
> Thanks for this. Read against the #3779 arc, the shape is right. The request side now says _what_ to authorize as plain intent, which is what lets an authorizer participate without inheriting Polaris RBAC. That was #3760's stated aim, and this PR takes it live at the call sites.
>
> The piece I'd settle before the decision path cuts over: the request became implementation-neutral, but the state object didn't. `AuthorizationState` still hands every authorizer a `PolarisResolutionManifest`, and each one just calls `resolveAll()` on it, so an authorizer that doesn't want Polaris-native resolution still has to accept and drive it to satisfy the contract. The intent half lets an authorizer bring its own machinery; the state half still requires the native one. Since the point of the series is to let the authorizer decide whether native resolution happens at all, I'd make that state opaque or optional before phases 5 and 6 lock the contract in.
>
> Two smaller things in the same spirit:
>
> * `updateTable` resolves with a placeholder operation to read catalog config, then authorizes a different set. It's fine today, but it quietly assumes resolution is operation-independent, which your own comment flags. That assumption is part of the contract now, so it belongs in the `resolveAuthorizationInputs` javadoc rather than a TODO. It's exactly what a selective authorizer would break.
> * The empty-namespace rejection turns a 404 into a 400 on public catalog paths (`commitTransaction`, `renameTable` source), not just admin grants, and the new mapper applies the guard unevenly (`policy()` is missing it). The 400 is the right call; the CHANGELOG just scopes it too narrowly.
>
> Direction's good. The contract is easiest to get neutral now, while the new decision API is still dormant at the call sites.
Hi @flyingImer - thanks for the review. Are these blocking concerns in your opinion? I'm looking to do one more pass to incorporate the consensus I've been able to reach with @dimas-b @flyrain on the `AuthorizationState`. If these aren't blocking concerns, I'd like to move this PR forward
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #4356:
URL: https://github.com/apache/polaris/pull/4356#issuecomment-4916026400
Thanks for the reviews @dimas-b , @flyrain and @flyingImer !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3352050466
##########
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:
resolved
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3461051787
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
@sungwy : that works for me :+1:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3188089227
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -227,7 +233,10 @@ private static CatalogRoleEntity getCatalogRoleByName(
private void authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation op) {
PolarisResolutionManifest resolutionManifest = newResolutionManifest(null);
- resolutionManifest.resolveAll();
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authzState, AuthorizationRequest.of(polarisPrincipal, op, List.of()));
Review Comment:
This will be replaced by the new authorize SPI that no longer accepts root container as a part of the PolarisSecurable
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3188107317
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.auth.ImmutablePolarisSecurable;
+import org.apache.polaris.core.auth.PathSegment;
+import org.apache.polaris.core.auth.PolarisSecurable;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.service.types.PolicyAttachmentTarget;
+import org.apache.polaris.service.types.PolicyIdentifier;
+
+/**
+ * Utility mapper for translating Polaris domain identifiers into canonical {@link
+ * org.apache.polaris.core.auth.PolarisSecurable} paths.
+ */
+public final class PolarisSecurableMapper {
+ private PolarisSecurableMapper() {}
+
+ public static PolarisSecurable catalog(String catalogName) {
+ return PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ }
+
+ public static PolarisSecurable catalogRole(String catalogName, String catalogRoleName) {
+ return ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName))
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG_ROLE, catalogRoleName))
+ .build();
+ }
+
+ public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+ ImmutablePolarisSecurable.Builder builder =
+ ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ Arrays.stream(namespace.levels())
+ .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+ .forEach(builder::addPathSegment);
+ return builder.build();
Review Comment:
Thanks Co-pilot, let me take a look into this
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3456723172
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.auth.ImmutablePolarisSecurable;
+import org.apache.polaris.core.auth.PathSegment;
+import org.apache.polaris.core.auth.PolarisSecurable;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.service.types.PolicyAttachmentTarget;
+import org.apache.polaris.service.types.PolicyIdentifier;
+
+/**
+ * Utility mapper for translating Polaris domain identifiers into canonical {@link
+ * org.apache.polaris.core.auth.PolarisSecurable} paths.
+ */
+public final class PolarisSecurableMapper {
+ private PolarisSecurableMapper() {}
+
+ public static PolarisSecurable catalog(String catalogName) {
+ return PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ }
+
+ public static PolarisSecurable catalogRole(String catalogName, String catalogRoleName) {
+ return ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName))
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG_ROLE, catalogRoleName))
+ .build();
+ }
+
+ public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+ if (namespace.isEmpty()) {
+ throw new IllegalArgumentException("Namespace target must not be empty");
+ }
+ ImmutablePolarisSecurable.Builder builder =
+ ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ Arrays.stream(namespace.levels())
+ .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+ .forEach(builder::addPathSegment);
+ return builder.build();
+ }
+
+ public static PolarisSecurable tableLike(String catalogName, TableIdentifier identifier) {
Review Comment:
Thanks @flyrain - this is a good callout. When I first ran into this, I decided that it would be safe to make this change because we don't support [empty namespaces in the IcebergCatalogHandler](https://github.com/apache/polaris/blob/6e883ed05388afbb608e5d515a9c99c89e0cccf2/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java#L434) today in Polaris.
But I agree that it's good to document any behavioral changes in the `CHANGELOG`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3461607524
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
Works for me, thanks @sungwy !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3181225105
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -244,6 +276,25 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
PolarisEntitySubType subType,
TableIdentifier identifier) {
ensureResolutionManifestForTable(identifier);
+ if (resolutionManifest.getPrimaryResolverStatus() == null) {
+ if (!ops.isEmpty()) {
+ // Pre-resolution runs once for the shared target set. Current built-in authorizers
+ // resolve broadly and do not vary resolution scope by operation, so any op in the set is
+ // sufficient here. If a future authorizer makes pre-resolution operation-specific, this
+ // path may need to resolve for each op or accept a multi-op request.
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer()
+ .resolveAuthorizationInputs(
+ authzState,
+ AuthorizationRequest.of(
+ polarisPrincipal(),
+ ops.iterator().next(),
+ List.of(
+ AuthorizationTargetBinding.of(
+ PolarisSecurableMapper.tableLike(catalogName(), identifier), null))));
Review Comment:
`updateTable()` can require multiple fine-grained operations, but pre-resolution is only run for `ops.iterator().next()`. An authorizer that resolves different inputs per operation will populate the manifest for just one privilege and the later `authorizeOrThrow()` calls will run against incomplete state, so the result depends on which enum happens to be first.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.auth.ImmutablePolarisSecurable;
+import org.apache.polaris.core.auth.PathSegment;
+import org.apache.polaris.core.auth.PolarisSecurable;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.service.types.PolicyAttachmentTarget;
+import org.apache.polaris.service.types.PolicyIdentifier;
+
+/**
+ * Utility mapper for translating Polaris domain identifiers into canonical {@link
+ * org.apache.polaris.core.auth.PolarisSecurable} paths.
+ */
+public final class PolarisSecurableMapper {
+ private PolarisSecurableMapper() {}
+
+ public static PolarisSecurable catalog(String catalogName) {
+ return PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ }
+
+ public static PolarisSecurable catalogRole(String catalogName, String catalogRoleName) {
+ return ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName))
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG_ROLE, catalogRoleName))
+ .build();
+ }
+
+ public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+ ImmutablePolarisSecurable.Builder builder =
+ ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ Arrays.stream(namespace.levels())
+ .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+ .forEach(builder::addPathSegment);
+ return builder.build();
Review Comment:
When `namespace` is `Namespace.empty()`, this builder returns a securable whose leaf type is `CATALOG` rather than `NAMESPACE`. Root-namespace operations (for example top-level `listNamespaces` / `sendNotification`) will therefore be presented to `resolveAuthorizationInputs()` as catalog targets, so a selective authorizer can resolve or authorize the wrong resource.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -325,7 +353,19 @@ private PolarisResolutionManifest authorizeGrantOnRootContainerToPrincipalRoleOp
PolarisResolutionManifest resolutionManifest = newResolutionManifest(null);
resolutionManifest.addTopLevelName(
principalRoleName, PolarisEntityType.PRINCIPAL_ROLE, false /* isOptional */);
- ResolverStatus status = resolutionManifest.resolveAll();
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authzState,
+ AuthorizationRequest.of(
+ polarisPrincipal,
+ op,
+ List.of(
+ AuthorizationTargetBinding.of(
+ null,
Review Comment:
The pre-resolution request only carries the grantee principal role, but the later authorization still uses the root container as the primary target. That means a selective authorizer has no explicit signal that it must resolve the ROOT resource for this operation, which breaks the new SPI contract as soon as an implementation stops resolving everything eagerly.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -227,7 +233,10 @@ private static CatalogRoleEntity getCatalogRoleByName(
private void authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation op) {
PolarisResolutionManifest resolutionManifest = newResolutionManifest(null);
- resolutionManifest.resolveAll();
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authzState, AuthorizationRequest.of(polarisPrincipal, op, List.of()));
Review Comment:
This passes an empty binding list to `resolveAuthorizationInputs()` even though the subsequent authorization is performed against the root container. Selective authorizers that drive resolution from `AuthorizationRequest` cannot discover that a ROOT path is required here, so root-scoped admin operations can fail once an authorizer stops doing unconditional `resolveAll()`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3439281123
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
Thanks for all the context, @sungwy . Do we expect adding more attributes to the `AuthorizationState`? Are they expected to be request scoped? It'd be nice to share more forward looking thoughts if you don't mind.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3456702890
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
Hi @flyrain I donโt currently have a concrete second attribute in mind for `AuthorizationState`. The main state I care about here is the `PolarisResolutionManifest`, because it represents the resolved view we use across authz and execution.
The forward-looking part is that this was meant to preserve todayโs resolution semantics while moving the decision of *what to resolve* into the `PolarisAuthorizer`. Longer term, I still think a larger Persistence/Resolution refactor is probably the cleaner direction, so handlers and authorizers are less coupled to a callsite-managed `PolarisResolutionManifest` directly. Keeping `AuthorizationState` request-scoped felt like a small step in that direction, but I agree it is not the full design.
Given that the larger refactor has not been designed yet, Iโm okay relaxing this for now and treating `AuthorizationState` as a per-resolution wrapper around the manifest. That keeps this PR focused on moving resolution choice into the authorizer, while leaving request-scoped persistence/resolution state for a more concrete follow-up design.
The tradeoff is that we lose the request-lifetime guard and keep responsibility for resolved-view consistency at the call sites, which is basically where it already lives today.
@flyrain , @dimas-b - does that sound like a good way to move this PR forward?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy merged PR #4356:
URL: https://github.com/apache/polaris/pull/4356
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3430102103
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -253,7 +269,15 @@ private PolarisResolutionManifest authorizeBasicTopLevelEntityOperationOrThrow(
@Nullable String referenceCatalogName) {
PolarisResolutionManifest resolutionManifest = newResolutionManifest(referenceCatalogName);
resolutionManifest.addTopLevelName(topLevelEntityName, entityType, false /* isOptional */);
- ResolverStatus status = resolutionManifest.resolveAll();
+ authorizationState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authorizationState,
+ new AuthorizationRequest(
+ polarisPrincipal,
+ List.of(
+ new SingleTargetAuthorizationIntent(
+ op, PolarisSecurable.of(new PathSegment(entityType, topLevelEntityName))))));
+ ResolverStatus status = resolutionManifest.getPrimaryResolverStatus();
Review Comment:
`getPrimaryResolverStatus()` is `@Nullable`, should we check it before using?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
`AuthorizationState` looks like a per-call value, not request-scoped shared state.
Tracing how it's used: the handler already holds the manifest in its own resolutionManifest field (CatalogHandler.java:96). Each authorize method does resolutionManifest = newResolutionManifest(), then authorizationState().setResolutionManifest(resolutionManifest), then reads results back from its own field, never from authorizationState().getResolutionManifest(). So AuthorizationState's only job is to be the parameter type carrying the manifest into resolveAuthorizationInputs(...) so the authorizer can read it back out.
That leaves two holders of the same reference with contradictory semantics: the handler field gets freely reassigned, but `AuthorizationState.setResolutionManifest` is write-once (compareAndSet(null, ...) that throws on a second set). The write-once guard only makes sense because the instance is @RequestScoped and shared, and it's effectively dead defensive code, since every call site just wants to wrap a freshly-built manifest and pass it along.
Since every setResolutionManifest is really "replace with the new manifest I just built," I think this should just be a fresh object per authorize call:
```
authorizer().resolveAuthorizationInputs(new AuthorizationState(resolutionManifest), request);
```
That would let us drop the `@Produces @RequestScoped producer`, the abstract `authorizationState()` accessor + injected factory fields, and the `AtomicReference/compareAndSet` guard (just a final field). This simplify the plumbing quite a bit. It also removes a latent landmine: any future flow that runs two authorize passes in one request would hit "resolution manifest already set" on the shared write-once instance.
If the intent was for AuthorizationState to accumulate state across auth phases, then the write-once guard contradicts that and we should design it as a real accumulator instead. Either way the current shape feels in-between. WDYT?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.common;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.auth.ImmutablePolarisSecurable;
+import org.apache.polaris.core.auth.PathSegment;
+import org.apache.polaris.core.auth.PolarisSecurable;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.service.types.PolicyAttachmentTarget;
+import org.apache.polaris.service.types.PolicyIdentifier;
+
+/**
+ * Utility mapper for translating Polaris domain identifiers into canonical {@link
+ * org.apache.polaris.core.auth.PolarisSecurable} paths.
+ */
+public final class PolarisSecurableMapper {
+ private PolarisSecurableMapper() {}
+
+ public static PolarisSecurable catalog(String catalogName) {
+ return PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ }
+
+ public static PolarisSecurable catalogRole(String catalogName, String catalogRoleName) {
+ return ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName))
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG_ROLE, catalogRoleName))
+ .build();
+ }
+
+ public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+ if (namespace.isEmpty()) {
+ throw new IllegalArgumentException("Namespace target must not be empty");
+ }
+ ImmutablePolarisSecurable.Builder builder =
+ ImmutablePolarisSecurable.builder()
+ .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+ Arrays.stream(namespace.levels())
+ .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+ .forEach(builder::addPathSegment);
+ return builder.build();
+ }
+
+ public static PolarisSecurable tableLike(String catalogName, TableIdentifier identifier) {
Review Comment:
`namespace()` throws on an empty namespace, but `tableLike()` here silently maps an empty namespace to `catalog/<name>`. Since this PR explicitly drops empty-namespace table-like support, add the same `identifier.namespace().isEmpty()` guard here rather than relying on the resolver to reject it downstream.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3439281123
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -78,6 +85,8 @@ public RealmContext realmContext() {
public abstract PolarisAuthorizer authorizer();
+ public abstract AuthorizationState authorizationState();
Review Comment:
Thanks for all the context, @sungwy . Do we expect adding more attributes to the `AuthorizationState`? Are they expected to be request scoped?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #4356:
URL: https://github.com/apache/polaris/pull/4356#discussion_r3188105367
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -325,7 +353,19 @@ private PolarisResolutionManifest authorizeGrantOnRootContainerToPrincipalRoleOp
PolarisResolutionManifest resolutionManifest = newResolutionManifest(null);
resolutionManifest.addTopLevelName(
principalRoleName, PolarisEntityType.PRINCIPAL_ROLE, false /* isOptional */);
- ResolverStatus status = resolutionManifest.resolveAll();
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ authorizer.resolveAuthorizationInputs(
+ authzState,
+ AuthorizationRequest.of(
+ polarisPrincipal,
+ op,
+ List.of(
+ AuthorizationTargetBinding.of(
+ null,
Review Comment:
This is an intended 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4356:
URL: https://github.com/apache/polaris/pull/4356#issuecomment-4605045562
@sungwy : Is this ready for another review round, I assume?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org