gracechen09 commented on code in PR #4409:
URL: https://github.com/apache/polaris/pull/4409#discussion_r3326066577
##########
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:
Just curious if this is intended to make the code less verbose in the call
site, or if it's just used for testing? if it's the former, would this method
duplicate what `AuthorizationRequest of(
@NonNull PolarisPrincipal principal, @NonNull AuthorizationIntent
intent)` does ?
##########
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:
I'm wondering if we could keep `request.formatForAuthorizationMessage()` in
the deny log for better observability
##########
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 might have missed some previous discussions on this. I'm wondering if
we've thought about substituting empty target with the root target on the
Polaris side instead of letting OPA to handle the empty target?
Sending the empty list may become a bit confusing for the OPA policy
maintainers. For example, they'll need to know that `LIST_CATALOG` auth request
has an empty target and maintain an OPA policy to handle 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
