-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Fix issue #94: Support for authlib 1.0.0 #95
base: master
Are you sure you want to change the base?
Fix issue #94: Support for authlib 1.0.0 #95
Conversation
Use FlaskOAuth2App instead of FlaskRemoteApp since authlib 1.0.0 refactored accordingly and removed FlaskRemoteApp.
loginpass/_flask.py
Outdated
|
||
class RemoteApp(backend_cls, FlaskRemoteApp): | ||
class RemoteApp(backend_cls, FlaskOAuth2App): |
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.
But this will not fix OAuth 1.0 service.
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.
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.
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.
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.
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.
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.
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.
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 ?
Register & use the correct remote application (OAuth 1 or 2) based on the backend configuration.
Changing
FlaskRemoteApp
toFlaskOAuth2App
since authlib 1.0.0 was refactored to:FlaskRemoteApp
FlaskOAuth2App
insteadThis was confirmed to work locally. No longer getting an ImportError exception and worked to auth using Github in the flask_example app.