collado-mike commented on code in PR #499:
URL: https://github.com/apache/polaris/pull/499#discussion_r1866310030
##########
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
+ * should be considered as "targets", etc.
+ *
+ * <p>The effective principal information is also provided in the manifest.
+ *
+ * @param manifest defines the input for authorization checks.
+ * @param operation the operation being authorized.
+ * @param considerCatalogRoles whether catalog roles should be considered
({@code true}) or only
+ * principal roles ({@code false}).
+ */
void authorizeOrThrow(
- @Nonnull AuthenticatedPolarisPrincipal authenticatedPrincipal,
- @Nonnull Set<PolarisBaseEntity> activatedEntities,
- @Nonnull PolarisAuthorizableOperation authzOp,
- @Nullable PolarisResolvedPathWrapper target,
- @Nullable PolarisResolvedPathWrapper secondary);
-
- void authorizeOrThrow(
- @Nonnull AuthenticatedPolarisPrincipal authenticatedPrincipal,
- @Nonnull Set<PolarisBaseEntity> activatedEntities,
- @Nonnull PolarisAuthorizableOperation authzOp,
- @Nullable List<PolarisResolvedPathWrapper> targets,
- @Nullable List<PolarisResolvedPathWrapper> secondaries);
+ @Nonnull PolarisResolutionManifest manifest,
Review Comment:
My intention in https://github.com/apache/polaris/pull/465 is to decouple
the PolarisAuthorizer logic from the Resolver code. IMO, the Authorizer should
take in a list of entities (principal and authz targets) without regard for how
those entities were retrieved. I kept the `PolarisResolvedPathWrapper` types in
the signature just because we need a way to pass in full entity paths (e.g., a
table's lineage, including parent namespaces and catalog) and I think it's more
friendly that passing around Lists of Lists. But overall, I think we should
avoid tying the Authorizer to the entity resolution.
##########
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 {
+ private final long id;
+ private final String name;
+ private final Set<String> roles;
+
+ public AuthenticatedPolarisPrincipalImpl(long id, String name, Set<String>
roles) {
Review Comment:
I'm actually thinking we should move toward resolving the `PrincipalRole`
entities prior to the authorization check. Right now, the Resolver code
validates that the `AuthenticatedPrincipal` actually has the grant records to
assume the roles specified in the authentication token. That means that we
can't decouple the authenticator from the resolver, as someone needs to
validate that the roles specified in the token can actually be assumed by the
principal. I think that validation should happen at the Authenticator. This
allows for third party identity providers to validate the roles in the token
without needing grant records, while the default authenticator can continue to
rely on the grant records stored by the grant manager.
--
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]
