dimas-b commented on code in PR #4409:
URL: https://github.com/apache/polaris/pull/4409#discussion_r3234767485
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -38,26 +39,85 @@ public interface PolarisAuthorizer {
* <p>This method should not perform authorization decisions directly.
*/
void resolveAuthorizationInputs(
- @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest
request);
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull AuthorizationRequest request);
+
+ /**
+ * Resolve authorizer-specific inputs for a batch of authorization requests
that share one
+ * principal.
+ *
+ * <p>Implementations must define their own batch pre-resolution behavior
explicitly because
+ * manifest registration is authorizer-specific.
+ */
+ void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull List<AuthorizationRequest> requests);
/**
* 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).
+ * intent captured by {@link AuthorizationRequest} (operation and target
securables), together
+ * with the explicit {@link PolarisPrincipal} argument.
*/
@Nonnull
AuthorizationDecision authorize(
- @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest
request);
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull AuthorizationRequest request);
+
+ /**
+ * Core authorization entry point for a batch of requests that share one
principal.
+ *
+ * <p>The default behavior preserves semantics by evaluating requests
independently in order and
+ * returning the first denial. Implementations may override this to use a
single batched
+ * downstream authorization call.
+ */
+ @Nonnull
+ default AuthorizationDecision authorize(
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull List<AuthorizationRequest> requests) {
+ Preconditions.checkArgument(
+ !requests.isEmpty(), "Authorization request batch must contain at
least one request");
+ for (AuthorizationRequest request : requests) {
+ AuthorizationDecision decision = authorize(authzState, polarisPrincipal,
request);
+ if (!decision.isAllowed()) {
+ return decision;
Review Comment:
This may not work well for list filtering... in that case the caller will
need a decision for each target, I think 🤔
I know list filtering a future feature, but IIRC the general consensus is
that it's worth implementing, so we should probably consider it in the AuthZ
SPI.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -38,26 +39,85 @@ public interface PolarisAuthorizer {
* <p>This method should not perform authorization decisions directly.
*/
void resolveAuthorizationInputs(
- @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest
request);
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
Review Comment:
I'm hesitant about using `PolarisPrincipal` as an explicit parameter.
`PolarisPrincipal` is generally a request-scoped contextual piece of data.
Non-authentication code cannot and should not do anything with
`PolarisPrincipal` aside from passing it through... the latter can be done by
the CDI framework more cleanly.
Making it an explicit parameter may feel compelling as a closure of all
relevant AuthZ inputs. On the other hand if we look at it from the caller side,
explicitly referencing `PolarisPrincipal` is a burden, IMHO.
What might work for both sides (impl. and callers) is a two-layer approach
similar to `RealmConfig`, where the interface does not have an explicit realm
parameter, but `RealmConfigImpl` has it.
WDYT about using `PolarisAuthorizer` as a caller-side API (no principal
parameters) and adding a `PolarisAuthorizerBackend` as an SPI for plugins (with
principal parameters). A shim will exits in-between as a request-scoped CDI
bean for extracting `PolarisPrincipal` from the CDI context?
This is just a idea for consideration... not a blocker for this PR.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -19,100 +19,56 @@
package org.apache.polaris.core.auth;
import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
import java.util.List;
-import java.util.Objects;
-import java.util.stream.Collectors;
import org.apache.polaris.core.entity.PolarisEntityType;
-import org.apache.polaris.immutables.PolarisImmutable;
-import org.immutables.value.Value;
/**
* 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.
+ * <p>This hierarchy makes the target shape explicit on the request itself
while preserving the
+ * normalized compatibility accessors used by current authorizer
implementations.
*/
-@PolarisImmutable
-public interface AuthorizationRequest {
+public sealed interface AuthorizationRequest
+ permits UntargetedAuthorizationRequest,
+ SingleTargetAuthorizationRequest,
+ PairwiseTargetAuthorizationRequest {
+ static AuthorizationRequest of(@Nonnull PolarisAuthorizableOperation
operation) {
+ return new UntargetedAuthorizationRequest(operation);
+ }
+
static AuthorizationRequest of(
- @Nonnull PolarisPrincipal principal,
- @Nonnull PolarisAuthorizableOperation operation,
- @Nonnull List<AuthorizationTargetBinding> targetBindings) {
- return ImmutableAuthorizationRequest.builder()
- .principal(principal)
- .operation(operation)
- .targetBindings(targetBindings)
- .build();
+ @Nonnull PolarisAuthorizableOperation operation, @Nonnull
PolarisSecurable target) {
+ return new SingleTargetAuthorizationRequest(operation, target);
}
- /** Returns the principal requesting authorization. */
- @Nonnull
- PolarisPrincipal getPrincipal();
+ static AuthorizationRequest of(
+ @Nonnull PolarisAuthorizableOperation operation,
+ @Nullable PolarisSecurable target,
+ @Nullable PolarisSecurable secondary) {
+ return new PairwiseTargetAuthorizationRequest(operation, target,
secondary);
+ }
/** Returns the operation being authorized. */
@Nonnull
PolarisAuthorizableOperation getOperation();
- /** Returns the target/secondary target bindings. */
+ /** Returns the primary target securables, if any. */
@Nonnull
- List<AuthorizationTargetBinding> getTargetBindings();
+ List<PolarisSecurable> getTargets();
- /**
- * Returns the primary target securables, if any.
- *
- * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
- */
+ /** Returns secondary securables, if any. */
@Nonnull
- @Value.Derived
- default List<PolarisSecurable> getTargets() {
- return getTargetBindings().stream()
- .map(AuthorizationTargetBinding::getTarget)
- .filter(Objects::nonNull)
- .toList();
- }
-
- /**
- * Returns secondary securables, if any.
- *
- * <p>Compatibility accessor derived from {@link #getTargetBindings()}.
- */
- @Nonnull
- @Value.Derived
- default List<PolarisSecurable> getSecondaries() {
- return getTargetBindings().stream()
- .map(AuthorizationTargetBinding::getSecondary)
- .filter(Objects::nonNull)
- .toList();
- }
-
- /**
- * Returns a stable debug string for authorization messages.
- *
- * <p>Includes the operation, principal name, formatted targets, and
formatted secondaries.
- */
- @Nonnull
- default String formatForAuthorizationMessage() {
- return String.format(
- "operation=%s principal=%s targets=%s secondaries=%s",
- getOperation(),
- getPrincipal().getName(),
- formatSecurables(getTargets()),
- formatSecurables(getSecondaries()));
- }
-
- private static String formatSecurables(List<PolarisSecurable> securables) {
- return securables.stream()
- .map(PolarisSecurable::formatForAuthorizationMessage)
- .collect(Collectors.joining(", ", "[", "]"));
- }
+ List<PolarisSecurable> getSecondaries();
Review Comment:
The interface breakdown (`PairwiseTargetAuthorizationRequest`, etc.) LGTM 👍
However, returning a list of targets and a list of secondaries appears to
contradict that interface hierarchy 🤔 it mixes pair-wise associations into a
list-to-list association... effectively making
`PairwiseTargetAuthorizationRequest` pretty useless, it seems 🤔
WDYT about using a visitor approach here?
`AuthorizationRequest.forEachRelation(visitor)` -- called by AuthZ
implementations
```
Visitor.process(operation)
Visitor.process(operation, PolarisSecurable target)
Visitor.process(operation, PolarisSecurable target, PolarisSecurable
secondary)
```
Then `AuthorizationRequest` can probably have only one concrete private
impl. for now without sub-interfaces.
Adding new relation types (shapes) will break existing AuthZ
implementations, but I think it would be a good break - it will force
implementors to analyze the new data shape and think about supporting it
property (rather than ignoring through omission).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
