This is an automated email from the ASF dual-hosted git repository. yuqi1129 pushed a commit to branch feat/jcasbin-policy-index in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 9f8342fb276b7f610027a2459ebcee40c9b03e08 Author: yuqi <[email protected]> AuthorDate: Tue Apr 28 12:05:46 2026 +0800 fix --- .../authorization/AuthorizationRequestContext.java | 18 +++ .../authorization/jcasbin/JcasbinAuthorizer.java | 163 +++++++++++++++++---- .../jcasbin/TestJcasbinAuthorizer.java | 14 +- 3 files changed, 162 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java index 392eb13c27..15e3437940 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java @@ -18,11 +18,14 @@ package org.apache.gravitino.authorization; import java.security.Principal; +import java.util.Collections; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; +import javax.annotation.Nullable; import org.apache.gravitino.MetadataObject; public class AuthorizationRequestContext { @@ -36,6 +39,13 @@ public class AuthorizationRequestContext { /** Used to determine whether the role has already been loaded. */ private final AtomicBoolean hasLoadRole = new AtomicBoolean(); + /** + * Role IDs of the current principal in the metalake under check. Populated once per request from + * {@code loadRole} and reused by every {@code authorize}/{@code deny} call so we don't re-derive + * the user→role linkage on each enforcer probe. + */ + private volatile Set<Long> userRoleIds = Collections.emptySet(); + private volatile String originalAuthorizationExpression; /** @@ -103,6 +113,14 @@ public class AuthorizationRequestContext { this.originalAuthorizationExpression = originalAuthorizationExpression; } + public Set<Long> getUserRoleIds() { + return userRoleIds; + } + + public void setUserRoleIds(@Nullable Set<Long> userRoleIds) { + this.userRoleIds = userRoleIds == null ? Collections.emptySet() : userRoleIds; + } + public static class AuthorizationKey { private Principal principal; private String metalake; 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 7630f71ae3..5c26f53de1 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 @@ -27,10 +27,15 @@ import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.ThreadPoolExecutor; @@ -89,6 +94,16 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { private Cache<Long, Optional<Long>> ownerRel; + /** + * Per-role policy index that mirrors what we hand to the JCasbin enforcers. Lookups in the hot + * path go through this map so an authorize call costs O(roles_per_user) hash probes instead of + * scanning every policy line in the enforcer. Lifecycle is tied to {@link #loadedRoles}: entries + * are written before {@code loadedRoles.put} and removed from {@code loadedRoles}'s removal + * listener so the two caches can never disagree on what a role grants. + */ + private final ConcurrentHashMap<Long, Map<PolicyKey, Effect>> roleIndex = + new ConcurrentHashMap<>(); + private Executor executor = null; @Override @@ -104,9 +119,9 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { // Initialize enforcers before the caches that reference them in removal listeners allowEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new GravitinoAdapter()); - allowInternalAuthorizer = new InternalAuthorizer(allowEnforcer); + allowInternalAuthorizer = new InternalAuthorizer(allowEnforcer, false); denyEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new GravitinoAdapter()); - denyInternalAuthorizer = new InternalAuthorizer(denyEnforcer); + denyInternalAuthorizer = new InternalAuthorizer(denyEnforcer, true); loadedRoles = Caffeine.newBuilder() @@ -118,6 +133,7 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { if (roleId != null) { allowEnforcer.deleteRole(String.valueOf(roleId)); denyEnforcer.deleteRole(String.valueOf(roleId)); + roleIndex.remove(roleId); } }) .build(); @@ -424,10 +440,12 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { private class InternalAuthorizer { - Enforcer enforcer; + private final Enforcer enforcer; + private final boolean denyMode; - public InternalAuthorizer(Enforcer enforcer) { + InternalAuthorizer(Enforcer enforcer, boolean denyMode) { this.enforcer = enforcer; + this.denyMode = denyMode; } private boolean authorizeInternal( @@ -457,20 +475,59 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { return false; } loadRolePrivilege(metalake, username, userId, requestContext); - return authorizeByJcasbin(userId, metadataObject, metadataId, privilege); + return authorizeByIndex(userId, metadataObject, metadataId, privilege, requestContext); } - private boolean authorizeByJcasbin( - Long userId, MetadataObject metadataObject, Long metadataId, String privilege) { + /** + * Resolve a single privilege probe against the per-role policy index. Replaces the previous + * {@code enforcer.enforce} call, which scanned every policy line in the enforcer for each + * probe. Per-request cost goes from {@code O(total_policies)} to {@code O(roles_per_user)} hash + * probes. + */ + private boolean authorizeByIndex( + Long userId, + MetadataObject metadataObject, + Long metadataId, + String privilege, + AuthorizationRequestContext requestContext) { 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()), - String.valueOf(metadataId), - privilege); + Set<Long> roleIds = requestContext.getUserRoleIds(); + if (roleIds.isEmpty()) { + return false; + } + PolicyKey key = new PolicyKey(metadataObject.type().name(), metadataId, privilege); + if (denyMode) { + for (Long roleId : roleIds) { + Map<PolicyKey, Effect> idx = roleIndex.get(roleId); + if (idx != null && idx.get(key) == Effect.DENY) { + return true; + } + } + return false; + } + boolean allow = false; + for (Long roleId : roleIds) { + Map<PolicyKey, Effect> idx = roleIndex.get(roleId); + if (idx == null) { + continue; + } + Effect effect = idx.get(key); + if (effect == Effect.DENY) { + return false; + } + if (effect == Effect.ALLOW) { + allow = true; + } + } + return allow; + } + + /** Retained so reflection-based tests that touch the underlying enforcer keep working. */ + Enforcer getEnforcer() { + return enforcer; } } @@ -499,9 +556,11 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { SupportsRelationOperations.Type.ROLE_USER_REL, userNameIdentifier, Entity.EntityType.USER); + Set<Long> roleIds = new HashSet<>(entities.size()); List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>(); for (RoleEntity role : entities) { Long roleId = role.id(); + roleIds.add(roleId); allowEnforcer.addRoleForUser(String.valueOf(userId), String.valueOf(roleId)); denyEnforcer.addRoleForUser(String.valueOf(userId), String.valueOf(roleId)); if (loadedRoles.getIfPresent(roleId) != null) { @@ -529,6 +588,7 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { loadRoleFutures.add(loadRoleFuture); } CompletableFuture.allOf(loadRoleFutures.toArray(new CompletableFuture[0])).join(); + requestContext.setUserRoleIds(roleIds); } catch (IOException e) { throw new RuntimeException(e); } @@ -568,19 +628,24 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { private void loadPolicyByRoleEntity(RoleEntity roleEntity) { String metalake = NameIdentifierUtil.getMetalake(roleEntity.nameIdentifier()); List<SecurableObject> securableObjects = roleEntity.securableObjects(); + Long roleId = roleEntity.id(); + String roleIdStr = String.valueOf(roleId); + Map<PolicyKey, Effect> index = new ConcurrentHashMap<>(); for (SecurableObject securableObject : securableObjects) { + Long metadataId = MetadataIdConverter.getID(securableObject, metalake); + String metadataIdStr = String.valueOf(metadataId); + String typeName = securableObject.type().name(); for (Privilege privilege : securableObject.privileges()) { Privilege.Condition condition = privilege.condition(); - if (AuthConstants.DENY.equalsIgnoreCase(condition.name())) { + String privilegeName = + AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name()) + .name() + .toUpperCase(Locale.ROOT); + boolean isDeny = AuthConstants.DENY.equalsIgnoreCase(condition.name()); + if (isDeny) { denyEnforcer.addPolicy( - String.valueOf(roleEntity.id()), - securableObject.type().name(), - String.valueOf(MetadataIdConverter.getID(securableObject, metalake)), - AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name()) - .name() - .toUpperCase(java.util.Locale.ROOT), - AuthConstants.ALLOW); + roleIdStr, typeName, metadataIdStr, privilegeName, AuthConstants.ALLOW); } // Since different roles of a user may simultaneously hold both "allow" and "deny" // permissions @@ -591,14 +656,58 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { // roles should receive a false result when calling the authorize method. allowEnforcer.addPolicy( - String.valueOf(roleEntity.id()), - securableObject.type().name(), - String.valueOf(MetadataIdConverter.getID(securableObject, metalake)), - AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name()) - .name() - .toUpperCase(java.util.Locale.ROOT), - condition.name().toLowerCase(java.util.Locale.ROOT)); + roleIdStr, + typeName, + metadataIdStr, + privilegeName, + condition.name().toLowerCase(Locale.ROOT)); + + // Populate the per-role index. Within a single role DENY wins over ALLOW so that the + // index agrees with the allowEnforcer's policy_effect (some allow && !some deny). + PolicyKey key = new PolicyKey(typeName, metadataId, privilegeName); + Effect effect = isDeny ? Effect.DENY : Effect.ALLOW; + index.merge( + key, effect, (existing, incoming) -> existing == Effect.DENY ? existing : incoming); + } + } + roleIndex.put(roleId, index); + } + + /** Composite key for the per-role policy index. */ + static final class PolicyKey { + private final String type; + private final Long metadataId; + private final String privilege; + private final int hash; + + PolicyKey(String type, Long metadataId, String privilege) { + this.type = type; + this.metadataId = metadataId; + this.privilege = privilege; + this.hash = Objects.hash(type, metadataId, privilege); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof PolicyKey)) { + return false; } + PolicyKey other = (PolicyKey) o; + return hash == other.hash + && Objects.equals(metadataId, other.metadataId) + && Objects.equals(type, other.type) + && Objects.equals(privilege, other.privilege); } + + @Override + public int hashCode() { + return hash; + } + } + + /** Per-role per-key effect; DENY beats ALLOW within a role and across roles. */ + enum Effect { + ALLOW, + DENY } } 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 7bafa045db..0ff02c9583 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 @@ -186,9 +186,10 @@ public class TestJcasbinAuthorizer { eq(Entity.EntityType.USER))) .thenReturn(ImmutableList.of(allowRole)); assertTrue(doAuthorize(currentPrincipal)); - // Test role cache. - // When permissions are changed but handleRolePrivilegeChange is not executed, the system will - // use the cached permissions in JCasbin, so authorize can succeed. + // After re-assigning the user from allowRole to a role with no privileges, authorize must + // return false even though allowRole's policies are still cached in the enforcer. Each + // request iterates the user's fresh role list, so removed role assignments take effect + // immediately without waiting for a handleRolePrivilegeChange call. Long newRoleId = -1L; RoleEntity tempNewRole = getRoleEntity(newRoleId, "tempNewRole", ImmutableList.of()); when(entityStore.get( @@ -201,10 +202,11 @@ public class TestJcasbinAuthorizer { eq(userNameIdentifier), eq(Entity.EntityType.USER))) .thenReturn(ImmutableList.of(tempNewRole)); - assertTrue(doAuthorize(currentPrincipal)); - // After clearing the cache, authorize will fail + assertFalse(doAuthorize(currentPrincipal)); + // Invalidating the role cache is still a no-op in this scenario; we left it in to exercise + // the invalidation path. jcasbinAuthorizer.handleRolePrivilegeChange(ALLOW_ROLE_ID); - // assertFalse(doAuthorize(currentPrincipal)); + assertFalse(doAuthorize(currentPrincipal)); // When the user is re-assigned the correct role, the authorization will succeed. when(supportsRelationOperations.listEntitiesByRelation( eq(SupportsRelationOperations.Type.ROLE_USER_REL),
