yuqi1129 commented on code in PR #10908:
URL: https://github.com/apache/gravitino/pull/10908#discussion_r3165711186
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -89,6 +94,16 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
private Cache<Long, Optional<Long>> ownerRel;
+ /**
+ * Per-role policy index that mirrors what we hand to the JCasbin enforcers.
Lookups in the hot
+ * path go through this map so an authorize call costs O(roles_per_user)
hash probes instead of
+ * scanning every policy line in the enforcer. Lifecycle is tied to {@link
#loadedRoles}: entries
+ * are written before {@code loadedRoles.put} and removed from {@code
loadedRoles}'s removal
+ * listener so the two caches can never disagree on what a role grants.
+ */
+ private final ConcurrentHashMap<Long, Map<PolicyKey, Effect>> roleIndex =
+ new ConcurrentHashMap<>();
Review Comment:
Yes, there is a concurrency issue here indeed. The problem is not concurrent
put for the same role itself, since ConcurrentHashMap can tolerate that. The
issue is that roleIndex and loadedRoles are maintained as two separate states,
so load and invalidation are not atomic.
One possible race is:
1. Thread A sees loadedRoles does not contain role1 and starts loading
role1 privileges.
2. Thread B also loads or refreshes role1 and puts the latest index into
roleIndex, then puts role1 into loadedRoles.
3. A role privilege change happens and handleRolePrivilegeChange(role1)
invalidates loadedRoles, whose removal listener removes role1 from roleIndex.
4. Thread A finishes later and puts the old role index back into
roleIndex, then puts role1 into loadedRoles again.
After that, the stale role policy index may be resurrected even though the
role was invalidated.
Another possible race is that roleIndex is removed after it is put, but
before loadedRoles.put(roleId, true). Then loadedRoles says the role is loaded
while roleIndex no longer has the index, causing later authorization to skip
reload and return a false negative.
I will fix this by avoiding two independent states for the same role. A
better approach is to store the per-role policy index directly in the role
cache, so “loaded” and “policy index” are updated/invalidated as one cache
entry.
--
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]
