collado-mike commented on code in PR #499: URL: https://github.com/apache/polaris/pull/499#discussion_r1866507211
########## 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 think this is something that was brought up in the Federated Entities doc. Principal Roles are identity roles, not like Catalog Roles, which are groups of privileges. If Principal Roles are a form of identity (e.g., LDAP groups, Okta groups, or IAM group), then membership in those groups is the purview of the Identity Provider, not the Authorizer. I.e., if LDAP says I'm a member of the ENGINEER group, then that claim should be authoritative. The privileges assigned to the ENGINEER group are the purview of the Authorizer (e.g., are there grant records to show the ENGINEER role has access to read a table, is there a policy document that granting the ENGINEER group privileges to create a table in a given namespace...). If the Authorizer is doubly responsible for validating group membership and for determining privileges, then either the Identity Provider must also be able to store privilege assignments or the identity/group membership must be duplicated in the Authorizer (e.g., an IAM policy must exist that asserts that I'm a member of the ENGINEER group and that the ENGINEER group has the `create_table` privilege). The main reason I think this strengthens the coupling is that the AuthenticatedPrincipal passes in only the role names without resolving the entities. If we remove the check for the grant records, then a caller could pass in whatever scope they want and be able to assume that role without confirmation they actually have access to that role. What I'm hoping to do is change this class to take in the list of validated `PrincipalRole` entities so that the Authenticator itself is responsible for resolving the roles and validating (by whatever means) that the user has access to those roles. At that point, we can change the Resolver and Authorizer to rely on the AuthenticatedPrincipal to directly return the list of `PrincipalRole` entities, rather than just the names, knowing that that list has already been validated by the Authenticator. -- 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]
