pallets-eco / flask-principal

Identity management for Flask applications
MIT License
496 stars 89 forks source link

Bug in flask-login example #22

Open mehaase opened 11 years ago

mehaase commented 11 years ago

The flask-login example in the documentation is extremely helpful (common use case, I would think) but it has a significant bug in it!

The sample code shows Flask-Principal being registered before Flask-Login, but if you do this, then flask principal will send the identity_loaded message before Flask-Login has had a chance to load the current_user proxy. Therefore, the identity_loaded handler will always return the anonymous user!

I realize it's not meant to be a complete example, but it tripped me up as well as at least one person on Stack Overflow: http://stackoverflow.com/questions/18190067/flask-login-and-principal-current-user-is-anonymous-even-though-im-logged-in/18935921

It can be fixed simply by reversing the order of Principal(app) and login_manager = LoginManager(app).

mattupstate commented 11 years ago

Right on. Thanks for catching this!

mehaase commented 10 years ago

Thanks for addressing my comment, however I'm a little surprised (and stumped) at the fix. I expected a simple change to the documentation, but instead it looks like you tried to make the order of flask plugins not matter.

I'm still digesting what exactly changed here (and I noticed this issue on a test server, so I haven't had time to look at it on a dev server yet), but Flask-Login 0.2.9 is creating an infinite loop:

[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/share/foo/lib/foo/bootstrap.py", line 69, in load_user
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     identity_changed.send(app, identity=Identity(user.id))
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/blinker/base.py", line 267, in send
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     for receiver in self.receivers_for(sender)]
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_principal.py", line 469, in _on_identity_changed
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     self.set_identity(identity)
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_principal.py", line 418, in set_identity
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     self._set_thread_identity(identity)
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_principal.py", line 463, in _set_thread_identity
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     identity=identity)
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/blinker/base.py", line 267, in send
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     for receiver in self.receivers_for(sender)]
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/share/foo/lib/foo/views/user.py", line 196, in on_identity_loaded
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     user = current_user._get_current_object()
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/werkzeug/local.py", line 297, in _get_current_object
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     return self.__local()
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_login.py", line 46, in <lambda>
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     current_user = LocalProxy(lambda: _get_user())
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_login.py", line 768, in _get_user
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     current_app.login_manager._load_user()
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_login.py", line 348, in _load_user
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     return self.reload_user()
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/local/lib/python2.7/dist-packages/flask_login.py", line 312, in reload_user
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     user = self.user_callback(user_id)
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]   File "/usr/share/foo/lib/foo/bootstrap.py", line 69, in load_user
[Mon Feb 10 19:20:15 2014] [error] [client 4.31.164.110]     identity_changed.send(app, identity=Identity(user.id))

You can see that load_user() ends up calling... load_user(). On my test server, this repeats until I get a RuntimeError: maximum recursion depth exceeded.

This is a new test server, so it took me quite a while to figure out what was going on. (I thought I had botched some step of the deployment process.) Eventually I tried using pip to remove Flask-Login 0.2.9 and replace it with 0.2.7 instead. Boom, problem fixed.

I will post back when I have more details, but my gut reaction is that the documentation still doesn't match the reality of what Flask-Login is actually doing.

joshfriend commented 9 years ago

I was having trouble with this for a while as well. Because I don't want to use the session for user login in a REST api, I can't call login_user from Flask-Login because that tries to set a session cookie. Thus, the current_user is unset until after load_user_from_request returns so I can't call principal.set_identity from inside the request loader without triggering the recursive loop. I solved the issue by adding an intermediate signal callback to catch the user_loaded_from_request signal which sets the identity:

@login_manager.request_loader
def load_user_from_request(request):
    user = get_user(request)
    return user

@user_loaded_from_request.connect
def on_user_loaded_from_request(sender, user):
    principal.set_identity(Identity(user.id))

@identity_loaded.connect
def on_identity_loaded(sender, identity):
    identity.user = current_user
    identity.provides.add(UserNeed(current_user.id))
    # Etc...

It would be nice if there was a way to configure Flask-Login to not use the session at all.

bodgit commented 8 years ago

Thanks @joshfriend for your example. I've been trying to tie Flask-Login/Flask-Principal together for a REST API yet I couldn't get the on_identity_loaded method to work if I attempt anything with current_user, I realise now this causes some sort of recursive loop. :+1:

joshfriend commented 8 years ago

this causes some sort of recursive loop.

Yes. You cannot use current_user inside login_manager.request_loader because the value that the current_user proxy references is not set until a value is returned from the request_loader. Referencing current_user before it is set causes flask-login to try to load the user from the request by calling request_loader and... :boom: :boom: :boom: