12167 Removed n+1 queries retrieving users with brief representation

[sschu]

Implements #12167

[sschu]

The basic idea to remove the n+1 query on the user attributes is to make the Infinispan caching layer aware of the fact that different user attributes might be stored in different places so it can make a decision how to request a certain attribute instead of always requesting all attributes. I am not 100% sure this change is sound, but it is the only way I found to actually make this distinction.

[sschu]

The basic idea to remove the n+1 query on the user credentials is to not always load credentials when the user is put into the cache but only when the password is actually requested. The change I introduced might also fix a potential bug as I found no code that actually updates cached values down the road after the password was cached when the user was loaded initially.

[sschu]

The basic idea to remove the n+1 query loading group memberships of a user is to only check for fine-grained permissions via groups if fine-grained permissions are actually enabled. However, this does not remove the n+1 query when they are enabled. As discussed in the linked issue, it would be possible to add a parameter to the request so returning access information can be made optional or suppressible.

[sschu]

I don't think the failing QuickStart test is due to my changes.

[stianst]

@hmlnarik can you or someone from the store team take a look at this one?

[hmlnarik]

Thank you for the PR.

Could you please extract the changes in services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java into a separate PR? That one seems viable to me.

#7080 moved the first/last name, username and email from properties into attributes. The changes in model/infinispan seem to refactor the code exactly in the other direction. Thus I suggest keeping UserAdapter without changes, and rather than introducing firstName and lastName properties in CachedUser, introduce a predefined set of attribute names that would be cached, while every other attribute would require the delegate. This set would contain UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.EMAIL and UserModel.USERNAME.

Changes in services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java introduce another direct dependency from services to model-infinispan, and thus are incompatible with #10279. This change cannot be accepted, as the direction is to break the direct dependecies.

[sschu]

@hmlnarik Thanks for your feedback!

Thank you for the PR.

Could you please extract the changes in services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java into a separate PR? That one seems viable to me.

Will do. What do you think about my suggestion to make returning access information optional or suppressible?

#7080 moved the first/last name, username and email from properties into attributes. The changes in model/infinispan seem to refactor the code exactly in the other direction. Thus I suggest keeping UserAdapter without changes, and rather than introducing firstName and lastName properties in CachedUser, introduce a predefined set of attribute names that would be cached, while every other attribute would require the delegate. This set would contain UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.EMAIL and UserModel.USERNAME.

I'll try that.

Changes in services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java introduce another direct dependency from services to model-infinispan, and thus are incompatible with #10279. This change cannot be accepted, as the direction is to break the direct dependecies.

Honestly, I do not understand this part. Where exactly am I introducing a dependency that was not there before? Is it this part?
if ((passwords !=null) && (user instanceof CachedUserModel)) {
CachedUserModel cached = (CachedUserModel) user;
cached.getCachedWith().put(PASSWORD_CACHE_KEY, passwords);
}
IMHO, it does two things: First, it caches the credentials on first access (in contrast to before, where it was cached when the user was put into the cache). Second, it also adds it to the cache in case the cached credentials were invalidated. My assumption is that credentials can be invalidated independent of the user. If that is true, it could otherwise happen that updated credentials are never put into the cache because in the old code, that only happens when the user is put into the cache. Could happen when the user just changes his password. But I might be wrong on this one.

[hmlnarik]

Honestly, I do not understand this part. Where exactly am I introducing a dependency that was not there before? Is it this part?

if ((passwords !=null) && (user instanceof CachedUserModel)) {
     CachedUserModel cached = (CachedUserModel) user;
     cached.getCachedWith().put(PASSWORD_CACHE_KEY, passwords);
}

At this moment, the dependency is there. However it collides with #10279.

The aim of #10279 is to provide a clear border between services which should have no knowledge of storage internals, and storage layer, so that the storage can then be switched to a new implementation without disrupting services.

