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/02/14 03:29:34 UTC
[PR] Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
sungwy opened a new pull request, #3760:
URL: https://github.com/apache/polaris/pull/3760
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
This is the first PR in a series to refactor the PolarisAuthorizer SPI so authorizer implementations can control whether Polaris-native principal/RBAC resolution is required.
Summary of changes in this PR:
- Added `authorizeDecision(...)` (returns `AuthorizationDecision`) and a new `authorizeOrThrow(...)` overload that uses `AuthorizationState` + `AuthorizationRequest`.
- Added request-scoped `RequestAuthorizationState` CDI bean implementing `AuthorizationState`.
- Implemented resolver selection planning so `resolveSelections(...)` can skip caller principal/role resolution when not requested.
- Included new selection-based resolution tests in ResolverTest.
- New classes introduced: `AuthorizationDecision`, `AuthorizationState`, `AuthorizationRequest`
In this PR, handlers still use the legacy call sites, and hence no behavior change is expected. The new SPI methods remain unimplemented in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer`.
Once this SPI is accepted, follow-up PRs will implement the new SPI methods in `PolarisAuthorizerImpl` and `OpaPolarisAuthorizer`.
RFC Google Doc on Authorization Refactor: [Link](https://docs.google.com/document/d/1OaiQG_C4-yUe0ihaDBxtw_mEcOOzUBnWPazzVbjQi5U/edit?tab=t.0#heading=h.dyow25dt9w1)
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช 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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2833672796
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link PolarisSecurable} targets and secondaries.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable List<PolarisSecurable> targets,
+ @Nullable List<PolarisSecurable> secondaries) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targets(targets)
+ .secondaries(secondaries)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Targets are the primary resources the operation applies to (for example, the table being
+ * created or dropped).
+ */
+ @Nullable
+ List<PolarisSecurable> getTargets();
+
+ /**
+ * Returns the secondary securables, if any.
+ *
+ * <p>Secondaries are related resources needed to evaluate the authorization decision but are not
+ * the direct object of the operation. Examples in current handlers include:
+ *
+ * <ul>
+ * <li>Table rename: the destination namespace (target is the source table).
+ * <li>Role grants: the grantee role/principal (target may be the role or the resource being
+ * granted on).
+ * <li>Policy attach/detach: the catalog/namespace/table being attached to (target is the
+ * policy).
+ * </ul>
+ */
+ @Nullable
+ List<PolarisSecurable> getSecondaries();
Review Comment:
> I know it might be a bit of rift in the existing code pattern, but since we're introducing a new SPI, WDYT about modeling this as List<Pair<PolarisSecurable, PolarisSecurable>> getTargets()? Of course, we have have a dedicated small immutable object with target/secondary attributes instead of the Pair.
Yeah sure thing - Now that you mention it, I think it'll be helpful to create a new class to represent this, than using a `Pair`
> Thanks for the javadoc! It certainly helped me refresh my memory ๐
Now that I'm thinking about it, I'm not a fan of the `secondaries` convention at all - this makes the `authorization` checks quite awkward and complicated. Instead what I think we should have done is to just break them into two authorization checks. As an example, for a check like renaming a table that requires table1 and table2, we should have just checked if the principal can rename from table1, and then subsequently to table2. However, I think this will result in an even larger refactoring and I want to keep it out of scope of this effort.
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2819925658
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
+
+ /** Returns the dot-joined name derived from {@link #getNameParts()}. */
+ default @Nonnull String getName() {
Review Comment:
Thank you for the scrutiny!
I assumed we'd need this because of the complexity in handling multi-part namespaces in OPA inputs, but I now I remember that we've actually decided to move forward with clearly preserving the parent hierarchy through an ordered list. I think it'll be safer to remove this so that we don't introduce an unnecessary string conversion that could be confusing 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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2836396672
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
Review Comment:
yep - you are right, this was a lousy follow-up your feedback. This should be a Singleton :)
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2835430510
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
+ * carry authorization-specific data, such as a {@link PolarisResolutionManifest} created by the
+ * caller and populated by the authorizer.
+ */
+public interface AuthorizationState {
Review Comment:
adopted this feedback - thanks @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] Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2806939327
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -247,7 +264,7 @@ public ResolverStatus resolveAll() {
int count = 0;
ResolverStatus status;
do {
- status = runResolvePass();
+ status = runResolvePass(plan);
count++;
Review Comment:
This bug seems to predate my change - taking a look to see if it would make sense to fix this as a part of this 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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2836399361
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
+public class RequestAuthorizationState {
+ @Produces
+ @RequestScoped
+ public AuthorizationState get() {
+ return new AuthorizationState();
Review Comment:
I think following convention is important in a project, so will adopt this feedback as well. Although I think the ServiceProducer is becoming very crowded...
Iโd like to keep convention for now in this PR to minimize churn, but do you see a world where we separate out the ServiceProducer into domain specific ones? (like: `AuthProducers`/`PersistenceProducers`) to improve discoverability?
Curious to hear your thoughts
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2841996926
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -22,11 +22,47 @@
import jakarta.annotation.Nullable;
import java.util.List;
import java.util.Set;
+import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Resolve authorizer-specific inputs before authorization.
+ *
+ * <p>Implementations may resolve only the entities required for the request (for example, the
+ * caller principal, principal roles, catalog roles, and requested targets) and store that state
+ * in {@link AuthorizationState}.
+ *
+ * <p>This method should not perform authorization decisions directly.
+ */
+ void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Core authorization entry point for the new SPI.
+ *
+ * <p>Implementations should rely on any required state in {@link AuthorizationState} and the
+ * intent captured by {@link AuthorizationRequest} (principal, operation, and target securables).
+ */
+ @Nonnull
+ AuthorizationDecision authorizeDecision(
Review Comment:
yeah I initially had it as `authorize` and then renamed it because I thought the new name implied that we are returning an `AuthorizationDecision`... but I think I agree with you that it's more confusing. I'll take the suggestion and revert it back
--
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] Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2819826176
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
Review Comment:
nit: maybe `getNameElements()`?... (really minor)
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link PolarisSecurable} targets and secondaries.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable List<PolarisSecurable> targets,
+ @Nullable List<PolarisSecurable> secondaries) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targets(targets)
+ .secondaries(secondaries)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /** Returns the primary target securables, if any. */
+ @Nullable
+ List<PolarisSecurable> getTargets();
+
+ /** Returns the secondary securables, if any. */
+ @Nullable
+ List<PolarisSecurable> getSecondaries();
Review Comment:
What is a "secondary" securable really? ๐
I looked at them in the core RBAC code many times, but the idea does not stick in my head ๐
Since this is an attempt to refactor the authZ SPI, could you add javadoc to describe targets and secondaries in more details, please?
I know the concept predates this effort, but it's a good opportunity to clarify this... WDYT?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
+
+ /** Returns the dot-joined name derived from {@link #getNameParts()}. */
+ default @Nonnull String getName() {
Review Comment:
Is it intended for any non-informational use? Joining with dot separators is not a reversible mapping WRT `getNameParts()`, I believe ๐ค
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
+ * carry authorization-specific data, such as a {@link PolarisResolutionManifest} created by the
+ * caller and populated by the authorizer.
+ */
+public interface AuthorizationState {
Review Comment:
Do we expect multiple different implementations of this "state"? Why not make it a class?
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2835575780
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link PolarisSecurable} targets and secondaries.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable List<PolarisSecurable> targets,
+ @Nullable List<PolarisSecurable> secondaries) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targets(targets)
+ .secondaries(secondaries)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Targets are the primary resources the operation applies to (for example, the table being
+ * created or dropped).
+ */
+ @Nullable
+ List<PolarisSecurable> getTargets();
+
+ /**
+ * Returns the secondary securables, if any.
+ *
+ * <p>Secondaries are related resources needed to evaluate the authorization decision but are not
+ * the direct object of the operation. Examples in current handlers include:
+ *
+ * <ul>
+ * <li>Table rename: the destination namespace (target is the source table).
+ * <li>Role grants: the grantee role/principal (target may be the role or the resource being
+ * granted on).
+ * <li>Policy attach/detach: the catalog/namespace/table being attached to (target is the
+ * policy).
+ * </ul>
+ */
+ @Nullable
+ List<PolarisSecurable> getSecondaries();
Review Comment:
I'm fine keeping secondaries in this PR. `AuthorizationTargetBinding` already makes things a lot clearer at the SPI level.
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2835571137
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
+public class RequestAuthorizationState {
+ @Produces
+ @RequestScoped
+ public AuthorizationState get() {
+ return new AuthorizationState();
Review Comment:
nit: traditionally, these methods are collected in `ServiceProducers`, but having a dedicated producer class is ok too.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
Review Comment:
nit: I do not think this annotation is required... If it is, it should probably be a `@Singleton`, 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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2841406686
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link AuthorizationTargetBinding} target bindings.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nonnull List<AuthorizationTargetBinding> targetBindings) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targetBindings(targetBindings)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /** Returns the target/secondary target bindings. */
+ @Nonnull
+ List<AuthorizationTargetBinding> getTargetBindings();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
+ */
+ @Nonnull
+ default List<PolarisSecurable> getTargets() {
Review Comment:
```suggestion
@Value.Derived
default List<PolarisSecurable> getTargets() {
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+
+/** Authorization decision returned by authorizer implementations. */
+public final class AuthorizationDecision {
Review Comment:
nit: this might benefit from being a `@PolarisImmutable` + usage of `Optional` for the message, in which case I don't think we need the `getMessageOrDefault` method:
```java
@PolarisImmutable
public interface AuthorizationDecision {
static AuthorizationDecision allow() {
return ImmutableAuthorizationDecision.of(true, Optional.empty());
}
static AuthorizationDecision deny(@Nullable String message) {
return ImmutableAuthorizationDecision.of(false, Optional.ofNullable(message));
}
@Value.Parameter(order = 1)
boolean isAllowed();
@Value.Parameter(order = 2)
Optional<String> getMessage();
}
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -22,11 +22,47 @@
import jakarta.annotation.Nullable;
import java.util.List;
import java.util.Set;
+import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Resolve authorizer-specific inputs before authorization.
+ *
+ * <p>Implementations may resolve only the entities required for the request (for example, the
+ * caller principal, principal roles, catalog roles, and requested targets) and store that state
+ * in {@link AuthorizationState}.
+ *
+ * <p>This method should not perform authorization decisions directly.
+ */
+ void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Core authorization entry point for the new SPI.
+ *
+ * <p>Implementations should rely on any required state in {@link AuthorizationState} and the
+ * intent captured by {@link AuthorizationRequest} (principal, operation, and target securables).
+ */
+ @Nonnull
+ AuthorizationDecision authorizeDecision(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Convenience method that throws a {@link ForbiddenException} when authorization is denied.
+ *
+ * <p>Implementations should provide allow/deny decisions via {@link #authorizeDecision}.
+ */
+ default void authorizeOrThrow(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
+ AuthorizationDecision decision = authorizeDecision(authzState, request);
+ if (!decision.isAllowed()) {
+ String message = decision.getMessageOrDefault("Authorization denied");
Review Comment:
```suggestion
String message = decision.getMessage().orElse("Authorization denied");
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -856,6 +888,68 @@ private ResolverStatus resolveReferenceCatalog(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ private static final class ResolvePlan {
Review Comment:
nit: very good candidate for a record class.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import 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) {
+ return ImmutableAuthorizationTargetBinding.builder()
+ .target(target)
+ .secondary(secondary)
+ .build();
+ }
+
+ /** Returns the primary target securable for the binding. */
+ @Nonnull
+ PolarisSecurable getTarget();
+
+ /**
+ * Returns the optional secondary securable associated with the target.
+ *
+ * <p>Secondaries are related resources needed to evaluate the authorization decision but are not
+ * the direct object of the operation. Examples in current Polaris authorization flows include:
+ *
+ * <ul>
+ * <li>Table rename: the destination namespace (target is the source table).
+ * <li>Role grants: the grantee role/principal (target may be the role or the resource being
+ * granted on).
+ * <li>Policy attach/detach: the catalog/namespace/table being attached to (target is the
+ * policy).
+ * </ul>
+ */
+ @Nullable
+ PolarisSecurable getSecondary();
Review Comment:
Two questions, but not for this PR:
1. Should we plan for the future and add the possibility of 1-N secondaries for a given target?
2. Wouldn't this be better modeled by inheritance? e.g.
```java
public interface TableRenameAuthorizationTargetBinding extends AuthorizationTargetBinding {
PolarisSecurable getDestinationNamespace();
}
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link AuthorizationTargetBinding} target bindings.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nonnull List<AuthorizationTargetBinding> targetBindings) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targetBindings(targetBindings)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /** Returns the target/secondary target bindings. */
+ @Nonnull
+ List<AuthorizationTargetBinding> getTargetBindings();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
+ */
+ @Nonnull
+ default List<PolarisSecurable> getTargets() {
+ return getTargetBindings().stream().map(AuthorizationTargetBinding::getTarget).toList();
+ }
+
+ /**
+ * Returns secondary securables, if any.
+ *
+ * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
+ */
+ @Nonnull
+ default List<PolarisSecurable> getSecondaries() {
Review Comment:
```suggestion
@Value.Derived
default List<PolarisSecurable> getSecondaries() {
```
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
+public class RequestAuthorizationState {
+ @Produces
+ @RequestScoped
+ public AuthorizationState get() {
+ return new AuthorizationState();
Review Comment:
I'm still not convinced we need to create a CDI bean for `AuthorizationState`, but why not.
In any case I would personally be in favor of adding it to `ServiceProducers` for now, and then later look into splitting `ServiceProducers` into concern-specific components as you suggested.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
Review Comment:
nit: not sure we need to call out a comparison with `CallContext` here. We have many other request-scoped beans, and that's fine.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -22,11 +22,47 @@
import jakarta.annotation.Nullable;
import java.util.List;
import java.util.Set;
+import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Resolve authorizer-specific inputs before authorization.
+ *
+ * <p>Implementations may resolve only the entities required for the request (for example, the
+ * caller principal, principal roles, catalog roles, and requested targets) and store that state
+ * in {@link AuthorizationState}.
+ *
+ * <p>This method should not perform authorization decisions directly.
+ */
+ void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Core authorization entry point for the new SPI.
+ *
+ * <p>Implementations should rely on any required state in {@link AuthorizationState} and the
+ * intent captured by {@link AuthorizationRequest} (principal, operation, and target securables).
+ */
+ @Nonnull
+ AuthorizationDecision authorizeDecision(
Review Comment:
nit: the current name (at least for me) implies that the authorization has been done already when this method is called, and that it merely returns the authorization decision. If this method actually performs the authorization, I would suggest renaming it to `authorize`.
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2819927001
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link PolarisSecurable} targets and secondaries.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable List<PolarisSecurable> targets,
+ @Nullable List<PolarisSecurable> secondaries) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targets(targets)
+ .secondaries(secondaries)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /** Returns the primary target securables, if any. */
+ @Nullable
+ List<PolarisSecurable> getTargets();
+
+ /** Returns the secondary securables, if any. */
+ @Nullable
+ List<PolarisSecurable> getSecondaries();
Review Comment:
I found this very confusing as well... I agree this is a good place for the Java doc to explain the difference between the two. Let me add it
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2850298631
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationTargetBinding.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import 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) {
+ return ImmutableAuthorizationTargetBinding.builder()
+ .target(target)
+ .secondary(secondary)
+ .build();
+ }
+
+ /** Returns the primary target securable for the binding. */
+ @Nonnull
+ PolarisSecurable getTarget();
+
+ /**
+ * Returns the optional secondary securable associated with the target.
+ *
+ * <p>Secondaries are related resources needed to evaluate the authorization decision but are not
+ * the direct object of the operation. Examples in current Polaris authorization flows include:
+ *
+ * <ul>
+ * <li>Table rename: the destination namespace (target is the source table).
+ * <li>Role grants: the grantee role/principal (target may be the role or the resource being
+ * granted on).
+ * <li>Policy attach/detach: the catalog/namespace/table being attached to (target is the
+ * policy).
+ * </ul>
+ */
+ @Nullable
+ PolarisSecurable getSecondary();
Review Comment:
I can see the value in doing that - but I agree with you that it would be good to scope it out from this refactoring. I think it would be easier to complete this refactoring by keeping feature parity, and then entertain the idea of changing the `AuthorizationTargetBinding` implementations.
Changing its members will result in a backward incompatible change in the `OpaPolarisAuthorizer`'s OPA request input json (e.g. `destinationNamespace` vs `secondaries` as the field name).
But I'm definitely open to changes in the OPA request input structure to make it more intuitive, as it is currently an experimental feature, and it would be best to make those changes before it becomes a stable contact.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
Review Comment:
I thought about this, but I think `getNameParts` sounds better because we do refer to Namespaces with multiple segments as `multi-part` Namespace:
https://github.com/apache/iceberg/blob/9534c9b3adc29d127ecc541ce131f49fd72f1980/open-api/rest-catalog-open-api.yaml#L5012-L5014
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link AuthorizationTargetBinding} target bindings.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nonnull List<AuthorizationTargetBinding> targetBindings) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targetBindings(targetBindings)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /** Returns the target/secondary target bindings. */
+ @Nonnull
+ List<AuthorizationTargetBinding> getTargetBindings();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
+ */
+ @Nonnull
+ default List<PolarisSecurable> getTargets() {
Review Comment:
Thanks @adutra - I've adopted this feedback
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2850530322
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -22,11 +22,47 @@
import jakarta.annotation.Nullable;
import java.util.List;
import java.util.Set;
+import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {
+ /**
+ * Resolve authorizer-specific inputs before authorization.
+ *
+ * <p>Implementations may resolve only the entities required for the request (for example, the
+ * caller principal, principal roles, catalog roles, and requested targets) and store that state
+ * in {@link AuthorizationState}.
+ *
+ * <p>This method should not perform authorization decisions directly.
+ */
+ void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Core authorization entry point for the new SPI.
+ *
+ * <p>Implementations should rely on any required state in {@link AuthorizationState} and the
+ * intent captured by {@link AuthorizationRequest} (principal, operation, and target securables).
+ */
+ @Nonnull
+ AuthorizationDecision authorizeDecision(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request);
+
+ /**
+ * Convenience method that throws a {@link ForbiddenException} when authorization is denied.
+ *
+ * <p>Implementations should provide allow/deny decisions via {@link #authorizeDecision}.
+ */
+ default void authorizeOrThrow(
+ @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest request) {
+ AuthorizationDecision decision = authorizeDecision(authzState, request);
+ if (!decision.isAllowed()) {
+ String message = decision.getMessageOrDefault("Authorization denied");
Review Comment:
+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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2819912584
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
+ * carry authorization-specific data, such as a {@link PolarisResolutionManifest} created by the
+ * caller and populated by the authorizer.
+ */
+public interface AuthorizationState {
Review Comment:
I did this intentionally because I wanted to keep `AuthorizationState` in `polaris-core` and the request-scoped CDI in `polaris-runtime`. But yes, I agree that we would not intend for there to be multiple implementations
--
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] Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2806891258
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -238,6 +238,23 @@ public void addPath(@Nonnull ResolverPath path) {
* getResolvedXYZ() method can be called.
*/
public ResolverStatus resolveAll() {
+ return resolveWithPlan(ResolvePlan.all(referenceCatalogName));
+ }
+
+ /**
+ * Run the resolution process for a subset of resolvables.
+ *
+ * @param selections explicit selection of resolvables to resolve
+ * @return the status of the resolver. If success, the requested entities have been resolved and
+ * the corresponding getResolvedXYZ() methods can be called.
+ */
+ public ResolverStatus resolveSelections(@Nonnull Set<Resolvable> selections) {
+ diagnostics.checkNotNull(selections, "resolver_selections_is_null");
+ diagnostics.check(!selections.isEmpty(), "resolver_selections_is_empty");
+ return resolveWithPlan(ResolvePlan.fromSelections(selections, referenceCatalogName));
Review Comment:
`resolveSelections(...)` can succeed without resolving some previously-guaranteed fields (e.g., caller principal). In that case, getters like `getResolvedCallerPrincipal()` (annotated `@Nonnull`) can return null after a successful resolution, which is a nullness contract violation and can trigger downstream NPEs. Consider tracking which components were resolved (store the plan/selections) and have getters either throw if the component wasnโt requested/resolved, or adjust the API/annotations to reflect optionality for selection-based resolution.
```suggestion
// Currently, some getters are annotated @Nonnull and are expected to be safe to call
// after a successful resolution. To avoid violating those contracts when only a subset
// of resolvables is requested, we perform a full resolution here.
return resolveAll();
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -247,7 +264,7 @@ public ResolverStatus resolveAll() {
int count = 0;
ResolverStatus status;
do {
- status = runResolvePass();
+ status = runResolvePass(plan);
count++;
Review Comment:
The resolve-pass loop increments `count` twice per iteration (`count++` inside the loop and `++count` in the while condition). This effectively halves the intended max-pass limit and makes the counter harder to reason about. Consider incrementing the counter in exactly one place (either inside the loop body or in the condition).
```suggestion
```
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.RequestScoped;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.auth.AuthorizationState;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped {@link AuthorizationState} implementation for runtime services.
+ *
+ * <p>Holds a single {@link PolarisResolutionManifest} reference that must be set once per request.
+ */
+@RequestScoped
+public class RequestAuthorizationState implements AuthorizationState {
+ private final AtomicReference<PolarisResolutionManifest> resolutionManifest =
+ new AtomicReference<>();
+
+ @Override
+ @Nonnull
+ public PolarisResolutionManifest getResolutionManifest() {
+ PolarisResolutionManifest manifest = resolutionManifest.get();
+ if (manifest == null) {
+ throw new IllegalStateException("AuthorizationState resolution manifest is not set");
+ }
+ return manifest;
+ }
+
+ @Override
+ public void setResolutionManifest(@Nonnull PolarisResolutionManifest resolutionManifest) {
+ if (!this.resolutionManifest.compareAndSet(null, resolutionManifest)) {
+ throw new IllegalStateException("AuthorizationState resolution manifest already set");
+ }
Review Comment:
`setResolutionManifest(...)` allows a null value to be stored (annotations arenโt enforced at runtime). That means the first call can succeed with null and later `getResolutionManifest()` will fail with a misleading โnot setโ error. Add an explicit null check (or use `Objects.requireNonNull`) before `compareAndSet`.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -856,6 +888,68 @@ private ResolverStatus resolveReferenceCatalog(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ private static final class ResolvePlan {
+ private final boolean resolveCallerPrincipal;
+ private final boolean resolvePrincipalRoles;
+ private final boolean resolveReferenceCatalog;
+ private final boolean resolveCatalogRoles;
+ private final boolean resolveTopLevelEntities;
+ private final boolean resolvePaths;
+
+ private ResolvePlan(
+ boolean resolveCallerPrincipal,
+ boolean resolvePrincipalRoles,
+ boolean resolveReferenceCatalog,
+ boolean resolveCatalogRoles,
+ boolean resolveTopLevelEntities,
+ boolean resolvePaths) {
+ this.resolveCallerPrincipal = resolveCallerPrincipal;
+ this.resolvePrincipalRoles = resolvePrincipalRoles;
+ this.resolveReferenceCatalog = resolveReferenceCatalog;
+ this.resolveCatalogRoles = resolveCatalogRoles;
+ this.resolveTopLevelEntities = resolveTopLevelEntities;
+ this.resolvePaths = resolvePaths;
+ }
+
+ private static ResolvePlan all(@Nullable String referenceCatalogName) {
+ boolean hasReferenceCatalog = referenceCatalogName != null;
+ return new ResolvePlan(
+ true, true, hasReferenceCatalog, hasReferenceCatalog, true, hasReferenceCatalog);
+ }
+
+ private static ResolvePlan fromSelections(
+ Set<Resolvable> selections, @Nullable String referenceCatalogName) {
+ boolean resolvePaths = selections.contains(Resolvable.REQUESTED_PATHS);
+ boolean resolveTopLevelEntities =
+ selections.contains(Resolvable.REQUESTED_TOP_LEVEL_ENTITIES);
+ boolean resolveCatalogRoles = selections.contains(Resolvable.CATALOG_ROLES);
+ // Principal roles depend on resolving the caller principal, and catalog roles depend on
+ // principal roles. Only those selections require caller principal resolution.
+ boolean resolvePrincipalRoles =
+ selections.contains(Resolvable.CALLER_PRINCIPAL_ROLES) || resolveCatalogRoles;
+ // Reference catalog is required for path resolution and catalog role expansion.
+ boolean resolveReferenceCatalog =
+ selections.contains(Resolvable.REFERENCE_CATALOG) || resolvePaths || resolveCatalogRoles;
+ boolean resolveCallerPrincipal =
+ selections.contains(Resolvable.CALLER_PRINCIPAL) || resolvePrincipalRoles;
+
+ boolean hasReferenceCatalog = referenceCatalogName != null;
+ if (!hasReferenceCatalog) {
+ resolveReferenceCatalog = false;
+ resolveCatalogRoles = false;
+ resolvePaths = false;
Review Comment:
`ResolvePlan.fromSelections(...)` silently disables `REFERENCE_CATALOG` / `CATALOG_ROLES` / `REQUESTED_PATHS` when `referenceCatalogName` is null. That can make `resolveSelections(...)` return SUCCESS even though the caller explicitly requested catalog-dependent resolution, violating the methodโs contract. Consider failing fast (diagnostics check) when selections require a reference catalog but none was provided, instead of turning the request into a no-op.
```suggestion
if (!hasReferenceCatalog && resolveReferenceCatalog) {
throw new IllegalArgumentException(
"Reference-catalog-dependent selections were requested, but no reference catalog "
+ "name was provided");
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.resolver;
+
+/**
+ * Explicit resolution components for {@link PolarisResolutionManifest#resolveSelections}.
Review Comment:
Javadoc link `{@link PolarisResolutionManifest#resolveSelections}` is missing the method signature. If an overload is ever introduced, this will become ambiguous and can break Javadoc generation. Consider linking with the parameter type (e.g., `#resolveSelections(Set)`).
```suggestion
* Explicit resolution components for {@link PolarisResolutionManifest}.
```
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2830878681
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
+
+ /** Returns the dot-joined name derived from {@link #getNameParts()}. */
+ default @Nonnull String getName() {
Review Comment:
removing `getName()` sounds good (not hard to re-add when / if we need it).
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2830890135
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Authorization request inputs for pre-authorization and core authorization.
+ *
+ * <p>This wrapper keeps authorization inputs together and conveys the intent to be authorized via
+ * {@link PolarisSecurable} targets and secondaries.
+ */
+@PolarisImmutable
+public interface AuthorizationRequest {
+ static AuthorizationRequest of(
+ @Nonnull PolarisPrincipal principal,
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable List<PolarisSecurable> targets,
+ @Nullable List<PolarisSecurable> secondaries) {
+ return ImmutableAuthorizationRequest.builder()
+ .principal(principal)
+ .operation(operation)
+ .targets(targets)
+ .secondaries(secondaries)
+ .build();
+ }
+
+ /** Returns the principal requesting authorization. */
+ @Nonnull
+ PolarisPrincipal getPrincipal();
+
+ /** Returns the operation being authorized. */
+ @Nonnull
+ PolarisAuthorizableOperation getOperation();
+
+ /**
+ * Returns the primary target securables, if any.
+ *
+ * <p>Targets are the primary resources the operation applies to (for example, the table being
+ * created or dropped).
+ */
+ @Nullable
+ List<PolarisSecurable> getTargets();
+
+ /**
+ * Returns the secondary securables, if any.
+ *
+ * <p>Secondaries are related resources needed to evaluate the authorization decision but are not
+ * the direct object of the operation. Examples in current handlers include:
+ *
+ * <ul>
+ * <li>Table rename: the destination namespace (target is the source table).
+ * <li>Role grants: the grantee role/principal (target may be the role or the resource being
+ * granted on).
+ * <li>Policy attach/detach: the catalog/namespace/table being attached to (target is the
+ * policy).
+ * </ul>
+ */
+ @Nullable
+ List<PolarisSecurable> getSecondaries();
Review Comment:
Thanks for the javadoc! It certainly helped me refresh my memory ๐
Given current use cases, I wonder if a List-to-List relationship between targets and secondary is actually correct in general. I'd imagine in all relevant cases it is a 1:1 association. When there are multiple targets, there are going to be multiple target/secondary pairs, right?
I know it might be a bit of rift in the existing code pattern, but since we're introducing a new SPI, WDYT about modeling this as `List<Pair<PolarisSecurable, PolarisSecurable>> getTargets()`? Of course, we have have a dedicated small immutable object with target/secondary attributes instead of the `Pair`.
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2841981274
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationDecision.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
+
+/** Authorization decision returned by authorizer implementations. */
+public final class AuthorizationDecision {
Review Comment:
Good suggestion @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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2842007269
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
Review Comment:
Very agreeable nit :)
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2850526980
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Intent-only target reference for authorization decisions.
+ *
+ * <p>Represents the target name and entity type without any resolved RBAC state. This captures the
+ * pure authorization intent of a request. Callers should provide the full name path as {@link
+ * #getNameParts()} for hierarchical entities.
+ */
+@PolarisImmutable
+public interface PolarisSecurable {
+ static PolarisSecurable of(
+ @Nonnull PolarisEntityType entityType, @Nonnull List<String> nameParts) {
+ return ImmutablePolarisSecurable.builder().entityType(entityType).nameParts(nameParts).build();
+ }
+
+ /** Returns the entity type of the securable. */
+ @Nonnull
+ PolarisEntityType getEntityType();
+
+ /** Returns the name parts that identify the securable in hierarchical order. */
+ @Nonnull
+ List<String> getNameParts();
Review Comment:
fair enough :+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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2830875958
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationState.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.auth;
+
+import jakarta.annotation.Nonnull;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped authorization state shared across authorization phases.
+ *
+ * <p>Distinct from {@link org.apache.polaris.core.context.CallContext}. This state is intended to
+ * carry authorization-specific data, such as a {@link PolarisResolutionManifest} created by the
+ * caller and populated by the authorizer.
+ */
+public interface AuthorizationState {
Review Comment:
We can still keep a "plain" impl. class in `polaris-core` and have a producer method in `polaris-runtime` with appropriate CDI annotations. It might cause some booiler-plate code for (injected) passing parameters to the constructor/builder, but hopefully not too much. 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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3760:
URL: https://github.com/apache/polaris/pull/3760#issuecomment-3931025288
I clicked "approve" too eagerly perhaps given my comment above, but I do not want to retract it as the PR LGTM overall :sweat_smile:
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2836396672
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
Review Comment:
yep - you are right, this was a lazy follow-up your feedback. This should be a Singleton :)
--
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: Refactor PolarisAuthorizer SPI (Phase 1): new interfaces and utilities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2843985967
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.enterprise.inject.Produces;
+import org.apache.polaris.core.auth.AuthorizationState;
+
+/** Request-scoped producer for {@link AuthorizationState} in runtime services. */
+@RequestScoped
+public class RequestAuthorizationState {
+ @Produces
+ @RequestScoped
+ public AuthorizationState get() {
+ return new AuthorizationState();
Review Comment:
+1 to refactoring `ServiceProducers` later.
In general (since we're on the subject), there is no technical difference where producer methods are located, so it's all about convenience of the code maintainer :sweat_smile:
--
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