adutra commented on code in PR #499: URL: https://github.com/apache/polaris/pull/499#discussion_r1864213021
########## polaris-service/src/main/java/org/apache/polaris/service/auth/AuthenticatedPolarisPrincipalImpl.java: ########## @@ -0,0 +1,49 @@ +/* + * 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 java.util.Set; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; + +public final class AuthenticatedPolarisPrincipalImpl implements AuthenticatedPolarisPrincipal { Review Comment: Maybe use a `record` here? ########## polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java: ########## @@ -19,26 +19,28 @@ package org.apache.polaris.core.auth; import jakarta.annotation.Nonnull; -import jakarta.annotation.Nullable; -import java.util.List; -import java.util.Set; -import org.apache.polaris.core.entity.PolarisBaseEntity; -import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; /** Interface for invoking authorization checks. */ public interface PolarisAuthorizer { + /** + * Validates whether the requested operation is permitted based on the collection of entities + * (including principals, roles, and catalog objects) that are affected by the operation. + * + * <p>"activated" entities, "targets" and "secondaries" are contained within the provided + * manifest. The extra selector parameters merely define what sub-set of objects from the manifest Review Comment: I think this sentence is obsolete now: "The extra selector parameters merely define what sub-set of objects from the manifest should be considered as "targets", etc." ########## polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java: ########## @@ -301,27 +288,22 @@ private void authorizeBasicTableLikeOperationOrThrow( // The underlying Catalog is also allowed to fetch "fresh" versions of the target entity. resolutionManifest.addPassthroughPath( + PRIMARY, new ResolverPath( PolarisCatalogHelpers.tableIdentifierToList(identifier), PolarisEntityType.TABLE_LIKE, true /* optional */), - identifier); + identifier, + subType, + () -> { + if (subType == PolarisEntitySubType.TABLE) { + throw new NoSuchTableException("Table does not exist: %s", identifier); + } else { + throw new NoSuchViewException("View does not exist: %s", identifier); + } Review Comment: Nitpicking, but below we are using the ternary operator for exactly the same logic: ```suggestion throw subType == PolarisEntitySubType.TABLE ? new NoSuchTableException("Table does not exist: %s", identifier) : new NoSuchViewException("View does not exist: %s", identifier); ``` ########## polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java: ########## @@ -18,55 +18,20 @@ */ package org.apache.polaris.core.auth; -import jakarta.annotation.Nonnull; -import java.util.List; import java.util.Set; -import org.apache.polaris.core.entity.PolarisEntity; -import org.apache.polaris.core.entity.PrincipalRoleEntity; /** Holds the results of request authentication. */ -public class AuthenticatedPolarisPrincipal implements java.security.Principal { - private final PolarisEntity principalEntity; - private final Set<String> activatedPrincipalRoleNames; - // only known and set after the above set of principal role names have been resolved. Before - // this, this list is null - private List<PrincipalRoleEntity> activatedPrincipalRoles; - - public AuthenticatedPolarisPrincipal( - @Nonnull PolarisEntity principalEntity, @Nonnull Set<String> activatedPrincipalRoles) { - this.principalEntity = principalEntity; - this.activatedPrincipalRoleNames = activatedPrincipalRoles; - this.activatedPrincipalRoles = null; - } - - @Override - public String getName() { - return principalEntity.getName(); - } - - public PolarisEntity getPrincipalEntity() { - return principalEntity; - } - - public Set<String> getActivatedPrincipalRoleNames() { - return activatedPrincipalRoleNames; - } - - public List<PrincipalRoleEntity> getActivatedPrincipalRoles() { - return activatedPrincipalRoles; - } - - public void setActivatedPrincipalRoles(List<PrincipalRoleEntity> activatedPrincipalRoles) { - this.activatedPrincipalRoles = activatedPrincipalRoles; - } - - @Override - public String toString() { - return "principalEntity=" - + getPrincipalEntity() - + ";activatedPrincipalRoleNames=" - + getActivatedPrincipalRoleNames() - + ";activatedPrincipalRoles=" - + getActivatedPrincipalRoles(); - } +public interface AuthenticatedPolarisPrincipal extends java.security.Principal { + + /** + * Principal entity ID obtained during request authentication (e.g. from the authorization token). + * + * <p>Negative values indicate that a principal ID was not provided in authenticated data, Review Comment: Nit: would an `OptionalLong` would be better here? -- 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]
