jarredhj0214 commented on code in PR #13364:
URL: https://github.com/apache/gravitino/pull/13364#discussion_r4059059766
##########
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:
Thanks for catching this. You are right: accessing `loadedRoles` under the
diagnostic read lock can synchronously trigger the Caffeine removal listener
and attempt to acquire the write lock.
I updated the diagnostic path so the read lock only covers the enforcer
snapshot (`boundRoles` + per-role policy counts). The subsequent
`loadedRoles.getIfPresent(...)` calls now run after releasing
`rolePolicyLock.readLock()`, so an expiration callback can safely enter
`clearRolePoliciesOnCacheRemoval` and acquire the write lock.
I also added a timeout regression test that simulates the synchronous
expiration cleaner taking the same write lock after the diagnostic read
snapshot.
Validated with:
```bash
./gradlew :server-common:test --tests
org.apache.gravitino.server.authorization.jcasbin.TestJcasbinAuthorizer
```
--
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]
