dimas-b commented on code in PR #465:
URL: https://github.com/apache/polaris/pull/465#discussion_r1884666490
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java:
##########
@@ -460,10 +462,13 @@ public class PolarisAuthorizerImpl implements
PolarisAuthorizer {
}
private final PolarisConfigurationStore featureConfig;
+ private final Provider<PolarisGrantManager> grantManagerFactory;
@Inject
- public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
+ public PolarisAuthorizerImpl(
+ PolarisConfigurationStore featureConfig, Provider<PolarisGrantManager>
grantManagerFactory) {
Review Comment:
Why are we injecting a `Provider` instead of `PolarisGrantManager` directly?
##########
dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/PolarisApplication.java:
##########
@@ -301,7 +303,12 @@ protected void configure() {
// manager
bindFactory(PolarisMetaStoreManagerFactory.class)
.in(RealmScoped.class)
+ .named("delegateGrantManager")
Review Comment:
The name looks too generic. It does not convey the purpose of the beam.
Also, the system could potentially have multiple delegates. How about
`primaryGrantManager`, `persistingGrantManager`?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.persistence.cache;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.auth.PolarisGrantManager;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.entity.PolarisGrantRecord;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+import org.apache.polaris.core.persistence.BaseResult;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * PolarisGrantManger implementation that uses an EntityCache to retrieve
entities and is backed by
+ * a delegate grant manager for persisting grant changes. This allows
consumers to reuse cache
+ * entities without necessarily being aware of the {@link EntityCache} or the
{@link
+ * EntityCacheEntry} specifics. Typically, the {@link
+ * org.apache.polaris.core.persistence.resolver.Resolver} is responsible for
validating the cache
+ * state. This class does no validation of the entity version or the grant
version of any cache
+ * record.
+ */
+public class EntityCacheGrantManager implements PolarisGrantManager {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(EntityCacheGrantManager.class);
+ private final PolarisGrantManager delegateGrantManager;
+ private final EntityCache entityCache;
+ private PolarisGrantRecord serviceAdminRootContainerGrant;
+ private PolarisBaseEntity serviceAdminEntity;
+
+ public EntityCacheGrantManager(
+ PolarisGrantManager delegateGrantManager, EntityCache entityCache) {
+ this.delegateGrantManager = delegateGrantManager;
+ this.entityCache = entityCache;
+ }
+
+ @Override
+ public @NotNull PrivilegeResult grantUsageOnRoleToGrantee(
+ @NotNull PolarisCallContext callCtx,
+ @Nullable PolarisEntityCore catalog,
+ @NotNull PolarisEntityCore role,
+ @NotNull PolarisEntityCore grantee) {
+ try {
+ return delegateGrantManager.grantUsageOnRoleToGrantee(callCtx, catalog,
role, grantee);
+ } finally {
+ LOGGER.debug("Invalidating cache for role {} and grantee {}", role,
grantee);
+ entityCache.removeCacheEntry(role);
+ entityCache.removeCacheEntry(grantee);
Review Comment:
I understand the internals of caches and persistence are still a mix
intermixed, but overall, this pattern looks concerning to me. Having to
invalidate the cache for `role` upon granting a privilege to it is
non-intuitive. I know I made API skew comments elsewhere already, so please
treat it as just another example... I do not think we should address this in
the current PR, but I think we ought to address it at some point :)
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCacheGrantManager.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.persistence.cache;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.auth.PolarisGrantManager;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.entity.PolarisGrantRecord;
+import org.apache.polaris.core.entity.PolarisPrivilege;
+import org.apache.polaris.core.persistence.BaseResult;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * PolarisGrantManger implementation that uses an EntityCache to retrieve
entities and is backed by
+ * a delegate grant manager for persisting grant changes. This allows
consumers to reuse cache
+ * entities without necessarily being aware of the {@link EntityCache} or the
{@link
+ * EntityCacheEntry} specifics. Typically, the {@link
+ * org.apache.polaris.core.persistence.resolver.Resolver} is responsible for
validating the cache
+ * state. This class does no validation of the entity version or the grant
version of any cache
+ * record.
+ */
+public class EntityCacheGrantManager implements PolarisGrantManager {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(EntityCacheGrantManager.class);
+ private final PolarisGrantManager delegateGrantManager;
+ private final EntityCache entityCache;
+ private PolarisGrantRecord serviceAdminRootContainerGrant;
+ private PolarisBaseEntity serviceAdminEntity;
+
+ public EntityCacheGrantManager(
+ PolarisGrantManager delegateGrantManager, EntityCache entityCache) {
+ this.delegateGrantManager = delegateGrantManager;
+ this.entityCache = entityCache;
+ }
+
+ @Override
+ public @NotNull PrivilegeResult grantUsageOnRoleToGrantee(
+ @NotNull PolarisCallContext callCtx,
+ @Nullable PolarisEntityCore catalog,
+ @NotNull PolarisEntityCore role,
+ @NotNull PolarisEntityCore grantee) {
+ try {
+ return delegateGrantManager.grantUsageOnRoleToGrantee(callCtx, catalog,
role, grantee);
+ } finally {
+ LOGGER.debug("Invalidating cache for role {} and grantee {}", role,
grantee);
+ entityCache.removeCacheEntry(role);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult revokeUsageOnRoleFromGrantee(
+ @NotNull PolarisCallContext callCtx,
+ @Nullable PolarisEntityCore catalog,
+ @NotNull PolarisEntityCore role,
+ @NotNull PolarisEntityCore grantee) {
+ try {
+ return delegateGrantManager.revokeUsageOnRoleFromGrantee(callCtx,
catalog, role, grantee);
+ } finally {
+ LOGGER.debug("Invalidating cache for role {} and grantee {}", role,
grantee);
+ entityCache.removeCacheEntry(role);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult grantPrivilegeOnSecurableToRole(
+ @NotNull PolarisCallContext callCtx,
+ @NotNull PolarisEntityCore grantee,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @NotNull PolarisEntityCore securable,
+ @NotNull PolarisPrivilege privilege) {
+ try {
+ return delegateGrantManager.grantPrivilegeOnSecurableToRole(
+ callCtx, grantee, catalogPath, securable, privilege);
+ } finally {
+ LOGGER.debug("Invalidating cache for securable {} and grantee {}",
securable, grantee);
+ entityCache.removeCacheEntry(securable);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull PrivilegeResult revokePrivilegeOnSecurableFromRole(
+ @NotNull PolarisCallContext callCtx,
+ @NotNull PolarisEntityCore grantee,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @NotNull PolarisEntityCore securable,
+ @NotNull PolarisPrivilege privilege) {
+ try {
+ return delegateGrantManager.revokePrivilegeOnSecurableFromRole(
+ callCtx, grantee, catalogPath, securable, privilege);
+ } finally {
+ LOGGER.debug("Invalidating cache for securable {} and grantee {}",
securable, grantee);
+ entityCache.removeCacheEntry(securable);
+ entityCache.removeCacheEntry(grantee);
+ }
+ }
+
+ @Override
+ public @NotNull LoadGrantsResult loadGrantsOnSecurable(
+ @NotNull PolarisCallContext callCtx, long securableCatalogId, long
securableId) {
+ EntityCacheLookupResult lookupResult =
+ entityCache.getOrLoadEntityById(callCtx, securableCatalogId,
securableId);
+ if (lookupResult == null || lookupResult.getCacheEntry() == null) {
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND,
null);
+ }
+ List<PolarisGrantRecord> grantRecords =
+ lookupResult.getCacheEntry().getGrantRecordsAsSecurable();
+ List<PolarisBaseEntity> granteeList = new ArrayList<>();
+ grantRecords.stream()
+ .map(
+ gr ->
+ entityCache.getOrLoadEntityById(
+ callCtx, gr.getGranteeCatalogId(), gr.getGranteeId()))
+ .filter(lr -> lr != null && lr.getCacheEntry() != null)
+ .map(lr -> lr.getCacheEntry().getEntity())
+ .forEach(granteeList::add);
+ if (granteeList.size() !=
lookupResult.getCacheEntry().getGrantRecordsAsSecurable().size()) {
+ LOGGER.error(
+ "Failed to resolve all grantees for securable {}",
+ lookupResult.getCacheEntry().getEntity());
+ return new LoadGrantsResult(BaseResult.ReturnStatus.GRANT_NOT_FOUND,
null);
+ }
+
+ // If the securable is the root container, then we need to add a grant
record for the
+ // service_admin PrincipalRole, which is the only role that has the
SERVICE_MANAGE_ACCESS
+ // privilege on the root
+ if (lookupResult
+ .getCacheEntry()
+ .getEntity()
+ .getName()
+ .equals(PolarisEntityConstants.getRootContainerName())) {
Review Comment:
Special cases by entity name look risky to me. The entity here is not
restricted, right? So, it can be a namespace, I suppose, and might have the
name of `root_container`, right?
--
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]
