Update to authlib related to issue #113 - slightly better PR #118

bitfinity · 2018-05-22T18:38:27Z

No description provided.

…enames. Tests are mostly failing due to the problems I was having with sessions, but also because there is no OAuth2Grant, and also Flask-login anonymous users seem to be handled differently.

frol

There are some initial comments.

frol · 2018-05-22T20:14:45Z

app/modules/auth/models.py

-    """
+class MyOAuth2ClientMixin(OAuth2ClientMixin):
+    def check_requested_scopes(self, scopes):
+        if type(self.scope) == str:


I think, isinstance() check is better.

Oh yeah definitely - was in hurry and lazy.

frol · 2018-05-22T20:19:14Z

tasks/app/run.py

@@ -45,6 +45,7 @@ def run(
    if upgrade_db:
        # After the installed dependencies the app.db.* tasks might need to be
        # reloaded to import all necessary dependencies.
+        import sqlalchemy_utils


Why is this needed?

Not sure. Probably not.

frol · 2018-05-22T20:20:46Z

tasks/app/db.py

+        'x_arg': "Additional arguments consumed by custom env.py scripts",
+    }
+)
+def droptables(context, directory='migrations', revision='head', sql=False, tag=None, x_arg=None,


It seems that all the parameters and help strings are irrelevant to the task.

Yes, didn't understand the invoke system. Also db is not imported.

frol · 2018-05-22T20:21:24Z

run_app.py

+    return app.run( host=host, port=port)
+
+if __name__ == "__main__":
+    run_app()


There is invoke app.run for this.

Yes, this is brought over from another project. I find the debugger works better without using invoke. But it was utility, not meant to be checked in.

frol · 2018-05-22T20:23:11Z

migrations/versions/4754e1427ac_.py

@@ -1,26 +0,0 @@
-"""empty message


Well, this is not how we deal with migrations. Even though it is an example, migrations are an essential part of the "real life" example.

Yes, I broke the migrations flow. I would really need to revert migrations, and redo that part.

frol · 2018-05-24T06:58:54Z

app/extensions/auth/oauth2.py

-                if 'form' in locations:
-                    if 'access_token' in request.form:
-                        request.authorization = 'Bearer %s' % request.form['access_token']
+                # don't think we need below lines because bearer validator already registered


Do you mean that this trick will not work anymore anyway or you just don't see a use-case of passing the token via POST form parameters? The use-case is designed for native HTML forms, where we don't control the request from JS, but, instead, rely on a browser to do all the work, and thus, we cannot set the access token in the header.

I guess I didn't understand what it was doing and didn't see a similar thing i the authlib example, so I thought it was related to flask-oauthlib. I will add it back in.

frol · 2018-05-24T07:00:11Z

app/extensions/flask_sqlalchemy/__init__.py

-        kwargs['session_options']['autocommit'] = True
+        # if 'session_options' not in kwargs:
+        #     kwargs['session_options'] = {}
+        # kwargs['session_options']['autocommit'] = True


We need to sort this out one way or another since it is quite a breaking change.

Yeah I had a lot of issues with autocommit, namely with sqlalchemy complaining that a transaction already was started when with db.session.begin(): was called. In authlib, the complaints were about no transaction has started. In my own project I turned it off, because I had my own transaction system, and also I was using postgresql.

frol · 2018-05-24T07:02:14Z

app/modules/auth/__init__.py

