-
Notifications
You must be signed in to change notification settings - Fork 29
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 OAuth flows to use OAuth2 as a base class. #84
Conversation
Ah, I see I've made a PR against an old git commit. I'll update it. |
8b01ad3
to
8f7e7ea
Compare
I'm ok with introducing a base class on OAuth2 authentication, but I'd rather keep the OAuth2 class a way to have a common OAuth2 setup instead of a being the base class itself |
If I understand it, you'd like a base class separate from Actually I'd rather move WDYT token_cache = TokenMemoryCache()
display = DisplaySettings()
class OAuth2(abc.ABC, httpx.Auth):
state: Optional[str] = None
early_expiry: float
refresh_token: Optional[Callable] |
If you want to introduce a base class, that's up to you, i'm fine with it. However OAuth2 is the way to setup common OAuth2 settings. this is not related to auth classes and should stay that way. The base class should not be exposed to clients, and OAuth2 is not extending Auth in any way, so it should not be confusing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments but I like the direction where this is headed !
Co-authored-by: Colin Bounouar <[email protected]>
…to the base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small TODO to add and we are good to go !
|
Thanks a lot, good starting point |
This change makes OAuth2 the base class for all OAuth2 related flow implementations.
In the future it may be used to handle locking and other IO in sync or async modes as per the httpx auth protocol.
Classes that also inherit from BrowserAuth currently don't use cooperative multiple inheritance. They should, but I thought I'd rather make it a separate change.
Prelude to #72