KEYCLOAK-3410 Ease creating user with initial roles via REST

[thomasdarimont]

Previously creating a new user with a set of initial realm
and client roles involved sending multiple requests.

We now honor the provided realmRoles and clientRoles
information in the UserRepresentation sent to the
UserResource#create endpoint. This reduces the number of requests
needed to create a user with inital roles to 1.

For symmetry reasons we also return realmRole and clientRole
information via the UserResource#getUser(..) endpoint.

We also reduced the visibility of the UsersResource#updateUserFromRep to
private and removed changed it to be an instance method.
Wasn't called anywhere in the codebase - and shouldn't be called
by user code anyways.

Signed-off-by: Thomas Darimont thomas.darimont@gmail.com

[stianst]

We'll need tests added to the new testsuite:
https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java

Also, I'm not 100% sure convinced we should return the roles on get user. This could be expensive if there are a lot of roles. What we could do is to add a "fields" query parameter that allows expanding certain fields, but have them not fetched by default. For example:

/users/sdfasfdr?fields=realmRoles,clientRoles

What do you think?

[thomasdarimont]

Thanks for the feedback.
I can move the test(s) to the new test-suite.

I wonder when it would make sense to fetch only a part of the roles of a user...
Usually when one does a user look-up via the Keycloak AdminClient or via the REST interface the role information is quite often (about 90%) of the time fetched as the next step.
Also since this information will be cached the database cost will probably be negligible.

So I'd rather return the full roles information and care about the performance problem if it really shows up.

[stianst]

It's not about fetching part of the roles, it's about fetching all or nothing.

In terms of the admin console this doesn't solve anything as the admin console still does the separate fetch. Sure that can be fixed.

However, we have large amount of people using the rest apis directly. If you need to fetch users to do bulk changes to a large number of users this could slow things down considerably. Consider the fact that not all users are in the cache as they haven't logged-in for a while, or they did so on another node.

[thomasdarimont]

Okay, you are right - I also noticed that the admin-console (currently) fetches the user roles in a separate step.

I can rework the PR to not return the roles by default when asking for a user by id such that existing clients will continue to see the same performance characteristic as before.

However I think controlling the returned representation with a fields parameter isn't the best option IMHO since it gives a user the impression that there would be full control over all fields of a representation which is (probably) not the case. Additionally for consistency this would need to be supported by other resources as well. Additionally the Keycloak java admin client needs to be adjusted as well. However the last two points apply to all other means for controlling an entity representation.

A different approach would be to differentiate the representations by using dedicated media-types, perhaps something like:

• User Overview application/vnd.keycloak.user-overview+json for a user overview and `
  (User Overview would be the default and returns the "flat" user to preserve the current behavior)
• User Detail application/vnd.keycloak.user-detail+json
  (User Detail would containt the information for User Overview + additional details like roles etc.)

[stianst]

I don't like using different media-types for it and it's often quite fiddly to do from different frameworks. For example AngularJS $resource defaults to json and also knows how to parse json, while wouldn't know how to deal with a custom media-type.

Maybe fields is not the right name. What about "expand=roles" or "include=roles"? I can see it being useful for other things as well in the future. For example ability to fetch just the user. Ability to fetch the user with all roles, credential information, social links, federation link, etc..

[thomasdarimont]

Yes include=roles sounds better, but I think this should be discussed on the mailing list first in order to ensure a consistent view about this.

[stianst]

Sure - can you start a thread on mailing list?

[stianst]

@thomasdarimont I suggest we remove the addition of roles to the response, then we can merge this PR. Then create a separate JIRA to have roles included with the response and start a discussion on the mailing list how it should be done proposing include=roles as the way to go. WDYT?

[thomasdarimont]

Okay, good idea.

I'll update the PR (now) by not adding the roles in the response.
Yes - sorry that I didn't started a discussion on the ML about this yet.

Cheers,
Thomas

[thomasdarimont]

I removed the role information in the UserRepresentation via an explicit commit 2afec29 such that it can easily added again.

Problem is that the KeycloakTest.createNewUserWithRealmAndClientRoles test now fails since the transaction in which the role information is written is probably not yet commit to the database and the roles are then read within a different transaction.
With the old code the save / load of the role-information was performed in the same TX.
@stianst any hints on how this could be fixed?

[stianst]

Missed that the tests where added to the old testsuite, could you add the test to:
https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java

If you call close() on the response that creates the user the roles should be available when you send the request to fetch the roles. It's a bit annoying, but all methods that return a response in the admin client lib needs to be closed, otherwise the tx is not always done and also the http connection is not released back to the pool.

[thomasdarimont]

Meeeh

Response response = keycloak.realm(realmName).users().create(user);
response.close();

Doesn't help - the roles are still not returned by:

String userId = extractIdFromLocation(response);
List<String> clientRoles = keycloak.realm(realmName)
                                           .users().get(userId).roles().clientLevel(APP_CLIENT_ID)
                                           .listEffective().stream().map(RoleRepresentation::toString)
                                           .collect(Collectors.toList());

[stianst]

Hmm... it should work, unless you've actually found a cache issue. Can you try disabling the realm and user caches and see if it works? If that doesn't work maybe the roles aren't added? Or not fetched properly?

[vikramadeshpande]

@thomasdarimont
I need roles assigned to a user with user info. I have used code snippet provided by you. And I am getting not found - 404.

Currently I am using keycloak 2.4 libarary.

Will you please help me know, what to do To fetch user(s) with it roles?

[baddlan]

The docs say it's possible, but this issue says it's not.

[dmitrymikheiev]

Are you planing to finish this enhancement?

[eduardev]

Why was this abandoned? I don't get how this is not priority :(

[sokil]

hello from September 2022