sungwy commented on code in PR #4409:
URL: https://github.com/apache/polaris/pull/4409#discussion_r3335058342
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java:
##########
@@ -18,111 +18,53 @@
*/
package org.apache.polaris.core.auth;
+import com.google.common.base.Preconditions;
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;
import org.jspecify.annotations.NonNull;
-/**
- * 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();
+/** Full authorization request containing the subject and one or more
authorization intents. */
+public record AuthorizationRequest(
+ @NonNull PolarisPrincipal principal, @NonNull List<AuthorizationIntent>
intents) {
+ public AuthorizationRequest {
+ Preconditions.checkNotNull(principal, "principal must be non-null");
+ Preconditions.checkNotNull(intents, "intents must be non-null");
+ intents = List.copyOf(intents);
+ Preconditions.checkArgument(
+ !intents.isEmpty(), "Authorization request must contain at least one
intent");
}
- /** 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
- @Value.Derived
- default List<PolarisSecurable> getTargets() {
- return getTargetBindings().stream()
- .map(AuthorizationTargetBinding::getTarget)
- .filter(Objects::nonNull)
- .toList();
+ public static AuthorizationRequest of(
+ @NonNull PolarisPrincipal principal, @NonNull AuthorizationIntent
intent) {
+ return new AuthorizationRequest(principal, List.of(intent));
}
- /**
- * 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();
+ public static AuthorizationRequest of(
+ @NonNull PolarisPrincipal principal, @NonNull List<AuthorizationIntent>
intents) {
+ return new AuthorizationRequest(principal, intents);
}
- /**
- * 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()));
+ public static AuthorizationRequest of(
+ @NonNull PolarisPrincipal principal, @NonNull
PolarisAuthorizableOperation operation) {
+ return of(principal, AuthorizationIntent.of(operation));
}
- private static String formatSecurables(List<PolarisSecurable> securables) {
- return securables.stream()
- .map(PolarisSecurable::formatForAuthorizationMessage)
- .collect(Collectors.joining(", ", "[", "]"));
+ public static AuthorizationRequest of(
Review Comment:
yes exactly - but based on what we are discussing with @flyrain - I think
we'll save a lot of confusion if I just removed these factory methods from this
PR and just reintroduced them when we work on switching the handler callsites.
https://github.com/apache/polaris/pull/4409/changes/BASE..0f91ce6646b19f96fa63dfb88b0a9f91db711fb1#r3312361740
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -296,9 +319,9 @@ private ImmutableContext buildContext() {
private ImmutableResource buildResource(
List<ResourceEntity> targets, List<ResourceEntity> secondaries) {
- // Backward compatibility: keep the existing OPA input shape with separate
target and
- // secondary lists. Future work can align this with
AuthorizationTargetBinding semantics
- // using binding tuples like [(target, secondary), ...].
+ // Keep the existing OPA input shape by always emitting target and
secondary lists, using
+ // empty lists when an intent does not carry that slot. Future work can
revisit the payload
+ // shape if OPA starts consuming intent subtype distinctions directly.
Review Comment:
I agree. In this PR I'm intentionally keeping the shape the same here
because I think it'd make sense to group those backwards incompatible changes
all in one go in a PR that's targeted at updating the OPA input contract.
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -117,17 +121,36 @@ public void resolveAuthorizationInputs(
@NonNull
public AuthorizationDecision authorize(
@NonNull AuthorizationState authzState, @NonNull AuthorizationRequest
request) {
- boolean allowed =
- queryOpa(
- buildOpaAuthorizationInput(
- request.getPrincipal(),
- request.getOperation(),
- toResourceEntitiesFromSecurables(request.getTargets()),
- toResourceEntitiesFromSecurables(request.getSecondaries())));
- return allowed
- ? AuthorizationDecision.allow()
- : AuthorizationDecision.deny(
- "OPA denied authorization for " +
request.formatForAuthorizationMessage());
+ for (AuthorizationIntent intent : request.intents()) {
+ PolarisAuthorizableOperation operation = intent.getOperation();
+ List<ResourceEntity> targets;
+ List<ResourceEntity> secondaries;
+ switch (intent) {
+ case TargetlessAuthorizationIntent ignored -> {
+ targets = List.of();
+ secondaries = List.of();
+ }
+ case SingleTargetAuthorizationIntent singleTargetIntent -> {
+ targets =
toResourceEntitiesFromSecurable(singleTargetIntent.target());
+ secondaries = List.of();
+ }
+ case PairwiseTargetAuthorizationIntent pairwiseTargetIntent -> {
+ targets =
toResourceEntitiesFromSecurable(pairwiseTargetIntent.target());
+ secondaries =
toResourceEntitiesFromSecurable(pairwiseTargetIntent.secondary());
+ }
+ }
+ boolean allowed =
+ queryOpa(
+ buildOpaAuthorizationInput(request.principal(), operation,
targets, secondaries));
+ if (!allowed) {
+ return AuthorizationDecision.deny(
Review Comment:
There was a prior discussion on a related PR, where we voiced concerns on
leaking too much Polaris internal security details to the external caller.
My understanding is that `AuthorizationDecision.deny()` message will be
forwarded directly in the error message of the Iceberg REST response.
--
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]
