requests-oauthlib icon indicating copy to clipboard operation
requests-oauthlib copied to clipboard

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

Open skray opened this issue 10 months ago • 5 comments

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.

skray avatar Apr 19 '24 19:04 skray

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.

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 avatar May 28 '24 13:05 violuke

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

violuke avatar May 28 '24 14:05 violuke

@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 avatar Jun 14 '24 13:06 skray

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?

skray avatar Aug 23 '24 14:08 skray