Skip to content

Adding refresh_token_request and access_token_request compliance hooks - #433

Merged
JonathanHuot merged 8 commits into
requests:masterfrom
violuke:master
Feb 21, 2022
Merged

JonathanHuot merged 8 commits into
requests:masterfrom
violuke:master

Conversation

@violuke

@violuke violuke commented Dec 8, 2020

Copy link
Copy Markdown
Contributor

This is based upon #214, so thanks to @jimcortez for basically solving this for me 👍 🙏

We needed this for a custom internal OAuth solution hence no included compliance fix example.

This solved the problem we're having and I believe will solve some or all of the points in issues #182, #379, #430 and #264 so should be useful for the wider community, despite not including a specific compliance fix example.

Here's our internal compliance fix:

import base64
from oauthlib.common import add_params_to_uri

def internal_oauth_compliance_fix(session, client_secret: str):
    def _non_compliant_auth_header(url, headers, body):
        basic_auth_value = "{}:{}".format(session._client.client_id, client_secret)
        headers["Authorization"] = 'Basic {}'.format(base64.b64encode(basic_auth_value.encode("utf-8")).decode("utf-8"))
        token = [('token', session._client.access_token)]
        url = add_params_to_uri(url, token)
        return url, headers, body

    session.register_compliance_hook('refresh_token_request', _non_compliant_auth_header)
    return session

I hope this helps someone else, thanks again to @jimcortez for the original work and if this could be merged soon that would be much appreciated. 👍

@coveralls

coveralls commented Dec 8, 2020 •

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.5%) to 90.31% when pulling 4e67803 on violuke:master into 6ac6133 on requests:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.3%) to 89.817% when pulling 460521c on violuke:master into 46f886c on requests:master.

@timdawborn

Copy link
Copy Markdown

An example of a large public application that needs Basic Auth for the refresh tokens is the Xero API: https://developer.xero.com/documentation/oauth2/auth-flow#refresh

@violuke

violuke commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

I've just also added support for an access_token_request hook. We were in the same situation as #244 (needing to send the data as json on a password grant type), but in our case the API was unable to accept via application/x-www-form-urlencoded. We also had to remove the redirect_uri key from the JSON or the request was rejected.

I know this is against the Oauth2 specs, but that's the whole point of compliant hooks 😉 Here's an example usage:

def redacted_company_compliance_fix(session):
    def _request_in_json_not_form_urlencoded(url, headers, request_kwargs):
        headers["Content-Type"] = "application/json"

        if "redirect_uri" in request_kwargs["data"]:
            del request_kwargs["data"]["redirect_uri"]

        request_kwargs["json"] = request_kwargs["data"]

        del request_kwargs["data"]

        return url, headers, request_kwargs

    session.register_compliance_hook("access_token_request", _request_in_json_not_form_urlencoded)
    return session

@violuke

violuke commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

@jtroussard would be please be possible for this to be reviewed? I think it will help in lots of incompliant situations. Thank you 🙏

@JonathanHuot JonathanHuot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add at least a unit test? This feature is a great addition, so thanks for the extra work.

Comment thread requests_oauthlib/oauth2_session.py
@violuke violuke changed the title Adding refresh_token_request hook Adding refresh_token_request and access_token_request compliance hooks Feb 19, 2022
@violuke

violuke commented Feb 20, 2022

Copy link
Copy Markdown
Contributor Author

@JonathanHuot I've added a test now for both of the new hooks. Thanks.

@JonathanHuot

Copy link
Copy Markdown
Contributor

Can you execute tox -e black and tox -e py27 as they seem not passing in pipeline. Thanks in advance

@violuke

violuke commented Feb 21, 2022

Copy link
Copy Markdown
Contributor Author

Both are sorted now, thanks.

@JonathanHuot JonathanHuot added this to the 1.4.0 milestone Feb 21, 2022
@JonathanHuot
JonathanHuot merged commit b3decdb into requests:master Feb 21, 2022
@JonathanHuot

Copy link
Copy Markdown
Contributor

Thanks for this useful feature!

@violuke

violuke commented Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

@JonathanHuot would you please be able to make a release on PyPI for this so that we can switch our package manager over to the main project again? Thank you.

@violuke

violuke commented Aug 6, 2023

Copy link
Copy Markdown
Contributor Author

Any chance of that PyPI release? 😉

@JonathanHuot

Copy link
Copy Markdown
Contributor

Hi @violuke, I will prepare a release this month. Thanks for your patience.

@violuke violuke mentioned this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants