[keycloak-user] Why doesn't UserCacheSession try to cache the new user model after invalidation?

[Bill Burke]

On 3/21/17 12:56 AM, Jared Blashka wrote:
> I've been looking into the UserCacheSession behavior after we updated to
> 2.5.5.Final and I'm seeing some strange behavior related to cached
> UserModel invalidation.
>
> After a UserModel in the user cache is flagged for invalidation, either
> because there was some update to the user model (e.g. Adding a required
> action when setting up OTP credentials) or the cache's invalidation time
> limit was reached, every single subsequent call within that thread to
> getUserById or getUserByUsername will delegate that call to the Storage
> Provider responsible for that user without caching the result. And it isn't
> until the *next* thread that tries to fetch that user from the infinispan
> cache that the result is actually cached again.
>
> Is this intended behavior?

Yes.  It is assumed that the user is in an uncommitted state, so it 
doesn't try to recache it.

>
> In my testing I have an OIDC client and a SAML client for my 2.5.5 server.
> If the OIDC client makes a token refresh request after the user has passed
> its lifespan time it triggers 7 (each UserPropertyMapper makes a separate
> userSession.getUser call) separate invocations of my Storage Provider's
> getUserById method, each one triggering a query against our data store. If
> I then try to access a second client (my SAML client) Keycloak still
> doesn't have the user model cached and delegates the call to my storage
> provider yet again, but caches the result this time.
Each HTTP request is a separate transaction.  If there is an 
invalidation (for whatever reason) within that transaction then the 
runtime assumes that the user loaded within that transaction is in an 
uncommitted state.  Transaction is not committed until the end of the 
http request.


> Are our custom Storage Providers expected to cache the results of our
> getUserById/Username calls for situations like these? I would think that
> UserCacheSession would more gracefully handle cache invalidation so that
> every customer with their own Storage Provider wouldn't have to manage
> their own additional cache layer on top of what Keycloak is already
> providing.
Your provider is not responsible for caching.

> My knowledge of infinispan is pretty lacking, so is there a reason that the
> UserCacheSesssion.getUserById call short-circuits to the delegate with:
>
> if (isRegisteredForInvalidation(realm, id)) {
>      logger.trace("registered for invalidation return delegate");
>      return getDelegate().getUserById(id, realm);
> }
>
>
> rather than caching the result with something like:
>
> if (invalidations.contains(id)) {
>      Long loaded = cache.getCurrentRevision(id);
>      UserModel delegate = getDelegate().getUserById(id, realm);
>      adapter = cacheUser(realm, delegate, loaded);
>      invalidations.remove(id)
>      managedUsers.put(id, adapter);
>      return adapter;
> }
>
The user might be flushed to the database, but the transaction might not 
have been committed.

>
> With this behavior we're actually seeing more activity against our backend
> server than if we were just using the NO_CACHE policy.
Isn't that a problem with your cache policy?  If your cache policy is 
too short, then of course a cache won't be useful.  I think you know 
this already, but, user cache is an invalidation cache, not a 
distributed one.  This means that each node has its own cache of the 
user.  So, if refresh token hits a different server, that server may 
have to query the database if that user hasn't been cached yet on that node.

I think we could improve things by differing from invalidation and 
eviction.  Invalidation would be due to an update and couldn't reload 
the cache within the same transaction.  Eviction would allow the user 
cached if its accessed again in the same transaction.

Thanks,

Bill