Better caching for federated users #35726
Conversation
| @Override | ||
| public Stream<UserModel> searchForUserStream(RealmModel realm, String search) { | ||
| return getDelegate().searchForUserStream(realm, search); | ||
| return getDelegate().searchForUserStream(realm, search).map(u -> updateCache(realm, u)); |
There was a problem hiding this comment.
I have a little bit of fear of the scenario, when user storage provider returns some way of "lightweight" user from the "search" operations.
For example some user-storage implementation myUserStorageProvider where method searchForUserStream returns UserModel instances with only filled id and username. As it is an assumption that if admin is interested about particular user john, he will then call subsequent call getUserById, which is then supposed to return "full" user with all the attributes filled.
If we do the change in this PR, it means that "lightweight" users will be added to the cache, which could then cause issues as subsequent getUserById will then also returns this "lightweight" user...
Could this be an issue?
If it could be an issue, the alternative might be to invalidate the users from cache, which are returned by the search operations? This will fix the "inconsistency" problem . At the same time, it will have performance impact (the "search" operations are probably not called very often as those are "admin" operations AFAIK, so not sure how much bad is the performance impact...)
There was a problem hiding this comment.
Yes, that could be an issue. In that case maybe we should just return the cached user instead of the returned user in the store. Not caching anything in searches.
There was a problem hiding this comment.
Yes, that is also an option. And IMO it is better than current behaviour with "inconsistent" results returned from search operations and from operations like getUserById . So my vote is to go that path (if @pedroigor and @sguilhen are fine with that).
There was a problem hiding this comment.
Modified searchForUserStream to return the cached entry if present and not cache new entries.
Closes keycloak#35637 Signed-off-by: rmartinc <rmartinc@redhat.com>
There was a problem hiding this comment.
Changes LGTM
@pedroigor Assigning PR to you, so you can merge if you are OK with the changes (As it is more a core-iam area).
Closes #35637
The PR performs mainly two changes in the infinispan caching to make it consistent with user federation providers (like ldap):
UserStorageManager(used in ldap when import users is ON) does not enforce the validation if the returned user is cached. The cached user will be valid for the configured cache time and will be validated from ldap when it needs to be refreshed (cache time is expired). The users were already cached (not validated) for other methods likegetUserByIdorgetUserByUsernameso I think it's OK.searchForUserStreammethods into the cache. The DB implementation already performs this, so we are already doing this in general. This point makes a little change in theaddRevisionedmethod inCacheManager(putis used instead ofputForExternalRead, because the latter only adds the user if not present likeputIfAbsent). This change is to cache the new user refreshed from the external storage (we can also return the cached one, but I think that this makes more sense, the user is refreshed from the provider so it's up to date). As this cache is always a local cache, I think it's OK and it's not a performance drawback.The change (in theory) improves the search for users in ldap in two ways. When importing into the DB, the search for users does not validate the cached users (avoiding the ldap access for each user if cached). When not importing, the search in the ldap caches the returned users from ldap for other operations. It can have a performance drawback too, because there are some minor changes. So probably we need to test ldap performance in general. Is there some tests/CI we can check for this?
It's a draft because I don't know if we want to follow this path.
@mposolda @pedroigor @sguilhen WDYT?