org.keycloak.models.cache package discloses internals of the storage to services. Any such disclosure couples tightly services to a particular implementation of a storage. Thus introducing such a dependency is to be avoided at this moment.

The separation has started in #11933 PR, and this initial part is close to completion. While it does not contain moving the CachedUserModel and org.keycloak.models.cache package, that is on the radar already.

Hope this helps.

[sschu]

@hmlnarik Thanks for the explanation. Then I would leave this part out for now and revisit it once the storage is separated.
Still, what do you think about my proposal to make fetching the access rights information optional/suppressible on the admin api?

[hmlnarik]

@hmlnarik Thanks for the explanation. Then I would leave this part out for now and revisit it once the storage is separated.

Thank you

Still, what do you think about my proposal to make fetching the access rights information optional/suppressible on the admin api?

This I believe relates to the following comment:

The basic idea to remove the n+1 query loading group memberships of a user is to only check for fine-grained permissions via groups if fine-grained permissions are actually enabled. However, this does not remove the n+1 query when they are enabled. [...]

Let me check - did you mean when they are disabled? If so, I believe this is exactly where the services should be able to let the store know the preference of obtaining the details of a particular user, e.g. based on whether fine-grained permissions are enabled, and that sounds like a good idea - as long as it is done in a store-independent way.

As discussed in the linked issue, it would be possible to add a parameter to the request so returning access information can be made optional or suppressible.

I would be interested in the details here. Maybe I'm misinterpreting the suggestion as it sounds that the client (application) would then drive whether the groups are fetched and evaluated, and that would mean a potential for a vulnerability. Perhaps you meant a specific endpoint and a specific use case.

[sschu]

Let me check - did you mean when they are disabled? If so, I believe this is exactly where the services should be able to let the store know the preference of obtaining the details of a particular user, e.g. based on whether fine-grained permissions are enabled, and that sounds like a good idea - as long as it is done in a store-independent way.

I really meant there is still an n+1 query when fine-grained permissions are enabled and the requesting user does not have manage-users for the realm. In that case, group memberships of every loaded user are requested to find out whether the requesting user can manage them via group fine-grained permissions.

I would be interested in the details here. Maybe I'm misinterpreting the suggestion as it sounds that the client (application) would then drive whether the groups are fetched and evaluated, and that would mean a potential for a vulnerability. Perhaps you meant a specific endpoint and a specific use case.

Yes, I mean a very specific use-case: requesting users via admin API with brief representation. That does normally not return any group membership information on a user. However, it does return added information on whether the requesting user can manage the loaded user. That information is mostly used in the admin console to decide whether the "Edit" button should be shown. But if you just want to fetch users outside of the API, that information is most likely not needed (and it is potentially expensive to retrieve because it loads all group memberships). Please also see the screenshot:

[sschu]

@hmlnarik I finally managed to strip this one here down to the changes for reading the user attribute.

You suggested to keep the UserAdapter without changes. I don't really get how this is supposed to work. The fact that the user adapter loads all attributes from the cache and subsequently the storage by triggering the LazyLoader in order to just return one attribute is the very core of the problem I am trying to fix. That just does not seem right to me.

The only way to kind of keep this logic would be to have CachedUser not return a MultivaluedHashMapof attributes but the underlying LazyLoader. However, that would still mean some changes to the UserAdapter.

[sschu]

