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

feat!: customizable client scopes in oidc_impl package #1052

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

kuannie1
Copy link
Contributor

@kuannie1 kuannie1 commented Apr 12, 2024

this has the potential to break some dependencies using an unpinned version of this package since it starts with no scopes, but there's a method to add the scopes through its helper function, AddScope

@kuannie1 kuannie1 requested a review from a team as a code owner April 12, 2024 18:42
@kuannie1 kuannie1 requested a review from jjb007 April 12, 2024 18:42
@kuannie1 kuannie1 marked this pull request as draft April 12, 2024 18:42
@kuannie1 kuannie1 changed the title wip: test a method of adding scopes feat: customizable scopes in oidc_impl package Apr 16, 2024
@kuannie1 kuannie1 changed the title feat: customizable scopes in oidc_impl package feat: customizable client scopes in oidc_impl package Apr 16, 2024
@kuannie1 kuannie1 changed the title feat: customizable client scopes in oidc_impl package feat!: customizable client scopes in oidc_impl package Apr 16, 2024
@kuannie1 kuannie1 marked this pull request as ready for review April 16, 2024 20:12
@@ -184,15 +180,31 @@ func (c *Client) ValidateState(ourState []byte, otherState []byte) error {
}
return nil
}
func format_scopes(ctx context.Context, scopes []string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, let's user camelcase, and rename to formatScopes().

Comment on lines -55 to -58
oidc.ScopeOpenID,
oidc.ScopeOfflineAccess,
"email",
"groups",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we always have to add all these scopes manually now? If so, would it be worth adding a function AddDefaultScopes that adds all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hspitzley-czi That was my initial idea, but I'd understand if that's not ideal!

Do you mean adding a function like this to oidc_cli/oidc_impl/client/config_options.go?

var AddDefaultScopes = func(scope string) Option {
	return func(c *Client) {
		c.oauthConfig.Scopes = []string{"openid", "groups", "email", "offline_access"}
	}
}

Or like, adding all the default scopes if none are provided?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah like the AddDefaultScopes you defined here! Adding default scopes if none are provided would be nice too if possible, that way it's backwards compatible

@kuannie1 kuannie1 marked this pull request as draft April 17, 2024 19:14
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.

None yet

3 participants