RealmsAdmin.AdminEvent.Time int? causes json deserialisation error
Closed this issue · 8 comments
Whilst trying to retrieve a list of the admin events on a keycloak instance using GetAdminEventsAsync(realmName), the following error is provided:
JsonReaderException: JSON integer 1660122818885 is too large or small for an Int32. Path '[0].time', line 1, position 22.
After a little digging around it seems that this should most likely be a long? rather than an int?.
I am happy to create a branch for this to make that one small change but it will likely also affect other models that use the same way of working with time.
Hi sorry for the delay could you create a pull request?
I can, whilst I do that, is there a way I could convince you to also implement interfaces for the keycloak client since we have a need to write unit tests against a class that is using the keycloak client but we cant mock it for various reasons and having interfaces would make things much easier for us and I should think quite a number of other people.
Sadly i currently don't use this lib, so I'm not sure when i will manage to get time to add interfaces, but you could check if there are any pull requests in the original repo, if that's the case link it here an I'll merge it if not please provide a separate pull request for that.
@ab-zackdemarco I have taken over ownership of this project from AnderssonPeter.
I looked into adding an interface for the KeycloakClient class as I too would like to make it testable, however my concern is that I am not sure how to best modify the KeycloakClient class to handle the various constructors.
I am aware that you can use the following to inject a KeycloakClient with the constructor properties set:
builder.Services.AddSingleton<IKeycloakClient, KeycloakClient>(provider => new KeycloakClient(url));
However if the _username, _password, etc. change (e.g. depending on the user), then these properties cannot be set when the application is initialized. Currently I am thinking that the best approach would be to add a series of methods named Configure() which match the constructors, so that you could configure the KeycloakClient after it had been injected. I would also add a check to GetBaseUrl() to ensure that the client had a valid url, etc.
Would this approach work for you?
@sps-campbellwray
This makes me exceedingly happy, though in addition to the Configure methods you were mentioning, will you also be able to add in DI for configuration using something like IOptions<KeycloakClientOptions>?
The reason I ask is that in some use cases, such as service start up, we would want to use a configured "service" account to interact with keycloak so being able to use appsettings and pass in the configuration from there would be immensely helpful.
@sps-campbellwray
Something else I recall was that the nuget package was missing some of the api definitions. we have a local copy that we made adjustments to so I can look back through our local git commits and pick out the missing endpoints for you if you needed?
@ab-zackdemarco do you mean it is missing methods for some of the API endpoints? If so, I have noticed this too, it is a little tricky to track them all down, but we have been adding them as we find them, so far it has mostly been individual properties that are missing, but it wouldn't surprise me if there were whole methods too.
I will look through my code, but so far what i can see that was missing was to do with getting the Client Protocol Mappers
public async Task<IEnumerable<ProtocolMapper>> GetClientProtocolMappersAsync(string realm, string clientId, CancellationToken cancellationToken = default) { var response = await GetBaseUrl(realm) .AppendPathSegment($"/admin/realms/{realm}/clients/{clientId}/protocol-mappers/models") .GetJsonAsync<IEnumerable<ProtocolMapper>>(cancellationToken) .ConfigureAwait(false); return response; }
We also changed some of the return types from things like AddClientRoleMappingsToUserAsync so rather than returning a boolean it returns the HttpResponseMessage which then allows us to do error handling in our services.
I also spotted that it was missing code for DeleteRoleByNameAsync to remove programatically generated roles that need to be seeded into keycloak.