@@ -26,7 +20,6 @@ def init_app(app, **kwargs):
    Init auth module.
    """
    # Bind Flask-Login for current_user
-    login_manager.request_loader(load_user_from_request)


Given you drop this, you may as well drop the function. How does this work now, though?

I actually did more work in my other project on this - I think I got login manager working as follows:

from flask import g from flask.sessions import SecureCookieSessionInterface from flask_login import user_loaded_from_header from flask_login import LoginManager as OriginalLoginManager class CustomSessionInterface(SecureCookieSessionInterface): """Prevent creating session from API requests.""" def save_session(self, *args, **kwargs): if g.get('login_via_header'): return return super(CustomSessionInterface, self).save_session(*args, **kwargs) @user_loaded_from_header.connect def user_loaded_from_header(self, user=None): g.login_via_header = True class LoginManager(OriginalLoginManager): def init_app(self, app, add_context_processor=True): app.session_interface = CustomSessionInterface() super().init_app(app, add_context_processor=add_context_processor) # in some other code from app.extensions import login_manager from authlib.flask.oauth2 import current_token @login_manager.request_loader def load_user_from_request(request): """ Load user from OAuth2 Authentication header. """ if current_token: user = current_token.user if user: return user user_id = current_token.user.id if user_id: return User.query.get(user_id) return None

frol · 2018-05-24T07:04:18Z

app/modules/auth/models.py


-    id = db.Column(db.Integer, primary_key=True)  # pylint: disable=invalid-name
-
-    user_id = db.Column(db.ForeignKey('user.id', ondelete='CASCADE'), index=True, nullable=False)


Why did you drop the index=True and nullable=False?

Not sure - I was probably having problems upgrading, and was really trying to get the auth going to see if it would work, and probably meant to go back and fix.

frol · 2018-05-24T07:05:45Z

app/modules/auth/models.py

    class ClientTypes(str, enum.Enum):
        public = 'public'
        confidential = 'confidential'

    client_type = db.Column(db.Enum(ClientTypes), default=ClientTypes.public, nullable=False)
-    redirect_uris = db.Column(ScalarListType(separator=' '), default=[], nullable=False)
    default_scopes = db.Column(ScalarListType(separator=' '), nullable=False)


I think you can just drop the default_scopes; it seems like it is a simple rename.

Well, that's one thing I fixed in my other project. So authlib uses lists differently in the mixins. I'm not a huge fan, but it's convenient to use the mixins. The authlib mixins define scope as a Text and it's a list divided by newlines. Same with grant, and redirect_uri. Then it has @hybrid_property definitions for them that return redirect_uri.splitlines() as an example. I tried to use your @scalarlisttypes but that didn't work because some of the authlib methods assume a textfield with newline split list.

I have a little more time now, so I will try to fix based on these comments, and integrate some of the fixes that I made in my own project.

James O'Brien added 3 commits May 18, 2018 09:57

Initial attempt to use authlib

c5fcf4b

Fixed

95acfb6

Working pretty well - moved oauth2 models and extension to proper fil…

1562c0d

…enames. Tests are mostly failing due to the problems I was having with sessions, but also because there is no OAuth2Grant, and also Flask-login anonymous users seem to be handled differently.

frol requested changes May 24, 2018

View reviewed changes

frol marked this pull request as draft April 4, 2024 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to authlib related to issue #113 - slightly better PR #118

Update to authlib related to issue #113 - slightly better PR #118

bitfinity commented May 22, 2018

frol left a comment

frol May 22, 2018

bitfinity May 25, 2018

frol May 22, 2018

bitfinity May 25, 2018

frol May 22, 2018

bitfinity May 25, 2018

frol May 22, 2018

bitfinity May 25, 2018

frol May 22, 2018

bitfinity May 25, 2018

frol May 24, 2018

bitfinity May 25, 2018

frol May 24, 2018

bitfinity May 25, 2018

frol May 24, 2018

bitfinity May 25, 2018

frol May 24, 2018

bitfinity May 25, 2018

frol May 24, 2018

bitfinity May 25, 2018 •

edited

Loading

bitfinity May 25, 2018


		id = db.Column(db.Integer, primary_key=True) # pylint: disable=invalid-name

		user_id = db.Column(db.ForeignKey('user.id', ondelete='CASCADE'), index=True, nullable=False)

Update to authlib related to issue #113 - slightly better PR #118

Are you sure you want to change the base?

Update to authlib related to issue #113 - slightly better PR #118

Conversation

bitfinity commented May 22, 2018

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfinity May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfinity May 25, 2018 •

edited

Loading