-
Notifications
You must be signed in to change notification settings - Fork 325
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 default/optional scopes first apply problem #594
fix: client default/optional scopes first apply problem #594
Conversation
Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
…keycloak into fix-openid-client-scopes
…& added error log for not found client Signed-off-by: Vlad Kirichenko <v.kirichenko@napoleongames.be>
…keycloak into fix-openid-client-scopes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this PR looks good, just had one question
@@ -232,7 +232,9 @@ func (keycloakClient *KeycloakClient) getOpenidClientScopes(realmId, clientId, t | |||
var scopes []*OpenidClientScope | |||
|
|||
err := keycloakClient.get(fmt.Sprintf("/realms/%s/clients/%s/%s-client-scopes", realmId, clientId, t), &scopes, nil) | |||
if err != nil { | |||
if err != nil && ErrorIs404(err) { | |||
return nil, fmt.Errorf("validation error: client with id %s does not exist", clientId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a particular reason that this is needed? normally, we check for 404 errors within the provider
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I saw a problem in the TestAccKeycloakOpenidClientDefaultScopes_validateClientDoesNotExist test, and saw that the validation is missing when getting for scopes. Therefore, I added it.
It's strange that this check didn't work here.
…keycloak into fix-openid-client-scopes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
Hello @mrparkers.
Changes in this PR gonna fix problem mentioned in #498.
Best regards,
Vlad