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