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 OAuth flows to use OAuth2 as a base class. #84

Merged
merged 6 commits into from
Feb 27, 2024

Conversation

rafalkrupinski
Copy link
Contributor

@rafalkrupinski rafalkrupinski commented Feb 21, 2024

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
Copy link
Contributor Author

rafalkrupinski commented Feb 21, 2024

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

@Colin-b
Copy link
Owner

Colin-b commented Feb 21, 2024

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
Copy link
Contributor Author

rafalkrupinski commented Feb 22, 2024

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
Copy link
Owner

Colin-b commented Feb 22, 2024

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.

Copy link
Owner

@Colin-b Colin-b left a 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 !

httpx_auth/_oauth2/resource_owner_password.py Outdated Show resolved Hide resolved
httpx_auth/_oauth2/common.py Outdated Show resolved Hide resolved
httpx_auth/_oauth2/common.py Outdated Show resolved Hide resolved
httpx_auth/_oauth2/common.py Outdated Show resolved Hide resolved
Copy link
Owner

@Colin-b Colin-b left a 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 !

httpx_auth/_oauth2/implicit.py Show resolved Hide resolved
Copy link

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 Colin-b merged commit 9858139 into Colin-b:develop Feb 27, 2024
5 checks passed
@Colin-b
Copy link
Owner

Colin-b commented Feb 27, 2024

Thanks a lot, good starting point

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.

2 participants