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 46cbef5bdd [#13748] fix(auth): Isolate request-specific role
membership (#13747)
46cbef5bdd is described below
commit 46cbef5bdd62c040eac385e6c1af079b666489a5
Author: Qi Yu <[email protected]>
AuthorDate: Wed Oct 14 19:08:52 2026 +0800
[#13748] fix(auth): Isolate request-specific role membership (#13747)
### What changes were proposed in this pull request?
Stop publishing request-specific user/group membership into shared
JCasbin grouping rows. Evaluate policies against the current request’s
held role IDs using the expression engine’s existing `include` function
in `jcasbin_model.conf`.
Remove the custom `RequestRoleFunction`, separate request model and
derived held/active subject caches. Compute active subjects locally,
keep one policy scan per probe, preserve global DENY across inactive
roles, and retain policy reload locking/generation checks. Make request
prefetch single-flight for parallel list workers, including runtime
failures. Cache the result or original exception only in the request
context; subsequent role-based checks in that failed request remain
denied, and a fresh request can retry. Successful query paths and SQL
are unchanged.
### Why are the changes needed?
Concurrent requests of the same stored user with different IdP groups
can inherit another request’s ALLOW or lose their own DENY, including
list filtering.
Fix: #13748
This is a request-isolation fix, separate from JCasbin replacement and
the broader performance work in #13593. No dependency upgrade or custom
matcher extension is introduced. Partial role-policy load failures
remain tracked separately in #13691.
### Does this PR introduce _any_ user-facing change?
Authorization uses only the current request’s group membership.
Active-role and global-DENY semantics remain unchanged. No REST API or
configuration changes.
### How was this patch tested?
- 21 core request-context tests, 260 server-common authorization tests
and 96 server REST/filter authorization tests passed. Core and
server-common Spotless checks passed. The full core suite was not run.
- Full server-common non-Docker suite: 494 tests, 493 passed; the
unchanged TLS rejection test
`TestHttpsServerAuthentication.testMutualTlsRejectsMissingClientCertificate`
received an HTTP read `IOException` instead of the expected handshake
exception. Isolated rerun of the TLS class also failed (5/7 passed;
missing and untrusted client certificate cases had the same
exception-shape mismatch). These Jetty-only tests do not execute the
authorizer; the previous PR head also exhibited this TLS failure. The
full suite is not claimed green.
- Retained interleaving/isolation regressions; added real-worker tests
for single-flight prefetch and concurrent named-active-role checks with
inactive-role DENY. Bypassing matcher membership made the role-isolation
model regression fail; the restored implementation passes. Reverting
failure memoization makes the 8-worker test issue 8 failed queries
instead of 1, and makes the public authorization regression incorrectly
allow a subsequent check in the same failed request. Tests also verify
recovery in a fresh request.
- Previously measured against this PR’s previous head, `f0f3d70aef`,
with the same dependencies and a 512 MiB heap. Three JVM forks per
variant cover 1/8/32 held roles, 0/100/1000 unrelated roles, cached
requests and real 1000-table filtering. For 8/32 held roles, list-filter
median fell from 186.8/182.1 ms to 175.6/171.4 ms. A longer warmed
32-role denied-request probe measured 108.0 -> 93.1 us.
- Same-JVM alternating matcher measurements cover 27 cases: median time
changes range from -17.2% to +3.4%. Paired concurrency throughput
changes range from -0.8% to +12.7%; a longer follow-up of the noisy
single-thread 6000-policy tail case measured p99 1412.1 -> 934.8 us.
Across JVMs, some short measurements varied substantially; these results
support no stable material regression in the tested warm-cache
workloads, not a universal zero-regression claim. They exclude HTTP, DB,
cold loading and failures; these benchmarks were not rerun for the
failure-memoization follow-up.
---
.../authorization/AuthorizationRequestContext.java | 47 ++
.../TestAuthorizationRequestContext.java | 71 +++
.../authorization/jcasbin/JcasbinAuthorizer.java | 262 ++++------
.../jcasbin/JcasbinLoadedRolesCache.java | 7 +-
.../src/main/resources/jcasbin_model.conf | 32 +-
.../server/authorization/TestJcasbinModel.java | 236 ++++++---
.../jcasbin/TestJcasbinAuthorizer.java | 562 ++++++++++++++++++++-
.../src/test/resources/jcasbin_policy.txt | 6 -
8 files changed, 899 insertions(+), 324 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 8b453a1e1a..22c47cf23f 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java
@@ -32,6 +32,7 @@ import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Function;
+import java.util.function.Supplier;
import javax.annotation.Nullable;
import lombok.AllArgsConstructor;
import lombok.EqualsAndHashCode;
@@ -103,6 +104,12 @@ public class AuthorizationRequestContext {
*/
private volatile Map<Long, RoleUpdatedAt> prefetchedRoleVersions;
+ /** Completed auth-subject prefetch result, including a missing user, for
this request only. */
+ @Nullable private volatile Optional<UserUpdatedAt> authSubjectPrefetchResult;
+
+ /** Failed prefetch attempts are shared by all workers of this request. */
+ @Nullable private volatile RuntimeException authSubjectPrefetchFailure;
+
private volatile String originalAuthorizationExpression;
/**
@@ -197,6 +204,46 @@ public class AuthorizationRequestContext {
}
}
+ /**
+ * Runs auth-subject prefetch at most once for this request, sharing its
result or failure.
+ *
+ * <p>The loader may populate the request's user, group and role-version
caches before returning
+ * the user identity, or {@link Optional#empty()} when the user is absent.
Waiting workers reuse
+ * the same outcome. A runtime failure is retained for the request so later
checks cannot repeat
+ * the failed query; a fresh request can try again. Fatal {@link Error}s are
not cached.
+ *
+ * @param loader the non-null prefetch operation, which must return a
non-null optional
+ * @return the prefetched user identity, or empty when the user is absent
+ * @throws RuntimeException if the loader failed, including on subsequent
calls in this request
+ */
+ public Optional<UserUpdatedAt> computeAuthSubjectPrefetchIfAbsent(
+ Supplier<Optional<UserUpdatedAt>> loader) {
+ Objects.requireNonNull(loader, "Prefetch loader must not be null");
+ RuntimeException failure = authSubjectPrefetchFailure;
+ if (failure != null) {
+ throw failure;
+ }
+ Optional<UserUpdatedAt> result = authSubjectPrefetchResult;
+ if (result != null) {
+ return result;
+ }
+ synchronized (this) {
+ if (authSubjectPrefetchFailure != null) {
+ throw authSubjectPrefetchFailure;
+ }
+ if (authSubjectPrefetchResult == null) {
+ try {
+ authSubjectPrefetchResult =
+ Objects.requireNonNull(loader.get(), "Prefetch loader must not
return null");
+ } catch (RuntimeException e) {
+ authSubjectPrefetchFailure = e;
+ throw e;
+ }
+ }
+ return authSubjectPrefetchResult;
+ }
+ }
+
/**
* Per-request {@link UserUpdatedAt} dedup. Loader may return {@link
Optional#empty()} to cache
* the "user not found" outcome and avoid repeated DB lookups within a
single request.
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 6e8437d957..7c30896fc8 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.authorization;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -547,4 +548,74 @@ public class TestAuthorizationRequestContext {
assertThrows(NullPointerException.class, () ->
context.setBoundRoleIds(null));
assertEquals(7L, context.getRolePolicyGeneration());
}
+
+ /** Successful auth-subject prefetch is reused without executing another
loader. */
+ @Test
+ public void testAuthSubjectPrefetchCachesUser() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ Optional<UserUpdatedAt> user = Optional.of(new UserUpdatedAt(1L, 2L));
+ AtomicInteger queries = new AtomicInteger();
+ assertSame(
+ user,
+ context.computeAuthSubjectPrefetchIfAbsent(
+ () -> {
+ queries.incrementAndGet();
+ return user;
+ }));
+ assertSame(
+ user,
+ context.computeAuthSubjectPrefetchIfAbsent(
+ () -> {
+ throw new AssertionError("Successful prefetch must not be
repeated");
+ }));
+ assertEquals(1, queries.get());
+ }
+
+ /** A missing user is a completed prefetch result, not a reason to repeat
the query. */
+ @Test
+ public void testAuthSubjectPrefetchCachesMissingUser() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ AtomicInteger queries = new AtomicInteger();
+ for (int i = 0; i < 2; i++) {
+ assertEquals(
+ Optional.empty(),
+ context.computeAuthSubjectPrefetchIfAbsent(
+ () -> {
+ queries.incrementAndGet();
+ return Optional.empty();
+ }));
+ }
+ assertEquals(1, queries.get());
+ }
+
+ /** Failed prefetch stays failed in this request, while a fresh request can
recover. */
+ @Test
+ public void testAuthSubjectPrefetchFailureIsRequestScoped() {
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ RuntimeException failure = new RuntimeException("database unavailable");
+ AtomicInteger queries = new AtomicInteger();
+ for (int i = 0; i < 2; i++) {
+ assertSame(
+ failure,
+ assertThrows(
+ RuntimeException.class,
+ () ->
+ context.computeAuthSubjectPrefetchIfAbsent(
+ () -> {
+ queries.incrementAndGet();
+ throw failure;
+ })));
+ }
+ assertEquals(1, queries.get());
+ AuthorizationRequestContext nextRequest = new
AuthorizationRequestContext();
+ Optional<UserUpdatedAt> recovered = Optional.of(new UserUpdatedAt(1L, 2L));
+ assertSame(
+ recovered,
+ nextRequest.computeAuthSubjectPrefetchIfAbsent(
+ () -> {
+ queries.incrementAndGet();
+ return recovered;
+ }));
+ assertEquals(2, queries.get());
+ }
}
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 4d0e4a0d94..695b61c299 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
@@ -159,12 +159,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* against authorization reads; otherwise a concurrent {@code enforce()} can
observe the temporary
* empty policy set and deny a request that should be allowed.
*
- * <p>Writers include {@link #invalidateRolePolicies}, {@link
#replaceRolePolicies}, {@link
- * #bindUserRoles}, stale grouping-row pruning, and the {@link
JcasbinLoadedRolesCache} removal
- * listener. Readers include all {@code enforce()}, grouping, and
policy-inspection calls. The
- * lock only guards in-memory enforcer access: metadata ids are resolved
before write-lock
- * acquisition (see {@link #resolveRolePolicies}), so no DB round-trip runs
inside the critical
- * section.
+ * <p>Writers include {@link #invalidateRolePolicies}, {@link
#replaceRolePolicies}, and the
+ * {@link JcasbinLoadedRolesCache} removal listener. Readers include all
{@code enforce()} and
+ * policy-inspection calls. The lock only guards in-memory enforcer access:
metadata ids are
+ * resolved before write-lock acquisition (see {@link
#resolveRolePolicies}), so no DB round-trip
+ * runs inside the critical section.
*/
private final ReentrantReadWriteLock rolePolicyLock = new
ReentrantReadWriteLock();
@@ -479,12 +478,10 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
UserUpdatedAt userInfo = userInfoOpt.get();
long userId = userInfo.getUserId();
- // Bind the user's (direct + group-inherited) roles into the enforcers so
the in-memory scan
- // below sees every policy the user can carry. Idempotent within a request.
+ // Capture this request's direct + group-inherited roles for the in-memory
policy scan.
loadRolePrivilege(metalake, principal.getName(), userId, userInfo,
requestContext);
Set<String> privilegeNames =
privileges.stream().map(Enum::name).collect(Collectors.toSet());
- String userIdStr = String.valueOf(userId);
// 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(
@@ -499,7 +496,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// 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)) {
+ for (String roleId : roleSubjects(requestContext, false)) {
// 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
@@ -570,7 +567,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
for (String groupname : currentPrincipalGroupNames()) {
- List<Long> groupRoleIds = loadGroupRoles(metalake, groupname,
userId, requestContext);
+ List<Long> groupRoleIds = loadGroupRoles(metalake, groupname,
requestContext);
if (groupRoleIds.contains(resolvedRoleId)) {
return true;
}
@@ -867,7 +864,8 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
/**
* When {@code true}, evaluation considers only the request's active roles
instead of every role
* the caller holds. Enabled for the allow authorizer so role assumption
can drop allows; the
- * deny authorizer leaves it {@code false} so denies always apply. See
{@link #enforceNarrowed}.
+ * deny authorizer leaves it {@code false} so denies always apply. Denies
always use the full
+ * request role union.
*/
private final boolean narrowByActiveRoles;
@@ -903,7 +901,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
String privilege,
AuthorizationRequestContext requestContext) {
// OWNER does not consult JCasbin policies — it short-circuits to the
owner cache in
- // authorizeByJcasbin. Skip the fat prefetch and role-binding work when
no non-OWNER
+ // authorizeByJcasbin. Skip the fat prefetch and role-loading work when
no non-OWNER
// privilege has been evaluated yet in this request.
boolean ownerOnly =
AuthConstants.OWNER.equals(privilege)
@@ -992,7 +990,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
String privilege,
AuthorizationRequestContext requestContext) {
// Step 4: JCasbin enforce (pure in-memory) — except OWNER, which is
resolved via the
- // owner cache rather than g-rows.
+ // owner cache rather than role policies.
if (AuthConstants.OWNER.equals(privilege)) {
// Cold-path: resolveOwnerId loads from DB when neither the
per-request nor the shared
// Caffeine cache has the entry, ensuring the first OWNER check
doesn't spuriously deny.
@@ -1005,85 +1003,58 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
String metadataType = String.valueOf(metadataObject.type());
String metadataIdStr = String.valueOf(metadataId);
- // Role assumption: if the caller activated only a subset of their
roles, check this allow
- // 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();
+ // Evaluate the request's role union in one policy scan. Shared g(user,
role)
+ // rows cannot represent concurrent requests carrying different IdP
group claims.
boolean allowed =
evaluateWithLoadedRolePolicies(
metalake,
requestContext,
() -> {
- if (narrowByActiveRoles && !activeRoles.isAll()) {
- return enforceNarrowed(
- userId,
- metadataType,
- metadataIdStr,
- privilege,
- activeRoles,
- requestContext);
+ Set<String> subjects = roleSubjects(requestContext,
narrowByActiveRoles);
+ if (subjects.isEmpty()) {
+ return false;
}
- return enforcer.enforce(
- String.valueOf(userId), metadataType, metadataIdStr,
privilege);
+ // Inactive roles still contribute DENY policies when
ALLOW is narrowed.
+ if (narrowByActiveRoles
+ && !requestContext.getActiveRoles().isAll()
+ && denyEnforcer.enforce(
+ roleSubjects(requestContext, false),
+ metadataType,
+ metadataIdStr,
+ privilege)) {
+ return false;
+ }
+ return enforcer.enforce(subjects, metadataType,
metadataIdStr, privilege);
})
.orElse(unstablePolicyResult);
if (narrowByActiveRoles && !allowed) {
- diagnoseDenial(userId, metadataType, metadataIdStr, privilege);
+ diagnoseDenial(userId, metadataType, metadataIdStr, privilege,
requestContext);
}
return allowed;
}
+ }
- /**
- * Enforces one object/privilege against only the active roles the caller
actually holds, so
- * narrowing can never grant access the caller lacks. {@link
ActiveRoles#none()} activates no
- * role and denies every role-derived privilege.
- *
- * <p>Deny stays global: denyEnforcer is checked over the caller's full
role union first, so a
- * deny on any held role still applies even when that role is inactive.
- */
- private boolean enforceNarrowed(
- long userId,
- String metadataType,
- String metadataIdStr,
- String privilege,
- ActiveRoles activeRoles,
- AuthorizationRequestContext requestContext) {
- String userIdStr = String.valueOf(userId);
- if (denyEnforcer.enforce(userIdStr, metadataType, metadataIdStr,
privilege)) {
- return false;
- }
- if (activeRoles.isNone()) {
- return false;
- }
- for (String roleId : activeRoleIds(userId, activeRoles, requestContext))
{
- if (enforcer.enforce(roleId, metadataType, metadataIdStr, privilege)) {
- return true;
- }
- }
- return false;
+ /** Builds subjects from this request only, without publishing derived
shared state. */
+ private static Set<String> roleSubjects(
+ AuthorizationRequestContext requestContext, boolean activeOnly) {
+ ActiveRoles activeRoles = requestContext.getActiveRoles();
+ if (activeOnly && activeRoles.isNone()) {
+ return Collections.emptySet();
}
-
- /**
- * Returns the ids of the caller's roles whose names are in the active
set. Only roles the
- * caller actually holds are considered, so naming a role the caller lacks
activates nothing.
- */
- private Set<String> activeRoleIds(
- long userId, ActiveRoles activeRoles, AuthorizationRequestContext
requestContext) {
- Map<Long, RoleUpdatedAt> roleVersions =
requestContext.getPrefetchedRoleVersions();
- if (roleVersions == null || roleVersions.isEmpty()) {
- return Collections.emptySet();
- }
- Set<String> activeNames = activeRoles.roleNames();
- Set<String> result = new HashSet<>();
- for (String roleId : enforcer.getRolesForUser(String.valueOf(userId))) {
- RoleUpdatedAt roleInfo = roleVersions.get(Long.parseLong(roleId));
- if (roleInfo != null && activeNames.contains(roleInfo.getRoleName())) {
- result.add(roleId);
+ Map<Long, RoleUpdatedAt> versions =
requestContext.getPrefetchedRoleVersions();
+ Set<String> subjects = new HashSet<>();
+ for (Long roleId : requestContext.getBoundRoleIds()) {
+ if (!activeOnly || activeRoles.isAll()) {
+ subjects.add(String.valueOf(roleId));
+ } else if (versions != null) {
+ RoleUpdatedAt role = versions.get(roleId);
+ if (role != null &&
activeRoles.roleNames().contains(role.getRoleName())) {
+ subjects.add(String.valueOf(roleId));
}
}
- return result;
}
+ return subjects;
}
//
---------------------------------------------------------------------------
@@ -1124,7 +1095,8 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* consumed by {@link #versionCheckAndLoadRoles} to skip its dedicated
probe.
* </ul>
*
- * <p>The fat prefetch runs at most once per request, gated by {@code
prefetchedRoleVersions}.
+ * <p>The fat prefetch runs at most once per request. Its success or runtime
failure is shared by
+ * parallel workers; a failed prefetch can be retried by a fresh request.
*/
private Optional<UserUpdatedAt> prefetchUserAndGroupInfo(
String metalake,
@@ -1132,6 +1104,20 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
List<String> groupNames,
AuthorizationRequestContext requestContext) {
+ if (requestContext.getPrefetchedRoleVersions() != null) {
+ return loadUserInfo(metalake, username, requestContext);
+ }
+ // Use the request's terminal result, including failures, so waiting list
workers do not
+ // serialize repeated failed queries. This shares the monitor used by role
loading.
+ return requestContext.computeAuthSubjectPrefetchIfAbsent(
+ () -> prefetchUserAndGroupInfoOnce(metalake, username, groupNames,
requestContext));
+ }
+
+ private Optional<UserUpdatedAt> prefetchUserAndGroupInfoOnce(
+ String metalake,
+ String username,
+ List<String> groupNames,
+ AuthorizationRequestContext requestContext) {
String userKey = JcasbinAuthorizationCacheKeys.userRoleKey(metalake,
username);
if (requestContext.getPrefetchedRoleVersions() != null) {
return loadUserInfo(metalake, username, requestContext);
@@ -1279,49 +1265,14 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// version-validated path as users (group_meta.updated_at as the
staleness sentinel).
List<Long> groupInheritedRoleIds = new ArrayList<>();
for (String groupname : currentPrincipalGroupNames()) {
- groupInheritedRoleIds.addAll(
- loadGroupRoles(metalake, groupname, userId, requestContext));
+ groupInheritedRoleIds.addAll(loadGroupRoles(metalake, groupname,
requestContext));
}
- // Prune stale g-rows: any role currently bound but no longer in the
desired
- // set (e.g. user removed from a group at the IdP, or role
unassigned).
- Set<String> desiredRoleIds = new HashSet<>();
- for (Long id : userDirectRoleIds) {
- desiredRoleIds.add(String.valueOf(id));
- }
- for (Long id : groupInheritedRoleIds) {
- desiredRoleIds.add(String.valueOf(id));
- }
- String userIdStr = String.valueOf(userId);
- List<String> staleRoleIds = new ArrayList<>();
- rolePolicyLock.readLock().lock();
- try {
- Set<String> currentRoleIds = new
HashSet<>(allowEnforcer.getRolesForUser(userIdStr));
- currentRoleIds.addAll(denyEnforcer.getRolesForUser(userIdStr));
- for (String currentRole : currentRoleIds) {
- if (!desiredRoleIds.contains(currentRole)) {
- staleRoleIds.add(currentRole);
- }
- }
- } finally {
- rolePolicyLock.readLock().unlock();
- }
- if (!staleRoleIds.isEmpty()) {
- rolePolicyLock.writeLock().lock();
- try {
- for (String currentRole : staleRoleIds) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- } finally {
- rolePolicyLock.writeLock().unlock();
- }
- }
-
- // Step 3: batch version-check all role IDs (direct +
group-inherited),
- // load stale ones (1 query for the version probe).
- List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
- allRoleIds.addAll(groupInheritedRoleIds);
+ // Deduplicate the request's role union without publishing user
membership to shared
+ // enforcers. Other requests may carry a different group set for the
same stored user.
+ Set<Long> roleUnion = new LinkedHashSet<>(userDirectRoleIds);
+ roleUnion.addAll(groupInheritedRoleIds);
+ List<Long> allRoleIds = new ArrayList<>(roleUnion);
if (!allRoleIds.isEmpty()) {
requestContext.setUnreadableRoleIds(
versionCheckAndLoadRoles(metalake, allRoleIds,
requestContext));
@@ -1342,7 +1293,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// Cache is still valid. The user id check prevents reusing roles after
deleting and
// recreating the same username with a new entity id.
CachedUserRoleRels cached = cachedOpt.get();
- bindUserRoles(userId, cached.getRoleIds());
return cached.getRoleIds();
}
@@ -1353,7 +1303,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
userRoleCache.put(
userCacheKey, new CachedUserRoleRels(userId, userInfo.getUpdatedAt(),
roleIds));
- bindUserRoles(userId, roleIds);
return roleIds;
}
@@ -1376,12 +1325,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* Version-validated group-role load, mirroring {@link #loadUserRoles}. A
cached snapshot is valid
* only when it belongs to the current group id and is at least as fresh as
{@code
* group_meta.updated_at}; the group id check prevents reusing stale roles
after a
- * delete-and-create of the same group name. In both cases the resulting
role IDs are bound to the
- * user's jcasbin g-rows so that the enforcer sees inherited privileges.
Groups missing from the
- * DB return an empty list.
+ * delete-and-create of the same group name. The resulting role IDs are
added only to the
+ * request's role union. Groups missing from the DB return an empty list.
*/
private List<Long> loadGroupRoles(
- String metalake, String groupname, long userId,
AuthorizationRequestContext requestContext) {
+ String metalake, String groupname, AuthorizationRequestContext
requestContext) {
Optional<GroupUpdatedAt> groupInfoOpt = loadGroupInfo(metalake, groupname,
requestContext);
if (!groupInfoOpt.isPresent()) {
return new ArrayList<>();
@@ -1394,7 +1342,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
if (cachedOpt.isPresent()) {
CachedGroupRoleRels cached = cachedOpt.get();
if (cached.getGroupId() == groupId && cached.getUpdatedAt() >=
groupInfo.getUpdatedAt()) {
- bindUserRoles(userId, cached.getRoleIds());
return cached.getRoleIds();
}
}
@@ -1405,7 +1352,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
groupRoleCache.put(
groupCacheKey, new CachedGroupRoleRels(groupId,
groupInfo.getUpdatedAt(), roleIds));
- bindUserRoles(userId, roleIds);
return roleIds;
}
@@ -1457,8 +1403,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
// Any roleId asked about but not returned has been deleted in the DB;
clear its policies so
- // a stale grouping row in the enforcer can't keep granting privileges
before the next
- // userRoleCache reload prunes the g-row itself.
+ // a request-local reference to the deleted role cannot keep granting
privileges.
Set<Long> existingRoleIds = new HashSet<>(roleVersions.size());
for (RoleUpdatedAt rv : roleVersions) {
existingRoleIds.add(rv.getRoleId());
@@ -1591,10 +1536,9 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
return false;
}
- // Refresh only permission policies. deleteRole would also remove the
current user's freshly
- // 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.
+ // Refresh permission policies. 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.
roleBeingReplaced = roleId;
try {
loadedRoles.invalidate(roleId);
@@ -1724,11 +1668,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
*
* <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.
+ * on one of the roles, may clear policies while the request still holds the
role id. 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.
@@ -1812,42 +1756,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
return false;
}
- private void bindUserRoles(long userId, List<Long> roleIds) {
- if (roleIds.isEmpty()) {
- return;
- }
-
- String userIdStr = String.valueOf(userId);
- List<Long> missingRoleIds = new ArrayList<>();
- rolePolicyLock.readLock().lock();
- try {
- Set<String> allowRoleIds = new
HashSet<>(allowEnforcer.getRolesForUser(userIdStr));
- Set<String> denyRoleIds = new
HashSet<>(denyEnforcer.getRolesForUser(userIdStr));
- for (Long roleId : roleIds) {
- String roleIdStr = String.valueOf(roleId);
- if (!allowRoleIds.contains(roleIdStr) ||
!denyRoleIds.contains(roleIdStr)) {
- missingRoleIds.add(roleId);
- }
- }
- } finally {
- rolePolicyLock.readLock().unlock();
- }
- if (missingRoleIds.isEmpty()) {
- return;
- }
-
- rolePolicyLock.writeLock().lock();
- try {
- for (Long roleId : missingRoleIds) {
- String roleIdStr = String.valueOf(roleId);
- allowEnforcer.addRoleForUser(userIdStr, roleIdStr);
- denyEnforcer.addRoleForUser(userIdStr, roleIdStr);
- }
- } finally {
- rolePolicyLock.writeLock().unlock();
- }
- }
-
//
---------------------------------------------------------------------------
// Policy loading from role entity
//
---------------------------------------------------------------------------
@@ -1993,7 +1901,11 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
* under investigation; the diagnosis itself is then reported at {@code
WARN}.
*/
private void diagnoseDenial(
- long userId, String metadataType, String metadataIdStr, String
privilege) {
+ long userId,
+ String metadataType,
+ String metadataIdStr,
+ String privilege,
+ AuthorizationRequestContext requestContext) {
if (!LOG.isDebugEnabled()) {
return;
}
@@ -2002,10 +1914,10 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
Map<String, Integer> rolePolicyCounts = new HashMap<>();
rolePolicyLock.readLock().lock();
try {
- List<String> boundRoles = allowEnforcer.getRolesForUser(userIdStr);
+ Set<String> boundRoles = roleSubjects(requestContext, false);
if (boundRoles.isEmpty()) {
LOG.debug(
- "Denied [{}, {}, {}, {}]: no role is bound to the user in the
allow enforcer",
+ "Denied [{}, {}, {}, {}]: no role is bound to this request",
userIdStr,
metadataType,
metadataIdStr,
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java
index 46f20068fe..467141a965 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java
@@ -34,10 +34,9 @@ import org.slf4j.LoggerFactory;
* cleaner may ignore a stale removal when the role was reloaded while the
callback waited for the
* authorizer's policy mutation lock.
*
- * <p>This cache owns role permission policies only. Therefore, eviction must
clear only {@code
- * p(roleId, ...)} policies and must not delete the role itself, because
JCasbin's {@code
- * deleteRole(roleId)} also removes {@code g(user/group, roleId)} bindings
that are managed
- * separately by {@link JcasbinAuthorizer}.
+ * <p>This cache owns role permission policies only. Eviction clears the
{@code p(roleId, ...)}
+ * policies; requests evaluate their own role-id sets, so there is no user or
group membership in
+ * the enforcers to remove.
*
* <p>The TTL is <b>write-based</b>, matching every other authorization cache.
An access-based TTL
* would be renewed by the version probe that {@code versionCheckAndLoadRoles}
performs on every
diff --git a/server-common/src/main/resources/jcasbin_model.conf
b/server-common/src/main/resources/jcasbin_model.conf
index ba17c87b09..cd0c7de05b 100644
--- a/server-common/src/main/resources/jcasbin_model.conf
+++ b/server-common/src/main/resources/jcasbin_model.conf
@@ -15,41 +15,17 @@
; specific language governing permissions and limitations
; under the License.
-; The model file of jcasbin defines a permission model used to describe the
permissions
-; of users, groups, and roles for a single metadata type.
-
-; "r" represents the parameters passed when making a request to jCasbin.
-; "sub" represents a roleId, userId, or groupCodeId.
-; The combination of `sub` and `metalakeId` uniquely identifies a user, role,
or group.
-; "metadataType" represents the type of metadata.
-; "metadataId" represents the id of metadata
-; "act" represents the privilege that needs to be authorized or whether it is
an OWNER.
+; r.sub is the set of role-id strings held by this request.
+; p.sub is one role-id string. No shared user/group membership is consulted.
[request_definition]
r = sub, metadataType, metadataId, act
-; "p" represents a permission policy.
-; "eft" stands for "effect" which can be either "allow" or "deny".
[policy_definition]
p = sub, metadataType, metadataId, act, eft
-; "g" represents the membership or ownership relationship of users, groups, or
roles.
-[role_definition]
-g = _, _
-
-; "e" represents the effect of the "eft",eft performs a logical combination
judgment on the matching results
-; of Matchers.
-; e = some(where(p.eft == allow)): This statement means that if the matching
strategy result p.eft has the result
-; of (some) allow
-; e = some(where (p.eft == allow)) && !some(where (p.eft == deny)): The
logical meaning of this example combination
-; is: if there is a strategy that matches the result of allow and no strategy
that matches the result of deny,
-; the result is true. In other words, it is true when the matching strategies
are all allow. If there is any deny,
-; both are false (more simply, when allow and deny exist at the same time,
deny takes precedence).
-;
-; see more in https://casbin.org/zh/docs/how-it-works/#effect
[policy_effect]
e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
-; "m" represents the matching rules of the model
[matchers]
-m = g(r.sub, p.sub) && r.metadataId == p.metadataId && r.metadataType ==
p.metadataType && ( (p.act == "OWNER" && r.act == "OWNER") || r.act == p.act )
-
+; include is provided by the existing expression engine; no custom function is
registered.
+m = include(r.sub, p.sub) && r.metadataId == p.metadataId && r.metadataType ==
p.metadataType && r.act == p.act
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java
index 5ddfe35ba2..d74791e1bc 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java
@@ -24,12 +24,17 @@ import java.io.InputStream;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.List;
+import java.util.Map;
+import java.util.Set;
import java.util.stream.Collectors;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.server.authorization.jcasbin.GravitinoAdapter;
+import org.apache.gravitino.server.authorization.jcasbin.JcasbinAuthorizer;
import org.casbin.jcasbin.main.Enforcer;
+import org.casbin.jcasbin.main.SyncedEnforcer;
import org.casbin.jcasbin.model.Model;
import org.casbin.jcasbin.persist.file_adapter.FileAdapter;
import org.junit.jupiter.api.Assertions;
@@ -39,6 +44,17 @@ import org.junit.jupiter.api.Test;
/** Test for jcasbin model. */
public class TestJcasbinModel {
+ /**
+ * Roles each test principal holds. {@link JcasbinAuthorizer} resolves user
and group membership
+ * before evaluation, so the model only sees the request's role ids.
+ */
+ private static final Map<String, Set<String>> ROLES_BY_PRINCIPAL =
+ Map.of(
+ "user1", Set.of("role5"),
+ "user2", Set.of("role5"),
+ "user3", Set.of("role1", "role5"),
+ "group1", Set.of("role5"));
+
/** Jcasbin enforcer */
private static Enforcer enforcer;
@@ -67,6 +83,13 @@ public class TestJcasbinModel {
}
}
+ private static Set<String> roles(String principal) {
+ if (principal.startsWith("role")) {
+ return Set.of(principal);
+ }
+ return ROLES_BY_PRINCIPAL.getOrDefault(principal, Set.of());
+ }
+
/**
* "role1" is the OWNER of metalake1. Determine whether role1 has the OWNER
permission for
* metalake1.
@@ -74,18 +97,20 @@ public class TestJcasbinModel {
@Test
public void testMetalakeOwner() {
Assertions.assertTrue(
- enforcer.enforce("role1", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("role1"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role1",
+ roles("role1"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
- enforcer.enforce("role1", MetadataObject.Type.METALAKE.name(),
"metalake2", "OWNER"));
+ enforcer.enforce(
+ roles("role1"), MetadataObject.Type.METALAKE.name(), "metalake2",
"OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role1",
+ roles("role1"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
@@ -98,24 +123,24 @@ public class TestJcasbinModel {
@Test
public void testCatalogOwner() {
Assertions.assertTrue(
- enforcer.enforce("role2", MetadataObject.Type.CATALOG.name(),
"catalog1", "OWNER"));
+ enforcer.enforce(roles("role2"), MetadataObject.Type.CATALOG.name(),
"catalog1", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role2",
+ roles("role2"),
MetadataObject.Type.CATALOG.name(),
"catalog1",
Privilege.Name.USE_SCHEMA.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role2",
+ roles("role2"),
MetadataObject.Type.CATALOG.name(),
"catalog1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
- enforcer.enforce("role2", MetadataObject.Type.CATALOG.name(),
"catalog2", "OWNER"));
+ enforcer.enforce(roles("role2"), MetadataObject.Type.CATALOG.name(),
"catalog2", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role2",
+ roles("role2"),
MetadataObject.Type.CATALOG.name(),
"catalog2",
Privilege.Name.USE_CATALOG.name()));
@@ -127,18 +152,18 @@ public class TestJcasbinModel {
@Test
public void testSchemaOwner() {
Assertions.assertTrue(
- enforcer.enforce("role3", MetadataObject.Type.SCHEMA.name(),
"schema1", "OWNER"));
+ enforcer.enforce(roles("role3"), MetadataObject.Type.SCHEMA.name(),
"schema1", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role3",
+ roles("role3"),
MetadataObject.Type.SCHEMA.name(),
"schema1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
- enforcer.enforce("role3", MetadataObject.Type.SCHEMA.name(),
"schema2", "OWNER"));
+ enforcer.enforce(roles("role3"), MetadataObject.Type.SCHEMA.name(),
"schema2", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role3",
+ roles("role3"),
MetadataObject.Type.SCHEMA.name(),
"schema2",
Privilege.Name.SELECT_TABLE.name()));
@@ -148,26 +173,26 @@ public class TestJcasbinModel {
@Test
public void testTableOwner() {
Assertions.assertTrue(
- enforcer.enforce("role4", MetadataObject.Type.TABLE.name(), "table1",
"OWNER"));
+ enforcer.enforce(roles("role4"), MetadataObject.Type.TABLE.name(),
"table1", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role4",
+ roles("role4"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role4",
+ roles("role4"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
- enforcer.enforce("role3", MetadataObject.Type.SCHEMA.name(), "table1",
"OWNER"));
+ enforcer.enforce(roles("role3"), MetadataObject.Type.SCHEMA.name(),
"table1", "OWNER"));
Assertions.assertFalse(
- enforcer.enforce("role3", MetadataObject.Type.SCHEMA.name(), "table2",
"OWNER"));
+ enforcer.enforce(roles("role3"), MetadataObject.Type.SCHEMA.name(),
"table2", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role4",
+ roles("role4"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
@@ -178,94 +203,96 @@ public class TestJcasbinModel {
public void testRolePrivilege() {
// "role5" has partial privilege.
Assertions.assertFalse(
- enforcer.enforce("role5", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("role5"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertTrue(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertTrue(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertTrue(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role5",
+ roles("role5"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
// role1000 has no privilege.
Assertions.assertFalse(
- enforcer.enforce("role1000", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("role1000"), MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "role1000",
+ roles("role1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
@@ -279,94 +306,96 @@ public class TestJcasbinModel {
public void testGroupPrivilege() {
// "group1" possesses role5, and therefore, group5 has the permissions of
role5.
Assertions.assertFalse(
- enforcer.enforce("group1", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("group1"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertTrue(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertTrue(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertTrue(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1",
+ roles("group1"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
// group1000 has no roles and therefore has no permissions.
Assertions.assertFalse(
- enforcer.enforce("group1000", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("group1000"), MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "group1000",
+ roles("group1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
@@ -380,190 +409,231 @@ public class TestJcasbinModel {
public void testUserPrivilege() {
// ”user1" has the privilege of role5.
Assertions.assertFalse(
- enforcer.enforce("user1", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("user1"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertTrue(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertTrue(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertTrue(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1",
+ roles("user1"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
// ”user2" has the privilege of group1.
Assertions.assertFalse(
- enforcer.enforce("user2", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("user2"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertTrue(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertTrue(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user2",
+ roles("user2"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
// "user3" has the privilege of both role1 and group1.
Assertions.assertTrue(
- enforcer.enforce("user3", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("user3"), MetadataObject.Type.METALAKE.name(), "metalake1",
"OWNER"));
Assertions.assertTrue(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertTrue(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user3",
+ roles("user3"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
- enforcer.enforce("user1000", MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
+ enforcer.enforce(
+ roles("user1000"), MetadataObject.Type.METALAKE.name(),
"metalake1", "OWNER"));
// "user1000" has no roles and therefore has no permissions.
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.METALAKE.name(),
"metalake1",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.SELECT_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.METALAKE.name(),
"metalake2",
Privilege.Name.USE_CATALOG.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.TABLE.name(),
"table2",
Privilege.Name.MODIFY_TABLE.name()));
Assertions.assertFalse(
enforcer.enforce(
- "user1000",
+ roles("user1000"),
MetadataObject.Type.TABLE.name(),
"table1",
Privilege.Name.SELECT_TABLE.name()));
}
+
+ /** Verifies role-set isolation and global deny precedence with the built-in
matcher. */
+ @Test
+ public void testRequestRolesAndDenyPrecedence() throws Exception {
+ Enforcer enforcer = newEnforcer();
+ enforcer.addPolicy("allow-role", "TABLE", "42", "SELECT_TABLE", "allow");
+ enforcer.addPolicy("deny-role", "TABLE", "42", "SELECT_TABLE", "deny");
+ Assertions.assertTrue(enforcer.enforce(Set.of("allow-role"), "TABLE",
"42", "SELECT_TABLE"));
+ Assertions.assertFalse(
+ enforcer.enforce(Set.of("allow-role", "deny-role"), "TABLE", "42",
"SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce(Set.of("unheld-role"), "TABLE",
"42", "SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce(Set.of(), "TABLE", "42",
"SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce("allow-role", "TABLE", "42",
"SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce(Set.of("allow-role"), "TABLE",
"43", "SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce(Set.of("allow-role"), "SCHEMA",
"42", "SELECT_TABLE"));
+ Assertions.assertFalse(enforcer.enforce(Set.of("allow-role"), "TABLE",
"42", "MODIFY_TABLE"));
+ }
+
+ /** Verifies the deny-only enforcer evaluates the request role set. */
+ @Test
+ public void testDenyEnforcerUsesRoleSet() throws Exception {
+ Enforcer deny = newEnforcer();
+ // The deny enforcer represents DENY policies with the allow effect to
answer existence checks.
+ deny.addPolicy("deny-role", "TABLE", "42", "SELECT_TABLE", "allow");
+ Assertions.assertTrue(
+ deny.enforce(Set.of("allow-role", "deny-role"), "TABLE", "42",
"SELECT_TABLE"));
+ Assertions.assertFalse(deny.enforce(Set.of("allow-role"), "TABLE", "42",
"SELECT_TABLE"));
+ }
+
+ private Enforcer newEnforcer() throws Exception {
+ Model model = new Model();
+ try (InputStream input =
getClass().getResourceAsStream("/jcasbin_model.conf")) {
+ model.loadModelFromText(new String(input.readAllBytes(),
StandardCharsets.UTF_8));
+ }
+ Enforcer enforcer = new SyncedEnforcer(model, new GravitinoAdapter());
+ return enforcer;
+ }
}
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 ad84591415..380b557f3d 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
@@ -23,6 +23,7 @@ import static
org.apache.gravitino.authorization.Privilege.Name.USE_SCHEMA;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
@@ -46,10 +47,12 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.Principal;
import java.time.Duration;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
@@ -58,18 +61,22 @@ import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executor;
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.AtomicInteger;
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;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.GravitinoEnv;
@@ -104,7 +111,10 @@ import org.apache.gravitino.semantic.SemanticModel;
import org.apache.gravitino.semantic.SemanticModelChange;
import org.apache.gravitino.server.ServerConfig;
import org.apache.gravitino.server.authorization.AuthorizationRequestScope;
+import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
+import org.apache.gravitino.server.authorization.MetadataAuthzHelper;
import org.apache.gravitino.server.authorization.MetadataIdConverter;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants;
import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
import org.apache.gravitino.storage.relational.mapper.EntityChangeLogMapper;
import org.apache.gravitino.storage.relational.mapper.GroupMetaMapper;
@@ -132,6 +142,8 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
@@ -202,7 +214,7 @@ public class TestJcasbinAuthorizer {
/**
* Recreated per test in {@link #createAuthorizer()} so each case starts
with empty enforcer state
- * and a fresh cache; the previous static instance leaked g-rows and cache
entries across cases.
+ * and a fresh cache; a shared instance would leak role policies and cache
entries across cases.
*/
private JcasbinAuthorizer jcasbinAuthorizer;
@@ -382,7 +394,7 @@ public class TestJcasbinAuthorizer {
@BeforeEach
public void createAuthorizer() throws Exception {
- // Build a fresh authorizer per test so enforcer g-rows and
version-validated cache state can
+ // Build a fresh authorizer per test so enforcer policies and
version-validated cache state can
// never bleed across cases regardless of the JUnit execution order.
CatalogManager catalogs = mock(CatalogManager.class);
BaseCatalog<?> catalog = mock(BaseCatalog.class);
@@ -479,8 +491,8 @@ public class TestJcasbinAuthorizer {
assertTrue(doAuthorize(currentPrincipal));
// Test role cache.
- // When the user's role changes to one with no privileges, the prune step
removes
- // the stale role's g-rows from the enforcer, so authorization fails
immediately.
+ // When the user's role changes to one with no privileges, the request's
role set no longer
+ // contains the old role, so authorization fails immediately.
Long newRoleId = -1L;
RoleEntity tempNewRole = getRoleEntity(newRoleId, "tempNewRole",
ImmutableList.of());
when(entityStore.get(
@@ -495,7 +507,7 @@ public class TestJcasbinAuthorizer {
.thenReturn(ImmutableList.of(new RoleUpdatedAt(newRoleId,
"tempNewRole", roleVersion2)));
when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
.thenReturn(new UserUpdatedAt(USER_ID, nextUserVersion()));
- // tempNewRole has no privileges; prune step removes stale allowRole
g-row, so authz fails.
+ // tempNewRole has no privileges and the request no longer holds
allowRole, so authz fails.
assertFalse(doAuthorize(currentPrincipal));
// After clearing the role policy cache, the next authorize forces a
reload.
@@ -631,7 +643,7 @@ public class TestJcasbinAuthorizer {
// a roleId whose version probe row is missing (e.g. cache window race,
future
// code path bypassing the fat-JOIN), the fix must still clear that role's
// p-rows from both enforcers and evict its loadedRoles entry so that any
- // residual user → deleted-role g-row grants nothing on subsequent
enforce()s.
+ // request-local membership in the deleted role grants nothing on
subsequent checks.
makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
@@ -1211,9 +1223,10 @@ public class TestJcasbinAuthorizer {
Enforcer allowEnforcer = getAllowEnforcer(jcasbinAuthorizer);
String roleIdStr = String.valueOf(ALLOW_ROLE_ID);
+ AuthorizationRequestContext requestContext = new
AuthorizationRequestContext();
assertTrue(
jcasbinAuthorizer.authorize(
- currentPrincipal, METALAKE, catalog, USE_CATALOG, new
AuthorizationRequestContext()));
+ currentPrincipal, METALAKE, catalog, USE_CATALOG, requestContext));
List<List<String>> policyRows = allowEnforcer.getFilteredPolicy(0,
roleIdStr);
assertFalse(policyRows.isEmpty());
@@ -1238,7 +1251,7 @@ public class TestJcasbinAuthorizer {
catalog,
CATALOG_ID,
USE_CATALOG,
- new AuthorizationRequestContext());
+ requestContext);
}));
}
@@ -1313,8 +1326,8 @@ public class TestJcasbinAuthorizer {
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.
+ // TTL expiry or size eviction runs the removal listener, which clears the
role's p-rows while
+ // the request still holds the role id. The request will not run its
one-time role load again.
getLoadedRolesCache(jcasbinAuthorizer).invalidate(ALLOW_ROLE_ID);
assertTrue(
getAllowEnforcer(jcasbinAuthorizer)
@@ -2608,8 +2621,8 @@ public class TestJcasbinAuthorizer {
// User is removed from the group at the IdP level -- next token has no
groups.
UserPrincipal noGroupPrincipal = setCurrentPrincipalWithGroup(null);
- // The prune step detects that the group-inherited role is no longer valid
- // (group not in token → role not in desiredRoleIds) and removes the stale
g-rows.
+ // The group is no longer in the token, so the request's role set excludes
the
+ // group-inherited role.
// Access is denied immediately without waiting for cache TTL expiry.
assertFalse(doAuthorize(noGroupPrincipal));
@@ -2947,6 +2960,511 @@ public class TestJcasbinAuthorizer {
getLoadedRolesCache(jcasbinAuthorizer).invalidateAll();
}
+ /** Another request with different groups must not grant roles to this
request. */
+ @Test
+ public void testInterleavedGroupRequestsMustNotInjectAllow() throws
Exception {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ try {
+ RoleEntity groupRole =
+ mockRoleInStore(901L, "probeGroupAllow",
ImmutableList.of(getAllowSecurableObject()));
+ mockNoDirectUserRoles();
+ mockGroupWithRoles(
+ GROUP_NAME, ImmutableList.of(groupRole.id()),
ImmutableList.of(groupRole.name()));
+
+ UserPrincipal noGroupPrincipal = setCurrentPrincipalWithGroup(null);
+ AuthorizationRequestContext noGroupContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ noGroupPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
noGroupContext));
+ assertTrue(noGroupContext.getBoundRoleIds().isEmpty());
+
+ UserPrincipal groupPrincipal = setCurrentPrincipalWithGroup(GROUP_NAME);
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ groupPrincipal,
+ METALAKE,
+ catalogObject(),
+ USE_CATALOG,
+ new AuthorizationRequestContext()));
+
+ principalUtilsMockedStatic
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(noGroupPrincipal);
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ noGroupPrincipal, METALAKE, catalogObject(), USE_CATALOG,
noGroupContext),
+ "request without groups must not inherit another request's group
ALLOW");
+
+ } finally {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ restoreDefaultPrincipal();
+ }
+ }
+
+ /** Another request with no groups must not remove this request's group
deny. */
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testInterleavedGroupRequestsMustNotRemoveDeny(boolean narrow)
throws Exception {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ try {
+ RoleEntity allowRole =
+ mockRoleInStore(
+ ALLOW_ROLE_ID, "probeDirectAllow",
ImmutableList.of(getAllowSecurableObject()));
+ RoleEntity denyRole =
+ mockRoleInStore(
+ DENY_ROLE_ID, "probeGroupDeny",
ImmutableList.of(getDenySecurableObject()));
+ mockDirectUserRoles(allowRole);
+ mockGroupWithRoles(
+ GROUP_NAME, ImmutableList.of(denyRole.id()),
ImmutableList.of(denyRole.name()));
+
+ UserPrincipal groupPrincipal = setCurrentPrincipalWithGroup(GROUP_NAME);
+ AuthorizationRequestContext groupContext = new
AuthorizationRequestContext();
+ if (narrow) {
+
groupContext.setActiveRoles(ActiveRoles.of(ImmutableList.of("probeDirectAllow")));
+ }
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ groupPrincipal, METALAKE, metalakeObject(), USE_CATALOG,
groupContext));
+ assertTrue(groupContext.getBoundRoleIds().contains(DENY_ROLE_ID));
+
+ UserPrincipal noGroupPrincipal = setCurrentPrincipalWithGroup(null);
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ noGroupPrincipal,
+ METALAKE,
+ catalogObject(),
+ USE_CATALOG,
+ new AuthorizationRequestContext()));
+
+ principalUtilsMockedStatic
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(groupPrincipal);
+ assertTrue(
+ jcasbinAuthorizer.deny(
+ groupPrincipal, METALAKE, catalogObject(), USE_CATALOG,
groupContext),
+ "another request must not prune this request's group DENY");
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ groupPrincipal, METALAKE, catalogObject(), USE_CATALOG,
groupContext));
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ groupPrincipal, METALAKE, ImmutableSet.of(USE_CATALOG),
groupContext));
+ principalUtilsMockedStatic
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(noGroupPrincipal);
+ assertFalse(
+ jcasbinAuthorizer.hasDenyPolicy(
+ noGroupPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()));
+
+ } finally {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ restoreDefaultPrincipal();
+ }
+ }
+
+ /**
+ * Parallel list workers share one prefetch outcome, including missing users
and query failures.
+ *
+ * @param failQuery whether the initial query fails
+ */
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testParallelRequestPrefetchIsSingleFlight(boolean failQuery)
throws Exception {
+ UserMetaMapper requestMapper = mock(UserMetaMapper.class);
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ AtomicInteger queries = new AtomicInteger();
+ RuntimeException queryFailure = new RuntimeException("auth subject
database unavailable");
+ CountDownLatch queried = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ CountDownLatch started = new CountDownLatch(8);
+ when(requestMapper.batchGetAuthSubjectsForUser(eq(METALAKE), eq(USERNAME),
anyList()))
+ .thenAnswer(
+ invocation -> {
+ queries.incrementAndGet();
+ queried.countDown();
+ assertTrue(release.await(10, TimeUnit.SECONDS));
+ if (failQuery) {
+ throw queryFailure;
+ }
+ return Collections.emptyList();
+ });
+ Method prefetch =
+ JcasbinAuthorizer.class.getDeclaredMethod(
+ "prefetchUserAndGroupInfo",
+ String.class,
+ String.class,
+ List.class,
+ AuthorizationRequestContext.class);
+ prefetch.setAccessible(true);
+ ExecutorService workers = Executors.newFixedThreadPool(8);
+ try {
+ List<Future<?>> results = new ArrayList<>();
+ for (int i = 0; i < 8; i++) {
+ results.add(
+ workers.submit(
+ () -> {
+ // Static mocks are thread-local; each real worker needs its
own mapper bridge.
+ try (MockedStatic<SessionUtils> sessions =
mockStatic(SessionUtils.class)) {
+ sessions
+ .when(() ->
SessionUtils.getWithoutCommit(eq(UserMetaMapper.class), any()))
+ .thenAnswer(
+ invocation -> {
+ Function<UserMetaMapper, ?> query =
invocation.getArgument(1);
+ return query.apply(requestMapper);
+ });
+ started.countDown();
+ if (failQuery) {
+ InvocationTargetException failure =
+ assertThrows(
+ InvocationTargetException.class,
+ () ->
+ prefetch.invoke(
+ jcasbinAuthorizer,
+ METALAKE,
+ USERNAME,
+ Collections.emptyList(),
+ context));
+ assertSame(queryFailure, failure.getCause());
+ } else {
+ assertEquals(
+ Optional.empty(),
+ prefetch.invoke(
+ jcasbinAuthorizer,
+ METALAKE,
+ USERNAME,
+ Collections.emptyList(),
+ context));
+ }
+ }
+ return null;
+ }));
+ }
+ assertTrue(started.await(10, TimeUnit.SECONDS));
+ assertTrue(queried.await(10, TimeUnit.SECONDS));
+ release.countDown();
+ for (Future<?> result : results) {
+ result.get(10, TimeUnit.SECONDS);
+ }
+ assertEquals(1, queries.get());
+ if (failQuery) {
+ assertNull(context.getPrefetchedRoleVersions());
+ // Recover the backend: the failed request must retain its error,
while a new one retries.
+ when(requestMapper.batchGetAuthSubjectsForUser(eq(METALAKE),
eq(USERNAME), anyList()))
+ .thenAnswer(
+ invocation -> {
+ queries.incrementAndGet();
+ return Collections.emptyList();
+ });
+ try {
+ sessionUtilsMockedStatic
+ .when(() ->
SessionUtils.getWithoutCommit(eq(UserMetaMapper.class), any()))
+ .thenAnswer(
+ invocation -> {
+ Function<UserMetaMapper, ?> query =
invocation.getArgument(1);
+ return query.apply(requestMapper);
+ });
+ InvocationTargetException failure =
+ assertThrows(
+ InvocationTargetException.class,
+ () ->
+ prefetch.invoke(
+ jcasbinAuthorizer, METALAKE, USERNAME,
Collections.emptyList(), context));
+ assertSame(queryFailure, failure.getCause());
+ assertEquals(1, queries.get());
+ assertEquals(
+ Optional.empty(),
+ prefetch.invoke(
+ jcasbinAuthorizer,
+ METALAKE,
+ USERNAME,
+ Collections.emptyList(),
+ new AuthorizationRequestContext()));
+ assertEquals(2, queries.get());
+ } finally {
+ sessionUtilsMockedStatic
+ .when(() ->
SessionUtils.getWithoutCommit(eq(UserMetaMapper.class), any()))
+ .thenAnswer(
+ invocation -> {
+ Function<UserMetaMapper, ?> query =
invocation.getArgument(1);
+ return query.apply(userMetaMapper);
+ });
+ }
+ } else {
+ assertEquals(Collections.emptyMap(),
context.getPrefetchedRoleVersions());
+ }
+ } finally {
+ release.countDown();
+ workers.shutdownNow();
+ assertTrue(workers.awaitTermination(10, TimeUnit.SECONDS));
+ }
+ }
+
+ /** Failed prefetch rejects subsequent role checks, while a fresh request
can recover. */
+ @Test
+ public void testFailedPrefetchDeniesUntilNextRequest() throws Exception {
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+ List<AuthPrefetchRow> rows =
+ userMetaMapper.batchGetAuthSubjectsForUser(METALAKE, USERNAME,
Collections.emptyList());
+ UserMetaMapper requestMapper = mock(UserMetaMapper.class);
+ RuntimeException failure = new RuntimeException("auth subject database
unavailable");
+ when(requestMapper.batchGetAuthSubjectsForUser(eq(METALAKE), eq(USERNAME),
anyList()))
+ .thenThrow(failure)
+ .thenReturn(rows);
+ sessionUtilsMockedStatic
+ .when(() -> SessionUtils.getWithoutCommit(eq(UserMetaMapper.class),
any()))
+ .thenAnswer(
+ invocation -> {
+ Function<UserMetaMapper, ?> query = invocation.getArgument(1);
+ return query.apply(requestMapper);
+ });
+ try {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(principal, METALAKE, metalakeObject(),
USE_CATALOG, context));
+ // Use another object to avoid reusing the first object's cached denial.
+ assertFalse(
+ jcasbinAuthorizer.authorize(principal, METALAKE, catalogObject(),
USE_CATALOG, context));
+ verify(requestMapper).batchGetAuthSubjectsForUser(eq(METALAKE),
eq(USERNAME), anyList());
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ principal,
+ METALAKE,
+ catalogObject(),
+ USE_CATALOG,
+ new AuthorizationRequestContext()));
+ verify(requestMapper, Mockito.times(2))
+ .batchGetAuthSubjectsForUser(eq(METALAKE), eq(USERNAME), anyList());
+ } finally {
+ sessionUtilsMockedStatic
+ .when(() -> SessionUtils.getWithoutCommit(eq(UserMetaMapper.class),
any()))
+ .thenAnswer(
+ invocation -> {
+ Function<UserMetaMapper, ?> query = invocation.getArgument(1);
+ return query.apply(userMetaMapper);
+ });
+ }
+ }
+
+ /** Parallel checks share only published request membership, including
inactive-role denies. */
+ @Test
+ public void testParallelNamedActiveRolesUsePublishedRequestMembership()
throws Exception {
+ Enforcer allow = getAllowEnforcer(jcasbinAuthorizer);
+ Enforcer deny = getDenyEnforcer(jcasbinAuthorizer);
+ allow.addPolicy(String.valueOf(ALLOW_ROLE_ID), "TABLE", "77",
"SELECT_TABLE", "allow");
+ allow.addPolicy(String.valueOf(ALLOW_ROLE_ID), "TABLE", "78",
"SELECT_TABLE", "allow");
+ deny.addPolicy(String.valueOf(DENY_ROLE_ID), "TABLE", "78",
"SELECT_TABLE", "allow");
+ AuthorizationRequestContext context = new AuthorizationRequestContext();
+ context.setActiveRoles(ActiveRoles.of(ImmutableList.of("parallelAllow")));
+ Field internalField =
JcasbinAuthorizer.class.getDeclaredField("allowInternalAuthorizer");
+ internalField.setAccessible(true);
+ Object internal = internalField.get(jcasbinAuthorizer);
+ Method authorize =
+ internal
+ .getClass()
+ .getDeclaredMethod(
+ "authorizeByJcasbin",
+ long.class,
+ String.class,
+ MetadataObject.class,
+ Long.class,
+ String.class,
+ AuthorizationRequestContext.class);
+ authorize.setAccessible(true);
+ ExecutorService workers = Executors.newFixedThreadPool(9);
+ CountDownLatch loading = new CountDownLatch(1);
+ CountDownLatch publish = new CountDownLatch(1);
+ CountDownLatch checking = new CountDownLatch(8);
+ try {
+ Future<?> loader =
+ workers.submit(
+ () ->
+ context.loadRole(
+ () -> {
+ loading.countDown();
+ try {
+ assertTrue(publish.await(10, TimeUnit.SECONDS));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new AssertionError(e);
+ }
+ context.setPrefetchedRoleVersions(
+ ImmutableMap.of(
+ ALLOW_ROLE_ID,
+ new RoleUpdatedAt(ALLOW_ROLE_ID,
"parallelAllow", 1L),
+ DENY_ROLE_ID, new RoleUpdatedAt(DENY_ROLE_ID,
"parallelDeny", 1L)));
+
context.setBoundRoleIds(ImmutableList.of(ALLOW_ROLE_ID, DENY_ROLE_ID));
+ }));
+ assertTrue(loading.await(10, TimeUnit.SECONDS));
+ List<Future<?>> checks = new ArrayList<>();
+ for (int i = 0; i < 8; i++) {
+ checks.add(
+ workers.submit(
+ () -> {
+ checking.countDown();
+ context.loadRole(
+ () -> {
+ throw new AssertionError("Role loading must be
single-flight");
+ });
+ for (int n = 0; n < 25; n++) {
+ assertTrue(
+ (boolean)
+ authorize.invoke(
+ internal,
+ 1L,
+ METALAKE,
+ MetadataObjects.of(
+ "testCatalog.testSchema", "visible",
MetadataObject.Type.TABLE),
+ 77L,
+ "SELECT_TABLE",
+ context));
+ assertFalse(
+ (boolean)
+ authorize.invoke(
+ internal,
+ 1L,
+ METALAKE,
+ MetadataObjects.of(
+ "testCatalog.testSchema", "hidden",
MetadataObject.Type.TABLE),
+ 78L,
+ "SELECT_TABLE",
+ context));
+ }
+ return null;
+ }));
+ }
+ assertTrue(checking.await(10, TimeUnit.SECONDS));
+ publish.countDown();
+ loader.get(10, TimeUnit.SECONDS);
+ for (Future<?> check : checks) {
+ check.get(10, TimeUnit.SECONDS);
+ }
+ } finally {
+ publish.countDown();
+ workers.shutdownNow();
+ assertTrue(workers.awaitTermination(10, TimeUnit.SECONDS));
+ }
+ }
+
+ /** The real list filter must not expose a table denied by this request's
group role. */
+ @Test
+ public void testInterleavedGroupRequestsMustNotExposeDeniedTable() throws
Exception {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ Field executorField =
MetadataAuthzHelper.class.getDeclaredField("executor");
+ executorField.setAccessible(true);
+ Object previousExecutor = executorField.get(null);
+ executorField.set(null, (Executor) Runnable::run);
+ Config previousConfig = gravitinoEnv.config();
+ Config config = mock(Config.class);
+ when(config.get(eq(Configs.ENABLE_AUTHORIZATION))).thenReturn(true);
+ when(gravitinoEnv.config()).thenReturn(config);
+ try {
+ SecurableObject deniedObject =
+ buildSecurableObject(
+ DENY_ROLE_ID,
+ MetadataObject.Type.TABLE,
+ "testCatalog.testSchema.hidden",
+ SELECT_TABLE,
+ "DENY");
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(eq(deniedObject),
eq(METALAKE)))
+ .thenReturn(Optional.of(42L));
+ RoleEntity allowRole =
+ mockRoleInStore(
+ ALLOW_ROLE_ID,
+ "probeListParentAllow",
+ ImmutableList.of(
+ buildSecurableObject(
+ ALLOW_ROLE_ID,
+ MetadataObject.Type.CATALOG,
+ "testCatalog",
+ SELECT_TABLE,
+ "ALLOW"),
+ buildSecurableObject(
+ ALLOW_ROLE_ID,
+ MetadataObject.Type.CATALOG,
+ "testCatalog",
+ USE_CATALOG,
+ "ALLOW"),
+ buildSecurableObject(
+ ALLOW_ROLE_ID,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
+ RoleEntity denyRole =
+ mockRoleInStore(DENY_ROLE_ID, "probeListGroupDeny",
ImmutableList.of(deniedObject));
+ mockDirectUserRoles(allowRole);
+ mockGroupWithRoles(
+ GROUP_NAME, ImmutableList.of(denyRole.id()),
ImmutableList.of(denyRole.name()));
+ UserPrincipal groupPrincipal = setCurrentPrincipalWithGroup(GROUP_NAME);
+ AuthorizationRequestContext groupContext = new
AuthorizationRequestContext();
+ assertFalse(
+ jcasbinAuthorizer.authorize(
+ groupPrincipal, METALAKE, metalakeObject(), SELECT_TABLE,
groupContext));
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ groupPrincipal, METALAKE, ImmutableSet.of(SELECT_TABLE),
groupContext));
+
+ UserPrincipal noGroupPrincipal = setCurrentPrincipalWithGroup(null);
+ assertTrue(
+ jcasbinAuthorizer.authorize(
+ noGroupPrincipal,
+ METALAKE,
+ catalogObject(),
+ SELECT_TABLE,
+ new AuthorizationRequestContext()));
+ principalUtilsMockedStatic
+ .when(PrincipalUtils::getCurrentPrincipal)
+ .thenReturn(groupPrincipal);
+
+ GravitinoAuthorizerProvider provider =
mock(GravitinoAuthorizerProvider.class);
+ when(provider.getGravitinoAuthorizer()).thenReturn(jcasbinAuthorizer);
+ try (MockedStatic<GravitinoAuthorizerProvider> providerMock =
+ mockStatic(GravitinoAuthorizerProvider.class);
+ AuthorizationRequestScope scope = AuthorizationRequestScope.open()) {
+
providerMock.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider);
+ scope.bind(METALAKE, groupContext);
+ NameIdentifier hidden = NameIdentifier.of(METALAKE, "testCatalog",
"testSchema", "hidden");
+ NameIdentifier visible =
+ NameIdentifier.of(METALAKE, "testCatalog", "testSchema",
"visible");
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ METALAKE,
+
AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.TABLE,
+ new NameIdentifier[] {hidden, visible});
+ assertTrue(
+ Arrays.asList(filtered).contains(visible), "the permitted table
must remain visible");
+ assertFalse(
+ Arrays.asList(filtered).contains(hidden),
+ "a concurrent no-group request must not expose the group-denied
table identifier");
+ }
+ } finally {
+ executorField.set(null, previousExecutor);
+ when(gravitinoEnv.config()).thenReturn(previousConfig);
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ restoreDefaultPrincipal();
+ }
+ }
+
/**
* Sets the current principal mock to a {@link UserPrincipal} with the given
group, or with no
* groups when {@code groupName} is null. Returns the principal for use in
assertions.
@@ -3413,33 +3931,26 @@ public class TestJcasbinAuthorizer {
// Add a role and its policy to the enforcer
Long testRoleId = 300L;
String roleIdStr = String.valueOf(testRoleId);
- String userIdStr = String.valueOf(USER_ID);
- // Add a policy and a user-role binding for this role.
- allowEnforcer.addRoleForUser(userIdStr, roleIdStr);
- denyEnforcer.addRoleForUser(userIdStr, roleIdStr);
+ // Add policies for this role.
allowEnforcer.addPolicy(roleIdStr, "CATALOG", "999", "USE_CATALOG",
"allow");
denyEnforcer.addPolicy(roleIdStr, "CATALOG", "999", "USE_CATALOG",
"allow");
// Add role to cache
loadedRoles.put(testRoleId, System.currentTimeMillis());
- // Verify role exists in enforcer (has policy and grouping).
+ // Verify the role policies exist in both enforcers.
assertTrue(allowEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
assertTrue(denyEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
- assertTrue(allowEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
- assertTrue(denyEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
// Invalidate the cache entry - this triggers the synchronous removal
listener
// (using executor(Runnable::run) to ensure synchronous execution)
loadedRoles.invalidate(testRoleId);
// Verify the role's policies have been deleted from enforcers
(synchronous, no need to wait),
- // but user-role bindings are preserved because loadedRoles owns role
policies only.
+ // Request role membership is independent of the policy cache.
assertFalse(allowEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
assertFalse(denyEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
- assertTrue(allowEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
- assertTrue(denyEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
}
@Test
@@ -3462,16 +3973,13 @@ public class TestJcasbinAuthorizer {
}
@Test
- public void testInvalidateRolePoliciesPreservesUserRoleBindings() throws
Exception {
+ public void testInvalidateRolePoliciesRemovesPoliciesAndBackoff() throws
Exception {
Enforcer allowEnforcer = getAllowEnforcer(jcasbinAuthorizer);
Enforcer denyEnforcer = getDenyEnforcer(jcasbinAuthorizer);
GravitinoCache<Long, Boolean> backoff =
getPartialRoleLoadBackoffCache(jcasbinAuthorizer);
Long testRoleId = 302L;
String roleIdStr = String.valueOf(testRoleId);
- String userIdStr = String.valueOf(USER_ID);
- allowEnforcer.addRoleForUser(userIdStr, roleIdStr);
- denyEnforcer.addRoleForUser(userIdStr, roleIdStr);
allowEnforcer.addPolicy(roleIdStr, "CATALOG", "999", "USE_CATALOG",
"allow");
denyEnforcer.addPolicy(roleIdStr, "CATALOG", "999", "USE_CATALOG",
"allow");
backoff.put(testRoleId, Boolean.TRUE);
@@ -3483,8 +3991,6 @@ public class TestJcasbinAuthorizer {
assertFalse(allowEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
assertFalse(denyEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
- assertTrue(allowEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
- assertTrue(denyEnforcer.getRolesForUser(userIdStr).contains(roleIdStr));
assertFalse(backoff.getIfPresent(testRoleId).isPresent());
}
diff --git a/server-common/src/test/resources/jcasbin_policy.txt
b/server-common/src/test/resources/jcasbin_policy.txt
index e71a5b6c54..e6b9a0c396 100644
--- a/server-common/src/test/resources/jcasbin_policy.txt
+++ b/server-common/src/test/resources/jcasbin_policy.txt
@@ -20,9 +20,3 @@ p, role4, TABLE, table1, OWNER, allow
p, role5, METALAKE, metalake1, SELECT_TABLE, allow
p, role5, METALAKE, metalake1, USE_CATALOG, allow
p, role5, TABLE, table1, SELECT_TABLE, allow
-
-g, user1, role5
-g, group1, role5
-g, user2, group1
-g, user3, role1
-g, user3, group1
