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

Add support for auto-refreshing token without refresh token #394

Closed
wants to merge 1 commit into from
Closed

Add support for auto-refreshing token without refresh token #394

wants to merge 1 commit into from

Conversation

denizdogan
Copy link

This is a basic idea on how to fix #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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 89.804% when pulling 1224b58 on denizdogan:master into 29ba9af on requests:master.

@JonathanHuot
Copy link
Contributor

This looks amazing and probably a documentation about it and a couple of unit tests will be ideal...
I know it has been long time already @denizdogan. I can also try to do this part.

@denizdogan
Copy link
Author

@JonathanHuot Heheh, it's been a while indeed! I had completely forgot about this PR. I think at this point, it's better that you fill in the blanks, or maybe just rewrite it entirely :) Thanks

@rohanliston
Copy link

This is exactly the functionality I'm looking for. Is there any chance of this being merged in the near future?

@denizdogan
Copy link
Author

@rohanliston Personally, I will not be working on adding docs or tests to this PR, I don't really use Python much anymore at all. If anyone really wants this to get merged, I would suggest maybe making a new PR with the same changes, plus some docs and a unit test.

@rohanliston
Copy link

@denizdogan Thanks. I'm happy to submit a fresh PR with tests/docs, but before doing so I'd like to know if the maintainers are likely to review/merge it since there appear to be questions around the status of this project.

@JonathanHuot are you able to comment on the above?

@JonathanHuot
Copy link
Contributor

@rohanliston, I can review it when ready and integrate it when ready.

@jtroussard
Copy link
Contributor

@JonathanHuot @rohanliston i can take some time this Sunday to review.

@rohanliston
Copy link

@jtroussard The project I was working on didn't go ahead and I'm now on something else. I don't think I'll have the bandwidth to come back to this, unfortunately.

@jtroussard
Copy link
Contributor

@rohanliston Thanks for the heads up.

@jtroussard
Copy link
Contributor

The torch has been passed to #526

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.

Possible to auto refresh client_credentials access token?
5 participants