From 30716783d40f788ae5b196feaf7ea5841b9b09fa Mon Sep 17 00:00:00 2001 From: Nikolay Turpitko <1192730+nikolay-turpitko@users.noreply.github.com> Date: Wed, 18 Jan 2023 14:19:44 +0700 Subject: [PATCH 1/3] Optionally send scopes on refresh for Microsoft Bing --- README-fork.md | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++ oauth2.go | 58 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 README-fork.md diff --git a/README-fork.md b/README-fork.md new file mode 100644 index 000000000..dcbcd9d14 --- /dev/null +++ b/README-fork.md @@ -0,0 +1,69 @@ +# Issue addressed by this fork + +Microsoft Advertising API returns access token with fewer scopes than initially +granted after using refresh token to obtain a new access token. + +It turned around, that it happens because "golang.org/x/oauth2" does not send +scope parameter on refresh request. + +It seems, that without this parameter Microsoft API returns access token with +default minimal set of scopes - not one was provided initially. + +When request is made with this parameter, it forces Microsoft API to return +access token with correct scope. + +# History of similar issues and PRs previously submitted to upstream project + +https://github.com/golang/oauth2/issues/234 + +https://github.com/golang/oauth2/pull/322 +https://go-review.googlesource.com/c/oauth2/+/135935 + +https://github.com/golang/oauth2/pull/448 +https://go-review.googlesource.com/c/oauth2/+/264037 + +https://github.com/golang/oauth2/pull/509 +https://go-review.googlesource.com/c/oauth2/+/332749 + +https://github.com/golang/oauth2/pull/540 +https://go-review.googlesource.com/c/oauth2/+/381916 + +https://github.com/golang/oauth2/pull/559 +https://go-review.googlesource.com/c/oauth2/+/394696 + +https://github.com/golang/oauth2/pull/579 +https://go-review.googlesource.com/c/oauth2/+/421174 + +https://github.com/golang/oauth2/pull/598 + +At this point it's obvious that changes have slim chances to make into +upstream, because of maintainers' position, best stated +[here](https://github.com/golang/oauth2/issues/112#issuecomment-101063921): + +> We are not willing to implement workarounds for each broken OAuth 2.0 +> implementation. If this is a blocker for you, please maintain your own of +> the oauth2 package with the necessary patches. + +It equally unlikely to persuade Microsoft to make the change. +So, here we are. + +# Some arguments why this option should be supported + +According to RFC, it seems, that issue is indeed on Microsoft side. But looking +at history it is clear that it persisted for quite a long already. Many PRs +have been submitted (and rejected or ignored) to the upstream project during +this time. It seems, that both sides are stubborn to change their mind. + +Obviously, we (application programmers) are not in position of endlessly +arguing with Microsoft and Google about correct RFC implementation. We need +some practical workaround to make our work at hands. But just in case, I'll +provide couple of arguments in support of this change. + +First of all, RFC 6749 sections 3.3 and 6 says, that indeed this parameter is +optional. But "optional" for me sounds like we should have an option to provide +it, if we have to. So, implementation should not prohibit us to do it. + +Second, taking aside Microsoft issue, RFC allows to either send the same or +fewer scopes. May be it is a rare use case, but it is in RFC and upstream +implementation does not address it. + diff --git a/oauth2.go b/oauth2.go index 291df5c83..930ce070c 100644 --- a/oauth2.go +++ b/oauth2.go @@ -232,6 +232,56 @@ func (c *Config) Client(ctx context.Context, t *Token) *http.Client { return NewClient(ctx, c.TokenSource(ctx, t)) } +// According to OAuth2 RFC 6749 section 3.3 and section 6, scope parameter is +// optional on refresh requests and: +// +// > The requested scope MUST NOT include any scope not originally granted by the +// > resource owner, and if omitted is treated as equal to the scope originally +// > granted by the resource owner. +// +// See https://tools.ietf.org/html/rfc6749#section-3.3 and +// https://tools.ietf.org/html/rfc6749#section-6 for more info. +// +// At the same time, RFC allows client to use this optional parameter and +// implementation should not restrict this usage. +// +// In fact, without specifing scopes on refresh request, at least Microsoft +// Advertising API resets access token scopes to (their own, not those +// specified in Config) defaults on refresh, meaning new access token has less +// scopes than initial. +// +// Though, it seems like issue is on Microsoft side, but for practical needs we +// need some workaround. It seems that issue exists for several years and +// highly unlikely it'll be ever fixed at Microsoft. +// +// From the other hand, RFC gives to client an option to provide same or fewer +// scopes on refresh request. So, this still can be useful in some valid use +// cases, when client want to obtain access token with fewer scopes. +// +// To minimise changes in the lib's interface for this rare use case and to not +// encourage using it, we are passing additional parameter via context. This +// parameter is a callback function, which is executed to fix scopes on refresh +// requests. +// +// Use it like this: +// ctx = context.WithValue(ctx, oauth2.ScopeFixer, oauth2.ScopeFixerExact) +// client := config.Client(ctx, token) + +type scopeFixerKey struct{} + +// ScopeFixer is a context key to provide function to fix scope on refresh +// request. Value should be a func([]string) []string - function, that consumes +// Config.Scope and returns scope to send on refresh request. If value is not +// set or returns nil or empty slice, then scope parameter will not be sent. +// It's up to ScopeFixer to provide correct set of scopes and up to auth server +// to check it. We're not checking if ScopeFixer returns additional scopes. +var ScopeFixer scopeFixerKey + +// ScopeFixerExact returns exactly same set of scopes. +func ScopeFixerExact(scopes []string) []string { + return scopes +} + // TokenSource returns a TokenSource that returns t until t expires, // automatically refreshing it as necessary using the provided context. // @@ -267,9 +317,17 @@ func (tf *tokenRefresher) Token() (*Token, error) { return nil, errors.New("oauth2: token expired and refresh token is not set") } + var scope []string + if scopeFixer, ok := tf.ctx.Value(ScopeFixer).(func([]string) []string); ok && len(tf.conf.Scopes) > 0 { + if scopes := scopeFixer(tf.conf.Scopes); len(scopes) > 0 { + scope = []string{strings.Join(scopes, " ")} + } + } + tk, err := retrieveToken(tf.ctx, tf.conf, url.Values{ "grant_type": {"refresh_token"}, "refresh_token": {tf.refreshToken}, + "scope": scope, }) if err != nil { From bd608033603013790b88eae3d66c948e58efbc5d Mon Sep 17 00:00:00 2001 From: Nikolay Turpitko <1192730+nikolay-turpitko@users.noreply.github.com> Date: Wed, 18 Jan 2023 14:23:40 +0700 Subject: [PATCH 2/3] Remove README-fork.md --- README-fork.md | 69 -------------------------------------------------- 1 file changed, 69 deletions(-) delete mode 100644 README-fork.md diff --git a/README-fork.md b/README-fork.md deleted file mode 100644 index dcbcd9d14..000000000 --- a/README-fork.md +++ /dev/null @@ -1,69 +0,0 @@ -# Issue addressed by this fork - -Microsoft Advertising API returns access token with fewer scopes than initially -granted after using refresh token to obtain a new access token. - -It turned around, that it happens because "golang.org/x/oauth2" does not send -scope parameter on refresh request. - -It seems, that without this parameter Microsoft API returns access token with -default minimal set of scopes - not one was provided initially. - -When request is made with this parameter, it forces Microsoft API to return -access token with correct scope. - -# History of similar issues and PRs previously submitted to upstream project - -https://github.com/golang/oauth2/issues/234 - -https://github.com/golang/oauth2/pull/322 -https://go-review.googlesource.com/c/oauth2/+/135935 - -https://github.com/golang/oauth2/pull/448 -https://go-review.googlesource.com/c/oauth2/+/264037 - -https://github.com/golang/oauth2/pull/509 -https://go-review.googlesource.com/c/oauth2/+/332749 - -https://github.com/golang/oauth2/pull/540 -https://go-review.googlesource.com/c/oauth2/+/381916 - -https://github.com/golang/oauth2/pull/559 -https://go-review.googlesource.com/c/oauth2/+/394696 - -https://github.com/golang/oauth2/pull/579 -https://go-review.googlesource.com/c/oauth2/+/421174 - -https://github.com/golang/oauth2/pull/598 - -At this point it's obvious that changes have slim chances to make into -upstream, because of maintainers' position, best stated -[here](https://github.com/golang/oauth2/issues/112#issuecomment-101063921): - -> We are not willing to implement workarounds for each broken OAuth 2.0 -> implementation. If this is a blocker for you, please maintain your own of -> the oauth2 package with the necessary patches. - -It equally unlikely to persuade Microsoft to make the change. -So, here we are. - -# Some arguments why this option should be supported - -According to RFC, it seems, that issue is indeed on Microsoft side. But looking -at history it is clear that it persisted for quite a long already. Many PRs -have been submitted (and rejected or ignored) to the upstream project during -this time. It seems, that both sides are stubborn to change their mind. - -Obviously, we (application programmers) are not in position of endlessly -arguing with Microsoft and Google about correct RFC implementation. We need -some practical workaround to make our work at hands. But just in case, I'll -provide couple of arguments in support of this change. - -First of all, RFC 6749 sections 3.3 and 6 says, that indeed this parameter is -optional. But "optional" for me sounds like we should have an option to provide -it, if we have to. So, implementation should not prohibit us to do it. - -Second, taking aside Microsoft issue, RFC allows to either send the same or -fewer scopes. May be it is a rare use case, but it is in RFC and upstream -implementation does not address it. - From ac28798cd0bac92f525b018b3ac91ad82fb49bb9 Mon Sep 17 00:00:00 2001 From: Nikolay Turpitko <1192730+nikolay-turpitko@users.noreply.github.com> Date: Tue, 30 Apr 2024 16:23:34 +0700 Subject: [PATCH 3/3] Fix typo --- oauth2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth2.go b/oauth2.go index 3a5945516..f3a14a30d 100644 --- a/oauth2.go +++ b/oauth2.go @@ -255,7 +255,7 @@ func (c *Config) Client(ctx context.Context, t *Token) *http.Client { // At the same time, RFC allows client to use this optional parameter and // implementation should not restrict this usage. // -// In fact, without specifing scopes on refresh request, at least Microsoft +// In fact, without specifying scopes on refresh request, at least Microsoft // Advertising API resets access token scopes to (their own, not those // specified in Config) defaults on refresh, meaning new access token has less // scopes than initial.