Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client): correct serverinfo endpoint #382

Merged
merged 1 commit into from
May 3, 2023

Conversation

bastianccm
Copy link
Contributor

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
Copy link
Owner

Nerzal commented Nov 7, 2022

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

@bastianccm
Copy link
Contributor Author

Looks like these tests:

gocloak/client_test.go

Lines 625 to 660 in 18f0f3d

func Test_GetUserInfo(t *testing.T) {
t.Parallel()
cfg := GetConfig(t)
client := NewClientWithDebug(t)
SetUpTestUser(t, client)
token := GetUserToken(t, client)
userInfo, err := client.GetUserInfo(
context.Background(),
token.AccessToken,
cfg.GoCloak.Realm,
)
require.NoError(t, err, "Failed to fetch userinfo")
t.Log(userInfo)
FailRequest(client, nil, 1, 0)
_, err = client.GetUserInfo(
context.Background(),
token.AccessToken,
cfg.GoCloak.Realm)
require.Error(t, err, "")
}
func Test_GetRawUserInfo(t *testing.T) {
t.Parallel()
cfg := GetConfig(t)
client := NewClientWithDebug(t)
SetUpTestUser(t, client)
token := GetUserToken(t, client)
userInfo, err := client.GetUserInfo(
context.Background(),
token.AccessToken,
cfg.GoCloak.Realm,
)
require.NoError(t, err, "Failed to fetch userinfo")
t.Log(userInfo)
require.NotEmpty(t, userInfo)
}

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

@bastianccm
Copy link
Contributor Author

See #396 which should resolve the test issues

@codecov
Copy link

codecov bot commented Jan 5, 2023

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%> (ø)

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

@bastianccm
Copy link
Contributor Author

@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
Copy link
Owner

Nerzal commented Jan 9, 2023

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
Copy link
Contributor Author

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: keycloak/keycloak@97897ca (Keycloak 1.0-alpha-2)

@Nerzal
Copy link
Owner

Nerzal commented Apr 13, 2023

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
Copy link
Contributor Author

@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:

func (client *gocloak) GetServerInfo(ctx context.Context, accessToken string) (*ServerInfoRepesentation, error) {

This is gocloak v12:
func (g *GoCloak) GetServerInfo(ctx context.Context, accessToken string) ([]*ServerInfoRepresentation, error) {

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

@Nerzal
Copy link
Owner

Nerzal commented Apr 14, 2023

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

@Nerzal Nerzal merged commit 70f6ad9 into Nerzal:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants