Colin-b / httpx_auth

Authentication classes to be used with httpx
MIT License
117 stars 26 forks source link

Refactor OAuth flows to use OAuth2 as a base class. #84

Closed rafalkrupinski closed 8 months ago

rafalkrupinski commented 8 months ago

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

rafalkrupinski commented 8 months ago

Ah, I see I've made a PR against an old git commit. I'll update it.

Colin-b commented 8 months ago

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

rafalkrupinski commented 8 months ago

If I understand it, you'd like a base class separate from _oauth2.common.OAuth2 for the OAuth2 flows implementations. Wouldn't that be confusing to have two separate classes for common OAuth2 stuff, one used for state and the other used to inherit functionality?

Actually I'd rather move token_cache and display out to the top level. Using class fields for global state looks a bit like it came from Java, and now with other fields they can look like they're instance fields.

WDYT

token_cache = TokenMemoryCache()
display = DisplaySettings()

class OAuth2(abc.ABC, httpx.Auth):
    state: Optional[str] = None
    early_expiry: float

    refresh_token: Optional[Callable]
Colin-b commented 8 months ago

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.

sonarcloud[bot] commented 8 months ago

Quality Gate Passed Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Colin-b commented 8 months ago

Thanks a lot, good starting point