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.

Sign up for GitHub

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

Fix issue #94: Support for authlib 1.0.0 #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix issue #94: Support for authlib 1.0.0 #95

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b5c592
Fix issue #94: Support for authlib 1.0.0
birkjernstrom May 13, 2022
b42feb4
Support Authlib >= 1.0.0 for Flask, Django & FastAPI.
birkjernstrom May 14, 2022
0c4f3ea
Fix import of OAuth module in Django example
birkjernstrom May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions loginpass/_flask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ def login(name):


def register_to(oauth, backend_cls):
from authlib.integrations.flask_client import FlaskRemoteApp
from authlib.integrations.flask_client import FlaskOAuth2App

class RemoteApp(backend_cls, FlaskRemoteApp):
class RemoteApp(backend_cls, FlaskOAuth2App):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will not fix OAuth 1.0 service.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Was reviewing the authlib changes in 1.0.0 late last night and saw that FlaskOAuth2App was imported somewhere instead of FlaskRemoteApp and figured it was intended to be the default and OAuth1 was handled differently.

I'll update this tonight or tomorrow morning. Since register_to receives the oauth object it should be fairly straightforward to introspect whether OAuth1 or OAuth2 is intended and import accordingly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not as straightforward as I originally thought via introspection of the oauth instance. After reviewing the full diff between authlib 1.0.0 vs 0.15.5, I now fully understand how and why the OAuth classes where refactored. Removing the abstract <Framework>RemoteApp in favour of being explicit and separate the concerns between OAuth 1 vs 2 for cleaner class APIs. So this issue is therefore also not isolated to Flask alone, but should be present in Django & FastAPI implementations of loginpass as well using authlib >= 1.0.0.

I'm working to address this, but will not have the time to finalize it this weekend. Aiming to push an updated fix ~Mon/Tue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got some time and managed to push an update. I've confirmed it works in Flask, but haven't tried Django or FastAPI yet. Can do that on Monday, but wanted to push the code sooner to get any feedback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as intended with Flask now. The accurate OAuth client is registered depending on the backend configuration and whether it has a request_token_url URL (OAuth v1) or not (OAuth v2), e.g Twitter (v1) vs. Github (v2).

Addressed the same issue for the Django & FastAPI clients, but running into issues setting up those environments so can't fully test it. No runtime exceptions related to this PR or loginpass though. Do you have environments prepared and can easily test this @lepture ?

OAUTH_APP_CONFIG = backend_cls.OAUTH_CONFIG

oauth.register(RemoteApp.NAME, overwrite=True, client_cls=RemoteApp)