-
Notifications
You must be signed in to change notification settings - Fork 87
fix: send scopes on refresh token requests #278
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
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.
Pull Request Overview
This PR ensures that the original scopes are sent along with refresh token requests so that they match what was originally requested, resolving compatibility issues with providers such as Azure Entra.
- The refresh token payload now includes a scope parameter built from a joined string of scopes.
- The authorization code handler now splits the scopes from a comma-separated parameter.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| oauth/refresh.go | Added scope parameter to the refresh token payload. |
| oauth/authcode.go | Initialized Scopes by splitting the scopes parameter. |
aiven-amartin
left a comment
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, but leaving for @cfunkhouser as I don't have time right now to test this out and it should be manually tested against a couple apis before merging given no tests.
|
Sorry for the delay here, folks, I was on vacation last week and through the weekend and I'm just getting caught up. Instead of doing direct string manipulation to create the URL-encoded scopes values, I think it's probably safer to use the Regarding sending scopes in the first place, it makes sense to me that it should work fine, especially since @danielgtaylor ran into an implementation specifically requesting it. Testing it, though, is another story! I'm working on test case against Google and Okta, and will report back. |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [danielgtaylor/restish](https://github.com/danielgtaylor/restish) | minor | `v0.20.0` -> `v0.21.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>danielgtaylor/restish (danielgtaylor/restish)</summary> ### [`v0.21.0`](https://github.com/rest-sh/restish/releases/tag/v0.21.0) [Compare Source](rest-sh/restish@v0.20.0...v0.21.0) In addition to the many changes (see the notes, below), this is the first release at the new package location. Go forth, and update your `go.mod`s! #### What's Changed - docs: improve link accuracy by [@​eitamal](https://github.com/eitamal) in rest-sh/restish#258 - chore(deps): bump golang.org/x/net from 0.17.0 to 0.23.0 by [@​dependabot](https://github.com/dependabot) in rest-sh/restish#251 - chore(deps): bump google.golang.org/protobuf from 1.28.1 to 1.33.0 by [@​dependabot](https://github.com/dependabot) in rest-sh/restish#242 - Make it possible to use a client certificate stored in hardware by [@​jeffallen](https://github.com/jeffallen) in rest-sh/restish#246 - Expand documentation on external-tool auth by [@​fflores97](https://github.com/fflores97) in rest-sh/restish#256 - Escape '&' for Windows command line oAuth flow by [@​MatthiasScholzTW](https://github.com/MatthiasScholzTW) in rest-sh/restish#253 - Do not output colorized config when environment does not support it by [@​cfunkhouser](https://github.com/cfunkhouser) in rest-sh/restish#270 - chore(deps): bump golang.org/x/image from 0.10.0 to 0.18.0 by [@​dependabot](https://github.com/dependabot) in rest-sh/restish#260 - docs: include command for fish completion help by [@​kbakk](https://github.com/kbakk) in rest-sh/restish#271 - feat: upgrade to Go 1.24 by [@​danielgtaylor](https://github.com/danielgtaylor) in rest-sh/restish#277 - chore(deps): bump golang.org/x/net from 0.23.0 to 0.38.0 by [@​dependabot](https://github.com/dependabot) in rest-sh/restish#276 - fix: send scopes on refresh token requests by [@​danielgtaylor](https://github.com/danielgtaylor) in rest-sh/restish#278 - feat: rename danielgtaylor/restish -> rest-sh/restish by [@​danielgtaylor](https://github.com/danielgtaylor) in rest-sh/restish#281 - docs: fix build status by [@​danielgtaylor](https://github.com/danielgtaylor) in rest-sh/restish#282 - Upgrade libopenapi by [@​terev](https://github.com/terev) in rest-sh/restish#283 #### New Contributors - [@​eitamal](https://github.com/eitamal) made their first contribution in rest-sh/restish#258 - [@​jeffallen](https://github.com/jeffallen) made their first contribution in rest-sh/restish#246 - [@​fflores97](https://github.com/fflores97) made their first contribution in rest-sh/restish#256 - [@​MatthiasScholzTW](https://github.com/MatthiasScholzTW) made their first contribution in rest-sh/restish#253 - [@​cfunkhouser](https://github.com/cfunkhouser) made their first contribution in rest-sh/restish#270 - [@​kbakk](https://github.com/kbakk) made their first contribution in rest-sh/restish#271 - [@​terev](https://github.com/terev) made their first contribution in rest-sh/restish#283 **Full Changelog**: rest-sh/restish@v0.20.0...v0.21.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC41MC4wIiwidXBkYXRlZEluVmVyIjoiNDAuNTAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
This small PR sends the scopes originally requested when fetching an access token to the refresh token endpoint (if one is available). I ran into an issue where Azure Entra refuses the refresh token unless the scopes match the original request. I believe this is safe to do as the spec labels this field as optional and other auth systems should either confirm the scopes are equal or ignore it, but could use some help testing.
BTW, the auth package is missing a lot of tests, so I haven't added one here, but eventually it would be nice to get some tests there.