This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new e96a2fb902 [#13605] fix(authz): Reload role policies cleared between
checks of one authorization request (#13606)
e96a2fb902 is described below
commit e96a2fb9024106904a8700d3e6236f6077cf3a9f
Author: Qi Yu <[email protected]>
AuthorDate: Fri Oct 9 17:36:46 2026 +0800
[#13605] fix(authz): Reload role policies cleared between checks of one
authorization request (#13606)
### What changes were proposed in this pull request?
Repair role policies cleared between policy evaluations of one
authorization request:
- Record a per-role generation when eviction or invalidation removes
policies. An explicit privilege change also records a generation for an
empty role, so its first new DENY is reloaded. Atomic policy replacement
does not record an intermediate clear.
- Capture the generation before loading roles, then retain the bound
role ids and unreadable-role results on the request. Advance its
generation monotonically across concurrent list workers.
- Before an uncached allow, deny, narrowed check or `hasDenyPolicy`
scan, check the generation under the policy read lock. Reload outside
the lock when needed, then validate again.
- After three reload attempts, or a reload failure, fail closed: allow
is false; deny and `hasDenyPolicy` are true. Unreadable role entities
during the initial load keep all policy evaluations of that request
closed; a fresh request can recover.
- Bound the clear-generation map at the role cache size; a pruning
watermark prevents older requests from missing a pruned clear.
- Coordinate the concurrent eviction test so each request completes one
eviction round before the next starts.
Earlier authorization decisions remain request-cached. Shared authorizer
logic serves REST, list filtering, Iceberg and Lance without changing
their transports.
### Why are the changes needed?
Eviction or privilege invalidation can clear a role's policies while its
user-role binding remains. A later check in the same request can then
reject a granted privilege or miss a DENY. Reloading before later policy
evaluations repairs this gap.
Fix: #13605
### Does this PR introduce _any_ user-facing change?
No API or configuration fields change. Requests that race with policy
clearing reload the affected policies. When the reload budget is
exhausted or role entities cannot be read, checks fail closed and log a
WARN.
Partial policy resolution preserves its existing per-object behavior.
Reliable completeness tracking for `hasDenyPolicy`, including backoff
expiry and concurrent partial/complete replacements, is tracked
separately in #13691. Per-role single-flight loading is tracked in
#13612.
### How was this patch tested?
Tests cover mid-request ALLOW/DENY eviction, privilege changes including
an initially empty role, narrowed checks, initial-load eviction and read
failure, reload exceptions, dropped roles, reload-budget exhaustion for
all three entry points, pruning, monotonic generation updates, and 1,000
composite decisions racing one eviction round per request. Partial-load
behavior remains covered explicitly.
Validation on head `4899c6bd41` (Java 17, non-Docker unit tests):
- Full `core`: 2,543 passed, 2 existing disabled tests.
- Full `server-common`: 398 passed on the follow-up run; server
authorization: 103 passed.
- Scoped formatting checks passed. Independent mutation checks caught a
removed initial-read guard (1 regression) and disabled clear detection
(4 regressions); all 5 passed after restoring the head.
The first combined run had one failure in the unchanged TLS rejection
test: the client observed a connection reset instead of the expected SSL
handshake exception. The isolated TLS class passed on both base and head
(7 cases each), and the full server-common follow-up passed. The initial
failure remains recorded; it was not reproduced on the baseline.
Docker/integration tests and full Iceberg/Lance module suites were not
run.
---
.../authorization/AuthorizationRequestContext.java | 82 +++
.../TestAuthorizationRequestContext.java | 63 +++
.../authorization/jcasbin/JcasbinAuthorizer.java | 347 ++++++++++--
.../jcasbin/TestJcasbinAuthorizer.java | 622 +++++++++++++++++++++
4 files changed, 1057 insertions(+), 57 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 76c4b2fef7..e504c359c4 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
@@ -20,11 +20,17 @@
package org.apache.gravitino.authorization;
import java.security.Principal;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Function;
import lombok.AllArgsConstructor;
import lombok.EqualsAndHashCode;
@@ -89,6 +95,22 @@ public class AuthorizationRequestContext {
private volatile String originalAuthorizationExpression;
+ /**
+ * Ids of the roles bound to the caller when this request loaded role
policies. The authorizer
+ * uses them to detect whether one of these roles lost its policies after
the load.
+ */
+ private volatile List<Long> boundRoleIds = Collections.emptyList();
+
+ /**
+ * Authorizer-defined generation of the in-memory role policies this request
last validated its
+ * bound roles against. A role cleared after this generation must be
reloaded before the request
+ * evaluates it again.
+ */
+ private final AtomicLong rolePolicyGeneration = new AtomicLong();
+
+ /** Roles whose entities could not be read during this request's initial
role load. */
+ private volatile Set<Long> unreadableRoleIds = Collections.emptySet();
+
/**
* The roles the caller has declared active for this request (role
assumption). Read from the
* current {@link UserPrincipal}; defaults to {@link ActiveRoles#all()} (no
narrowing) when the
@@ -232,6 +254,66 @@ public class AuthorizationRequestContext {
this.prefetchedRoleVersions = prefetchedRoleVersions;
}
+ /**
+ * Returns the ids of the roles bound to the caller when this request loaded
role policies.
+ *
+ * @return the bound role ids; empty when no role has been loaded yet
+ */
+ public List<Long> getBoundRoleIds() {
+ return boundRoleIds;
+ }
+
+ /**
+ * Records the ids of the roles bound to the caller by this request's role
load.
+ *
+ * @param boundRoleIds the bound role ids; must not be {@code null}
+ */
+ public void setBoundRoleIds(List<Long> boundRoleIds) {
+ this.boundRoleIds =
+ Collections.unmodifiableList(
+ new ArrayList<>(Objects.requireNonNull(boundRoleIds, "boundRoleIds
must not be null")));
+ }
+
+ /**
+ * Returns the role policy generation this request last validated its bound
roles against.
+ *
+ * @return the role policy generation
+ */
+ public long getRolePolicyGeneration() {
+ return rolePolicyGeneration.get();
+ }
+
+ /**
+ * Advances the role policy generation this request validated its bound
roles against. Concurrent
+ * workers cannot move the recorded generation backwards.
+ *
+ * @param rolePolicyGeneration the role policy generation
+ */
+ public void setRolePolicyGeneration(long rolePolicyGeneration) {
+ this.rolePolicyGeneration.accumulateAndGet(rolePolicyGeneration,
Math::max);
+ }
+
+ /**
+ * Returns the roles whose entities could not be read during the initial
role load.
+ *
+ * @return the unreadable role ids; empty when all role entities were
readable
+ */
+ public Set<Long> getUnreadableRoleIds() {
+ return unreadableRoleIds;
+ }
+
+ /**
+ * Records unreadable roles during the initial load so every check of this
request fails closed.
+ *
+ * @param unreadableRoleIds the unreadable role ids; must not be {@code null}
+ */
+ public void setUnreadableRoleIds(Set<Long> unreadableRoleIds) {
+ this.unreadableRoleIds =
+ Collections.unmodifiableSet(
+ new HashSet<>(
+ Objects.requireNonNull(unreadableRoleIds, "unreadableRoleIds
must not be null")));
+ }
+
/**
* Returns the roles declared active for this request. Never {@code null};
defaults to {@link
* ActiveRoles#all()}.
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
index 313d490eac..755e10e97d 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
@@ -23,8 +23,12 @@ import static
org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
@@ -344,4 +348,63 @@ public class TestAuthorizationRequestContext {
PrincipalUtils.doAs(principal, () -> new
AuthorizationRequestContext().getActiveRoles());
assertEquals(named, seen);
}
+
+ @Test
+ public void testRolePolicyGenerationDoesNotMoveBackwardsAcrossWorkers()
throws Exception {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ CountDownLatch newerRecorded = new CountDownLatch(1);
+ Thread olderWorker =
+ new Thread(
+ () -> {
+ try {
+ if (newerRecorded.await(5, TimeUnit.SECONDS)) {
+ context.setRolePolicyGeneration(100L);
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ });
+ Thread newerWorker =
+ new Thread(
+ () -> {
+ context.setRolePolicyGeneration(105L);
+ newerRecorded.countDown();
+ });
+ olderWorker.start();
+ newerWorker.start();
+ olderWorker.join(5000);
+ newerWorker.join(5000);
+ assertFalse(olderWorker.isAlive());
+ assertFalse(newerWorker.isAlive());
+ assertEquals(105L, context.getRolePolicyGeneration());
+ }
+
+ @Test
+ public void testUnreadableRoleIdsAreCopied() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ assertTrue(context.getUnreadableRoleIds().isEmpty());
+ Set<Long> roleIds = new HashSet<>(Arrays.asList(1L, 2L));
+ context.setUnreadableRoleIds(roleIds);
+ roleIds.add(3L);
+ assertEquals(new HashSet<>(Arrays.asList(1L, 2L)),
context.getUnreadableRoleIds());
+ assertThrows(UnsupportedOperationException.class, () ->
context.getUnreadableRoleIds().add(4L));
+ assertThrows(NullPointerException.class, () ->
context.setUnreadableRoleIds(null));
+ }
+
+ @Test
+ public void testBoundRoleIdsAndRolePolicyGeneration() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ assertTrue(context.getBoundRoleIds().isEmpty());
+ assertEquals(0L, context.getRolePolicyGeneration());
+
+ List<Long> roleIds = new ArrayList<>(Arrays.asList(1L, 2L));
+ context.setBoundRoleIds(roleIds);
+ context.setRolePolicyGeneration(7L);
+ roleIds.add(3L);
+
+ assertEquals(Arrays.asList(1L, 2L), context.getBoundRoleIds());
+ assertThrows(UnsupportedOperationException.class, () ->
context.getBoundRoleIds().add(4L));
+ assertThrows(NullPointerException.class, () ->
context.setBoundRoleIds(null));
+ assertEquals(7L, context.getRolePolicyGeneration());
+ }
}
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 06f17d4366..9e67e07389 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
@@ -34,9 +34,13 @@ import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.function.BooleanSupplier;
import java.util.stream.Collectors;
+import javax.annotation.Nullable;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Configs;
@@ -57,6 +61,7 @@ import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.authorization.SecurableObject;
import org.apache.gravitino.cache.CaffeineGravitinoCache;
import org.apache.gravitino.cache.GravitinoCache;
+import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.meta.RoleEntity;
import org.apache.gravitino.server.authorization.MetadataIdConverter;
import org.apache.gravitino.storage.relational.SupportsEntityChangeLog;
@@ -111,7 +116,9 @@ import org.slf4j.LoggerFactory;
*
* <p>JCasbin enforcer state ({@link #allowEnforcer}/{@link #denyEnforcer}) is
kept in sync with
* {@link #loadedRoles} via the removal listener inside {@link
JcasbinLoadedRolesCache} — evicting a
- * role id also deletes that role's policies from both enforcers.
+ * role id also deletes that role's policies from both enforcers. A request
that loaded the role
+ * before such an eviction reloads it before its next check; see {@link
+ * #evaluateWithLoadedRolePolicies}.
*/
public class JcasbinAuthorizer implements GravitinoAuthorizer {
@@ -138,6 +145,13 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*/
private static final long PARTIAL_ROLE_LOAD_RETRY_MS = 10_000L;
+ /**
+ * How many times a single authorization check reloads the caller's roles
when their policies were
+ * cleared after the request loaded them. Beyond this the check fails
closed; see {@link
+ * #evaluateWithLoadedRolePolicies}.
+ */
+ private static final int MAX_ROLE_POLICY_RELOADS = 3;
+
/**
* Guards every access to the in-memory JCasbin enforcer state. Policy
reload mutates that state
* with a clear-then-add sequence, which must be atomic not only against
other writers but also
@@ -153,6 +167,43 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*/
private final ReentrantReadWriteLock rolePolicyLock = new
ReentrantReadWriteLock();
+ /**
+ * Source of role policy generations. Advanced under the write lock of
{@link #rolePolicyLock}
+ * whenever a role's policies are cleared without being replaced in the same
critical section.
+ */
+ private final AtomicLong rolePolicyGenerationCounter = new AtomicLong();
+
+ /**
+ * roleId -> generation of the most recent clear of that role's policies. A
request records the
+ * generation before it loads its roles; a bound role whose clear generation
is newer has lost
+ * policies the request relies on and must be reloaded before the request
evaluates it again. The
+ * map is bounded by {@link #maxRoleClearGenerations}; see {@link
#prunedRoleClearGeneration} for
+ * how pruned entries stay safe.
+ */
+ private final Map<Long, Long> roleClearGenerations = new
ConcurrentHashMap<>();
+
+ /**
+ * Upper bound on {@link #roleClearGenerations} entries, set once to the
role cache size in {@link
+ * #initialize()} before use, and read under the write lock of {@link
#rolePolicyLock}.
+ */
+ private long maxRoleClearGenerations;
+
+ /**
+ * The newest generation removed from {@link #roleClearGenerations} by
pruning. A request whose
+ * recorded generation is older can no longer prove that none of its roles
was cleared, so it
+ * reloads its roles once before the next check. Written under the write
lock of {@link
+ * #rolePolicyLock} and only ever increases.
+ */
+ private volatile long prunedRoleClearGeneration;
+
+ /**
+ * The role whose marker {@link #replaceRolePolicies} is removing, or {@code
null}. Guarded by the
+ * write lock of {@link #rolePolicyLock}. The synchronous removal callback
clears that role's old
+ * policies, but the replacement is installed in the same critical section,
so no request can
+ * observe the role without policies and the clear is not recorded as a
generation.
+ */
+ @Nullable private Long roleBeingReplaced;
+
/** Jcasbin enforcer is used for metadata authorization. */
private Enforcer allowEnforcer;
@@ -226,12 +277,13 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
.get(Configs.GRAVITINO_AUTHORIZATION_CHANGE_POLL_INTERVAL_SECS);
long ttlMs = TimeUnit.SECONDS.toMillis(cacheExpirationSecs);
+ maxRoleClearGenerations = roleCacheSize;
// Initialize enforcers before caches that reference them in removal
listeners
allowEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new
GravitinoAdapter());
- allowInternalAuthorizer = new InternalAuthorizer(allowEnforcer, true);
+ allowInternalAuthorizer = new InternalAuthorizer(allowEnforcer, true,
false);
denyEnforcer = new SyncedEnforcer(getModel("/jcasbin_model.conf"), new
GravitinoAdapter());
- denyInternalAuthorizer = new InternalAuthorizer(denyEnforcer, false);
+ denyInternalAuthorizer = new InternalAuthorizer(denyEnforcer, false, true);
// loadedRoles: roleId -> updated_at.
// When evicted, we must clean up the corresponding JCasbin policies.
@@ -432,35 +484,39 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
Set<String> privilegeNames =
privileges.stream().map(Enum::name).collect(Collectors.toSet());
String userIdStr = String.valueOf(userId);
- rolePolicyLock.readLock().lock();
- try {
- // This is an existence query, not a per-object check: it answers "does
any deny on these
- // privileges exist for the user's roles, at any scope?" The standard
enforce path needs a
- // concrete metadataId, so reusing it would mean iterating every listed
object and defeat the
- // short-circuit. Filtering the deny enforcer's policies by role keeps
the scan bounded by the
- // user's role/policy count, never by the number of listed objects. The
match is intentionally
- // scope-agnostic (no metadataType filter): a parent-scope deny hides
the whole subtree and an
- // object-scope deny hides one object, and both must disable the
short-circuit.
- for (String roleId : denyEnforcer.getRolesForUser(userIdStr)) {
- // getFilteredNamedPolicy returns every "p" row (p = sub,
metadataType, metadataId, act,
- // eft)
- // whose field at POLICY_SUBJECT_FIELD_INDEX (sub) equals roleId, i.e.
all rules carried by
- // this role. denyEnforcer is a dedicated enforcer that is only ever
loaded with privileges
- // whose condition is DENY (see loadPolicyByRoleEntity), so every row
here represents a deny
- // regardless of its stored eft string. Each returned row is the list
of those five fields,
- // so we read field POLICY_ACTION_FIELD_INDEX (act) to compare the
denied privilege.
- for (List<String> policy :
- denyEnforcer.getFilteredNamedPolicy("p",
POLICY_SUBJECT_FIELD_INDEX, roleId)) {
- if (policy.size() > POLICY_ACTION_FIELD_INDEX
- &&
privilegeNames.contains(policy.get(POLICY_ACTION_FIELD_INDEX))) {
- return true;
- }
- }
- }
- return false;
- } finally {
- rolePolicyLock.readLock().unlock();
- }
+ // A missing answer means the user's roles kept losing their policies;
report a deny so the
+ // caller falls back to per-object checks instead of short-circuiting on
incomplete policies.
+ return evaluateWithLoadedRolePolicies(
+ metalake,
+ requestContext,
+ () -> {
+ // This is an existence query, not a per-object check: it
answers "does any deny on
+ // these privileges exist for the user's roles, at any scope?"
The standard enforce
+ // path needs a concrete metadataId, so reusing it would mean
iterating every listed
+ // object and defeat the short-circuit. Filtering the deny
enforcer's policies by role
+ // keeps the scan bounded by the user's role/policy count, never
by the number of
+ // listed objects. The match is intentionally scope-agnostic (no
metadataType filter):
+ // a parent-scope deny hides the whole subtree and an
object-scope deny hides one
+ // object, and both must disable the short-circuit.
+ for (String roleId : denyEnforcer.getRolesForUser(userIdStr)) {
+ // getFilteredNamedPolicy returns every "p" row (p = sub,
metadataType, metadataId,
+ // act, eft) whose field at POLICY_SUBJECT_FIELD_INDEX (sub)
equals roleId, i.e. all
+ // rules carried by this role. denyEnforcer is a dedicated
enforcer that is only
+ // ever loaded with privileges whose condition is DENY (see
resolveRolePolicies), so
+ // every row here represents a deny regardless of its stored
eft string. Each
+ // returned row is the list of those five fields, so we read
field
+ // POLICY_ACTION_FIELD_INDEX (act) to compare the denied
privilege.
+ for (List<String> policy :
+ denyEnforcer.getFilteredNamedPolicy("p",
POLICY_SUBJECT_FIELD_INDEX, roleId)) {
+ if (policy.size() > POLICY_ACTION_FIELD_INDEX
+ &&
privilegeNames.contains(policy.get(POLICY_ACTION_FIELD_INDEX))) {
+ return true;
+ }
+ }
+ }
+ return false;
+ })
+ .orElse(true);
}
@Override
@@ -707,7 +763,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
@Override
public void handleRolePrivilegeChange(Long roleId) {
- invalidateRolePolicies(roleId);
+ invalidateRolePolicies(roleId, true);
}
@Override
@@ -814,9 +870,18 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*/
private final boolean narrowByActiveRoles;
- public InternalAuthorizer(Enforcer enforcer, boolean narrowByActiveRoles) {
+ /**
+ * The fail-closed result returned when the caller's role policies cannot
be kept loaded for the
+ * duration of a check: {@code false} for the allow authorizer and {@code
true} for the deny
+ * authorizer, so an unstable policy state can only reject a request.
+ */
+ private final boolean unstablePolicyResult;
+
+ public InternalAuthorizer(
+ Enforcer enforcer, boolean narrowByActiveRoles, boolean
unstablePolicyResult) {
this.enforcer = enforcer;
this.narrowByActiveRoles = narrowByActiveRoles;
+ this.unstablePolicyResult = unstablePolicyResult;
}
private boolean authorizeInternal(
@@ -936,20 +1001,24 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// against just those roles (enforceNarrowed). ALL or an absent header
falls through to the
// normal check over every role the caller holds.
ActiveRoles activeRoles = requestContext.getActiveRoles();
- boolean allowed;
- rolePolicyLock.readLock().lock();
- try {
- if (narrowByActiveRoles && !activeRoles.isAll()) {
- allowed =
- enforceNarrowed(
- userId, metadataType, metadataIdStr, privilege, activeRoles,
requestContext);
- } else {
- allowed =
- enforcer.enforce(String.valueOf(userId), metadataType,
metadataIdStr, privilege);
- }
- } finally {
- rolePolicyLock.readLock().unlock();
- }
+ boolean allowed =
+ evaluateWithLoadedRolePolicies(
+ metalake,
+ requestContext,
+ () -> {
+ if (narrowByActiveRoles && !activeRoles.isAll()) {
+ return enforceNarrowed(
+ userId,
+ metadataType,
+ metadataIdStr,
+ privilege,
+ activeRoles,
+ requestContext);
+ }
+ return enforcer.enforce(
+ String.valueOf(userId), metadataType, metadataIdStr,
privilege);
+ })
+ .orElse(unstablePolicyResult);
if (narrowByActiveRoles && !allowed) {
diagnoseDenial(userId, metadataType, metadataIdStr, privilege);
@@ -1190,6 +1259,10 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
+ // Read the generation before any role is loaded, so a clear that
races with this load is
+ // newer than the recorded generation and is caught by the first
check that follows.
+ long rolePolicyGeneration = rolePolicyGenerationCounter.get();
+
// Step 1a: version-validated user-direct roles via cache.
List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
@@ -1242,8 +1315,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
allRoleIds.addAll(groupInheritedRoleIds);
if (!allRoleIds.isEmpty()) {
- versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
+ requestContext.setUnreadableRoleIds(
+ versionCheckAndLoadRoles(metalake, allRoleIds,
requestContext));
}
+ requestContext.setBoundRoleIds(allRoleIds);
+ requestContext.setRolePolicyGeneration(rolePolicyGeneration);
});
}
@@ -1345,7 +1421,13 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
return
groups.stream().map(UserGroup::getGroupName).collect(Collectors.toList());
}
- private void versionCheckAndLoadRoles(
+ /**
+ * Reloads every role in {@code roleIds} whose loaded version is missing or
older than the DB.
+ *
+ * @return ids of stale roles whose entity could not be read, so their
policies were not reloaded.
+ * A role that no longer exists is not included: having no policies is
correct for it.
+ */
+ private Set<Long> versionCheckAndLoadRoles(
String metalake, List<Long> roleIds, AuthorizationRequestContext
requestContext) {
List<Long> uniqueRoleIds =
roleIds.stream().distinct().collect(Collectors.toList());
@@ -1396,9 +1478,10 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
if (staleRoleVersions.isEmpty()) {
- return;
+ return Collections.emptySet();
}
+ Set<Long> unreadableRoleIds = new HashSet<>();
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
List<NameIdentifier> roleIdents =
staleRoleVersions.stream()
@@ -1425,8 +1508,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
NameIdentifierUtil.ofRole(metalake, rv.getRoleName()),
Entity.EntityType.ROLE,
RoleEntity.class));
+ } catch (NoSuchEntityException e) {
+ LOG.debug("Role {} was dropped before its policies could be loaded",
rv.getRoleId(), e);
} catch (Exception e) {
LOG.warn("Failed to load role policies for roleId {}",
rv.getRoleId(), e);
+ unreadableRoleIds.add(rv.getRoleId());
}
}
}
@@ -1467,6 +1553,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
PARTIAL_ROLE_LOAD_RETRY_MS);
}
}
+ return unreadableRoleIds;
}
/**
@@ -1495,7 +1582,12 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// bound grouping links. If a marker exists, its synchronous removal
callback clears the old
// policies. With no marker, rows can still remain from a previous
partial load and must be
// cleared explicitly.
- loadedRoles.invalidate(roleId);
+ roleBeingReplaced = roleId;
+ try {
+ loadedRoles.invalidate(roleId);
+ } finally {
+ roleBeingReplaced = null;
+ }
if (!latestLoadedAt.isPresent()) {
clearRolePoliciesWithoutLock(roleId);
}
@@ -1519,8 +1611,13 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
/** Clears a role's policies and removes its loaded marker as one serialized
operation. */
private void invalidateRolePolicies(long roleId) {
+ invalidateRolePolicies(roleId, false);
+ }
+
+ private void invalidateRolePolicies(long roleId, boolean
recordEmptyRoleChange) {
rolePolicyLock.writeLock().lock();
try {
+ Long previousClearGeneration = roleClearGenerations.get(roleId);
// An explicit invalidation is stronger than the retry throttle. Clear
it under the same lock
// before removing the loaded marker so the removal callback cannot
mistake the old partial
// state for a newer load and skip policy cleanup.
@@ -1529,7 +1626,13 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
loadedRoles.invalidate(roleId);
if (!markerPresent) {
// An incomplete load has policies but deliberately has no loadedRoles
marker.
- clearRolePoliciesWithoutLock(roleId);
+ clearRolePoliciesAndRecordGeneration(roleId);
+ }
+ // A privilege change can grant a previously empty role its first deny
policy. Such a role
+ // has no p-row to remove, but in-flight requests must still reload
before their next check.
+ if (recordEmptyRoleChange
+ && Objects.equals(previousClearGeneration,
roleClearGenerations.get(roleId))) {
+ recordRoleClearGeneration(roleId);
}
} finally {
rolePolicyLock.writeLock().unlock();
@@ -1554,16 +1657,146 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
roleId);
return;
}
- clearRolePoliciesWithoutLock(roleId);
+ if (Objects.equals(roleBeingReplaced, roleId)) {
+ clearRolePoliciesWithoutLock(roleId);
+ } else {
+ clearRolePoliciesAndRecordGeneration(roleId);
+ }
} finally {
rolePolicyLock.writeLock().unlock();
}
}
- private void clearRolePoliciesWithoutLock(long roleId) {
+ private boolean clearRolePoliciesWithoutLock(long roleId) {
String roleIdStr = String.valueOf(roleId);
- allowEnforcer.removeFilteredPolicy(0, roleIdStr);
- denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean allowRemoved = allowEnforcer.removeFilteredPolicy(0, roleIdStr);
+ boolean denyRemoved = denyEnforcer.removeFilteredPolicy(0, roleIdStr);
+ return allowRemoved || denyRemoved;
+ }
+
+ /**
+ * Clears a role's policies that are not replaced in the same critical
section, and records the
+ * clear so that in-flight requests bound to the role reload it before their
next check. Only a
+ * clear that removed policies is recorded: clearing an already empty role
changes nothing a
+ * request could have observed, and recording it would make a deleted role
look unstable forever.
+ * Must be called under the write lock of {@link #rolePolicyLock}.
+ */
+ private void clearRolePoliciesAndRecordGeneration(long roleId) {
+ if (clearRolePoliciesWithoutLock(roleId)) {
+ recordRoleClearGeneration(roleId);
+ }
+ }
+
+ /**
+ * Records a new clear generation for a role, pruning the older half of the
generations when the
+ * map outgrows {@link #maxRoleClearGenerations}. Must be called under the
write lock of {@link
+ * #rolePolicyLock}, so that readers see the pruned entries and the raised
{@link
+ * #prunedRoleClearGeneration} together.
+ */
+ private void recordRoleClearGeneration(long roleId) {
+ roleClearGenerations.put(roleId,
rolePolicyGenerationCounter.incrementAndGet());
+ if (roleClearGenerations.size() <= maxRoleClearGenerations) {
+ return;
+ }
+ long[] generations =
+
roleClearGenerations.values().stream().mapToLong(Long::longValue).sorted().toArray();
+ long cutoff = generations[generations.length / 2];
+ roleClearGenerations.values().removeIf(generation -> generation <= cutoff);
+ prunedRoleClearGeneration = cutoff;
+ }
+
+ /**
+ * Evaluates one authorization check after repairing role policies cleared
since this request
+ * loaded its roles.
+ *
+ * <p>A request loads its roles once, but each check releases {@link
#rolePolicyLock} before the
+ * next one starts. In between, a {@link #loadedRoles} TTL or size eviction,
or a privilege change
+ * on one of the roles, may clear that role's policies while the user's
grouping row stays. A
+ * later check would then evaluate without the role: it can deny what the
role allows, or allow
+ * what the role denies. So before each evaluation this method checks, under
the read lock,
+ * whether a bound role was cleared after the request's recorded generation.
If so, it reloads the
+ * cleared roles outside the lock, the same way a request's role load does,
and checks again.
+ *
+ * <p>Earlier decisions keep their existing request-scoped results. Each new
policy evaluation
+ * runs inside one read-locked section without holding the lock across
metadata or DB lookups.
+ * Partial role-policy resolution keeps its existing per-object behavior;
reliable completeness
+ * tracking for the deny-existence scan is handled separately in issue
#13691.
+ *
+ * @return the evaluation result, or empty when the request's roles kept
being cleared or could
+ * not be reloaded; the caller must then fail closed
+ */
+ private Optional<Boolean> evaluateWithLoadedRolePolicies(
+ String metalake, AuthorizationRequestContext requestContext,
BooleanSupplier evaluation) {
+ if (!requestContext.getUnreadableRoleIds().isEmpty()) {
+ LOG.warn(
+ "Failed to read roles {} during the initial role load; failing
closed",
+ requestContext.getUnreadableRoleIds());
+ return Optional.empty();
+ }
+ for (int reloads = 0; ; reloads++) {
+ rolePolicyLock.readLock().lock();
+ try {
+ if (!hasClearedBoundRole(requestContext)) {
+ return Optional.of(evaluation.getAsBoolean());
+ }
+ } finally {
+ rolePolicyLock.readLock().unlock();
+ }
+
+ if (reloads == MAX_ROLE_POLICY_RELOADS) {
+ LOG.warn(
+ "Policies of roles {} were cleared again after {} reloads within
one authorization "
+ + "check; failing closed. Check for frequent privilege changes
on these roles or a "
+ + "loaded role cache that is too small on this node.",
+ requestContext.getBoundRoleIds(),
+ MAX_ROLE_POLICY_RELOADS);
+ return Optional.empty();
+ }
+
+ long rolePolicyGeneration = rolePolicyGenerationCounter.get();
+ Set<Long> unreadableRoleIds;
+ try {
+ unreadableRoleIds =
+ versionCheckAndLoadRoles(metalake,
requestContext.getBoundRoleIds(), requestContext);
+ } catch (RuntimeException e) {
+ LOG.warn(
+ "Failed to reload cleared role policies for roles {}; failing
closed",
+ requestContext.getBoundRoleIds(),
+ e);
+ return Optional.empty();
+ }
+ // A role whose entity could not be read has lost its policies without a
replacement. Its
+ // missing rows prove neither the absence of a deny nor of an allow, so
the check cannot be
+ // evaluated. Partially loaded and dropped roles are evaluated as usual:
their policy state is
+ // the same one any request would see.
+ if (!unreadableRoleIds.isEmpty()) {
+ LOG.warn(
+ "Failed to read roles {} to reload their cleared policies; failing
closed",
+ unreadableRoleIds);
+ return Optional.empty();
+ }
+ requestContext.setRolePolicyGeneration(rolePolicyGeneration);
+ }
+ }
+
+ /**
+ * Returns whether a role bound by this request was cleared after the
request's recorded
+ * generation. Must be called under {@link #rolePolicyLock}.
+ */
+ private boolean hasClearedBoundRole(AuthorizationRequestContext
requestContext) {
+ long rolePolicyGeneration = requestContext.getRolePolicyGeneration();
+ List<Long> boundRoleIds = requestContext.getBoundRoleIds();
+ if (!boundRoleIds.isEmpty() && rolePolicyGeneration <
prunedRoleClearGeneration) {
+ // A clear of one of these roles may have been pruned from
roleClearGenerations.
+ return true;
+ }
+ for (Long roleId : boundRoleIds) {
+ Long clearedAt = roleClearGenerations.get(roleId);
+ if (clearedAt != null && clearedAt > rolePolicyGeneration) {
+ return true;
+ }
+ }
+ return false;
}
private void bindUserRoles(long userId, List<Long> roleIds) {
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 e2570a348c..3bd54a64d6 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
@@ -60,9 +60,12 @@ import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.LockSupport;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Function;
import java.util.stream.Collectors;
@@ -88,6 +91,7 @@ import org.apache.gravitino.catalog.SemanticModelDispatcher;
import org.apache.gravitino.connector.BaseCatalog;
import org.apache.gravitino.connector.capability.Capability;
import org.apache.gravitino.connector.capability.CapabilityResult;
+import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.hook.SemanticModelHookDispatcher;
import org.apache.gravitino.meta.AuditInfo;
import org.apache.gravitino.meta.BaseMetalake;
@@ -402,6 +406,8 @@ public class TestJcasbinAuthorizer {
@Test
public void testIsMetalakeUserUsesUserInfoCache() {
+ // userMetaMapper is shared by every test, so only count the calls made by
this one.
+ Mockito.clearInvocations(userMetaMapper);
assertTrue(jcasbinAuthorizer.isMetalakeUser(METALAKE, new
AuthorizationRequestContext()));
verify(userMetaMapper).getUserUpdatedAt(METALAKE, USERNAME);
}
@@ -937,6 +943,622 @@ public class TestJcasbinAuthorizer {
});
}
+ @Test
+ public void testAllowPoliciesEvictedMidRequestAreReloadedBeforeLaterCheck()
throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ // The first check of a composite expression loads the role but does not
match its scope.
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ // TTL expiry or size eviction runs the removal listener, which clears the
role's p-rows but
+ // keeps the user's g-row. The request will not run its one-time role load
again.
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
+ assertTrue(
+ getAllowEnforcer(jcasbinAuthorizer)
+ .getFilteredPolicy(0, String.valueOf(ALLOW_ROLE_ID))
+ .isEmpty());
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "a later check in the same request must see the evicted role's
policies again");
+ }
+
+ @Test
+ public void testRolePrivilegeChangeMidRequestDoesNotDenyLaterCheck() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ // A grant or revoke on the role elsewhere on this node clears its
policies immediately.
+ jcasbinAuthorizer.handleRolePrivilegeChange(ALLOW_ROLE_ID);
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ }
+
+ @Test
+ public void testDenyPoliciesEvictedMidRequestStillDeny() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ mockDirectUserRoles(allowRole, denyRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ // Evicting the deny role between two checks must not let the allow role
win: that would turn a
+ // cache eviction into privilege escalation.
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(DENY_ROLE_ID);
+
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "the deny check must still see the evicted deny role");
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "the allow check must still honor the evicted deny role");
+ }
+
+ @Test
+ public void testReloadReadFailureMustNotLoseDeny() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ String roleName = "unavailableDenyRole";
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, roleName,
ImmutableList.of(getDenySecurableObject()));
+ mockDirectUserRoles(denyRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(DENY_ROLE_ID);
+ NameIdentifier roleIdent = NameIdentifierUtil.ofRole(METALAKE, roleName);
+ when(entityStore.get(eq(roleIdent), eq(Entity.EntityType.ROLE),
eq(RoleEntity.class)))
+ .thenThrow(new IllegalStateException("Role store temporarily
unavailable"));
+
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "an unsuccessful reload must fail closed instead of treating missing
p-rows as no deny");
+ }
+
+ @Test
+ public void testInitialRoleReadFailureMustNotLoseDeny() throws Exception {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ String roleName = "unavailableInitialDenyRole";
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, roleName,
ImmutableList.of(getDenySecurableObject()));
+ mockDirectUserRoles(allowRole, denyRole);
+ when(entityStore.get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, roleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class)))
+ .thenThrow(new IllegalStateException("Role store temporarily
unavailable"));
+
+ // Each entry point must preserve the failure from its own initial load,
including when
+ // another readable role grants the privilege. Subsequent checks of that
request stay closed.
+ AuthorizationRequestContext allowContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ principal, METALAKE, catalogObject(), USE_CATALOG, allowContext));
+ assertTrue(
+ jcasbinAuthorizer.deny(principal, METALAKE, catalogObject(),
USE_CATALOG, allowContext));
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ principal, METALAKE, ImmutableSet.of(USE_CATALOG), allowContext));
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ principal, METALAKE, catalogObject(), USE_CATALOG, new
AuthorizationRequestContext()));
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ principal, METALAKE, ImmutableSet.of(USE_CATALOG), new
AuthorizationRequestContext()));
+
+ Mockito.doReturn(denyRole)
+ .when(entityStore)
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, roleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ AuthorizationRequestContext recoveredContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.hasDenyPolicy(
+ principal, METALAKE, ImmutableSet.of(USE_SCHEMA),
recoveredContext));
+ assertTrue(recoveredContext.getUnreadableRoleIds().isEmpty());
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ principal, METALAKE, ImmutableSet.of(USE_SCHEMA), allowContext),
+ "the failed request stays closed even after another request
successfully loads the role");
+ }
+
+ @Test
+ public void testReloadExceptionFailsClosed() throws Exception {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity role =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(role);
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(principal, METALAKE, metalakeObject(),
USE_CATALOG, context));
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
+ // Force the reload to probe versions rather than use the request's
fat-JOIN snapshot.
+ context.setPrefetchedRoleVersions(Collections.emptyMap());
+ when(roleMetaMapper.batchGetRoleUpdatedAt(any()))
+ .thenThrow(new IllegalStateException("Role version probe
unavailable"));
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(principal, METALAKE, catalogObject(),
USE_CATALOG, context));
+ assertTrue(jcasbinAuthorizer.deny(principal, METALAKE, catalogObject(),
USE_CATALOG, context));
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ principal, METALAKE, ImmutableSet.of(USE_CATALOG), context));
+ }
+
+ @Test
+ public void testNewDenyOnInitiallyEmptyRoleIsSeenMidRequest() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ Long roleId = 32L;
+ String roleName = "newDenyRole";
+ RoleEntity emptyRole = mockRoleInStore(roleId, roleName,
ImmutableList.of());
+ mockDirectUserRoles(emptyRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ mockRoleInStore(
+ roleId,
+ roleName,
+ ImmutableList.of(
+ buildSecurableObject(
+ roleId, MetadataObject.Type.CATALOG, "testCatalog",
USE_CATALOG, "DENY")));
+ jcasbinAuthorizer.handleRolePrivilegeChange(roleId);
+
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "a privilege change on an empty role must trigger a reload in the same
request");
+ }
+
+ @Test
+ public void testPartiallyLoadedRoleDoesNotFailClosedAfterEviction() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ // A role that still references a dropped catalog loads partially and
never gets a loaded
+ // marker. It must not turn every mid-request eviction of another role
into a denial.
+ long partialRoleId = 40L;
+ RoleEntity partialRole =
+ mockRoleInStore(
+ partialRoleId,
+ "partialRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ partialRoleId,
+ MetadataObject.Type.CATALOG,
+ "droppedCatalog",
+ USE_CATALOG,
+ "ALLOW")));
+ metadataIdConverterMockedStatic
+ .when(
+ () ->
+ MetadataIdConverter.getID(
+ Mockito.argThat(
+ object -> object != null &&
"droppedCatalog".equals(object.name())),
+ eq(METALAKE)))
+ .thenReturn(Optional.empty());
+ try {
+ mockDirectUserRoles(allowRole, partialRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
assertFalse(getLoadedRolesCache(jcasbinAuthorizer).getIfPresent(partialRoleId).isPresent());
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ } finally {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ }
+ }
+
+ @Test
+ public void testRoleClearGenerationsArePrunedWithoutMissingClears() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ String roleName = "prunedGenerationRole";
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, roleName,
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ Field maxField =
JcasbinAuthorizer.class.getDeclaredField("maxRoleClearGenerations");
+ maxField.setAccessible(true);
+ maxField.setLong(jcasbinAuthorizer, 4L);
+ Mockito.clearInvocations(entityStore);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ // Clear the request's role first, then enough other roles to prune its
generation away.
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
+ for (long roleId = 100L; roleId < 110L; roleId++) {
+ jcasbinAuthorizer.handleRolePrivilegeChange(roleId);
+ }
+ Field generationsField =
JcasbinAuthorizer.class.getDeclaredField("roleClearGenerations");
+ generationsField.setAccessible(true);
+ Map<?, ?> generations = (Map<?, ?>)
generationsField.get(jcasbinAuthorizer);
+ assertTrue(generations.size() <= 4, "the generation map must stay
bounded");
+ assertFalse(
+ generations.containsKey(ALLOW_ROLE_ID), "the role's own clear must
have been pruned");
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext),
+ "a pruned clear must still make the request reload the role");
+ verify(entityStore, Mockito.times(2))
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, roleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ }
+
+ @Test
+ public void testHasDenyPolicyReloadsEvictedDenyPolicies() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ mockDirectUserRoles(denyRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal, METALAKE, ImmutableSet.of(USE_CATALOG),
requestContext));
+
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(DENY_ROLE_ID);
+
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal, METALAKE, ImmutableSet.of(USE_CATALOG),
requestContext),
+ "an evicted deny role must not disable the list short-circuit guard");
+ }
+
+ @Test
+ public void testUnrelatedRoleEvictionDoesNotReloadRequestRoles() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(
+ ALLOW_ROLE_ID, "unrelatedEvictionRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ Mockito.clearInvocations(entityStore);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ // Evict a loaded role that this request does not hold.
+ long otherRoleId = 99L;
+ getAllowEnforcer(jcasbinAuthorizer)
+ .addPolicy(
+ String.valueOf(otherRoleId),
+ MetadataObject.Type.CATALOG.name(),
+ String.valueOf(CATALOG_ID),
+ USE_CATALOG.name(),
+ AuthConstants.ALLOW);
+ GravitinoCache<Long, Long> loadedRoles =
getLoadedRolesCache(jcasbinAuthorizer);
+ loadedRoles.put(otherRoleId, 1L);
+ loadedRoles.invalidate(otherRoleId);
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ verify(entityStore, Mockito.times(1))
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, "unrelatedEvictionRole")),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ }
+
+ @Test
+ public void testRepeatedEvictionsFailClosed() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ long otherRoleId = 31L;
+ String roleName = "pingRole";
+ String otherRoleName = "pongRole";
+ RoleEntity role =
+ getRoleEntity(ALLOW_ROLE_ID, roleName,
ImmutableList.of(getAllowSecurableObject()));
+ RoleEntity otherRole =
+ getRoleEntity(otherRoleId, otherRoleName,
ImmutableList.of(getAllowSecurableObject()));
+ mockedRoleVersions.put(
+ ALLOW_ROLE_ID, new RoleUpdatedAt(ALLOW_ROLE_ID, roleName,
nextRoleVersion()));
+ mockedRoleVersions.put(
+ otherRoleId, new RoleUpdatedAt(otherRoleId, otherRoleName,
nextRoleVersion()));
+ GravitinoCache<Long, Long> loadedRoles =
getLoadedRolesCache(jcasbinAuthorizer);
+ AtomicReference<Boolean> armed = new AtomicReference<>(false);
+ // Reloading either role evicts the other one, so the request's roles
never become stable.
+ doAnswer(
+ invocation -> {
+ if (armed.get()) {
+ loadedRoles.invalidate(otherRoleId);
+ }
+ return role;
+ })
+ .when(entityStore)
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, roleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ doAnswer(
+ invocation -> {
+ if (armed.get()) {
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ }
+ return otherRole;
+ })
+ .when(entityStore)
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, otherRoleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ mockDirectUserRoles(role, otherRole);
+
+ AuthorizationRequestContext allowContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
allowContext));
+ armed.set(true);
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
allowContext),
+ "an allow check must fail closed when the request's policies never
stabilize");
+
+ armed.set(false);
+ AuthorizationRequestContext denyContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
denyContext));
+ armed.set(true);
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
denyContext),
+ "a deny check must fail closed when the request's policies never
stabilize");
+ armed.set(false);
+ AuthorizationRequestContext scanContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal, METALAKE, ImmutableSet.of(USE_CATALOG),
scanContext));
+ armed.set(true);
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal, METALAKE, ImmutableSet.of(USE_CATALOG),
scanContext),
+ "a deny scan must fail closed when the request's policies never
stabilize");
+ armed.set(false);
+ }
+
+ @Test
+ public void testRoleDeletedMidRequestDeniesWithoutLooping() throws Exception
{
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ String roleName = "deletedMidRequestRole";
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, roleName,
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ Mockito.clearInvocations(entityStore);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+
+ NameIdentifier roleIdent = NameIdentifierUtil.ofRole(METALAKE, roleName);
+ when(entityStore.get(eq(roleIdent), eq(Entity.EntityType.ROLE),
eq(RoleEntity.class)))
+ .thenThrow(new NoSuchEntityException("Role %s is dropped", roleName));
+ Mockito.clearInvocations(entityStore);
+ jcasbinAuthorizer.handleRolePrivilegeChange(ALLOW_ROLE_ID);
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ verify(entityStore, Mockito.times(1))
+ .get(eq(roleIdent), eq(Entity.EntityType.ROLE), eq(RoleEntity.class));
+ }
+
+ @Test
+ public void testDenialByEmptyRoleDoesNotReloadIt() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity emptyRole = mockRoleInStore(32L, "emptyRole",
ImmutableList.of());
+ mockDirectUserRoles(emptyRole);
+ Mockito.clearInvocations(entityStore);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ assertFalse(
+ jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+
+ verify(entityStore, Mockito.times(1))
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, "emptyRole")),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ }
+
+ @Test
+ public void testEvictionDuringRoleLoadIsDetected() throws Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ assertTrue(doAuthorize(currentPrincipal));
+
+ // The next request loads a second role. While that load is in progress,
the already loaded
+ // role is evicted; the eviction must not slip between the load and the
first check.
+ long otherRoleId = 33L;
+ String otherRoleName = "roleLoadedDuringEviction";
+ RoleEntity otherRole = getRoleEntity(otherRoleId, otherRoleName,
ImmutableList.of());
+ mockedRoleVersions.put(
+ otherRoleId, new RoleUpdatedAt(otherRoleId, otherRoleName,
nextRoleVersion()));
+ GravitinoCache<Long, Long> loadedRoles =
getLoadedRolesCache(jcasbinAuthorizer);
+ doAnswer(
+ invocation -> {
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ return otherRole;
+ })
+ .when(entityStore)
+ .get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, otherRoleName)),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class));
+ mockDirectUserRoles(allowRole, otherRole);
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal,
+ METALAKE,
+ catalogObject(),
+ USE_CATALOG,
+ new AuthorizationRequestContext()));
+ }
+
+ @Test
+ public void testNarrowedAuthorizationReloadsEvictedPolicies() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "narrowedRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
+
requestContext.setActiveRoles(ActiveRoles.of(ImmutableList.of("narrowedRole")));
+
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
requestContext));
+ getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
+
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
requestContext));
+ }
+
+ /** Verifies bounded cache eviction races preserve composite authorization
decisions. */
+ @Test
+ public void testConcurrentEvictionNeverFlipsCompositeDecisions() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ GravitinoCache<Long, Long> loadedRoles =
getLoadedRolesCache(jcasbinAuthorizer);
+
+ // Requests run on this thread because the static mocks are thread-local.
The evictor only
+ // touches the loaded-role cache, whose removal listener clears policies
under the write lock,
+ // the same way a TTL or size eviction does. It evicts at most once per
request, at a random
+ // point in it, so a request never sees more clears than a check may
reload; a check that runs
+ // out of reloads fails closed by design, which is covered by
testRepeatedEvictionsFailClosed.
+ // Each request waits for its eviction round to finish before the next
request starts, so a
+ // delayed eviction cannot spill into another request and exceed the
intended reload budget.
+ Semaphore evictionRequests = new Semaphore(0);
+ Semaphore completedEvictions = new Semaphore(0);
+ AtomicLong evictions = new AtomicLong();
+ Thread evictor =
+ new Thread(
+ () -> {
+ try {
+ while (!Thread.currentThread().isInterrupted()) {
+ evictionRequests.acquire();
+
LockSupport.parkNanos(ThreadLocalRandom.current().nextLong(200_000L));
+ loadedRoles.invalidate(ALLOW_ROLE_ID);
+ loadedRoles.invalidate(DENY_ROLE_ID);
+ evictions.incrementAndGet();
+ completedEvictions.release();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ });
+ evictor.setDaemon(true);
+ try {
+ evictor.start();
+
+ // Granted: only the allow role is held. Every composite decision must
be allowed.
+ mockDirectUserRoles(allowRole);
+ for (int i = 0; i < 500; i++) {
+ AuthorizationRequestContext ctx = new AuthorizationRequestContext();
+ evictionRequests.release();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
ctx));
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG, ctx),
+ "an eviction must not deny a granted check");
+ assertTrue(completedEvictions.tryAcquire(5, TimeUnit.SECONDS));
+ }
+
+ // Denied: the allow role and the deny role are both held. No decision
may be allowed.
+ mockDirectUserRoles(allowRole, denyRole);
+ for (int i = 0; i < 500; i++) {
+ AuthorizationRequestContext ctx = new AuthorizationRequestContext();
+ evictionRequests.release();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
ctx));
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
ctx)
+ && !jcasbinAuthorizer.deny(
+ currentPrincipal, METALAKE, catalogObject(), USE_CATALOG,
ctx),
+ "an eviction must not allow a denied check");
+ assertTrue(completedEvictions.tryAcquire(5, TimeUnit.SECONDS));
+ }
+ } finally {
+ evictor.interrupt();
+ evictor.join(5000L);
+ }
+ assertFalse(evictor.isAlive(), "the evictor must stop before the test
exits");
+ assertEquals(1000L, evictions.get(), "each request must complete exactly
one eviction round");
+ }
+
+ private static MetadataObject metalakeObject() {
+ return MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE);
+ }
+
+ private static MetadataObject catalogObject() {
+ return MetadataObjects.of(null, "testCatalog",
MetadataObject.Type.CATALOG);
+ }
+
/** Reflectively invoke the private versionCheckAndLoadRoles. */
private static void invokeVersionCheckAndLoadRoles(
JcasbinAuthorizer authorizer,
