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

refactor: Add support for auto-refreshing token without refresh token #526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dabla
Copy link

@dabla dabla commented Jan 16, 2024

Hello,

This is a pull request based on the changes made by @denizdogan but which finally never got merged due to missing tests.

Here is the original explaination:

This is a basic idea on how to fix https://github.com/requests/requests-oauthlib/issues/260 since nothing has happened to the issue in three years. Let's get a discussion going, because this is a hindrance to me in almost every OAuth2 implementation I make using this library and it's clear that lots of other people have the same issue.

So the idea is to add a new constructor parameter which I currently call auto_refresh_type, it can be either "refresh_token" (which will do exactly what it does today) or "access_token" (which will just get a new access token as you normally do).

It's not a beautiful design by any means, but I intentionally tried to keep the diff as minimal as possible without any major refactorings, which would introduce breaking changes and make it more difficult to get this functionality out there.

I've done some minor refactoring but also added 2 tests which test both cases when the update_token method is invoked.

Kind regards,
David

@jtroussard
Copy link
Contributor

@dabla Is it possible to update this PR/fix conflicts. This will kick off the test runners and I will have a look and try to get this merged.

@jtroussard
Copy link
Contributor

for documentation purposes, linking these related resources.

#394 #260

issue and pr related to previous attempts to get this feature merged.

@zach-flaglerhealth
Copy link

zach-flaglerhealth commented Jul 31, 2024

For anyone coming across this PR and thinking about vendoring in this particular PR...

To utilize this, following the basic BackendApplicationClient example from the docs, you'll need to add the following parameters to your OAuth2Session:

client = BackendApplicationClient(client_id="some_client_id", scope="your scopes here") # No changes
oauth = OAuth2Session(
    client=client,
    client_id="some_client_id",
    # New params
    auto_refresh_type="access_token",
    auto_refresh_url="The OAuth2 token URL",
    auto_refresh_kwargs={"client_secret": "Your client secret"}
)

The reason you need to pass it in there is that the Session tries to either utilize the auth attribute it's passing around, or tries re-encoding the client_id and client_secret. However, the class doesn't like it as an attribute, but does properly get pulled into fetch_token.

There are two other changes I made for my purposes, which may be more controversial:

  1. I removed the raise TokenUpdated(token), since the whole point of the session is to handle this for me.
  2. I copied the self._client.add_token from lines 528 - 530, because otherwise it just continues to make the request with the invalid token.

There may be things I've overlooked. My changes make the tests that were added here fail (since they're looking for a raised exception). This seems to now be running and auto-refreshing for me.

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.

4 participants