search

requests / requests-oauthlib

OAuthlib support for Python-Requests!
https://requests-oauthlib.readthedocs.org/
ISC License
1.72k stars 424 forks source link

Support non-x-www-form-urlencoded bodies returned from refresh_token_request compliance hook #545

Open skray opened 5 months ago

skray commented 5 months ago

Addresses https://github.com/requests/requests-oauthlib/issues/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.

JoepvandenHoven-Bluemine commented 4 months ago

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 commented 4 months ago

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) :pray:

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.

violuke commented 4 months ago

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

skray commented 3 months ago

@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 commented 1 month ago

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?