This is an automated email from the ASF dual-hosted git repository. yuqi4733 pushed a commit to branch revert-10119-ISSUE-10117 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit d593e35fad0ec14cf16f6c699fda67685d1a4515 Author: Qi Yu <[email protected]> AuthorDate: Fri Mar 6 14:50:44 2026 +0800 Revert "[#10117] improvement(authz): Remove ownership relation cache (#10119)" This reverts commit 7f94ffdb51117346b092716eb00a9c318fc76705. --- .../main/java/org/apache/gravitino/Configs.java | 9 ++++ .../authorization/GravitinoAuthorizer.java | 12 +++++ .../gravitino/authorization/OwnerManager.java | 30 ++++++++++++ docs/security/access-control.md | 7 ++- .../authorization/PassThroughAuthorizer.java | 4 ++ .../authorization/jcasbin/JcasbinAuthorizer.java | 55 ++++++++++++++++------ .../authorization/MockGravitinoAuthorizer.java | 4 ++ .../jcasbin/TestJcasbinAuthorizer.java | 44 +++++++++++++++++ .../filter/TestGravitinoInterceptionService.java | 4 ++ 9 files changed, 153 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/Configs.java b/core/src/main/java/org/apache/gravitino/Configs.java index a26f3c36a5..3cc9d703e6 100644 --- a/core/src/main/java/org/apache/gravitino/Configs.java +++ b/core/src/main/java/org/apache/gravitino/Configs.java @@ -320,6 +320,15 @@ public class Configs { .longConf() .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE); + public static final long DEFAULT_GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE = 100000L; + + public static final ConfigEntry<Long> GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE = + new ConfigBuilder("gravitino.authorization.jcasbin.ownerCacheSize") + .doc("The maximum size of the owner cache for authorization") + .version(ConfigConstants.VERSION_1_1_1) + .longConf() + .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE); + public static final ConfigEntry<List<String>> SERVICE_ADMINS = new ConfigBuilder("gravitino.authorization.serviceAdmins") .doc("The admins of Gravitino service") diff --git a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java index 3a4fbfc0a9..7965f173ad 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java +++ b/core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java @@ -152,4 +152,16 @@ public interface GravitinoAuthorizer extends Closeable { throw new RuntimeException("Can not get Role Entity", e); } } + + /** + * This method is called to clear the owner relationship in jcasbin when the owner of the metadata + * changes. + * + * @param metalake metalake; + * @param oldOwnerId The old owner id; + * @param nameIdentifier The metadata name identifier; + * @param type entity type + */ + void handleMetadataOwnerChange( + String metalake, Long oldOwnerId, NameIdentifier nameIdentifier, Entity.EntityType type); } diff --git a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java index 13fa04a80d..5b7ce28c98 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java @@ -25,6 +25,7 @@ import java.util.Optional; import lombok.Getter; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.SupportsRelationOperations; @@ -36,6 +37,7 @@ import org.apache.gravitino.lock.TreeLockUtils; import org.apache.gravitino.meta.GroupEntity; import org.apache.gravitino.meta.UserEntity; import org.apache.gravitino.utils.MetadataObjectUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -118,6 +120,7 @@ public class OwnerManager implements OwnerDispatcher { metadataObject, authorizationPlugin -> authorizationPlugin.onOwnerSet(metadataObject, originOwner.orElse(null), newOwner)); + originOwner.ifPresent(owner -> notifyOwnerChange(owner, metalake, metadataObject)); } catch (NoSuchEntityException nse) { LOG.warn( "Metadata object {} or owner {} is not found", metadataObject.fullName(), ownerName, nse); @@ -133,6 +136,33 @@ public class OwnerManager implements OwnerDispatcher { } } + private void notifyOwnerChange(Owner oldOwner, String metalake, MetadataObject metadataObject) { + GravitinoAuthorizer gravitinoAuthorizer = GravitinoEnv.getInstance().gravitinoAuthorizer(); + if (gravitinoAuthorizer != null) { + if (oldOwner.type() == Owner.Type.USER) { + try { + UserEntity userEntity = + GravitinoEnv.getInstance() + .entityStore() + .get( + NameIdentifierUtil.ofUser(metalake, oldOwner.name()), + Entity.EntityType.USER, + UserEntity.class); + gravitinoAuthorizer.handleMetadataOwnerChange( + metalake, + userEntity.id(), + MetadataObjectUtil.toEntityIdent(metalake, metadataObject), + Entity.EntityType.valueOf(metadataObject.type().name())); + } catch (IOException e) { + LOG.warn(e.getMessage(), e); + } + } else { + throw new UnsupportedOperationException( + "Notification for Group Owner is not supported yet."); + } + } + } + @Override public Optional<Owner> getOwner(String metalake, MetadataObject metadataObject) { NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); diff --git a/docs/security/access-control.md b/docs/security/access-control.md index bb8e85aee1..1724cdde07 100644 --- a/docs/security/access-control.md +++ b/docs/security/access-control.md @@ -446,17 +446,20 @@ To enable access control in Gravitino, configure the following settings in your | `gravitino.authorization.serviceAdmins` | Comma-separated list of service administrator usernames | (none) | Yes (when authorization is enabled) | 0.5.0 | | `gravitino.authorization.jcasbin.cacheExpirationSecs` | The expiration time in seconds for authorization cache entries | `3600` | No | 1.1.1 | | `gravitino.authorization.jcasbin.roleCacheSize` | The maximum size of the role cache for authorization | `10000` | No | 1.1.1 | +| `gravitino.authorization.jcasbin.ownerCacheSize` | The maximum size of the owner cache for authorization | `100000` | No | 1.1.1 | ### Authorization Cache -Gravitino uses Caffeine caches to improve authorization performance by caching role information. The cache configuration options allow you to tune the cache behavior: +Gravitino uses Caffeine caches to improve authorization performance by caching role and owner information. The cache configuration options allow you to tune the cache behavior: - **`cacheExpirationSecs`**: Controls how long cache entries remain valid. After this time, entries are automatically evicted and reloaded from the backend on the next access. Lower values provide more up-to-date authorization decisions but may increase load on the backend. - **`roleCacheSize`**: Controls the maximum number of role entries that can be cached. When the cache reaches this size, the least recently used entries are evicted. +- **`ownerCacheSize`**: Controls the maximum number of owner relationship entries that can be cached. This cache maps metadata object IDs to their owner IDs. + :::info -When role privileges are changed through the Gravitino API, the corresponding cache entries are automatically invalidated to ensure authorization decisions reflect the latest state. +When role privileges or ownership are changed through the Gravitino API, the corresponding cache entries are automatically invalidated to ensure authorization decisions reflect the latest state. ::: ### Important Notes diff --git a/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java b/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java index 82a5d3a82a..06e20b8ada 100644 --- a/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java +++ b/server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java @@ -109,6 +109,10 @@ public class PassThroughAuthorizer implements GravitinoAuthorizer { @Override public void handleRolePrivilegeChange(String metalake, String roleName) {} + @Override + public void handleMetadataOwnerChange( + String metalake, Long oldOwnerId, NameIdentifier nameIdentifier, Entity.EntityType type) {} + @Override public void close() throws IOException {} } diff --git a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java index 524c90f032..cea47e353b 100644 --- a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java +++ b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java @@ -29,6 +29,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; import java.util.concurrent.Executors; @@ -86,6 +87,8 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { */ private Cache<Long, Boolean> loadedRoles; + private Cache<Long, Optional<Long>> ownerRel; + private Executor executor = null; @Override @@ -96,6 +99,8 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { .get(Configs.GRAVITINO_AUTHORIZATION_CACHE_EXPIRATION_SECS); long roleCacheSize = GravitinoEnv.getInstance().config().get(Configs.GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE); + long ownerCacheSize = + GravitinoEnv.getInstance().config().get(Configs.GRAVITINO_AUTHORIZATION_OWNER_CACHE_SIZE); // Initialize enforcers before the caches that reference them in removal listeners allowEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new GravitinoAdapter()); @@ -116,7 +121,11 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { } }) .build(); - + ownerRel = + Caffeine.newBuilder() + .expireAfterAccess(cacheExpirationSecs, TimeUnit.SECONDS) + .maximumSize(ownerCacheSize) + .build(); executor = Executors.newFixedThreadPool( GravitinoEnv.getInstance() @@ -209,9 +218,15 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { String metalake, MetadataObject metadataObject, AuthorizationRequestContext requestContext) { + Long userId; boolean result; try { - result = checkOwner(metalake, metadataObject, principal.getName()); + Long metadataId = MetadataIdConverter.getID(metadataObject, metalake); + loadOwnerPolicy(metalake, metadataObject, metadataId); + UserEntity userEntity = getUserEntity(principal.getName(), metalake); + userId = userEntity.id(); + metadataId = MetadataIdConverter.getID(metadataObject, metalake); + result = Objects.equals(Optional.of(userId), ownerRel.getIfPresent(metadataId)); } catch (Exception e) { LOG.debug("Can not get entity id", e); result = false; @@ -372,6 +387,14 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { loadedRoles.invalidate(roleId); } + @Override + public void handleMetadataOwnerChange( + String metalake, Long oldOwnerId, NameIdentifier nameIdentifier, Entity.EntityType type) { + MetadataObject metadataObject = NameIdentifierUtil.toMetadataObject(nameIdentifier, type); + Long metadataId = MetadataIdConverter.getID(metadataObject, metalake); + ownerRel.invalidate(metadataId); + } + @Override public void close() throws IOException { if (executor != null) { @@ -408,9 +431,6 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { AuthorizationRequestContext requestContext) { Long metadataId; Long userId; - if (AuthConstants.OWNER.equals(privilege)) { - return checkOwner(metalake, metadataObject, username); - } try { UserEntity userEntity = getUserEntity(username, metalake); userId = userEntity.id(); @@ -419,13 +439,16 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { LOG.debug("Can not get entity id", e); return false; } - loadRolePrivilege(metalake, username, userId, requestContext); return authorizeByJcasbin(userId, metadataObject, metadataId, privilege); } private boolean authorizeByJcasbin( Long userId, MetadataObject metadataObject, Long metadataId, String privilege) { + if (AuthConstants.OWNER.equals(privilege)) { + Optional<Long> owner = ownerRel.getIfPresent(metadataId); + return Objects.equals(Optional.of(userId), owner); + } return enforcer.enforce( String.valueOf(userId), String.valueOf(metadataObject.type()), @@ -495,7 +518,11 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { }); } - private boolean checkOwner(String metalake, MetadataObject metadataObject, String userName) { + private void loadOwnerPolicy(String metalake, MetadataObject metadataObject, Long metadataId) { + if (ownerRel.getIfPresent(metadataId) != null) { + LOG.debug("Metadata {} OWNER has been loaded.", metadataId); + return; + } try { NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); EntityStore entityStore = GravitinoEnv.getInstance().entityStore(); @@ -506,19 +533,19 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { SupportsRelationOperations.Type.OWNER_REL, entityIdent, Entity.EntityType.valueOf(metadataObject.type().name())); - - for (Entity ownerEntity : owners) { - if (ownerEntity instanceof UserEntity) { - UserEntity user = (UserEntity) ownerEntity; - if (user.name().equals(userName)) { - return true; + if (owners.isEmpty()) { + ownerRel.put(metadataId, Optional.empty()); + } else { + for (Entity ownerEntity : owners) { + if (ownerEntity instanceof UserEntity) { + UserEntity user = (UserEntity) ownerEntity; + ownerRel.put(metadataId, Optional.of(user.id())); } } } } catch (IOException e) { LOG.warn("Can not load metadata owner", e); } - return false; } private void loadPolicyByRoleEntity(RoleEntity roleEntity) { diff --git a/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java b/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java index acb388c689..f9ef64bab0 100644 --- a/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java +++ b/server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java @@ -112,6 +112,10 @@ public class MockGravitinoAuthorizer implements GravitinoAuthorizer { @Override public void handleRolePrivilegeChange(Long roleId) {} + @Override + public void handleMetadataOwnerChange( + String metalake, Long oldOwnerId, NameIdentifier nameIdentifier, Entity.EntityType type) {} + @Override public void close() {} } diff --git a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java index 41088df93c..4388ef0952 100644 --- a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java +++ b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java @@ -36,7 +36,9 @@ import com.google.common.collect.ImmutableList; import java.io.IOException; import java.lang.reflect.Field; import java.security.Principal; +import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.concurrent.Executor; import java.util.stream.Collectors; import org.apache.commons.lang3.reflect.FieldUtils; @@ -243,7 +245,17 @@ public class TestJcasbinAuthorizer { eq(SupportsRelationOperations.Type.OWNER_REL), eq(catalogIdent), eq(Entity.EntityType.CATALOG)); + getOwnerRelCache(jcasbinAuthorizer).invalidateAll(); assertTrue(doAuthorizeOwner(currentPrincipal)); + doReturn(new ArrayList<>()) + .when(supportsRelationOperations) + .listEntitiesByRelation( + eq(SupportsRelationOperations.Type.OWNER_REL), + eq(catalogIdent), + eq(Entity.EntityType.CATALOG)); + jcasbinAuthorizer.handleMetadataOwnerChange( + METALAKE, USER_ID, catalogIdent, Entity.EntityType.CATALOG); + assertFalse(doAuthorizeOwner(currentPrincipal)); } private Boolean doAuthorize(Principal currentPrincipal) { @@ -367,6 +379,28 @@ public class TestJcasbinAuthorizer { assertNull(loadedRoles.getIfPresent(testRoleId)); } + @Test + public void testOwnerCacheInvalidation() throws Exception { + // Get the ownerRel cache via reflection + Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer); + + // Manually add an owner relation to the cache + ownerRel.put(CATALOG_ID, Optional.of(USER_ID)); + + // Verify it's in the cache + assertNotNull(ownerRel.getIfPresent(CATALOG_ID)); + + // Create a mock NameIdentifier for the metadata object + NameIdentifier catalogIdent = NameIdentifierUtil.ofCatalog(METALAKE, "testCatalog"); + + // Call handleMetadataOwnerChange which should invalidate the cache entry + jcasbinAuthorizer.handleMetadataOwnerChange( + METALAKE, USER_ID, catalogIdent, Entity.EntityType.CATALOG); + + // Verify it's removed from the cache + assertNull(ownerRel.getIfPresent(CATALOG_ID)); + } + @Test public void testRoleCacheSynchronousRemovalListenerDeletesPolicy() throws Exception { makeCompletableFutureUseCurrentThread(jcasbinAuthorizer); @@ -406,8 +440,10 @@ public class TestJcasbinAuthorizer { public void testCacheInitialization() throws Exception { // Verify that caches are initialized Cache<Long, Boolean> loadedRoles = getLoadedRolesCache(jcasbinAuthorizer); + Cache<Long, Optional<Long>> ownerRel = getOwnerRelCache(jcasbinAuthorizer); assertNotNull(loadedRoles, "loadedRoles cache should be initialized"); + assertNotNull(ownerRel, "ownerRel cache should be initialized"); } @SuppressWarnings("unchecked") @@ -418,6 +454,14 @@ public class TestJcasbinAuthorizer { return (Cache<Long, Boolean>) field.get(authorizer); } + @SuppressWarnings("unchecked") + private static Cache<Long, Optional<Long>> getOwnerRelCache(JcasbinAuthorizer authorizer) + throws Exception { + Field field = JcasbinAuthorizer.class.getDeclaredField("ownerRel"); + field.setAccessible(true); + return (Cache<Long, Optional<Long>>) field.get(authorizer); + } + private static Enforcer getAllowEnforcer(JcasbinAuthorizer authorizer) throws Exception { Field field = JcasbinAuthorizer.class.getDeclaredField("allowEnforcer"); field.setAccessible(true); diff --git a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java index 7a7cbad24a..a96160da84 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java +++ b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java @@ -337,6 +337,10 @@ public class TestGravitinoInterceptionService { @Override public void handleRolePrivilegeChange(Long roleId) {} + @Override + public void handleMetadataOwnerChange( + String metalake, Long oldOwnerId, NameIdentifier nameIdentifier, Entity.EntityType type) {} + @Override public void close() throws IOException {} }
