Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

skray
Copy link

@skray skray commented Apr 19, 2024

Addresses #544

Adding similar capabilities to the refresh_token_request compliance hook as is possible in the access_token_request hook, to allow for non standard, non x-www-form-urlencoded request bodies in token refresh requests.

Additionally, adding in a Wix compliance fix showing how to use the access_token_request and refresh_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-token

I 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 in test_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.

…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
@JoepvandenHoven-Bluemine

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-www-form-urlencoded then the Content-Type should be something other than application/x-www-form-urlencoded and conversely if the Content-Type is not application/x-www-form-urlencoded then it makes little sense to force the value to be x-www-form-urlencoded.

Relying on a ValueError is a bit iffy since it relies on the underlying oauthlib function to throw the correct Exceptions.

@violuke
Copy link
Contributor

violuke commented May 28, 2024

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) 🙏

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-www-form-urlencoded then the Content-Type should be something other than application/x-www-form-urlencoded and conversely if the Content-Type is not application/x-www-form-urlencoded then it makes little sense to force the value to be x-www-form-urlencoded.

Relying on a ValueError is a bit iffy since it relies on the underlying oauthlib function to throw the correct Exceptions.

I agree this would be a better solution.

r = self.post(
token_url,
data=dict(urldecode(body)),
data=body,
Copy link
Contributor

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)),

@violuke
Copy link
Contributor

violuke commented May 28, 2024

@JonathanHuot would you consider accepting this? Please and thank you 🙏

@skray
Copy link
Author

skray commented Jun 14, 2024

@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.

@skray
Copy link
Author

skray commented Aug 23, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants