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

User access control does not work well when different computers/browsers are used #556

Closed
AmedeeBulle opened this issue Aug 21, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@AmedeeBulle
Copy link

commented Aug 21, 2014

It took me some time to understand what was happening...

Here is the scenario:

  • On one computer, login in Octoprint with 'Remember me', then close your session (that is, close the browser, but don't logout)
  • On another computer (or a different browser on the same computer), login again and close your session.
  • Go back to the first computer/browser: your are still logged in (as expected), but you have lost privileges -- e.g. you cannot run system actions, they will return 'Error, The command "..." could not be executed'. More shows 'Forbidden'.
  • Try to logout: it is like in Hotel California, you can never leave ;) The only way to logout is to delete your cookies (and btw, I love cookies).

Environement:
OS: raspbian on rasberry pi (no OctoPi)
Octoprint:: Version: 1.2.0-dev-118-gfbd9440

Logs:
Attempt to logout generates a stack trace: https://gist.github.com/AmedeeBulle/f2a02d86aee4f8c8dea0
No other error.

It is a 'fresh' install form a couple of days ago, so all libraries should be current. As it seems related to Flask, i have:

Flask==0.9
Flask-Login==0.2.2
Flask-Principal==0.3.5

My python skill are somewhat limited, can't drill down much more on this...

@AmedeeBulle

This comment has been minimized.

Copy link
Author

commented Aug 21, 2014

Maybe I've been to fast is filing an issue, i eventually found #107 but supposed to be fixed...

foosel added a commit that referenced this issue Aug 29, 2014

Make key removal from session more solid
Do not die a horrible death if the key you are attempting to remove from the session upon logout doesn't exist. Attempt at
fixing #556
@foosel

This comment has been minimized.

Copy link
Owner

commented Aug 29, 2014

@AmedeeBulle I'm sorry, I can't for the life of me reproduce this (tried the exact steps outlined above with the exact same module versions), but given the stack trace I've whipped up a quick possible workaround, see commit e77efde

Could you test if that helps/what it changes?

@AmedeeBulle

This comment has been minimized.

Copy link
Author

commented Aug 29, 2014

Thank you, I'll test that early next week when back home...

@AmedeeBulle

This comment has been minimized.

Copy link
Author

commented Sep 1, 2014

@foosel I tried to be systematic in my testing:

  • Reproduce the problem with my current version
  • Shutdown OctoPrint
  • Pull latest sources (and verified I got e77efde )
  • Install OctoPrint
  • Start the OctoPrint daemon
  • (try to) reproduce

What happens is consistent with the fix:

  • The initial problem remains; when you come back to your first session you are still logged in, but without privilege
  • But now you can successfully logout and re-login to get your privileges back, so it is not blocking anymore.

I don't speak very well Python and don't know much about Flask, so I can't easily debug myself...
... but in the same file src/octoprint/server/api/init.py around line 206 we have:

    elif "passive" in request.values.keys():
            user = current_user
            if user is not None and not user.is_anonymous():
                identity_changed.send(current_app._get_current_object(), identity=Identity(user.get_id()))
                return jsonify(user.asDict())

So if I understand correctly, the 'passive' login method is called from the LoginStateViewModel 'class' when we have a user in the environment (cookie).
In the code above, if there is a user, we grant access immediately.
Couldn't we 'just' check first that there is a valid session/key for this user before granting access?

(This is a shot in the dark, maybe I am saying something stupid -- as I said I have very limited knowledge in this area)

foosel added a commit that referenced this issue Oct 24, 2014

Allow login of the same user from multiple browsers without side effects
So far when logging in from two different browsers, then logging out in one of them the user was logged out across all browsers. This should now be changed in so far as that each individual browser session is tracked and only that session is ended by a logout that belongs to the browser where the logout button was clicked.

Should fix #556
@foosel

This comment has been minimized.

Copy link
Owner

commented Oct 24, 2014

@AmedeeBulle it took me a while to get to the ground of this, and once I understood what was causing this, I immediately could reproduce it. Yes, I know, it's supposed to go the other way around, I don't know why I couldn't reproduce it earlier. Thanks for the thorough report and the patience.

I just pushed a patch (see above) that enables proper session tracking across multiple browsers (that not being there was the core reason here) and should fix this issue (and also allow for some interesting things in the future, like "how many users are logged in right now" which could also contribute to #527). Could you please test if this is indeed the case for you? It would also be great if you could give the whole logging in/out etc stuff a critical but quick test -- I did my best to check through various scenarios, but I've looked on that code now for so long that I might have missed some obvious things (not seeing the forest due to all those trees as we say in .de).

Booli added a commit to Booli/OctoPrint that referenced this issue Dec 23, 2014

Allow login of the same user from multiple browsers without side effects
So far when logging in from two different browsers, then logging out in one of them the user was logged out across all browsers. This should now be changed in so far as that each individual browser session is tracked and only that session is ended by a logout that belongs to the browser where the logout button was clicked.

Should fix foosel#556
@foosel

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2015

Considering this solved due to not getting any reply from OP and my tests so far looked good.

@foosel foosel closed this Jan 19, 2015

@AmedeeBulle

This comment has been minimized.

Copy link
Author

commented Jan 19, 2015

Apologies for no answer...
I am running this version for about a month, and I have not been able to reproduce since then, so indeed this should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.