Update to authlib related to issue #113 - slightly better PR by bitfinity · Pull Request #118 · frol/flask-restplus-server-example

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

    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

    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


    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


		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)

Conversation

bitfinity commented May 22, 2018

Uh oh!

frol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitfinity May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bitfinity May 25, 2018 •

edited

Loading