yuqi1129 commented on PR #12231: URL: https://github.com/apache/gravitino/pull/12231#issuecomment-5238388351
> 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: > > ```shell > ./gradlew :server-common:test \ > --tests 'org.apache.gravitino.server.authorization.jcasbin.TestJcasbinAuthorizer' \ > --tests 'org.apache.gravitino.server.authorization.jcasbin.TestJcasbinLoadedRolesCache' \ > -PskipITs > ``` > > Result: `BUILD SUCCESSFUL`. Modified. -- 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]
