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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
