roryqi commented on PR #12231:
URL: https://github.com/apache/gravitino/pull/12231#issuecomment-5237956103
Thanks for the fix. I reviewed the role-policy loading path and the new
tests; the main approach looks sound to me: resolving metadata ids before
taking `rolePolicyLock`, then serializing `clear -> apply -> record`, closes
the loaded-marker/enforcer-state race without holding the lock across DB
lookups.
I only noticed two non-blocking follow-ups:
1. `authorizeByJcasbin` returns early for active-role narrowing
(`narrowByActiveRoles && !activeRoles.isAll()`), so denied narrowed requests do
not call `diagnoseDenial`. That does not affect the core fix, but it means the
new denial diagnostics will miss one request path.
2. The cache architecture comment in `JcasbinAuthorizer` still says loaded
role policies use access-based TTLs through `JcasbinLoadedRolesCache`; this PR
changes that cache to write-based TTL, so the comment should be updated.
I also verified the focused tests locally on the PR head:
```bash
./gradlew :server-common:test \
--tests
'org.apache.gravitino.server.authorization.jcasbin.TestJcasbinAuthorizer' \
--tests
'org.apache.gravitino.server.authorization.jcasbin.TestJcasbinLoadedRolesCache'
\
-PskipITs
```
Result: `BUILD SUCCESSFUL`.
--
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]
