fosite icon indicating copy to clipboard operation
fosite copied to clipboard

Fix: Use proper scopes in refresh token handler

Open silverspace opened this issue 3 years ago • 3 comments

Fixes #696

The token refresh handler currently has two issues related to scopes:

  1. It does not read or honor the optional scope form parameter
  2. It defaults to the current client scopes, rather than the originally granted scopes. One issue related to this is that if the client scopes are narrowed, then the existing refresh tokens may fail to refresh, even if the user originally consented to those scopes.

This PR fixes the OAuth2 token refresh handler to:

  • Read and use the optional 'scope' form parameter, if present.
  • Otherwise default to requesting the originally granted scopes.

The token refresh handler endpoint should be completely agnostic of:

  • The originally requested scopes
  • The client scopes (both current and past client scopes)

Related Issue or Design Document

#696

Checklist

  • [x] I have read the contributing guidelines and signed the CLA.
  • [x] I have referenced an issue containing the design document if my change introduces a new feature.
  • [x] I have read the security policy.
  • [x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • [x] I have added tests that prove my fix is effective or that my feature works.
  • [ ] I have added necessary documentation within the code base (if appropriate).

Further comments

silverspace avatar Aug 31 '22 23:08 silverspace

Perhaps we can put aside the "client scope" discussion for a moment, and simply agree that the Fosite library DOES have a bug whereby the scope form parameter is completely ignored. The refresh token handler does not read the scope form parameter in the current implementation, so it is currently impossible to request a more narrowly scoped access token.

silverspace avatar Sep 02 '22 16:09 silverspace

Perhaps we can put aside the "client scope" discussion for a moment, and simply agree that the Fosite library DOES have a bug whereby the scope form parameter is completely ignored. The refresh token handler does not read the scope form parameter in the current implementation, so it is currently impossible to request a more narrowly scoped access token.

Yeah I agree with focusing on the one aspect, it's more efficient. Especially with focusing on something I think is less ambiguous/open to interpretation/not explicitly stated in the spec, is a good idea.

james-d-elliott avatar Sep 03 '22 01:09 james-d-elliott

@aeneasr please let me know if you have time to discuss https://github.com/ory/fosite/pull/698#discussion_r961835824

silverspace avatar Sep 08 '22 03:09 silverspace

Given the discussion and reasoning above I will close this PR. Any ideas for future work outlined in this PR we‘ll happily accept though :)

aeneasr avatar Nov 01 '22 13:11 aeneasr