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

[Jared Blashka]

On Thu, Mar 23, 2017 at 12:26 PM, Bill Burke <bburke at redhat.com> wrote:

>
>
> 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.
>

This all makes sense now. You don't want to update users in the cache in
case the commit fails. Thanks

I just wish we weren't forced to make multiple queries to our external
datastore to get the same exact data in these situations when the only
thing that was modified on the user were attributes managed inside Keycloak.

Differentiating between invalidation and eviction would be very nice
though, if that change happened the large majority of our invalidations
today would become evictions.

Thanks for clarifying!
Jared


>
> Thanks,
>
> Bill
> _______________________________________________
> keycloak-user mailing list
> keycloak-user at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-user
>