dimas-b commented on code in PR #623: URL: https://github.com/apache/polaris/pull/623#discussion_r1911877115
########## service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.inject.Inject; +import jakarta.inject.Provider; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisGrantManager; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of the {@link ActiveRolesProvider} looks up the grant records for a + * principal to determine roles that are available. {@link + * AuthenticatedPolarisPrincipal#getActivatedPrincipalRoleNames()} is used to determine which of the + * available roles are active for this request. + */ +public class DefaultActiveRolesProvider implements ActiveRolesProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultActiveRolesProvider.class); + @Inject Provider<RealmContext> realmContextProvider; + @Inject MetaStoreManagerFactory metaStoreManagerFactory; + @Inject Provider<PolarisGrantManager> polarisGrantManagerProvider; + + @Override + public Set<String> getActiveRoles(AuthenticatedPolarisPrincipal principal) { + List<PrincipalRoleEntity> activeRoles = + loadActivePrincipalRoles( + principal.getActivatedPrincipalRoleNames(), + principal.getPrincipalEntity(), + metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContextProvider.get())); + return activeRoles.stream().map(PrincipalRoleEntity::getName).collect(Collectors.toSet()); + } + + protected List<PrincipalRoleEntity> loadActivePrincipalRoles( Review Comment: Why `protected`? There are no overrides in the Polaris codebase, and the class is not really part of public API, is it? ########## service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.inject.Inject; +import jakarta.inject.Provider; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisGrantManager; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of the {@link ActiveRolesProvider} looks up the grant records for a + * principal to determine roles that are available. {@link + * AuthenticatedPolarisPrincipal#getActivatedPrincipalRoleNames()} is used to determine which of the + * available roles are active for this request. + */ +public class DefaultActiveRolesProvider implements ActiveRolesProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultActiveRolesProvider.class); + @Inject Provider<RealmContext> realmContextProvider; + @Inject MetaStoreManagerFactory metaStoreManagerFactory; + @Inject Provider<PolarisGrantManager> polarisGrantManagerProvider; + + @Override + public Set<String> getActiveRoles(AuthenticatedPolarisPrincipal principal) { + List<PrincipalRoleEntity> activeRoles = + loadActivePrincipalRoles( + principal.getActivatedPrincipalRoleNames(), + principal.getPrincipalEntity(), + metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContextProvider.get())); + return activeRoles.stream().map(PrincipalRoleEntity::getName).collect(Collectors.toSet()); + } + + protected List<PrincipalRoleEntity> loadActivePrincipalRoles( + Set<String> tokenRoles, PolarisEntity principal, PolarisMetaStoreManager metaStoreManager) { + PolarisCallContext polarisContext = CallContext.getCurrentContext().getPolarisCallContext(); + PolarisGrantManager.LoadGrantsResult principalGrantResults = + polarisGrantManagerProvider.get().loadGrantsToGrantee(polarisContext, principal); + polarisContext + .getDiagServices() + .check( + principalGrantResults.isSuccess(), + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + if (!principalGrantResults.isSuccess()) { + LOGGER.warn( + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + throw new NotAuthorizedException("Unable to authenticate"); + } + boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL); + Predicate<PrincipalRoleEntity> includeRoleFilter = + allRoles ? r -> true : r -> tokenRoles.contains(r.getName()); + List<PrincipalRoleEntity> activeRoles = + principalGrantResults.getGrantRecords().stream() + .map( + gr -> + metaStoreManager.loadEntity( + polarisContext, gr.getSecurableCatalogId(), gr.getSecurableId())) + .filter(PolarisMetaStoreManager.EntityResult::isSuccess) + .map(PolarisMetaStoreManager.EntityResult::getEntity) + .map(PrincipalRoleEntity::of) Review Comment: Callers always use only the names of active roles... why bother converting to a different object type? ########## service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.inject.Inject; +import jakarta.inject.Provider; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisGrantManager; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of the {@link ActiveRolesProvider} looks up the grant records for a + * principal to determine roles that are available. {@link + * AuthenticatedPolarisPrincipal#getActivatedPrincipalRoleNames()} is used to determine which of the + * available roles are active for this request. + */ +public class DefaultActiveRolesProvider implements ActiveRolesProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultActiveRolesProvider.class); + @Inject Provider<RealmContext> realmContextProvider; + @Inject MetaStoreManagerFactory metaStoreManagerFactory; + @Inject Provider<PolarisGrantManager> polarisGrantManagerProvider; + + @Override + public Set<String> getActiveRoles(AuthenticatedPolarisPrincipal principal) { + List<PrincipalRoleEntity> activeRoles = + loadActivePrincipalRoles( + principal.getActivatedPrincipalRoleNames(), + principal.getPrincipalEntity(), + metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContextProvider.get())); + return activeRoles.stream().map(PrincipalRoleEntity::getName).collect(Collectors.toSet()); + } + + protected List<PrincipalRoleEntity> loadActivePrincipalRoles( + Set<String> tokenRoles, PolarisEntity principal, PolarisMetaStoreManager metaStoreManager) { + PolarisCallContext polarisContext = CallContext.getCurrentContext().getPolarisCallContext(); + PolarisGrantManager.LoadGrantsResult principalGrantResults = + polarisGrantManagerProvider.get().loadGrantsToGrantee(polarisContext, principal); + polarisContext + .getDiagServices() + .check( + principalGrantResults.isSuccess(), + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + if (!principalGrantResults.isSuccess()) { + LOGGER.warn( + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + throw new NotAuthorizedException("Unable to authenticate"); + } + boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL); + Predicate<PrincipalRoleEntity> includeRoleFilter = + allRoles ? r -> true : r -> tokenRoles.contains(r.getName()); + List<PrincipalRoleEntity> activeRoles = + principalGrantResults.getGrantRecords().stream() + .map( + gr -> + metaStoreManager.loadEntity( + polarisContext, gr.getSecurableCatalogId(), gr.getSecurableId())) + .filter(PolarisMetaStoreManager.EntityResult::isSuccess) Review Comment: Why do we treat failures to load grant records as an error (line 79) but filter out failures to load related roles? ########## dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/auth/PolarisPrincipalAuthenticator.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.dropwizard.auth; + +import jakarta.annotation.Priority; +import jakarta.inject.Inject; +import jakarta.ws.rs.Priorities; +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerRequestFilter; +import jakarta.ws.rs.core.SecurityContext; +import java.io.IOException; +import java.security.Principal; +import java.util.Optional; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.service.auth.Authenticator; + [email protected] +@Priority(Priorities.AUTHENTICATION) +public class PolarisPrincipalAuthenticator implements ContainerRequestFilter { + @Inject private Authenticator<String, AuthenticatedPolarisPrincipal> authenticator; + + @Override + public void filter(ContainerRequestContext requestContext) throws IOException { + String authHeader = requestContext.getHeaderString("Authorization"); + if (authHeader == null) { + throw new IOException("Authorization header is missing"); + } + int spaceIdx = authHeader.indexOf(' '); + if (spaceIdx <= 0 || !authHeader.substring(0, spaceIdx).equalsIgnoreCase("Bearer")) { + throw new IOException("Authorization header is not a Bearer token"); Review Comment: Why is this not a `NotAuthorizedException`? Ideally the response should be 401 with the `WWW-Authenticate: Bearer` header, I think. ########## service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.inject.Inject; +import jakarta.inject.Provider; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisGrantManager; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of the {@link ActiveRolesProvider} looks up the grant records for a + * principal to determine roles that are available. {@link + * AuthenticatedPolarisPrincipal#getActivatedPrincipalRoleNames()} is used to determine which of the + * available roles are active for this request. + */ +public class DefaultActiveRolesProvider implements ActiveRolesProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultActiveRolesProvider.class); + @Inject Provider<RealmContext> realmContextProvider; + @Inject MetaStoreManagerFactory metaStoreManagerFactory; + @Inject Provider<PolarisGrantManager> polarisGrantManagerProvider; + + @Override + public Set<String> getActiveRoles(AuthenticatedPolarisPrincipal principal) { + List<PrincipalRoleEntity> activeRoles = + loadActivePrincipalRoles( + principal.getActivatedPrincipalRoleNames(), + principal.getPrincipalEntity(), + metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContextProvider.get())); + return activeRoles.stream().map(PrincipalRoleEntity::getName).collect(Collectors.toSet()); + } + + protected List<PrincipalRoleEntity> loadActivePrincipalRoles( + Set<String> tokenRoles, PolarisEntity principal, PolarisMetaStoreManager metaStoreManager) { + PolarisCallContext polarisContext = CallContext.getCurrentContext().getPolarisCallContext(); + PolarisGrantManager.LoadGrantsResult principalGrantResults = + polarisGrantManagerProvider.get().loadGrantsToGrantee(polarisContext, principal); + polarisContext + .getDiagServices() + .check( + principalGrantResults.isSuccess(), + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + if (!principalGrantResults.isSuccess()) { + LOGGER.warn( + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + throw new NotAuthorizedException("Unable to authenticate"); Review Comment: This looks like an overreach to me... The `ActiveRolesProvider` interface does not imply that the provider makes authorization decisions. I'd think we should either return an empty list of active roles in this case, or raise an "internal server error" instead. Also, the message mentions "authenticate", but there was no authentication decision made in the context of this method. The input was already an `AuthenticatedPolarisPrincipal` :thinking: ########## service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.inject.Inject; +import jakarta.inject.Provider; +import java.util.List; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import org.apache.iceberg.exceptions.NotAuthorizedException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisGrantManager; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of the {@link ActiveRolesProvider} looks up the grant records for a + * principal to determine roles that are available. {@link + * AuthenticatedPolarisPrincipal#getActivatedPrincipalRoleNames()} is used to determine which of the + * available roles are active for this request. + */ +public class DefaultActiveRolesProvider implements ActiveRolesProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultActiveRolesProvider.class); + @Inject Provider<RealmContext> realmContextProvider; + @Inject MetaStoreManagerFactory metaStoreManagerFactory; + @Inject Provider<PolarisGrantManager> polarisGrantManagerProvider; + + @Override + public Set<String> getActiveRoles(AuthenticatedPolarisPrincipal principal) { + List<PrincipalRoleEntity> activeRoles = + loadActivePrincipalRoles( + principal.getActivatedPrincipalRoleNames(), + principal.getPrincipalEntity(), + metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContextProvider.get())); + return activeRoles.stream().map(PrincipalRoleEntity::getName).collect(Collectors.toSet()); + } + + protected List<PrincipalRoleEntity> loadActivePrincipalRoles( + Set<String> tokenRoles, PolarisEntity principal, PolarisMetaStoreManager metaStoreManager) { + PolarisCallContext polarisContext = CallContext.getCurrentContext().getPolarisCallContext(); + PolarisGrantManager.LoadGrantsResult principalGrantResults = + polarisGrantManagerProvider.get().loadGrantsToGrantee(polarisContext, principal); + polarisContext + .getDiagServices() + .check( + principalGrantResults.isSuccess(), + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + if (!principalGrantResults.isSuccess()) { + LOGGER.warn( + "Failed to resolve principal roles for principal name={} id={}", + principal.getName(), + principal.getId()); + throw new NotAuthorizedException("Unable to authenticate"); + } + boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL); + Predicate<PrincipalRoleEntity> includeRoleFilter = + allRoles ? r -> true : r -> tokenRoles.contains(r.getName()); + List<PrincipalRoleEntity> activeRoles = + principalGrantResults.getGrantRecords().stream() + .map( + gr -> + metaStoreManager.loadEntity( + polarisContext, gr.getSecurableCatalogId(), gr.getSecurableId())) + .filter(PolarisMetaStoreManager.EntityResult::isSuccess) + .map(PolarisMetaStoreManager.EntityResult::getEntity) + .map(PrincipalRoleEntity::of) + .filter(includeRoleFilter) + .toList(); + if (activeRoles.size() != principalGrantResults.getGrantRecords().size()) { + LOGGER + .atWarn() + .addKeyValue("principal", principal.getName()) + .addKeyValue("scopes", tokenRoles) + .addKeyValue("roles", activeRoles) + .log("Some principal roles were not found in the principal's grants"); Review Comment: Is this message helpful for trouble-shooting? Why not log individual failures? -- 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]
