-
Notifications
You must be signed in to change notification settings - Fork 315
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
Improve client scope #320
Improve client scope #320
Conversation
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.
Thanks for the PR! I just had a couple of changes I'd like to suggest.
Default: true, | ||
}, | ||
"gui_order": { | ||
Type: schema.TypeString, |
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.
Any reason for not using schema.TypeInt
for this attribute?
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.
Keycloak REST API treats it as string type. Also, to remove the attribute, empty string ("") is used in admin console. That's why I use schema.TypeString
here.
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.
I understand why you made that decision, but I think it would be better to change the type of this attribute to a schema.TypeInt
anyways. Unfortunately, there is a lot of areas for improvement for the Keycloak REST API, so I don't want to exactly model provider resources based on the types that the API uses.
We can convert the integer from the provider to a string to make the API call, and we can treat 0 as an empty string for the purpose of omitting the attribute to reset the order.
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.
All right. I'll change the code.
keycloak/openid_client_scope.go
Outdated
@@ -13,6 +13,8 @@ type OpenidClientScope struct { | |||
Attributes struct { | |||
DisplayOnConsentScreen string `json:"display.on.consent.screen"` // boolean in string form | |||
ConsentScreenText string `json:"consent.screen.text"` | |||
GuiOrder string `json:"gui.order"` | |||
IncludeInTokenScope string `json:"include.in.token.scope"` // boolean in string form |
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.
You can set this attribute's type to KeycloakBoolQuoted
, which is just a bool that understands how to marshal and unmarshal as a string. Then you won't have to worry about converting it back and forth between those two types.
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.
OK, I'll use KeycloakBoolQuoted
.
keycloak/saml_client_scope.go
Outdated
Description string `json:"description"` | ||
Protocol string `json:"protocol"` | ||
Attributes struct { | ||
DisplayOnConsentScreen string `json:"display.on.consent.screen"` // boolean in string form |
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.
Same comment as above, let's use KeycloakBoolQuoted
here.
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.
I see.
- Added include_in_token_scope and gui_order arguments into keycloak_openid_client_scope resource - Added keycloak_saml_client_scope resource - Added keycloak_saml_client_default_scopes resource
@mrparkers I modified the code and rebased onto the latest master. Could you check it again? |
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.
Looks good! Thanks for the PR!
I have made the following improvements regarding client scope.
include_in_token_scope
andgui_order
arguments intokeycloak_openid_client_scope
resourcekeycloak_saml_client_scope
resourcekeycloak_saml_client_default_scopes
resource