adutra commented on code in PR #3681:
URL: https://github.com/apache/polaris/pull/3681#discussion_r2793117597
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -170,6 +176,7 @@ public PolarisAdminService(
this.userSecretsManager = userSecretsManager;
this.serviceIdentityProvider = serviceIdentityProvider;
this.reservedProperties = reservedProperties;
+ this.authorizationState = authorizationState;
Review Comment:
Wondering: do we need to make this object a CDI request-scoped bean? I think
it could be instantiated by the handler manually, which would have the
advantage that we wouldn't need to expose setters in `AuthorizationState`.
That is, instead of:
```java
AuthorizationState authzContext = authorizationState();
authzContext.setResolutionManifest(resolutionManifest);
```
The handler would do:
```java
AuthorizationState authzContext = new
RequestAuthorizationState(resolutionManifest);
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import java.util.Set;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together while preserving legacy
semantics.
+ */
+public final class AuthorizationRequest {
Review Comment:
nit: now that polaris-core is on Java 17, this class, as well as
`PolarisSecurable`, could be records. Alternatively, they could be annotated
with `@PolarisImmutable`.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -908,4 +913,73 @@ public boolean hasTransitivePrivilege(
resolvedPath);
return false;
}
+
+ @Override
+ public void preAuthorize(@Nonnull AuthorizationState ctx, @Nonnull
AuthorizationRequest request) {
+ PolarisResolutionManifest manifest = ctx.getResolutionManifest();
+ if (manifest.hasResolution()) {
+ return;
+ }
+
+ // Phase 2: preserve existing RBAC ordering by resolving the manifest
within preAuthorize.
+ manifest.resolveSelections(
+ EnumSet.of(
+ Resolvable.CALLER_PRINCIPAL,
+ Resolvable.CALLER_PRINCIPAL_ROLES,
+ Resolvable.CATALOG_ROLES,
+ Resolvable.REFERENCE_CATALOG,
+ Resolvable.REQUESTED_PATHS,
+ Resolvable.TOP_LEVEL_ENTITIES));
+ }
+
+ @Override
+ public void authorize(@Nonnull AuthorizationState ctx, @Nonnull
AuthorizationRequest request) {
+ PolarisResolutionManifest manifest = ctx.getResolutionManifest();
+ Set<PolarisBaseEntity> activatedEntities =
+ manifest == null ? Set.of() :
manifest.getAllActivatedCatalogRoleAndPrincipalRoles();
Review Comment:
nit: `manifest` cannot be null here.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -27,14 +27,36 @@
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Pre-authorization hook for resolving authorizer-specific inputs.
+ *
+ * <p>Implementations may resolve or validate any inputs needed to make an
authorization decision.
+ */
+ void preAuthorize(@Nonnull AuthorizationState ctx, @Nonnull
AuthorizationRequest request);
Review Comment:
Just to confirm: are the new methods still expected to throw on deny?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -27,14 +27,36 @@
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Pre-authorization hook for resolving authorizer-specific inputs.
+ *
+ * <p>Implementations may resolve or validate any inputs needed to make an
authorization decision.
+ */
+ void preAuthorize(@Nonnull AuthorizationState ctx, @Nonnull
AuthorizationRequest request);
+ /** Core authorization entry point for the new SPI. */
+ void authorize(@Nonnull AuthorizationState ctx, @Nonnull
AuthorizationRequest request);
+
+ /**
+ * Backwards-compatible external API that throws on deny for legacy call
sites.
+ *
+ * <p>Default implementation delegates to {@link
#authorize(AuthorizationState,
+ * AuthorizationRequest)}.
+ */
+ default void authorizeOrThrow(
Review Comment:
I'm confused: I don't see this method used anywhere, and (contrary to what
the javadocs say) it's a new method, not a legacy one.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -217,23 +258,23 @@ private static CatalogEntity getCatalogByName(
private static CatalogRoleEntity getCatalogRoleByName(
PolarisResolutionManifest resolutionManifest, String catalogRoleName) {
- return
Optional.ofNullable(resolutionManifest.getResolvedPath(catalogRoleName))
+ PolarisSecurable catalogRoleSecurable =
+ new PolarisSecurable(PolarisEntityType.CATALOG_ROLE,
List.of(catalogRoleName));
+ return
Optional.ofNullable(resolutionManifest.getResolvedPath(catalogRoleSecurable))
.map(PolarisResolvedPathWrapper::getRawLeafEntity)
.map(CatalogRoleEntity::of)
.orElseThrow(() -> new NotFoundException("CatalogRole %s not found",
catalogRoleName));
}
private void authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation
op) {
PolarisResolutionManifest resolutionManifest = newResolutionManifest(null);
- resolutionManifest.resolveAll();
- PolarisResolvedPathWrapper rootContainerWrapper =
- resolutionManifest.getResolvedRootContainerEntityAsPath();
- authorizer.authorizeOrThrow(
- polarisPrincipal,
- resolutionManifest.getAllActivatedPrincipalRoleEntities(),
- op,
- rootContainerWrapper,
- null /* secondary */);
+ AuthorizationState authzContext = authorizationState;
+ authzContext.setResolutionManifest(resolutionManifest);
+ authorizer.preAuthorize(authzContext, newAuthorizationRequest(op));
+ PolarisSecurable rootSecurable =
Review Comment:
nit: I think the root securable could be a public constant in
`PolarisSecurable`:
```java
public static final PolarisSecurable ROOT =
new PolarisSecurable(
PolarisEntityType.ROOT,
List.of(PolarisEntityConstants.getRootContainerName()));
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -131,6 +147,29 @@ public void addPassthroughPath(ResolverPath path, Object
key) {
passthroughPaths.put(key, path);
}
+ /** Adds an alias key for a previously added path key. */
+ public void addPathAlias(Object existingKey, Object aliasKey) {
Review Comment:
This method looks a bit too low-level, could we avoid introducing it if we
change `addPath` to take several keys? E.g.
```java
public void addPath(ResolverPath path, Object... key) {
primaryResolver.addPath(path);
for (Object k : key) {
pathLookup.put(k, currentPathIndex);
}
addedPaths.add(path);
++currentPathIndex;
}
```
Then at call sites, instead of:
```java
resolutionManifest.addPath(
new ResolverPath(parentNamespaceSecurable.getNameParts(),
PolarisEntityType.NAMESPACE),
parentNamespaceSecurable);
resolutionManifest.addPathAlias(parentNamespaceSecurable,
parentNamespace);
```
We'd have:
```java
resolutionManifest.addPath(
new ResolverPath(parentNamespaceSecurable.getNameParts(),
PolarisEntityType.NAMESPACE),
parentNamespaceSecurable,
parentNamespace);
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -179,12 +237,17 @@ public PolarisResolvedPathWrapper getResolvedPath(
*/
@Override
public PolarisResolvedPathWrapper getPassthroughResolvedPath(Object key) {
- diagnostics.check(
- passthroughPaths.containsKey(key),
- "invalid_key_for_passthrough_resolved_path",
- "key={} passthroughPaths={}",
- key,
- passthroughPaths);
+ if (!passthroughPaths.containsKey(key)) {
+ if (pathLookup.containsKey(key)) {
+ return getResolvedPath(key);
+ }
Review Comment:
This if block looks like a change in the logic, can you explain why it is
needed?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.core.persistence.resolver;
+
+/** Explicit resolution components for {@link
PolarisResolutionManifest#resolveSelections}. */
+public enum Resolvable {
Review Comment:
Would you mind adding some javadocs explaining the scope of each constant?
It might be just me 😅 but I'm not immediately seeing what `TOP_LEVEL_ENTITIES`
is referring to: according to our docs, only catalogs are top-level entities.
Polaris code also considers principals and roles as top-level entities for
authorization purposes. But these entities are all covered already by
`CALLER_PRINCIPAL`,
`CALLER_PRINCIPAL_ROLES` and `REFERENCE_CATALOG`. Is `TOP_LEVEL_ENTITIES`
reserved to "optional" top-level entities?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogUtils.java:
##########
@@ -45,13 +47,19 @@ public class CatalogUtils {
*/
public static PolarisResolvedPathWrapper findResolvedStorageEntity(
PolarisResolutionManifestCatalogView resolvedEntityView, TableIdentifier
tableIdentifier) {
+ PolarisSecurable tableSecurable =
+ new PolarisSecurable(
+ PolarisEntityType.TABLE_LIKE,
+ PolarisCatalogHelpers.tableIdentifierToList(tableIdentifier));
PolarisResolvedPathWrapper resolvedTableEntities =
resolvedEntityView.getResolvedPath(
- tableIdentifier, PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE);
+ tableSecurable, PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE);
Review Comment:
Shouldn't the path have been registered also under the alias key
`tableIdentifier`?
I'm wondering if this change (and many other similar ones where a path is
looked up by a `PolarisSecurable` instead of some other key) is necessary.
Or is it because you plan to deprecate/remove lookup keys that are not
`PolarisSecurable` instances later on?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1315,12 +1318,30 @@ public void close() throws Exception {
}
public ConfigResponse getConfig() {
- Resolver resolver = resolverFactory().createResolver(polarisPrincipal(),
catalogName());
- ResolverStatus resolverStatus = resolver.resolveAll();
- if (!resolverStatus.getStatus().equals(ResolverStatus.StatusEnum.SUCCESS))
{
+ // 'catalogName' is taken from the REST request's 'warehouse' query
parameter.
+ // 'warehouse' as an output will be treated by the client as a default
catalog
+ // storage base location.
+ // 'prefix' as an output is the REST subpath that routes to the catalog
+ // resource, which may be URL-escaped catalogName or potentially a
different
+ // unique identifier for the catalog being accessed.
+ if (catalogName() == null) {
+ throw new BadRequestException("Please specify a warehouse");
+ }
Review Comment:
This has been moved to `IcebergCatalogAdapter`. Here, `catalogName()` is
guaranteed to never be null.
```suggestion
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -27,14 +27,36 @@
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Pre-authorization hook for resolving authorizer-specific inputs.
+ *
+ * <p>Implementations may resolve or validate any inputs needed to make an
authorization decision.
+ */
+ void preAuthorize(@Nonnull AuthorizationCallContext ctx, @Nonnull
AuthorizationRequest request);
+ /** Core authorization entry point for the new SPI. */
+ void authorize(@Nonnull AuthorizationCallContext ctx, @Nonnull
AuthorizationRequest request);
+
+ /**
+ * Backwards-compatible external API that throws on deny for legacy call
sites.
+ *
+ * <p>Default implementation delegates to {@link
#authorize(AuthorizationCallContext,
+ * AuthorizationRequest)}.
+ */
+ default void authorizeOrThrow(
+ @Nonnull AuthorizationCallContext ctx, @Nonnull AuthorizationRequest
request) {
+ authorize(ctx, request);
+ }
+
+ @Deprecated
void authorizeOrThrow(
@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
@Nonnull PolarisAuthorizableOperation authzOp,
@Nullable PolarisResolvedPathWrapper target,
@Nullable PolarisResolvedPathWrapper secondary);
+ @Deprecated
void authorizeOrThrow(
Review Comment:
I agree: this one seems used only in `OpaPolarisAuthorizerTest`.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -143,6 +182,25 @@ public ResolverStatus resolveAll() {
return primaryResolverStatus;
}
+ /**
+ * Resolves explicitly requested components.
+ *
+ * <p>Phase 1 behavior delegates to {@link #resolveAll()} to preserve
existing semantics until
+ * resolver selection is fully implemented.
+ */
+ public ResolverStatus resolveSelections(Set<Resolvable> selections) {
Review Comment:
nit: would `resolvePartially` better capture the opposition to `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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
