Nerzal / gocloak

golang keycloak client
Apache License 2.0
1.03k stars 283 forks source link

fix(client): correct serverinfo endpoint #382

Closed bastianccm closed 1 year ago

bastianccm commented 2 years ago

Hi @Nerzal,

I assume the serverinfo broke with the latest update, it should be one ServerInfoRepresentation and the path should be /serverinfo, not /realms.

Let me know if I'm missing something here?

Best and thanks for all the work, Basti

Nerzal commented 1 year ago

I guess the test failure is unrelated i'll have to have a closer look at that

bastianccm commented 1 year ago

Looks like these tests: https://github.com/Nerzal/gocloak/blob/18f0f3d14cb8dd31a49d357ca37fab0aa8ca337f/client_test.go#L625-L660

As they are marked as parallel, it could be a race condition somehow?

bastianccm commented 1 year ago

See #396 which should resolve the test issues

codecov[bot] commented 1 year ago

Codecov Report

Merging #382 (dfc9b76) into main (c59ba6e) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #382   +/-   ##
=======================================
  Coverage   77.14%   77.14%           
=======================================
  Files           4        4           
  Lines        2140     2140           
=======================================
  Hits         1651     1651           
  Misses        333      333           
  Partials      156      156           
Impacted Files Coverage Δ
client.go 75.42% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

bastianccm commented 1 year ago

@Nerzal i assume the tests failed because keycloak was not yet ready, maybe you can run them again? I rebased the branch so it should work actually

Nerzal commented 1 year ago

Tests do now run, but as this is a breaking change, we'd have to bump the major version. Do you know which keycloak versions this does support?

bastianccm commented 1 year ago

Actually it broke in Gocloak v12, as in v11 it was working just fine: https://github.com/Nerzal/gocloak/blob/v11.2.0/client.go#L256

I think it has been part of keycloak "ever since", it was introduced 2014 in this commit: https://github.com/keycloak/keycloak/commit/97897cab1db14dff2b823e24699558fb7f91860c (Keycloak 1.0-alpha-2)

Nerzal commented 1 year ago

I'm not 100% sure what to do with this. We could bump the major version and say, that we only support keycloak v12+ from now on.

bastianccm commented 1 year ago

@Nerzal i think we are mixing things up, it broke in gocloak v12, Keycloak has this API since v1. While the gocloak api will break, it's a regression, it worked up to v11, and broke with the refactoring. This is gocloak v11: https://github.com/Nerzal/gocloak/blob/9556250acd6a20d04682540e7a40230daaac9ca7/client.go#L256 This is gocloak v12: https://github.com/Nerzal/gocloak/blob/18f0f3d14cb8dd31a49d357ca37fab0aa8ca337f/client.go#L270

The serverinfo was never an array, I think a typo in gocloak made it one?

Nerzal commented 1 year ago

Okay, i'll have a look into that on Monday.