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.

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

Bug in flask-login example #22

Open
mehaase opened this issue Sep 21, 2013 · 5 comments
Open

Bug in flask-login example #22

mehaase opened this issue Sep 21, 2013 · 5 comments

Comments

@mehaase
Copy link

mehaase commented Sep 21, 2013

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
Copy link
Collaborator

Right on. Thanks for catching this!

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Jan 2, 2014
Version 0.2.9-dev
-----------------

Released on December 28th, 2013

- Fixes anonymous user assignment.
- Fixes localization in Python 3.


Version 0.2.8
-------------

Released on December 21st 2013

- Support login via authorization header. This allows login via Basic Auth, for
  example. Useful in an API presentation context.
- Ability to override user ID method name. This is useful if the ID getter is
  named differently than the default.
- Session data is now only read when the user is requested. This can be
  beneficial for cookie and caching control when differenting between
  requests that use user information for rendering and ones where all users
  (including anonymous) get the same result (e.g. static pages)
- BREAKING: User *must* always be accessed through the ``current_user``
  local. This breaks any previous direct access to ``_request_ctx.top.user``.
  This is because user is not loaded until current_user is accessed.
- Fixes unnecessary access to the session when the user is anonymous
  and session protection is active.
  see maxcountryman/flask-login#120
- Fixes issue where order dependency of applying the login manager
  before dependent applications was required.
  see pallets-eco/flask-principal#22
- Fixes Python 3 ``UserMixin`` hashing.
- Fixes incorrect documentation.
jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Jan 20, 2014
Version 0.2.9-dev
-----------------

Released on December 28th, 2013

- Fixes anonymous user assignment.
- Fixes localization in Python 3.


Version 0.2.8
-------------

Released on December 21st 2013

- Support login via authorization header. This allows login via Basic Auth, for
  example. Useful in an API presentation context.
- Ability to override user ID method name. This is useful if the ID getter is
  named differently than the default.
- Session data is now only read when the user is requested. This can be
  beneficial for cookie and caching control when differenting between
  requests that use user information for rendering and ones where all users
  (including anonymous) get the same result (e.g. static pages)
- BREAKING: User *must* always be accessed through the ``current_user``
  local. This breaks any previous direct access to ``_request_ctx.top.user``.
  This is because user is not loaded until current_user is accessed.
- Fixes unnecessary access to the session when the user is anonymous
  and session protection is active.
  see maxcountryman/flask-login#120
- Fixes issue where order dependency of applying the login manager
  before dependent applications was required.
  see pallets-eco/flask-principal#22
- Fixes Python 3 ``UserMixin`` hashing.
- Fixes incorrect documentation.
@mehaase
Copy link
Author

mehaase commented Feb 10, 2014

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.

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Mar 12, 2014
Version 0.2.9-dev
-----------------

Released on December 28th, 2013

- Fixes anonymous user assignment.
- Fixes localization in Python 3.


Version 0.2.8
-------------

Released on December 21st 2013

- Support login via authorization header. This allows login via Basic Auth, for
  example. Useful in an API presentation context.
- Ability to override user ID method name. This is useful if the ID getter is
  named differently than the default.
- Session data is now only read when the user is requested. This can be
  beneficial for cookie and caching control when differenting between
  requests that use user information for rendering and ones where all users
  (including anonymous) get the same result (e.g. static pages)
- BREAKING: User *must* always be accessed through the ``current_user``
  local. This breaks any previous direct access to ``_request_ctx.top.user``.
  This is because user is not loaded until current_user is accessed.
- Fixes unnecessary access to the session when the user is anonymous
  and session protection is active.
  see maxcountryman/flask-login#120
- Fixes issue where order dependency of applying the login manager
  before dependent applications was required.
  see pallets-eco/flask-principal#22
- Fixes Python 3 ``UserMixin`` hashing.
- Fixes incorrect documentation.
jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Oct 11, 2014
Version 0.2.9-dev
-----------------

Released on December 28th, 2013

- Fixes anonymous user assignment.
- Fixes localization in Python 3.


Version 0.2.8
-------------

Released on December 21st 2013

- Support login via authorization header. This allows login via Basic Auth, for
  example. Useful in an API presentation context.
- Ability to override user ID method name. This is useful if the ID getter is
  named differently than the default.
- Session data is now only read when the user is requested. This can be
  beneficial for cookie and caching control when differenting between
  requests that use user information for rendering and ones where all users
  (including anonymous) get the same result (e.g. static pages)
- BREAKING: User *must* always be accessed through the ``current_user``
  local. This breaks any previous direct access to ``_request_ctx.top.user``.
  This is because user is not loaded until current_user is accessed.
- Fixes unnecessary access to the session when the user is anonymous
  and session protection is active.
  see maxcountryman/flask-login#120
- Fixes issue where order dependency of applying the login manager
  before dependent applications was required.
  see pallets-eco/flask-principal#22
- Fixes Python 3 ``UserMixin`` hashing.
- Fixes incorrect documentation.
@joshfriend
Copy link

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
Copy link

bodgit commented Nov 18, 2015

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. 👍

@joshfriend
Copy link

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... 💥 💥 💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants