yuqi1129 commented on code in PR #13606:
URL: https://github.com/apache/gravitino/pull/13606#discussion_r4215768236
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1554,16 +1656,140 @@ private void clearRolePoliciesOnCacheRemoval(long
roleId) {
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 against role policies that are at least
as complete as the
+ * ones the request loaded.
+ *
+ * <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 checks of the request stay valid because they ran against
complete policies. This
+ * keeps each check's evaluation inside one read-locked section without
holding the lock across
+ * metadata or DB lookups.
+ *
+ * @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) {
+ 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, new ArrayList<>(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()) {
Review Comment:
Tracked in #13691, based on [this review
comment](https://github.com/apache/gravitino/pull/13606#discussion_r4215594650).
I agree that incomplete role policies cannot prove the absence of a DENY. We
will address reliable policy-completeness tracking and conservative
`hasDenyPolicy` behavior in a separate PR, including backoff expiry and
concurrent complete/partial replacements. This is separate from #13612's
single-flight work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
