feat: sync policies with casbin redis watcher ex plugin#2159
Closed
caro3801 wants to merge 20 commits into
Closed
Conversation
caro3801
changed the title
…loseable - Replace Enforcer with SyncedEnforcer for thread-safe concurrent policy reloads - Add Authorizer(adapter, Watcher) constructor to support policy change notifications - Add Authorizer(adapter, reloadIntervalMs) constructor for polling-based auto-reload - Implement Closeable to allow stopping the auto-load background thread - Pin jcasbin to 1.96.0 to override transitive downgrade from jcasbin-redis-watcher-ex:1.1.0
Replace bind(Authorizer.class) with a @provides @singleton method that creates a RedisWatcherEx-backed Authorizer when busType=REDIS, falling back to polling-interval Authorizer otherwise.
…n up imports - Remove unused Provider<RedissonClient> from provideAuthorizer signature - Use REDIS_ADDRESS_OPT/DEFAULT_REDIS_ADDRESS constants instead of hardcoded strings - Move RedisURI, WatcherOptions, RedisWatcherEx to top-level imports - Add comment documenting ignoreSelf=false tradeoff - Fix spurious blank line inside test_authorizer_injectable_and_singleton_in_memory_mode
…herEx in test Register the Redis-watcher Authorizer with addCloseable() for consistent lifecycle. Update test mock from Watcher to WatcherEx and verify updateForAddPolicy() which jcasbin invokes instead of update() when the watcher implements WatcherEx.
jcasbin-redis-watcher-ex 1.1.0 pulls in lettuce-core 6.1.5, which transitively brings Netty 4.1.68. This conflicts with Redisson (4.1.90, used by extract-lib) and AWS SDK (4.1.126, used by datashare-api), causing a NoSuchFieldError on NetUtil.NETWORK_INTERFACES at runtime (field added after 4.1.68). Excluding netty-common/handler/transport from the lettuce transitive tree lets Maven resolve all Netty artifacts to 4.1.126.Final from the AWS SDK path, which satisfies both Redisson and Lettuce. Fixes RedisBlockingQueueTest.
Mirror the watcher branch's coverage: assert that a 0 ms reload interval starts no auto-load thread and that close() is a safe no-op in that case.
caro3801
force-pushed
the
feat/casbin-redis-watcher-ex
branch
from
da51050 to
093c517
Compare
…lback After a grant or revoke, the updated user is now pushed to the session store (usersWritable.saveOrUpdate) so that GET /api/users/me reflects the new project list immediately on the next page reload, without waiting for session expiry. Also adds the UsersWritable fallback in requireUser() so grant/revoke work for users whose account exists in the auth provider (e.g. UsersInRedis) but not yet in the SQL repository.
UsersInRedis.saveOrUpdate always stamped transaction.expire() with this.ttl, which defaults to 1 second in the CLI process. Running `project grant` called appendProjectToInventory → saveOrUpdate and overwrote a manually-provisioned (no-TTL) Redis user entry with a 1-second expiry, logging the user out immediately after the grant. Fix: read the key's current TTL before writing. If it is -1 (persistent, no expiry), skip the expire call. Only apply sessionTtlSeconds for new keys (-2) or keys that already carry a session TTL.
The previous fix skipped expire() only for persistent keys (ttl=-1), but still applied this.ttl=1s for keys with an existing session TTL (ttl>0), logging out OAuth2 users seconds after a project-grant. Replace the -1 guard with an extend-only rule: - new key (ttl=-2): set this.ttl (initial session creation) - active session (ttl>0): set max(existing, this.ttl) — never shorten - persistent key (ttl=-1): omit expire (SET already cleared it) This also preserves the server-side OAuth2 session-refresh behaviour: on re-login the server's this.ttl (e.g. 600s) will always be ≥ any remaining TTL, so the session is refreshed as expected.
The TTL read (jedis.ttl) was outside the MULTI/EXEC block. A concurrent OAuth2 login could create the key with a long TTL after we read -2, then our EXEC would stamp EXPIRE=1s on the live session, logging the user out. Fix: WATCH the key before reading the TTL. EXEC returns null when the key is modified between WATCH and EXEC; the loop retries with a fresh TTL read so the extend-only rule is applied on consistent data. The try/finally discard() cleans up an open MULTI on unexpected errors.
…xception When project grant fails because a user exists only in Redis (OAuth2 session before first grant) but --authUsersProvider=UsersInRedis was not passed, the error message now reads: user 'alice' not found; OAuth2 sessions exist only in Redis; pass --authUsersProvider=UsersInRedis The hint is attached only when usersWritable is not UsersInRedis, i.e. when Redis was never consulted during the lookup. A plain "not found" is kept for the case where Redis was consulted and still empty.
jcasbin catches exceptions from WatcherEx.updateFor*() inside notifyWatcher() and calls Util.logPrint() — INFO-level, gated on enableLog, no stack trace. A Redis outage during project grant exits 0 with no operator-visible warning. Fix: SafeWatcherEx wraps RedisWatcherEx and catches exceptions in all updateFor*() methods before jcasbin's catch, emitting a WARN with context. Exceptions are swallowed (not rethrown) because the Casbin SQL write already committed; rethrowing would unwind casbinWithRollback and corrupt the inventory/Casbin consistency invariant.
A server that loses Redis connectivity during a watcher-based deployment misses policy publications and stays stale indefinitely. Adding a periodic reload as a backstop guarantees convergence after the partition heals, without sacrificing the low-latency watcher path. - Authorizer gains a public startAutoLoadPolicy(long intervalMs) method (mirrors the private call in the interval constructor). - CommonMode.provideAuthorizer (Redis branch) calls it after wiring the watcher, using the same policyReloadInterval property as the interval-only branch. A value of 0 (the default) leaves the backstop disabled, preserving existing behaviour for operators who haven't configured an interval.
… for Redis-only users Two correctness gaps introduced by the Redis inventory sync: 1. Redis not rolled back on Casbin failure (casbinWithRollback) appendProjectToInventory writes to both SQL and Redis before the Casbin call. If swapCasbinRole throws, casbinWithRollback only restored SQL. Result: SQL = original, Redis = updated, Casbin = no role — the user saw the project in their dashboard but every enforce() returned false. Fix: add usersWritable.saveOrUpdate(original) to the catch block, mirroring the existing repository.save(original) compensating write. 2. revokeIfExists silently skipped Redis-only OAuth2 users revokeIfExists called repository.getUser() directly and short-circuited to noop on null, never consulting the Redis fallback. A user granted via the new Redis-fallback path could therefore never be revoked via revokeIfExists. Fix: extract the SQL→Redis lookup into findUser() and use it in both requireUser() and revokeIfExists().
Three minor fixes: - UsersInRedis.saveOrUpdate: replace unbounded while(true) retry with a 10-attempt for loop; throw IllegalStateException on exhaustion so pathological contention is observable rather than an infinite spin. - CommonMode.provideAuthorizer: replace Long.parseLong with parseInt (already statically imported) to match the Integer.class type declared in DatashareCliOptions.policyReloadInterval(); result widens to long at the call site. - AuthorizerTest.java: add missing trailing newline.
Member
|
I believe we can close this PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative implementation of Redis-based Casbin policy sync using the
jcasbin-redis-watcher-exlibrary instead of a hand-rolled Redisson topic watcher. SwitchesAuthorizertoSyncedEnforcerand wraps the library'sRedisWatcherExin aSafeWatcherExshim that logs WARN on publish failures (jcasbin swallows them silently at INFO level). All session and project-admin correctness fixes from the companion branch are included.This branch exists for comparison against
feat/casbin-redis-watcherbefore deciding which approach to merge.What changed
pom.xml- addsjcasbin-redis-watcher-ex 1.1.0; excludes Lettuce's Netty to avoid classpath conflictAuthorizer- switches toSyncedEnforcer; addsWatcherconstructor,startAutoLoadPolicy,Closeable; restoresenableLogbehavior suppressed bySyncedEnforcer's constructorSafeWatcherEx- newWatcherExwrapper aroundRedisWatcherEx; catches allupdateFor*()failures and logs WARN (jcasbin'snotifyWatcher()catches these at INFO and returnsfalsewithenableLoggating, making failures invisible in prod)CommonMode- wiresnew SafeWatcherEx(new RedisWatcherEx(...))whenbusType=REDIS; registers authorizer withaddCloseable; callsstartAutoLoadPolicyProjectAdminServiceImpl/UserNotFoundException/UsersInRedis- same correctness fixes as the companion branch (Redis rollback,findUserfallback, WATCH retry, extend-only TTL)DatashareCliOptions/GlobalOptions---policyReloadIntervaloptionTests added
AuthorizerRedisSyncIntTest- integration test viaRedisWatcherEx: grant on instance A visible on instance BAuthorizerTest- watcher notification, zero-interval no-polling lifecycleUsersInRedisTest- WATCH retry loop, extend-only TTLCommonModeTest- wiring smoke test