@hmlnarik So I experimented a bit more with this and it is possible to have the eager loaded attributes attributes in a collection. This would look somewhat like this in the constructor when initializing the CachedUser:
this.eagerLoadedAttributes = new MultivaluedHashMap<>(); this.eagerLoadedAttributes.putSingle(UserModel.USERNAME,user.getUsername()); this.eagerLoadedAttributes.putSingle(UserModel.FIRST_NAME,user.getFirstName()); this.eagerLoadedAttributes.putSingle(UserModel.LAST_NAME,user.getLastName()); this.eagerLoadedAttributes.putSingle(UserModel.EMAIL,user.getEmail()); this.lazyLoadedAttributes = new DefaultLazyLoader<>(userModel -> new MultivaluedHashMap<(userModel.getAttributes()), MultivaluedHashMap::new);
And fetching an attribute would look like this:
public String getFirstAttribute(String name, Supplier<UserModel> userModel) { if(eagerLoadedAttributes.containsKey(name)) return eagerLoadedAttributes.getFirst(name); else return this.lazyLoadedAttributes.get(userModel).getFirst(name); }
However, I don't really see the point of doing this. The added flexibility is probably not needed and it also contradicts how these special attributes are handled in other places. For example MapUserAdapter contains pretty similar code:

keycloak/model/map/src/main/java/org/keycloak/models/map/user/MapUserAdapter.java

Line 92 in 5248815

} else if (UserModel.LAST_NAME.equals(name)) {

Also the infinispan.UserAdapter has special code to handle FIRST_NAME, LAST_NAME, USER_NAME and EMAIL, since it is part of the UserModel interface e.g. here:

keycloak/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java

Line 78 in ae7c01b

public String getLastName() {

so I don't know why the CachedUser that's also part of the Infinispan module should have special handling only for USER_NAME and EMAIL but not for FIRST_NAME and LAST_NAME.

WDYT?

[hmlnarik]

@hmlnarik So I experimented a bit more with this and it is possible to have the eager loaded attributes attributes in a collection. This would look somewhat like this in the constructor when initializing the CachedUser:

this.eagerLoadedAttributes = new MultivaluedHashMap<>();
this.eagerLoadedAttributes.putSingle(UserModel.USERNAME,user.getUsername()); 
this.eagerLoadedAttributes.putSingle(UserModel.FIRST_NAME,user.getFirstName()); 
this.eagerLoadedAttributes.putSingle(UserModel.LAST_NAME,user.getLastName()); 
this.eagerLoadedAttributes.putSingle(UserModel.EMAIL,user.getEmail());
this.lazyLoadedAttributes = new DefaultLazyLoader<>(userModel -> new MultivaluedHashMap<(userModel.getAttributes()), MultivaluedHashMap::new);

And fetching an attribute would look like this:

public String getFirstAttribute(String name, Supplier<UserModel> userModel) {
   if(eagerLoadedAttributes.containsKey(name)) return eagerLoadedAttributes.getFirst(name);
   else return this.lazyLoadedAttributes.get(userModel).getFirst(name);
}

I like this since it treats the special fields consistently. In the getFirstAttribute method I would say it needs also to be conditioned on this.updated == null, since otherwise the eagerLoadedAttributes could be invalid.

However, I don't really see the point of doing this. The added flexibility is probably not needed and it also contradicts how these special attributes are handled in other places.

It precisely to make the handling the special attributes consistent in the CachedUser which currently has inconsistent handling of username / email, and first / last name. With this change, all of the four fields would finally be treated as the attributes which is the desired state expected after user profile work.

For example MapUserAdapter contains pretty similar code:

Let's put aside map store adapters as those are not relevant in the context of legacy store and are under scrutiny (#10004), specifically see #10004 (comment) for more details.

so I don't know why the CachedUser that's also part of the Infinispan module should have special handling only for USER_NAME and EMAIL but not for FIRST_NAME and LAST_NAME.

Me neither 😃 These should be consistent - and that's what the code using the getFirstAttribute method enables.

[sschu]

@hmlnarik So I implemented the minimum changes to make this work. I think checking for updatedas you suggested is not necessary as this is done in the UserAdapter. Removing the getUsername() and getEmail() methods would have been a kind of a hassle since they are used by the UserCacheSessionwhich has no access to a ModelSupplierto correctly call getFirstAttribute(). I did not want to just call it providing null, I hope that's ok...

[sschu]

@hmlnarik Any feedback?

[hmlnarik]

@sschu , apologies for late response. Yes, now it is good to go. Thanks for the PR!