Handling of Id and ClientId in Keycloak.Net.Core/Clients/KeycloakClient.cs

[jurc11]

Hi,

in Keycloak.Net.Core/Clients/KeycloakClient.cs (and elsewhere where client info is required), there are methods that take string clientId as an input parameter to generate URLs to /admin/realms/{realm}/clients/.... For example, GetClientAsync, UpdateClientAsync, etc.

However, looking at the documentation of Keycloak, the API wants the Id property of Client, not ClientId. Because the parameter gets pasted into an URL, your code still works as long as the Id is supplied in the call, obviously, but the name of the parameter is misleading. Using ClientId in place of Id gets rejected with 400 Bad Request by the API (and Keycloak's own frontend uses the Id in the relevant links, as well).

Is the name of the parameter deliberate? It's very easy to confuse it with Client.ClientId.

[societ-campbellwray]

Hi @jurc11,

I will admit that I have also found this confusing. The use of clientId was carried over from the project that this one was forked from, so it was established long before I was involved in the project and I can't really comment on why the name was chosen.

It's worth noting that this pattern is present in most of the methods in this library, e.g. GetUserAsync(string realm, string userId) which in the Keycloak documentation is GET /admin/realms/{realm}/users/{id}. If I had to guess as to why this pattern is used I would assume that it was created to avoid confusion when a method accepts multiple Ids, for example DeleteUserGroupAsync(string realm, string userId, string groupId) could be confusing with the signature DeleteUserGroupAsync(string realm, string id, string groupId) as it might not be clear what the id parameter represents in this case (the User or the Group). The Keycloak documentation is able to get around this by showing the parameter as it is used in the request URL /users/{id}/groups/{groupId} which makes it pretty obvious that the id parameter is the Id of a User, unfortunately this is not true in C# methods.

This pattern works fine in most cases, but as you discovered the main disadvantage comes with the Client methods, as Clients have two Ids (ClientId and Id), so it is not clear which one clientId is intended to represent.

The two solutions that I can think of to resolve this are:

1. We add XML comments to the Client methods indicating that clientId should be the Id of the Client, not the ClientId
2. Change all of the clientId, userId, etc. to just id, which has the potential to cause confusion as described in the above example, so we fix one source of confusion but add another.

Do you have any other ideas as to how this might be resolved?

[jurc11]

Hey,

I did figure out the naming pattern shortly after posting the original post. You are correct to point that out and I agree w.r.t. consistency. The naming scheme makes sense in light of that, except in this one case.

Solution (1) is a good idea, it would be at least something marking the ambiguous name. One other solution that comes to mind is using the same approach as with the Realm._Realm property. There, the underscore is used to circumvent the "property with the same name as the type" issue. Here, you could name the property as ClientClientId or simply _ClientId, directly on the Client type, thus leading to the method parameter being named clientClientId or _clientId. Well, at least the former, the latter seems a bit stupid. Or just renaming the parameter, but not the actual property. That's inconsistent but possibly the most understandable.

I'll be in the process of building our KC related solution over the next week or two and I'll probably have a better a better understanding of the library by then. I may be able to suggest something better then. Given the low quality of suggestions above, doing nothing and closing this issue might be sensible, even.

[societ-campbellwray]

It looks like Keycloak has switched to using the term client-uuid in their documentation, so this will probably be the direction we take with this ticket when I have some time to go through and rename everything.