yuqi1129 commented on code in PR #13364:
URL: https://github.com/apache/gravitino/pull/13364#discussion_r4059019396
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1642,54 +1694,61 @@ private void diagnoseDenial(
}
try {
String userIdStr = String.valueOf(userId);
- List<String> boundRoles = allowEnforcer.getRolesForUser(userIdStr);
- if (boundRoles.isEmpty()) {
- LOG.debug(
- "Denied [{}, {}, {}, {}]: no role is bound to the user in the
allow enforcer",
- userIdStr,
- metadataType,
- metadataIdStr,
- privilege);
- return;
- }
+ rolePolicyLock.readLock().lock();
+ try {
+ List<String> boundRoles = allowEnforcer.getRolesForUser(userIdStr);
+ if (boundRoles.isEmpty()) {
+ LOG.debug(
+ "Denied [{}, {}, {}, {}]: no role is bound to the user in the
allow enforcer",
+ userIdStr,
+ metadataType,
+ metadataIdStr,
+ privilege);
+ return;
+ }
- List<String> rolesWithoutPolicies = new ArrayList<>();
- List<String> roleStates = new ArrayList<>(boundRoles.size());
- for (String roleIdStr : boundRoles) {
- int policyCount =
- allowEnforcer.getFilteredNamedPolicy("p",
POLICY_SUBJECT_FIELD_INDEX, roleIdStr).size();
- Optional<Long> loadedAt =
loadedRoles.getIfPresent(Long.parseLong(roleIdStr));
- roleStates.add(
- roleIdStr
- + "{loadedAt="
- + (loadedAt.isPresent() ? loadedAt.get() : "absent")
- + ", allowPolicies="
- + policyCount
- + "}");
- if (loadedAt.isPresent() && policyCount == 0) {
- rolesWithoutPolicies.add(roleIdStr);
+ List<String> rolesWithoutPolicies = new ArrayList<>();
+ List<String> roleStates = new ArrayList<>(boundRoles.size());
+ for (String roleIdStr : boundRoles) {
+ int policyCount =
+ allowEnforcer
+ .getFilteredNamedPolicy("p", POLICY_SUBJECT_FIELD_INDEX,
roleIdStr)
+ .size();
+ Optional<Long> loadedAt =
loadedRoles.getIfPresent(Long.parseLong(roleIdStr));
Review Comment:
This `getIfPresent` runs while `rolePolicyLock.readLock()` is held.
`JcasbinLoadedRolesCache` uses Caffeine with `.executor(Runnable::run)`; when
this access expires a role, its removal listener runs on the calling thread and
enters `clearRolePoliciesOnCacheRemoval`, which requests the same lock's write
side. A read-to-write upgrade cannot complete, so a denied request with DEBUG
diagnostics enabled can hang permanently and block subsequent policy reloads. I
reproduced the synchronous expiration callback with the project's Caffeine
2.9.3 dependency. Please avoid accessing `loadedRoles` under the read lock (or
use the reentrant write lock for this diagnostic), and add an expiration/denial
regression test with a timeout.
--
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]
