-
Notifications
You must be signed in to change notification settings - Fork 59
Custom scopes support in U2M authentication #1390
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
base: main
Are you sure you want to change the base?
Changes from all commits
a4e263c
0670200
3c3886a
92e3b5a
19acdcd
31fb457
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,14 @@ type PersistentAuth struct { | |
| // netListen is an optional function to listen on a TCP address. If not set, | ||
| // it will use net.Listen by default. This is useful for testing. | ||
| netListen func(network, address string) (net.Listener, error) | ||
|
|
||
| // scopes is the list of OAuth scopes to request. | ||
| scopes []string | ||
|
|
||
| // disableOfflineAccess controls whether offline_access scope is requested. | ||
| // When true, offline_access will NOT be automatically added to scopes, | ||
| // meaning the token will not include a refresh token. | ||
| disableOfflineAccess bool | ||
| } | ||
|
|
||
| type PersistentAuthOption func(*PersistentAuth) | ||
|
|
@@ -135,6 +143,25 @@ func WithPort(port int) PersistentAuthOption { | |
| } | ||
| } | ||
|
|
||
| // WithScopes sets the OAuth scopes for the PersistentAuth. | ||
| func WithScopes(scopes []string) PersistentAuthOption { | ||
| return func(a *PersistentAuth) { | ||
| a.scopes = scopes | ||
| } | ||
| } | ||
|
|
||
| // WithDisableOfflineAccess controls whether offline_access scope is requested. | ||
| func WithDisableOfflineAccess(disable bool) PersistentAuthOption { | ||
| return func(a *PersistentAuth) { | ||
| a.disableOfflineAccess = disable | ||
| } | ||
| } | ||
|
|
||
| // GetScopes returns the OAuth scopes configured for this PersistentAuth. | ||
| func (a *PersistentAuth) GetScopes() []string { | ||
| return a.scopes | ||
| } | ||
|
|
||
| // NewPersistentAuth creates a new PersistentAuth with the provided options. | ||
| func NewPersistentAuth(ctx context.Context, opts ...PersistentAuthOption) (*PersistentAuth, error) { | ||
| p := &PersistentAuth{} | ||
|
|
@@ -368,10 +395,16 @@ func (a *PersistentAuth) validateArg() error { | |
|
|
||
| // oauth2Config returns the OAuth2 configuration for the given OAuthArgument. | ||
| func (a *PersistentAuth) oauth2Config() (*oauth2.Config, error) { | ||
| scopes := []string{ | ||
| "offline_access", // ensures OAuth token includes refresh token | ||
| "all-apis", // ensures OAuth token has access to all control-plane APIs | ||
| // Default to "all-apis" for backwards compatibility with direct users of PersistentAuth | ||
| // i.e. people implementing their own U2M authentication. | ||
| scopes := a.scopes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be a behavioral change, right? Because it is possible to have the scopes as empty. What's the impact of that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Where exactly? It used to be hard coded to never be empty. Now, for existing users, a.scopes will first be empty but the code below will add Could you give an example for the behavioural change? I don't see it. |
||
| if len(scopes) == 0 { | ||
| scopes = []string{"all-apis"} | ||
| } | ||
| if !a.disableOfflineAccess { | ||
| scopes = append([]string{"offline_access"}, scopes...) | ||
| } | ||
|
|
||
| var endpoints *OAuthAuthorizationServer | ||
| var err error | ||
| switch argg := a.oAuthArgument.(type) { | ||
|
|
||
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.
What's the use of this method? If it's just testing, I am not sure this should be public.
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.
It is just for testing but it is not used in the same package (used in auth_u2m_test.go). I think this indicates that the files are not in the right packages (auth_u2m is not in credentials/u2m while persistent_auth is), but I'm not sure if this is the right PR to fix that. I can rename it to GetScopesForTesting and add a warning comment, or perhaps move the files in /credentials/u2m up into the config 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.
Or perhaps the tests in auth_u2m shouldn't be making assertions on persistent_auth