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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
