-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
Support non-x-www-form-urlencoded bodies returned from refresh_token_request compliance hook #545
base: master
Are you sure you want to change the base?
Conversation
…request compliance hook * Handle the case where a non-x-www-form-urlencoded string is returned from the refresh_token_request compliance hook, and allow it to be used as a raw string in the subsequent request * Add wix compliance fix to send token request and refresh requests with JSON bodies * Add unit tests for wix compliance fixes
Instead of relying on a ValueError would it not make more sense to inspect the Content-Type in the headers? If the value is not x- Relying on a ValueError is a bit iffy since it relies on the underlying oauthlib function to throw the correct Exceptions. |
We would also find this very useful, so if this could be merged that would be very much appreciated (Wix is also our use case) 🙏
I agree this would be a better solution. |
r = self.post( | ||
token_url, | ||
data=dict(urldecode(body)), | ||
data=body, |
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.
To check the content type instead of catching the ValueError, this could just become:
data=body if headers["Content-Type"] == "application/json" else dict(urldecode(body)),
@JonathanHuot would you consider accepting this? Please and thank you 🙏 |
@violuke @JoepvandenHoven-Bluemine good points, I'm happy to change the logic to check the content-type, instead of handling the ValueError. Before doing any more work here I'd like to make sure this PR will be accepted by one of the project maintainers, then I can do any other changes needed and update tests all at once. |
I had some extra time on my hands so I went ahead and updated the code based on feedback so far. Does anyone have advice for getting this reviewed by a maintainer? |
Addresses #544
Adding similar capabilities to the
refresh_token_request
compliance hook as is possible in theaccess_token_request
hook, to allow for non standard, nonx-www-form-urlencoded
request bodies in token refresh requests.Additionally, adding in a Wix compliance fix showing how to use the
access_token_request
andrefresh_token_request
compliance hooks to successfully request and refresh tokens with Wix. Docs here: https://dev.wix.com/docs/rest/app-management/oauth-2/request-an-access-tokenI added tests for the compliance fix to show the
refresh_token_request
updates work (as well as testing them manually against the Wix API from my own project). I'm happy to add tests intest_oauth2_session.py
as well, I just wasn't sure exactly what to add, as the code I added should only ever be triggered by compliance hooks changing the request body to something non-standard.I am also open to a completely different implementation here, this was just the option that I saw would solve my problem and looked to be fully backwards compatible with anyone's existing
refresh_token_request
compliance hooks.