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/03/15 17:07:46 UTC
[PR] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
sungwy opened a new pull request, #3999:
URL: https://github.com/apache/polaris/pull/3999
This PR implements phases 2 and 3 of the authorization refactor tracking issue. (https://github.com/apache/polaris/issues/3779)
It introduces request-based authorization with `AuthorizationRequest`, in both `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer`. The new SPI method `authorize` is still not called by Polaris endpoint handlers. That will be done in phase 4 and 5 of the authorization refactoring effort. Hence, no behavior changes are expected in existing endpoint handling, except for `ROOT` handling in `OpaPolarisAuthorizer` noted below.
In order to decouple RBAC semantics from Polaris's core, we move RBAC operation semantics like _Privileges_ and newly introduced _Scope_ semantic into `RbacOperationSemantics`. The PR also adds `FullyQualifiedPath` as a lexical path with parent representation that `OpaPolarisAuthorizer` can use to construct the OPA request input payload.
### Breaking change
OPA input payloads no longer include `ROOT` in resource parent chains. This is intentional, since `ROOT` is an internal Polaris RBAC semantic and should not be part of the external OPA request contract
## 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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941945359
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
nit: swap argument maybe? In JRE's `Path` parents go first ๐ค
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
nit: swap arguments maybe? In JRE's `Path` parents go first ๐ค
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2942021097
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
sweet, let's merge that first :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2940485848
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
> Having both models in polaris-core seems a bit overkill.
I thought about this as well, I'm currently leaning towards keeping it in polaris-core because new PolarisAuthorizer implementations like the Ranger Authorizer may also need this.
> This model is imho richer than PolarisSecurable. I wonder if you couldn't merge both models together, e.g.:
I'm open to this idea. In order to correctly construct the parent though, we'd need to pass the `referenceCatalogName` to an arg referenceCatalogName arg here like
```
default PathSegment getParents(@Nullable String referenceCatalogName) {
...
}
```
Would that make sense?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2937027371
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
+
+ public FullyQualifiedPath {
+ Preconditions.checkNotNull(leaf, "leaf");
+ Preconditions.checkNotNull(parents, "parents");
+ parents = List.copyOf(parents);
+ }
+
+ public static FullyQualifiedPath of(
+ PolarisSecurable securable, @Nullable String referenceCatalogName) {
+ List<PathSegment> namePartSegments = namePartSegments(securable);
+ Preconditions.checkState(
+ !namePartSegments.isEmpty(),
+ "FullyQualifiedPath produced no segments for entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+
+ int leafIndex = namePartSegments.size() - 1;
+ PathSegment leaf = namePartSegments.get(leafIndex);
+ List<PathSegment> parents = new ArrayList<>();
+ if (isCatalogScopedType(securable.getEntityType())) {
+ Preconditions.checkArgument(
+ referenceCatalogName != null && !referenceCatalogName.isBlank(),
+ "referenceCatalogName must be non-empty for catalog-scoped securable entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+ parents.add(new PathSegment(PolarisEntityType.CATALOG, referenceCatalogName));
+ }
+ parents.addAll(namePartSegments.subList(0, leafIndex));
+ return new FullyQualifiedPath(leaf, parents);
+ }
+
+ /**
+ * Temporary adapter used while OPA serialization still accepts resolved paths alongside intent
+ * paths. Remove this overload when resolved-path callers are retired.
+ */
+ public static FullyQualifiedPath of(PolarisResolvedPathWrapper wrapper) {
+ Preconditions.checkNotNull(wrapper, "wrapper");
+ ResolvedPolarisEntity resolvedLeaf = wrapper.getResolvedLeafEntity();
+ Preconditions.checkNotNull(resolvedLeaf, "wrapper.getResolvedLeafEntity()");
+
+ PathSegment leaf =
+ new PathSegment(resolvedLeaf.getEntity().getType(), resolvedLeaf.getEntity().getName());
+ List<PathSegment> parents = new ArrayList<>();
+ List<ResolvedPolarisEntity> resolvedParents = wrapper.getResolvedParentPath();
+ if (resolvedParents != null) {
+ for (ResolvedPolarisEntity resolvedParent : resolvedParents) {
+ // Omit ROOT from fully qualified paths. It is an internal RBAC-only ancestor
+ if (resolvedParent.getEntity().getType() == PolarisEntityType.ROOT) {
+ continue;
+ }
Review Comment:
Note to reviewer: important behavior change to remove `ROOT` semantics from OPA request payload, since this is an RBAC-only semantic
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3034801207
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -18,23 +18,22 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
/** A resource binding containing a primary target and optional secondary. */
@PolarisImmutable
public interface AuthorizationTargetBinding {
static AuthorizationTargetBinding of(
- @Nonnull PolarisSecurable target, @Nullable PolarisSecurable secondary) {
+ @Nullable PolarisSecurable target, @Nullable PolarisSecurable secondary) {
Review Comment:
Reversing those arguments sounds reasonable to me, but it might be too late ๐ค
I wonder about @dennishuo 's view on this.
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -158,9 +175,15 @@ public void authorizeOrThrow(
@Nonnull PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries) {
- boolean allowed = queryOpa(polarisPrincipal, activatedEntities, authzOp, targets, secondaries);
+ boolean allowed =
+ queryOpa(
+ buildOpaAuthorizationInput(
+ polarisPrincipal,
+ authzOp,
+ toResourceEntitiesFromResolvedPaths(targets),
+ toResourceEntitiesFromResolvedPaths(secondaries)));
if (!allowed) {
- throw new ForbiddenException("OPA denied authorization");
+ throw new ForbiddenException("%s", "OPA denied authorization");
Review Comment:
nit: why such an odd message construct? ๐ค
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
+ : getResolvedSecurables(resolutionManifest, secondaries, prependRootContainer);
+ authorizeOrThrow(
+ request.getPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ request.getOperation(),
+ resolvedTargets,
+ resolvedSecondaries);
+ return AuthorizationDecision.allow();
+ } catch (ForbiddenException e) {
+ return AuthorizationDecision.deny(e.getMessage());
Review Comment:
nit: Ideally we should have the exception-throwing method delegate to the non-exception method (less runtime overhead), but I guess we can do that optimization later.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076824911
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
That's a great point. I'm just thinking loudly, but wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using? So I feel that this is rather safe information to expose to the user - if we decide to log the `AuthorizationDecision` message in the handlers.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3077112696
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> this will be logged in the Polaris logs, but excluded in the Polaris API response sent back to the user.
Agreed. We should be careful about the exception message. For example, https://github-personal/polaris-catalog/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java#L109-L109, it sends back as part of response. Otherwise, we should be good.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3031057774
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -18,23 +18,22 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
/** A resource binding containing a primary target and optional secondary. */
@PolarisImmutable
public interface AuthorizationTargetBinding {
static AuthorizationTargetBinding of(
- @Nonnull PolarisSecurable target, @Nullable PolarisSecurable secondary) {
+ @Nullable PolarisSecurable target, @Nullable PolarisSecurable secondary) {
Review Comment:
updated to support operations like `ADD_ROOT_GRANT_TO_PRINCIPAL_ROLE` that expect null target (ROOT) and a valid target (principal role) in its TargetBinding.
I wonder if we could have chosen a single target binding (_PRINCIPAL_ROLE_A_, _null_) instead of (_ROOT_, _PRINCIPAL_ROLE_A_) for this type of operation instead.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3032763125
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -245,108 +259,118 @@ private boolean queryOpaCheckResponse(ClassicHttpResponse response) throws IOExc
* configured OPA policies, which receive the raw principal information and must implement their
* own role/permission logic.
*
- * @param principal the principal requesting authorization
- * @param entities the set of activated entities
- * @param op the operation to authorize
- * @param targets the list of main target entities
- * @param secondaries the list of secondary entities (if any)
+ * @param input OPA authorization input model
* @return the OPA input JSON string
* @throws IOException if JSON serialization fails
*/
- private String buildOpaInputJson(
+ private String buildOpaInputJson(ImmutableOpaAuthorizationInput input) throws IOException {
+ // Wrap in OPA request
+ var request = ImmutableOpaRequest.builder().input(input).build();
+ return objectMapper.writeValueAsString(request);
+ }
+
+ private ImmutableOpaAuthorizationInput buildOpaAuthorizationInput(
PolarisPrincipal principal,
- Set<PolarisBaseEntity> entities,
PolarisAuthorizableOperation op,
- List<PolarisResolvedPathWrapper> targets,
- List<PolarisResolvedPathWrapper> secondaries)
- throws IOException {
+ List<ResourceEntity> targets,
+ List<ResourceEntity> secondaries) {
+ return ImmutableOpaAuthorizationInput.builder()
+ .actor(buildActor(principal))
+ .action(op.name())
+ .resource(buildResource(targets, secondaries))
+ .context(buildContext())
+ .build();
+ }
- // Build actor from principal
- var actor =
- ImmutableActor.builder()
- .principal(principal.getName())
- .addAllRoles(principal.getRoles())
- .build();
+ private ImmutableActor buildActor(PolarisPrincipal principal) {
+ return ImmutableActor.builder()
+ .principal(principal.getName())
+ .addAllRoles(principal.getRoles())
+ .build();
+ }
- // Build resource entities for targets
- List<ResourceEntity> targetEntities = new ArrayList<>();
- if (targets != null) {
- for (PolarisResolvedPathWrapper target : targets) {
- ResourceEntity entity = buildResourceEntity(target);
- if (entity != null) {
- targetEntities.add(entity);
- }
- }
+ private ImmutableContext buildContext() {
+ return ImmutableContext.builder().requestId(UUID.randomUUID().toString()).build();
+ }
+
+ private ImmutableResource buildResource(
+ List<ResourceEntity> targets, List<ResourceEntity> secondaries) {
+ // Backward compatibility: keep the existing OPA input shape with separate target and
+ // secondary lists. Future work can align this with AuthorizationTargetBinding semantics
+ // using binding tuples like [(target, secondary), ...].
+ return ImmutableResource.builder().targets(targets).secondaries(secondaries).build();
+ }
+
+ private ResourceEntity buildResourceEntity(PolarisSecurable securable) {
+ // This is the target shape we want going forward where we derive the OPA payload from
+ // PolarisSecurable. This will exclude RBAC-only concepts like ROOT container.
+ PathSegment leaf = securable.getLeaf();
+ var builder =
+ ImmutableResourceEntity.builder().type(leaf.entityType().name()).name(leaf.name());
+ List<ResourceEntity> parents = new ArrayList<>();
+ for (PathSegment parent : securable.getParents()) {
+ parents.add(
+ ImmutableResourceEntity.builder()
+ .type(parent.entityType().name())
+ .name(parent.name())
+ .build());
}
+ builder.parents(parents);
+ return builder.build();
+ }
- // Build resource entities for secondaries
- List<ResourceEntity> secondaryEntities = new ArrayList<>();
- if (secondaries != null) {
- for (PolarisResolvedPathWrapper secondary : secondaries) {
- ResourceEntity entity = buildResourceEntity(secondary);
- if (entity != null) {
- secondaryEntities.add(entity);
- }
+ private ResourceEntity buildResourceEntity(PolarisResolvedPathWrapper path) {
+ // Currently, authorizeOrThrow still evaluate through resolved paths, including
+ // root-scoped operations that may surface a resolved ROOT leaf. Preserve that legacy
+ // behavior for compatibility until those callers migrate to the intent-based flow.
+ ResolvedPolarisEntity resolvedLeaf = path.getResolvedLeafEntity();
Review Comment:
PR description calls out a breaking change that OPA input payloads no longer include `ROOT` in resource parent chains, but the legacy `authorizeOrThrow` path here still serializes all resolved parents (including `ROOT`) for backward compatibility. Please either update the PR description to scope the breaking change to the new intent-based `authorize(...)` flow only, or adjust this legacy serialization to also exclude `ROOT` if the breaking change is intended to apply to existing endpoint traffic.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
`authorize()` can pass `resolvedTargets = null` when the request has no targets and `prependRootContainer` is false (i.e., `ResolvedPathRooting.CATALOG`). Since `RbacOperationSemantics` enforces non-empty `targetPrivileges` for every operation, `authorizeOrThrow`/`isAuthorized` will then fail with an `IllegalStateException` instead of returning a deny decision. Consider explicitly validating that a target is present for non-ROOT-rooted operations and returning `AuthorizationDecision.deny(...)` (or throwing a `ForbiddenException`) with a clear message when targets are missing.
```suggestion
if (targets.isEmpty() && !prependRootContainer) {
throw new ForbiddenException(
"Authorization request for operation %s requires at least one target",
request.getOperation());
}
List<PolarisResolvedPathWrapper> resolvedTargets =
!targets.isEmpty()
? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
: List.of(resolutionManifest.getResolvedRootContainerEntityAsPath());
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
When `targets` is empty and `prependRootContainer` is true, `authorize()` uses `resolutionManifest.getResolvedRootContainerEntityAsPath()`. `PolarisResolutionManifest#getResolvedRootContainerEntityAsPath()` can currently wrap a null root entity (it returns `List.of(getResolvedRootContainerEntity())` and `getResolvedRootContainerEntity()` may return null), which can later cause NPEs in RBAC privilege evaluation. It would be safer to detect an unresolved root container here and fail closed with a deny decision (or throw a `ForbiddenException`) with a resolution error message.
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -245,108 +259,118 @@ private boolean queryOpaCheckResponse(ClassicHttpResponse response) throws IOExc
* configured OPA policies, which receive the raw principal information and must implement their
* own role/permission logic.
*
- * @param principal the principal requesting authorization
- * @param entities the set of activated entities
- * @param op the operation to authorize
- * @param targets the list of main target entities
- * @param secondaries the list of secondary entities (if any)
+ * @param input OPA authorization input model
* @return the OPA input JSON string
* @throws IOException if JSON serialization fails
*/
- private String buildOpaInputJson(
+ private String buildOpaInputJson(ImmutableOpaAuthorizationInput input) throws IOException {
+ // Wrap in OPA request
+ var request = ImmutableOpaRequest.builder().input(input).build();
+ return objectMapper.writeValueAsString(request);
+ }
+
+ private ImmutableOpaAuthorizationInput buildOpaAuthorizationInput(
PolarisPrincipal principal,
- Set<PolarisBaseEntity> entities,
PolarisAuthorizableOperation op,
- List<PolarisResolvedPathWrapper> targets,
- List<PolarisResolvedPathWrapper> secondaries)
- throws IOException {
+ List<ResourceEntity> targets,
+ List<ResourceEntity> secondaries) {
+ return ImmutableOpaAuthorizationInput.builder()
+ .actor(buildActor(principal))
+ .action(op.name())
+ .resource(buildResource(targets, secondaries))
+ .context(buildContext())
+ .build();
+ }
- // Build actor from principal
- var actor =
- ImmutableActor.builder()
- .principal(principal.getName())
- .addAllRoles(principal.getRoles())
- .build();
+ private ImmutableActor buildActor(PolarisPrincipal principal) {
+ return ImmutableActor.builder()
+ .principal(principal.getName())
+ .addAllRoles(principal.getRoles())
+ .build();
+ }
- // Build resource entities for targets
- List<ResourceEntity> targetEntities = new ArrayList<>();
- if (targets != null) {
- for (PolarisResolvedPathWrapper target : targets) {
- ResourceEntity entity = buildResourceEntity(target);
- if (entity != null) {
- targetEntities.add(entity);
- }
- }
+ private ImmutableContext buildContext() {
+ return ImmutableContext.builder().requestId(UUID.randomUUID().toString()).build();
+ }
+
+ private ImmutableResource buildResource(
+ List<ResourceEntity> targets, List<ResourceEntity> secondaries) {
+ // Backward compatibility: keep the existing OPA input shape with separate target and
+ // secondary lists. Future work can align this with AuthorizationTargetBinding semantics
+ // using binding tuples like [(target, secondary), ...].
+ return ImmutableResource.builder().targets(targets).secondaries(secondaries).build();
+ }
+
+ private ResourceEntity buildResourceEntity(PolarisSecurable securable) {
+ // This is the target shape we want going forward where we derive the OPA payload from
+ // PolarisSecurable. This will exclude RBAC-only concepts like ROOT container.
+ PathSegment leaf = securable.getLeaf();
+ var builder =
+ ImmutableResourceEntity.builder().type(leaf.entityType().name()).name(leaf.name());
+ List<ResourceEntity> parents = new ArrayList<>();
+ for (PathSegment parent : securable.getParents()) {
+ parents.add(
+ ImmutableResourceEntity.builder()
+ .type(parent.entityType().name())
+ .name(parent.name())
+ .build());
}
+ builder.parents(parents);
+ return builder.build();
+ }
- // Build resource entities for secondaries
- List<ResourceEntity> secondaryEntities = new ArrayList<>();
- if (secondaries != null) {
- for (PolarisResolvedPathWrapper secondary : secondaries) {
- ResourceEntity entity = buildResourceEntity(secondary);
- if (entity != null) {
- secondaryEntities.add(entity);
- }
+ private ResourceEntity buildResourceEntity(PolarisResolvedPathWrapper path) {
+ // Currently, authorizeOrThrow still evaluate through resolved paths, including
+ // root-scoped operations that may surface a resolved ROOT leaf. Preserve that legacy
+ // behavior for compatibility until those callers migrate to the intent-based flow.
+ ResolvedPolarisEntity resolvedLeaf = path.getResolvedLeafEntity();
+ PathSegment leaf =
+ new PathSegment(resolvedLeaf.getEntity().getType(), resolvedLeaf.getEntity().getName());
+ List<ResourceEntity> parents = new ArrayList<>();
+ List<ResolvedPolarisEntity> resolvedParents = path.getResolvedParentPath();
+ if (resolvedParents != null) {
+ for (ResolvedPolarisEntity resolvedParent : resolvedParents) {
+ parents.add(
+ ImmutableResourceEntity.builder()
+ .type(resolvedParent.getEntity().getType().name())
+ .name(resolvedParent.getEntity().getName())
+ .build());
}
Review Comment:
`buildResourceEntity(PolarisResolvedPathWrapper)` assumes every element in `path.getResolvedParentPath()` is non-null. `PolarisResolvedPathWrapper` can contain null entries (see its TODO about nulls in the chain), and `PolarisResolutionManifest` can prepend a null root container entity in some cases, which would make this loop throw an NPE on `resolvedParent.getEntity()`. Consider skipping null parent entries (and/or parents with null `getEntity()`) to make OPA payload construction robust.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3032741670
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
cc @dennishuo @dimas-b
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3033539831
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -245,108 +259,118 @@ private boolean queryOpaCheckResponse(ClassicHttpResponse response) throws IOExc
* configured OPA policies, which receive the raw principal information and must implement their
* own role/permission logic.
*
- * @param principal the principal requesting authorization
- * @param entities the set of activated entities
- * @param op the operation to authorize
- * @param targets the list of main target entities
- * @param secondaries the list of secondary entities (if any)
+ * @param input OPA authorization input model
* @return the OPA input JSON string
* @throws IOException if JSON serialization fails
*/
- private String buildOpaInputJson(
+ private String buildOpaInputJson(ImmutableOpaAuthorizationInput input) throws IOException {
+ // Wrap in OPA request
+ var request = ImmutableOpaRequest.builder().input(input).build();
+ return objectMapper.writeValueAsString(request);
+ }
+
+ private ImmutableOpaAuthorizationInput buildOpaAuthorizationInput(
PolarisPrincipal principal,
- Set<PolarisBaseEntity> entities,
PolarisAuthorizableOperation op,
- List<PolarisResolvedPathWrapper> targets,
- List<PolarisResolvedPathWrapper> secondaries)
- throws IOException {
+ List<ResourceEntity> targets,
+ List<ResourceEntity> secondaries) {
+ return ImmutableOpaAuthorizationInput.builder()
+ .actor(buildActor(principal))
+ .action(op.name())
+ .resource(buildResource(targets, secondaries))
+ .context(buildContext())
+ .build();
+ }
- // Build actor from principal
- var actor =
- ImmutableActor.builder()
- .principal(principal.getName())
- .addAllRoles(principal.getRoles())
- .build();
+ private ImmutableActor buildActor(PolarisPrincipal principal) {
+ return ImmutableActor.builder()
+ .principal(principal.getName())
+ .addAllRoles(principal.getRoles())
+ .build();
+ }
- // Build resource entities for targets
- List<ResourceEntity> targetEntities = new ArrayList<>();
- if (targets != null) {
- for (PolarisResolvedPathWrapper target : targets) {
- ResourceEntity entity = buildResourceEntity(target);
- if (entity != null) {
- targetEntities.add(entity);
- }
- }
+ private ImmutableContext buildContext() {
+ return ImmutableContext.builder().requestId(UUID.randomUUID().toString()).build();
+ }
+
+ private ImmutableResource buildResource(
+ List<ResourceEntity> targets, List<ResourceEntity> secondaries) {
+ // Backward compatibility: keep the existing OPA input shape with separate target and
+ // secondary lists. Future work can align this with AuthorizationTargetBinding semantics
+ // using binding tuples like [(target, secondary), ...].
+ return ImmutableResource.builder().targets(targets).secondaries(secondaries).build();
+ }
+
+ private ResourceEntity buildResourceEntity(PolarisSecurable securable) {
+ // This is the target shape we want going forward where we derive the OPA payload from
+ // PolarisSecurable. This will exclude RBAC-only concepts like ROOT container.
+ PathSegment leaf = securable.getLeaf();
+ var builder =
+ ImmutableResourceEntity.builder().type(leaf.entityType().name()).name(leaf.name());
+ List<ResourceEntity> parents = new ArrayList<>();
+ for (PathSegment parent : securable.getParents()) {
+ parents.add(
+ ImmutableResourceEntity.builder()
+ .type(parent.entityType().name())
+ .name(parent.name())
+ .build());
}
+ builder.parents(parents);
+ return builder.build();
+ }
- // Build resource entities for secondaries
- List<ResourceEntity> secondaryEntities = new ArrayList<>();
- if (secondaries != null) {
- for (PolarisResolvedPathWrapper secondary : secondaries) {
- ResourceEntity entity = buildResourceEntity(secondary);
- if (entity != null) {
- secondaryEntities.add(entity);
- }
+ private ResourceEntity buildResourceEntity(PolarisResolvedPathWrapper path) {
+ // Currently, authorizeOrThrow still evaluate through resolved paths, including
+ // root-scoped operations that may surface a resolved ROOT leaf. Preserve that legacy
+ // behavior for compatibility until those callers migrate to the intent-based flow.
+ ResolvedPolarisEntity resolvedLeaf = path.getResolvedLeafEntity();
+ PathSegment leaf =
+ new PathSegment(resolvedLeaf.getEntity().getType(), resolvedLeaf.getEntity().getName());
+ List<ResourceEntity> parents = new ArrayList<>();
+ List<ResolvedPolarisEntity> resolvedParents = path.getResolvedParentPath();
+ if (resolvedParents != null) {
+ for (ResolvedPolarisEntity resolvedParent : resolvedParents) {
+ parents.add(
+ ImmutableResourceEntity.builder()
+ .type(resolvedParent.getEntity().getType().name())
+ .name(resolvedParent.getEntity().getName())
+ .build());
}
Review Comment:
this is only the case in the legacy API and is the current behavior.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2943983526
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
Good point, but `parents` could be an empty list though. Feels counterintuitive to have an empty list as the first argument as well
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076488325
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
This is used in _denied_ responses ๐ค I'm inclined to be cautious about including a lot of information in them to avoid accidental exposure to untrustworthy clients. I believe it would be preferable to log details for the admin user to read later and send a UUID for the logged details to the client.
That said, I'll leave the decision to you @sungwy . In any case this kind of usability adjustments might be best done in a separate PR to avoid mixing pure refactoring related to SPI changes and functional changes.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2937819734
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +755,75 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ // Delegate to legacy authorizeOrThrow() to preserve existing RBAC privilege
+ // evaluation semantics. A later cleanup can refactor internals to a
+ // decision-first approach and remove this exception-to-decision adaptation.
+ try {
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ !semantics.hasSecondaryPrivileges() || request.getSecondaries().isEmpty()
+ ? null
+ : getResolvedSecurables(
+ resolutionManifest, semantics, request.getSecondaries(), TargetType.SECONDARY);
+ authorizeOrThrow(
+ request.getPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ request.getOperation(),
+ getResolvedSecurables(
+ resolutionManifest, semantics, request.getTargets(), TargetType.TARGET),
+ resolvedSecondaries);
+ return AuthorizationDecision.allow();
+ } catch (ForbiddenException e) {
+ return AuthorizationDecision.deny(e.getMessage());
+ }
+ }
+
+ private List<PolarisResolvedPathWrapper> getResolvedSecurables(
+ PolarisResolutionManifest resolutionManifest,
+ RbacOperationSemantics semantics,
+ List<PolarisSecurable> securables,
+ TargetType targetType) {
+ boolean prependRootContainer =
+ switch (targetType) {
+ case TARGET -> semantics.targetScope() == PathEvaluationScope.ROOT;
Review Comment:
Hi @jbonofre - thanks for the early feedback on the PR :)
I was confused initially as well, so your confusion isn't unfounded. the `ROOT` scope here refers to whether `ROOT` container should be prepended to the resolved path, and hence whether the `ROOT` container resource should be included for privilege evaluation.
For example, `PolarisAuthorizableOperation.LIST_NAMESPACES` prepends root container:
https://github.com/apache/polaris/blob/00061ff0dc44570eba8d3ce72835ab66758baa7f/runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java#L135
`PolarisAuthorizableOperation.GET_APPLICABLE_POLICIES_ON_CATALOG` sets prependRootContainer to `false`.
https://github.com/apache/polaris/blob/00061ff0dc44570eba8d3ce72835ab66758baa7f/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java#L195
I think the name `PathEvaluationScope` may be misleading. I'll think of a better name for this enum
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2940192496
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -0,0 +1,713 @@
+/*
+ * 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 static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_ATTACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DETACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_READ;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_WRITE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_RESET_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROTATE_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.SERVICE_MANAGE_ACCESS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_PARTITION_SPEC;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SNAPSHOT;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ASSIGN_UUID;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PARTITION_SPECS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_CURRENT_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_DEFAULT_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_LOCATION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_UPGRADE_FORMAT_VERSION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_WRITE_PROPERTIES;
+
+import com.google.common.base.Preconditions;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+/** Polaris RBAC-specific interpretation of an authorizable operation. */
+record RbacOperationSemantics(
+ EnumSet<PolarisPrivilege> targetPrivileges,
+ EnumSet<PolarisPrivilege> secondaryPrivileges,
+ ResolvedPathRooting targetScope,
+ ResolvedPathRooting secondaryScope) {
+
+ private RbacOperationSemantics(
Review Comment:
Not sure we need this constructor, why don't you update `RBAC_SEMANTICS_BY_OPERATION` using `EnumSet` as below:
```java
RBAC_SEMANTICS_BY_OPERATION.put(
PolarisAuthorizableOperation.LIST_NAMESPACES,
new RbacOperationSemantics(EnumSet.of(NAMESPACE_LIST), null, ResolvedPathRooting.ROOT, null));
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -0,0 +1,713 @@
+/*
+ * 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 static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_ATTACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DETACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_READ;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_WRITE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_RESET_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROTATE_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.SERVICE_MANAGE_ACCESS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_PARTITION_SPEC;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SNAPSHOT;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ASSIGN_UUID;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PARTITION_SPECS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_CURRENT_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_DEFAULT_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_LOCATION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_UPGRADE_FORMAT_VERSION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_WRITE_PROPERTIES;
+
+import com.google.common.base.Preconditions;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+/** Polaris RBAC-specific interpretation of an authorizable operation. */
+record RbacOperationSemantics(
+ EnumSet<PolarisPrivilege> targetPrivileges,
+ EnumSet<PolarisPrivilege> secondaryPrivileges,
+ ResolvedPathRooting targetScope,
+ ResolvedPathRooting secondaryScope) {
Review Comment:
`secondaryScope` is either null (90% of cases), or identical to `targetScope`. Can we use one single `ResolvedPathRooting` maybe?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
Nice!
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
+
+ public FullyQualifiedPath {
+ Preconditions.checkNotNull(leaf, "leaf");
+ Preconditions.checkNotNull(parents, "parents");
+ parents = List.copyOf(parents);
+ }
+
+ public static FullyQualifiedPath of(
+ PolarisSecurable securable, @Nullable String referenceCatalogName) {
+ List<PathSegment> namePartSegments = namePartSegments(securable);
+ Preconditions.checkState(
+ !namePartSegments.isEmpty(),
+ "FullyQualifiedPath produced no segments for entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+
+ int leafIndex = namePartSegments.size() - 1;
+ PathSegment leaf = namePartSegments.get(leafIndex);
+ List<PathSegment> parents = new ArrayList<>();
+ if (isCatalogScopedType(securable.getEntityType())) {
+ Preconditions.checkArgument(
+ referenceCatalogName != null && !referenceCatalogName.isBlank(),
+ "referenceCatalogName must be non-empty for catalog-scoped securable entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+ parents.add(new PathSegment(PolarisEntityType.CATALOG, referenceCatalogName));
+ }
+ parents.addAll(namePartSegments.subList(0, leafIndex));
+ return new FullyQualifiedPath(leaf, parents);
+ }
+
+ /**
+ * Temporary adapter used while OPA serialization still accepts resolved paths alongside intent
+ * paths. Remove this overload when resolved-path callers are retired.
+ */
+ public static FullyQualifiedPath of(PolarisResolvedPathWrapper wrapper) {
+ Preconditions.checkNotNull(wrapper, "wrapper");
+ ResolvedPolarisEntity resolvedLeaf = wrapper.getResolvedLeafEntity();
+ Preconditions.checkNotNull(resolvedLeaf, "wrapper.getResolvedLeafEntity()");
+
+ PathSegment leaf =
+ new PathSegment(resolvedLeaf.getEntity().getType(), resolvedLeaf.getEntity().getName());
+ List<PathSegment> parents = new ArrayList<>();
+ List<ResolvedPolarisEntity> resolvedParents = wrapper.getResolvedParentPath();
+ if (resolvedParents != null) {
+ for (ResolvedPolarisEntity resolvedParent : resolvedParents) {
+ // Omit ROOT from fully qualified paths. It is an internal RBAC-only ancestor
+ if (resolvedParent.getEntity().getType() == PolarisEntityType.ROOT) {
+ continue;
+ }
Review Comment:
Ack, but we are introducing a specific behavior in polaris-core. Shouldn't we instead keep the `ROOT` entity here, and let specific authorizers filter it out? I feel like the `ROOT` case deserves some more thinking. It's also troubling that the current code sometimes prepends it, and sometimes not.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -56,6 +67,13 @@ static AuthorizationRequest of(
@Nonnull
List<AuthorizationTargetBinding> getTargetBindings();
+ /** Returns the request reference catalog name, if provided by the caller. */
+ @Nullable
+ @Value.Default
+ default String getReferenceCatalogName() {
+ return null;
+ }
Review Comment:
This should be enough:
```suggestion
@Nullable
String getReferenceCatalogName();
```
But how about using an `Optional` here?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
But this class is in polaris-core, which tends to indicate that it's not only for the OPA authorizer. Having both models in polaris-core seems a bit overkill.
This model is imho richer than `PolarisSecurable`. I wonder if you couldn't merge both models together, e.g.:
```java
@PolarisImmutable
public interface PolarisSecurable {
/** Returns the path segments that identify the securable, in hierarchical order. */
@Nonnull
List<PathSegment> getPathSegments();
default PathSegment getLeafPathSegment() {
return getPathSegments().getLast();
}
@Value.Check
default void validate() {
Preconditions.checkState(!getPathSegments().isEmpty(), "path segments must not be 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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3031067896
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
I am not a fan of this fallback logic to replace empty target with ROOT. I think we should really review what `prependingRootContainer` boolean means, and review whether we can safely combine that semantic with whether ROOT container should be the top level container for all resource targets (target & secondary) when evaluating a specific operation.
The semantic as it is asks two separate questions:
1. should we prepend Root container when resolving path from the resolution manifest?
2. should we have ROOT container in place of an empty target for this operation?
Maintaining these two is complicated, and we should know if the overhead of managing them as separate semantics is absolutely necessary.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076837507
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
I also think it's likely that this will be logged in the Polaris logs, but excluded in the Polaris API response sent back to the user. In that case, that feels even more safe to keep this in the `AuthorizationDecision` message, as it'll be up to the system integrator to decide whether those logs should be exposed to the end user.
I appreciate your suggestion to merge in this PR and make adjustments in the followup PRs. I'll add an item in the Polaris Authorization Sync to discuss this as well.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079693939
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
All in all, I do not wish to block this PR, but I'd be more comfortable if we did not have this level of message details in this PR (essentially technical SPI change), but improved this area in a separate PR dedicated to AuthZ usability.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079638347
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> [...] wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using?
Partially. I did not review the message in depth, but the client operates on names, but the message might contain IDs, etc. My concern is about about legitimate clients, but about malicious clients.
Re: principal, if the auth token is stolen, the malicious client will not know the principal name, but may discover it via this message.
It's not a critical issue, though, IMHO, just trying to be proactive in tightening security :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2942051797
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
> What do you folks think about just replacing PolarisSecurable with FullyQualifiedPath's shape instead?
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079714904
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
I forgot about the fact that the token is a part of the `PolarisPrincipal` - I hope we can revise that at some point.
I'll move forward with this PR, but maybe we could discuss more through this on the PR where we switch the handler call sites to use `authorize` instead of `authorizeOrThrow` - thanks for the thoughtful discussion @dimas-b
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2937076921
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +755,75 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ // Delegate to legacy authorizeOrThrow() to preserve existing RBAC privilege
+ // evaluation semantics. A later cleanup can refactor internals to a
+ // decision-first approach and remove this exception-to-decision adaptation.
+ try {
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ !semantics.hasSecondaryPrivileges() || request.getSecondaries().isEmpty()
+ ? null
+ : getResolvedSecurables(
+ resolutionManifest, semantics, request.getSecondaries(), TargetType.SECONDARY);
+ authorizeOrThrow(
+ request.getPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ request.getOperation(),
+ getResolvedSecurables(
+ resolutionManifest, semantics, request.getTargets(), TargetType.TARGET),
+ resolvedSecondaries);
+ return AuthorizationDecision.allow();
+ } catch (ForbiddenException e) {
+ return AuthorizationDecision.deny(e.getMessage());
+ }
+ }
+
+ private List<PolarisResolvedPathWrapper> getResolvedSecurables(
+ PolarisResolutionManifest resolutionManifest,
+ RbacOperationSemantics semantics,
+ List<PolarisSecurable> securables,
+ TargetType targetType) {
+ boolean prependRootContainer =
+ switch (targetType) {
+ case TARGET -> semantics.targetScope() == PathEvaluationScope.ROOT;
Review Comment:
I'm a bit confused here. Every operation uses `PathEvaluationScope.ROOT` except the policy attach/detach operations which use `CATALOG`.
Is this correct for catalog-scoped operations like `LIST_NAMESPACES`, `LIST_TABLES`, ...
These operations semantically happen within a catalog context. If the existing `authorizeOrThrow` callers already prepend the root container themselves, then `ROOT` make sense for backward compatibility. Maybe worth to add a comment here to clarify.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941954890
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -0,0 +1,713 @@
+/*
+ * 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 static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_ATTACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DETACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_READ;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_WRITE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_RESET_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROTATE_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.SERVICE_MANAGE_ACCESS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_PARTITION_SPEC;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SNAPSHOT;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ASSIGN_UUID;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PARTITION_SPECS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_CURRENT_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_DEFAULT_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_LOCATION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_UPGRADE_FORMAT_VERSION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_WRITE_PROPERTIES;
+
+import com.google.common.base.Preconditions;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+/** Polaris RBAC-specific interpretation of an authorizable operation. */
+record RbacOperationSemantics(
+ EnumSet<PolarisPrivilege> targetPrivileges,
+ EnumSet<PolarisPrivilege> secondaryPrivileges,
+ ResolvedPathRooting targetScope,
+ ResolvedPathRooting secondaryScope) {
Review Comment:
Yeah that's a great insight, but I also didn't want to make assumptions so I went with a safer approach of keeping them separate (feels more logical to do so)
On second thought, maybe we keep them as the same for now, since this is an internally used SPI, maybe it's okay to change it to decouple them in the future if we need to
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3039751562
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
+ : getResolvedSecurables(resolutionManifest, secondaries, prependRootContainer);
+ authorizeOrThrow(
+ request.getPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ request.getOperation(),
+ resolvedTargets,
+ resolvedSecondaries);
+ return AuthorizationDecision.allow();
+ } catch (ForbiddenException e) {
+ return AuthorizationDecision.deny(e.getMessage());
Review Comment:
nit: maybe add a DEBUG log for the exception (just in case someone needs it for trouble-shooting)?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
Review Comment:
nit: negation of the ternary operator's condition makes the larger expression a bit hard to follow
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3046094488
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
+ : getResolvedSecurables(resolutionManifest, secondaries, prependRootContainer);
+ authorizeOrThrow(
+ request.getPrincipal(),
+ resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+ request.getOperation(),
+ resolvedTargets,
+ resolvedSecondaries);
+ return AuthorizationDecision.allow();
+ } catch (ForbiddenException e) {
+ return AuthorizationDecision.deny(e.getMessage());
+ }
+ }
+
+ private List<PolarisResolvedPathWrapper> getResolvedSecurables(
+ PolarisResolutionManifest resolutionManifest,
+ List<PolarisSecurable> securables,
+ boolean prependRootContainer) {
+ return securables.stream()
+ .map(
+ securable -> {
+ PolarisResolvedPathWrapper resolvedSecurable =
+ getResolvedSecurable(resolutionManifest, securable, prependRootContainer);
+ Preconditions.checkState(
+ resolvedSecurable != null,
+ "Resolved path for securable is null for entityType=%s leaf=%s parents=%s",
+ securable.getLeaf().entityType(),
+ securable.getLeaf(),
+ securable.getParents());
+ return resolvedSecurable;
+ })
+ .toList();
+ }
+
+ private PolarisResolvedPathWrapper getResolvedSecurable(
+ PolarisResolutionManifest resolutionManifest,
+ PolarisSecurable securable,
+ boolean prependRootContainer) {
+ if (securable.getLeaf().entityType().isTopLevel()) {
+ return resolutionManifest.getResolvedTopLevelEntity(
Review Comment:
```suggestion
// Ignore prependRootContainer for top-level entities.
return resolutionManifest.getResolvedTopLevelEntity(
```
(just for clarity)
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -18,23 +18,22 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
/** A resource binding containing a primary target and optional secondary. */
@PolarisImmutable
public interface AuthorizationTargetBinding {
static AuthorizationTargetBinding of(
- @Nonnull PolarisSecurable target, @Nullable PolarisSecurable secondary) {
+ @Nullable PolarisSecurable target, @Nullable PolarisSecurable secondary) {
return ImmutableAuthorizationTargetBinding.builder()
.target(target)
.secondary(secondary)
.build();
}
- /** Returns the primary target securable for the binding. */
- @Nonnull
+ /** Returns the primary target securable for the binding, if any. */
+ @Nullable
Review Comment:
Is `AuthorizationTargetBinding.of(null, null)` a valid call? If not, I'd suggest adding a check method.
##########
polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisAuthorizerImplTest.java:
##########
@@ -39,4 +55,140 @@ void subsumingPrivilegesOf(PolarisPrivilege privilege) {
: EnumSet.of(privilege);
assertThat(actual).isEqualTo(expected);
}
+
+ @Test
+ void authorizeUsesRootTargetForRootGrantRequestWithoutPrimaryTarget() {
+ // Verify that new authorize SPI call without primary target uses root_container
+ // for resolution and authorization
+ PolarisAuthorizerImpl authorizer = spy(new PolarisAuthorizerImpl(mock(RealmConfig.class)));
+ AuthorizationState authzState = new AuthorizationState();
+ PolarisResolutionManifest manifest = mock(PolarisResolutionManifest.class);
+ PolarisResolvedPathWrapper rootWrapper = mock(PolarisResolvedPathWrapper.class);
+ PolarisResolvedPathWrapper principalRoleWrapper = mock(PolarisResolvedPathWrapper.class);
+
+ authzState.setResolutionManifest(manifest);
+ when(manifest.getResolvedRootContainerEntityAsPath()).thenReturn(rootWrapper);
+ when(manifest.getResolvedTopLevelEntity("analytics-admin", PolarisEntityType.PRINCIPAL_ROLE))
+ .thenReturn(principalRoleWrapper);
+ when(manifest.getAllActivatedCatalogRoleAndPrincipalRoles())
+ .thenReturn(Set.<PolarisBaseEntity>of());
Review Comment:
Nit:
```suggestion
.thenReturn(Set.of());
```
Same for other occurrences below.
##########
polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisAuthorizerImplTest.java:
##########
@@ -39,4 +55,140 @@ void subsumingPrivilegesOf(PolarisPrivilege privilege) {
: EnumSet.of(privilege);
assertThat(actual).isEqualTo(expected);
}
+
+ @Test
+ void authorizeUsesRootTargetForRootGrantRequestWithoutPrimaryTarget() {
+ // Verify that new authorize SPI call without primary target uses root_container
+ // for resolution and authorization
+ PolarisAuthorizerImpl authorizer = spy(new PolarisAuthorizerImpl(mock(RealmConfig.class)));
+ AuthorizationState authzState = new AuthorizationState();
+ PolarisResolutionManifest manifest = mock(PolarisResolutionManifest.class);
+ PolarisResolvedPathWrapper rootWrapper = mock(PolarisResolvedPathWrapper.class);
+ PolarisResolvedPathWrapper principalRoleWrapper = mock(PolarisResolvedPathWrapper.class);
+
+ authzState.setResolutionManifest(manifest);
+ when(manifest.getResolvedRootContainerEntityAsPath()).thenReturn(rootWrapper);
+ when(manifest.getResolvedTopLevelEntity("analytics-admin", PolarisEntityType.PRINCIPAL_ROLE))
+ .thenReturn(principalRoleWrapper);
+ when(manifest.getAllActivatedCatalogRoleAndPrincipalRoles())
+ .thenReturn(Set.<PolarisBaseEntity>of());
+ doNothing()
+ .when(authorizer)
+ .authorizeOrThrow(
+ any(PolarisPrincipal.class),
+ org.mockito.ArgumentMatchers.<Set<PolarisBaseEntity>>any(),
+ eq(PolarisAuthorizableOperation.ADD_ROOT_GRANT_TO_PRINCIPAL_ROLE),
+ org.mockito.ArgumentMatchers.<List<PolarisResolvedPathWrapper>>any(),
+ org.mockito.ArgumentMatchers.<List<PolarisResolvedPathWrapper>>any());
Review Comment:
Nit:
```suggestion
doNothing()
.when(authorizer)
.authorizeOrThrow(
any(PolarisPrincipal.class),
ArgumentMatchers.any(),
eq(PolarisAuthorizableOperation.ADD_ROOT_GRANT_TO_PRINCIPAL_ROLE),
ArgumentMatchers.any(),
ArgumentMatchers.<List<PolarisResolvedPathWrapper>>any());
```
Same for other occurrences below.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
Review Comment:
So, there seems to be a disymmetry between targets and secondaries: we never prepend the ROOT container for secondaries. Is this on purpose or just mirroring the previous status quo?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2940495190
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
+
+ public FullyQualifiedPath {
+ Preconditions.checkNotNull(leaf, "leaf");
+ Preconditions.checkNotNull(parents, "parents");
+ parents = List.copyOf(parents);
+ }
+
+ public static FullyQualifiedPath of(
+ PolarisSecurable securable, @Nullable String referenceCatalogName) {
+ List<PathSegment> namePartSegments = namePartSegments(securable);
+ Preconditions.checkState(
+ !namePartSegments.isEmpty(),
+ "FullyQualifiedPath produced no segments for entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+
+ int leafIndex = namePartSegments.size() - 1;
+ PathSegment leaf = namePartSegments.get(leafIndex);
+ List<PathSegment> parents = new ArrayList<>();
+ if (isCatalogScopedType(securable.getEntityType())) {
+ Preconditions.checkArgument(
+ referenceCatalogName != null && !referenceCatalogName.isBlank(),
+ "referenceCatalogName must be non-empty for catalog-scoped securable entityType=%s nameParts=%s",
+ securable.getEntityType(),
+ securable.getNameParts());
+ parents.add(new PathSegment(PolarisEntityType.CATALOG, referenceCatalogName));
+ }
+ parents.addAll(namePartSegments.subList(0, leafIndex));
+ return new FullyQualifiedPath(leaf, parents);
+ }
+
+ /**
+ * Temporary adapter used while OPA serialization still accepts resolved paths alongside intent
+ * paths. Remove this overload when resolved-path callers are retired.
+ */
+ public static FullyQualifiedPath of(PolarisResolvedPathWrapper wrapper) {
+ Preconditions.checkNotNull(wrapper, "wrapper");
+ ResolvedPolarisEntity resolvedLeaf = wrapper.getResolvedLeafEntity();
+ Preconditions.checkNotNull(resolvedLeaf, "wrapper.getResolvedLeafEntity()");
+
+ PathSegment leaf =
+ new PathSegment(resolvedLeaf.getEntity().getType(), resolvedLeaf.getEntity().getName());
+ List<PathSegment> parents = new ArrayList<>();
+ List<ResolvedPolarisEntity> resolvedParents = wrapper.getResolvedParentPath();
+ if (resolvedParents != null) {
+ for (ResolvedPolarisEntity resolvedParent : resolvedParents) {
+ // Omit ROOT from fully qualified paths. It is an internal RBAC-only ancestor
+ if (resolvedParent.getEntity().getType() == PolarisEntityType.ROOT) {
+ continue;
+ }
Review Comment:
I fully agree.
```
public static FullyQualifiedPath of(PolarisResolvedPathWrapper wrapper)
```
This is only a temporary constructor introduced for compatibility, and will be removed from `FullyQualifiedPath` in the future. Which means `ROOT` semantics will no longer be in this module.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941932117
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
+1 to this sub-section of the current PR. Does it depend on other changes a lot? I think it might be preferable to do this part in a dedicated PR. This way, OPA changes will hopefully be isolated from the internal RBAC changes in that commit (or at least less intertwined) WDYT?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3034960270
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
I'll defer to @dennishuo 's opinion on 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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076824911
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
That's a great point. I'm just thinking loudly, but wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using? So I feel that this is rather safe information to expose to the user - even if we decide to log the `AuthorizationDecision` message in the handlers in the future.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079714904
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
I forgot about the fact that the token is a part of the `PolarisPrincipal` - I hope we can revise that at some point.
I'll move forward with this PR, but maybe we could discuss this idea more thoroughly on the PR where we switch the handler call sites to use `authorize` instead of `authorizeOrThrow` - thanks for the thoughtful discussion @dimas-b
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy merged PR #3999:
URL: https://github.com/apache/polaris/pull/3999
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941947792
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
Honestly, I had that thought as well. I'm happy to do it in a dedicated PR...
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076441549
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -98,18 +101,32 @@ public OpaPolarisAuthorizer(
this.objectMapper = objectMapper;
}
+ /**
+ * Resolves authorization inputs using {@code resolveAll()} for backward compatibility.
+ *
+ * <p>This scope is intentionally broad for now and will be narrowed in a future refactoring to
+ * resolve only the selections required by OPA authorization.
+ */
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for OpaPolarisAuthorizer");
+ authzState.getResolutionManifest().resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for OpaPolarisAuthorizer");
+ boolean allowed =
+ queryOpa(
+ buildOpaAuthorizationInput(
+ request.getPrincipal(),
+ request.getOperation(),
+ toResourceEntitiesFromSecurables(request.getTargets()),
+ toResourceEntitiesFromSecurables(request.getSecondaries())));
+ return allowed
+ ? AuthorizationDecision.allow()
+ : AuthorizationDecision.deny("OPA denied authorization");
Review Comment:
yes, I was just mimicking the status quo, but I think it's a good idea to at least string format the `AuthorizationRequest` input. In the future, I think we could also enhance this to include the status response of OPA request as well (e.g. `200` & `false` vs `403` vs `401`)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3066037647
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -18,23 +18,22 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
/** A resource binding containing a primary target and optional secondary. */
@PolarisImmutable
public interface AuthorizationTargetBinding {
static AuthorizationTargetBinding of(
- @Nonnull PolarisSecurable target, @Nullable PolarisSecurable secondary) {
+ @Nullable PolarisSecurable target, @Nullable PolarisSecurable secondary) {
return ImmutableAuthorizationTargetBinding.builder()
.target(target)
.secondary(secondary)
.build();
}
- /** Returns the primary target securable for the binding. */
- @Nonnull
+ /** Returns the primary target securable for the binding, if any. */
+ @Nullable
Review Comment:
That's a good idea. Thanks @adutra
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079638347
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> [...] wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using?
Partially. My concern is about about legitimate clients, but about malicious clients. I suppose the message could be used to deduce the existence of certain entities.
Re: principal, if the auth token is stolen, the malicious client will not know the principal name, but may discover it via this message.
Re: operation, I'm thinking that exposing it to clients is more like exposing internal impl. details. They make sense to the admin user / developer, but if clients start relying on observing that data in responses (and they will) it will complicate future internal refactorings.
It's not a critical issue, though, IMHO, just trying to be proactive in tightening security :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079737600
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
+1 to @flyrain message (the link did not work for me, BTW), `IcebergExceptionMapper` does appear to pass the exception message to clients verbatim.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3077112696
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> this will be logged in the Polaris logs, but excluded in the Polaris API response sent back to the user.
Agreed. We should be careful about the exception message. For example, https://github.com/polaris-catalog/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java#L109-L109, it sends back as part of response. Otherwise, we should be good.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #3999:
URL: https://github.com/apache/polaris/pull/3999#issuecomment-4130496600
Hi @flyrain - not yet, I will need to update the branch with some recent changes.
I've been at KubeCon this week, I'll get back to it soon :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076494023
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
Moreover, I'd like to standardize AuthZ error reporting across all authorizers to avoid exposing the configured authZ to clients too (future work).
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079638347
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> [...] wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using?
Partially. I did not review the message in depth, but the client operates on names, but the message might contain IDs, etc.
Re: principal, if the auth token is stolen, the malicious client will not know the principal name, but may discover it via this message.
It's not a critical issue, though, IMHO, just trying to be proactive in tightening security :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3066081832
##########
polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisAuthorizerImplTest.java:
##########
@@ -39,4 +55,140 @@ void subsumingPrivilegesOf(PolarisPrivilege privilege) {
: EnumSet.of(privilege);
assertThat(actual).isEqualTo(expected);
}
+
+ @Test
+ void authorizeUsesRootTargetForRootGrantRequestWithoutPrimaryTarget() {
+ // Verify that new authorize SPI call without primary target uses root_container
+ // for resolution and authorization
+ PolarisAuthorizerImpl authorizer = spy(new PolarisAuthorizerImpl(mock(RealmConfig.class)));
+ AuthorizationState authzState = new AuthorizationState();
+ PolarisResolutionManifest manifest = mock(PolarisResolutionManifest.class);
+ PolarisResolvedPathWrapper rootWrapper = mock(PolarisResolvedPathWrapper.class);
+ PolarisResolvedPathWrapper principalRoleWrapper = mock(PolarisResolvedPathWrapper.class);
+
+ authzState.setResolutionManifest(manifest);
+ when(manifest.getResolvedRootContainerEntityAsPath()).thenReturn(rootWrapper);
+ when(manifest.getResolvedTopLevelEntity("analytics-admin", PolarisEntityType.PRINCIPAL_ROLE))
+ .thenReturn(principalRoleWrapper);
+ when(manifest.getAllActivatedCatalogRoleAndPrincipalRoles())
+ .thenReturn(Set.<PolarisBaseEntity>of());
+ doNothing()
+ .when(authorizer)
+ .authorizeOrThrow(
+ any(PolarisPrincipal.class),
+ org.mockito.ArgumentMatchers.<Set<PolarisBaseEntity>>any(),
+ eq(PolarisAuthorizableOperation.ADD_ROOT_GRANT_TO_PRINCIPAL_ROLE),
+ org.mockito.ArgumentMatchers.<List<PolarisResolvedPathWrapper>>any(),
+ org.mockito.ArgumentMatchers.<List<PolarisResolvedPathWrapper>>any());
Review Comment:
sure - that's a lot cleaner
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3034961696
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
Is it a valid concern? @sungwy , WDYT?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3035048449
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -18,23 +18,22 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
/** A resource binding containing a primary target and optional secondary. */
@PolarisImmutable
public interface AuthorizationTargetBinding {
static AuthorizationTargetBinding of(
- @Nonnull PolarisSecurable target, @Nullable PolarisSecurable secondary) {
+ @Nullable PolarisSecurable target, @Nullable PolarisSecurable secondary) {
Review Comment:
If it's too late, totally fine to move forward with this shape, and make target in AuthorizationTargetBinding nullable as well
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -158,9 +175,15 @@ public void authorizeOrThrow(
@Nonnull PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries) {
- boolean allowed = queryOpa(polarisPrincipal, activatedEntities, authzOp, targets, secondaries);
+ boolean allowed =
+ queryOpa(
+ buildOpaAuthorizationInput(
+ polarisPrincipal,
+ authzOp,
+ toResourceEntitiesFromResolvedPaths(targets),
+ toResourceEntitiesFromResolvedPaths(secondaries)));
if (!allowed) {
- throw new ForbiddenException("OPA denied authorization");
+ throw new ForbiddenException("%s", "OPA denied authorization");
Review Comment:
great eye - let me fix 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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #3999:
URL: https://github.com/apache/polaris/pull/3999#issuecomment-4130483795
Hi @sungwy , is this PR ready for review?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2937029101
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
Large refactoring here to move `PolarisPrivilege` semantics out of `PolarisAuthorizableOperation`, and into a completely separate module specific to RBAC instead. See `RbacOperationSemantics.java`
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizableOperation.java:
##########
Review Comment:
Note to reviewer: large refactoring here to move `PolarisPrivilege` semantics out of `PolarisAuthorizableOperation`, and into a completely separate module specific to RBAC instead. See `RbacOperationSemantics.java`
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2937026152
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
Note to reviewer: this class was needed to absorb the logic required in converting a `PolarisSecurable` to a fully qualified path representation that included the correct parents and their respective `PolarisEntityType` that could be constructed and inserted into the OPA request payload.
I'm on the fence as to whether we'd want this to be the representation in `AuthorizationRequest` instead of `PolarisSecurable`. Leaning towards keeping them separate for now.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2940504423
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -0,0 +1,713 @@
+/*
+ * 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 static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.CATALOG_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.NAMESPACE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_ATTACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DETACH;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_READ;
+import static org.apache.polaris.core.entity.PolarisPrivilege.POLICY_WRITE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_RESET_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_FOR_GRANTEE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_ROTATE_CREDENTIALS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.PRINCIPAL_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.SERVICE_MANAGE_ACCESS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_PARTITION_SPEC;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SNAPSHOT;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ADD_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ASSIGN_UUID;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_ATTACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DETACH_POLICY;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PARTITION_SPECS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_REMOVE_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_CURRENT_SCHEMA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_DEFAULT_SORT_ORDER;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_LOCATION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_SNAPSHOT_REF;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_SET_STATISTICS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_UPGRADE_FORMAT_VERSION;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_DATA;
+import static org.apache.polaris.core.entity.PolarisPrivilege.TABLE_WRITE_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_CREATE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_DROP;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_LIST_GRANTS;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_MANAGE_GRANTS_ON_SECURABLE;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_READ_PROPERTIES;
+import static org.apache.polaris.core.entity.PolarisPrivilege.VIEW_WRITE_PROPERTIES;
+
+import com.google.common.base.Preconditions;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+/** Polaris RBAC-specific interpretation of an authorizable operation. */
+record RbacOperationSemantics(
+ EnumSet<PolarisPrivilege> targetPrivileges,
+ EnumSet<PolarisPrivilege> secondaryPrivileges,
+ ResolvedPathRooting targetScope,
+ ResolvedPathRooting secondaryScope) {
+
+ private RbacOperationSemantics(
Review Comment:
It was more for convenience / beautification. Do you think using EnumSet directly is better? I'm okay either way
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079638347
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> [...] wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using?
Partially. My concern is not about about legitimate clients, but about malicious clients. I suppose the message could be used to deduce the existence of certain entities.
Re: principal, if the auth token is stolen, the malicious client will not know the principal name, but may discover it via this message.
Re: operation, I'm thinking that exposing it to clients is more like exposing internal impl. details. They make sense to the admin user / developer, but if clients start relying on observing that data in responses (and they will) it will complicate future internal refactorings.
It's not a critical issue, though, IMHO, just trying to be proactive in tightening security :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3031052894
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +78,15 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
Review Comment:
beautification / consistency with above
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3060898493
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
Review Comment:
This behavior mirrors the previous status quo. ROOT container is only added to replace an empty target in primary target, but not in secondary (otherwise, we'd always have ROOT container in secondaries wherever `semantics.rooting() == ResolvedPathRooting.ROOT`
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3066265847
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
+ List<PolarisSecurable> secondaries = request.getSecondaries();
+ List<PolarisResolvedPathWrapper> resolvedSecondaries =
+ semantics.secondaryPrivileges().isEmpty() || secondaries.isEmpty()
+ ? null
Review Comment:
I think it is reasonable to change `PolarisAuthorizerImpl` internals as little as possible as we go through the SPI refactoring. So current `ROOT` processing looks good from my POV.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076220835
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,95 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
Review Comment:
Could you add tests for the deny case?
```
AuthorizationDecision decision = authorizer.authorize(authzState, request);
assertThat(decision.isAllowed()).isFalse();
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,95 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
Review Comment:
There seems be no test for this new logic. Could you add a few?
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -98,18 +101,32 @@ public OpaPolarisAuthorizer(
this.objectMapper = objectMapper;
}
+ /**
+ * Resolves authorization inputs using {@code resolveAll()} for backward compatibility.
+ *
+ * <p>This scope is intentionally broad for now and will be narrowed in a future refactoring to
+ * resolve only the selections required by OPA authorization.
+ */
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for OpaPolarisAuthorizer");
+ authzState.getResolutionManifest().resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for OpaPolarisAuthorizer");
+ boolean allowed =
+ queryOpa(
+ buildOpaAuthorizationInput(
+ request.getPrincipal(),
+ request.getOperation(),
+ toResourceEntitiesFromSecurables(request.getTargets()),
+ toResourceEntitiesFromSecurables(request.getSecondaries())));
+ return allowed
+ ? AuthorizationDecision.allow()
+ : AuthorizationDecision.deny("OPA denied authorization");
Review Comment:
Question(not a blocker): are we going to provide more detailed message for the deny case? My understanding of having a dedicated class `AuthorizationDecision` is to allow more information passed through instead of less.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3076501529
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
Cf. https://github.com/apache/polaris/pull/3928#discussion_r2983548645
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3079638347
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -75,18 +79,36 @@ default List<PolarisSecurable> getTargets() {
@Nonnull
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
- List<PolarisSecurable> secondaries = new ArrayList<>();
- for (AuthorizationTargetBinding targetBinding : getTargetBindings()) {
- if (targetBinding.getSecondary() != null) {
- secondaries.add(targetBinding.getSecondary());
- }
- }
- return secondaries;
+ return getTargetBindings().stream()
+ .map(AuthorizationTargetBinding::getSecondary)
+ .filter(Objects::nonNull)
+ .toList();
+ }
+
+ /**
+ * Returns a stable debug string for authorization messages.
+ *
+ * <p>Includes the operation, principal name, formatted targets, and formatted secondaries.
+ */
+ @Nonnull
+ default String formatForAuthorizationMessage() {
+ return String.format(
+ "operation=%s principal=%s targets=%s secondaries=%s",
Review Comment:
> [...] wouldn't a denied user already know what targets, and secondaries they are trying to access, and what operation, and principal they are using?
Partially. I did not review the message in depth, but the client operates on names, but the message might contain IDs, etc. My concern is about about legitimate clients, but about malicious clients.
Re: principal, if the auth token is stolen, the malicious client will not know the principal name, but may discover it via this message.
Re: operation, I'm thinking that exposing it to clients is more like exposing internal impl. details. They make sense to the admin user / developer, but if clients start relying on observing that data in responses (and they will) it will complicate future internal refactorings.
It's not a critical issue, though, IMHO, just trying to be proactive in tightening security :)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3039733933
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
Sounds good to 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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r3035167342
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -747,15 +750,84 @@ public PolarisAuthorizerImpl(RealmConfig realmConfig) {
@Override
public void resolveAuthorizationInputs(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "resolveAuthorizationInputs is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ resolutionManifest.resolveAll();
}
@Override
+ @Nonnull
public AuthorizationDecision authorize(
@Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
- throw new UnsupportedOperationException(
- "authorize is not implemented yet for PolarisAuthorizerImpl");
+ PolarisResolutionManifest resolutionManifest = authzState.getResolutionManifest();
+ RbacOperationSemantics semantics = RbacOperationSemantics.forOperation(request.getOperation());
+ boolean prependRootContainer = semantics.rooting() == ResolvedPathRooting.ROOT;
+ try {
+ List<PolarisSecurable> targets = request.getTargets();
+ List<PolarisResolvedPathWrapper> resolvedTargets =
+ !targets.isEmpty()
+ ? getResolvedSecurables(resolutionManifest, targets, prependRootContainer)
+ : prependRootContainer
+ ? List.of(resolutionManifest.getResolvedRootContainerEntityAsPath())
+ : null;
Review Comment:
It is indeed a valid concern, and I think a cleaner semantic would have been for us to have a metadata on the operation that specifies if target resource binding is expected for a given PolarisAuthorizableOperation.
That's harder to achieve in the current state where root-grant add and revoke operations have a primary target that is essentially empty (root container primary target). I've made note of this on another comment regarding the ADD_ROOT_GRANT_TO_PRINCIPAL_ROLE.
Given that it's hard to introduce a clear semantic at PolarisAuthorizableOperation to validate the AuthorizationTargetBinding values based on the operation, I think IllegalStateException is an acceptable fallback exception type (we shouldn't expect it to happen in any case)
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2942036930
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
Yeah that's a good point.
I'm starting to think that the shape of `FullyQualifiedPath` is more in line with what we'd want to have in place of `PolarisSecurable` in the `AuthorizationRequest`, instead of having it represented as a `PolarisEntityType` and a List of String as its name.
Having clear PathSegment with ordered parents and reference catalog is a better representation of the resource... What do you folks think about just replacing `PolarisSecurable` with `FullyQualifiedPath`'s shape instead?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2942076787
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
It sounds good yeah.
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941986425
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
I tend to think `PolarisSecurable` must know its catalog name anyway. Permission checks in Polaris are always performed within the context of a catalog ๐ค WDYT?
--
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] core: implement new authorize SPI in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer` [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3999:
URL: https://github.com/apache/polaris/pull/3999#discussion_r2941986425
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/FullyQualifiedPath.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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 com.google.common.base.Preconditions;
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
+
+/** Lexical fully qualified resource path split into leaf and ordered parent segments. */
+public record FullyQualifiedPath(PathSegment leaf, List<PathSegment> parents) {
Review Comment:
I tend to think `PolarisSecurable` must know its catalog name anyway. Permission in Polaris are always performed within the context of a catalog ๐ค WDYT?
--
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