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

session_security breaks "End Session" on the current session when using user_sessions #65

Open
sdann opened this issue Jan 4, 2017 · 5 comments
Labels

Comments

@sdann
Copy link

sdann commented Jan 4, 2017

The middleware

session_security.middleware.SessionSecurityMiddleware

Provides a session last_activity update on each http request, to decrease chance of session logout.

When using the user_sessions middleware alone, clicking "End Session" will behave the same way as "Logout". Unfortunately, when combined with the session_security middleware, clicking "End Session" on the current session has no effect.

With some pdb tracing, I've figured out the following rough series of events:

  1. request:session_security updates the last_activity in the session
  2. request:user_sessions deletes the session object
  3. response:user_sessions detects the session was modified in step 1) and re-saves the session to the backing DB

The user is redirected to the same Session List page, with their current session still active.

I've opened an issue in session_security and have a proposed fix for that code base, which will add a configuration change that disables the last_activity update on the session_delete view.

yourlabs/django-session-security#89

Tracking here to update user_sessions documentation if/when my proposed fix is accepted.

@Bouke
Copy link
Collaborator

Bouke commented Jan 5, 2017

That's quite a debug session in order to find that! I've made a minor change in 317e337 to logout a user when he's trying to delete his current session. This should set the session_key to None (request.session.flush()), which prevents the session being re-written to db.

However this might also point to another issue. What if a user ends all other sessions, and the above happens? Will there still be a new session object written to db in step 3? If so, that could be quite major issue.

@sdann
Copy link
Author

sdann commented Jan 6, 2017

@Bouke Yeah, it took a while stepping through code. Your change looks like it should fix the issue.

In my testing, there's no conflict between the two middleware modules when deleting other sessions, because the session_security module only updates the last_activity on the current session. So deleting other sessions works as expected.

@Bouke Bouke added the bug label Jan 17, 2017
@Bouke
Copy link
Collaborator

Bouke commented Jan 17, 2017

I'm thinking along these lines;

  1. Session A clicks "end all other sessions"
  2. Session B clicks "heavy page" (a page that requires a while to process)
  3. Server starts processing both A and B (multi-threaded / -process server)
  4. Server completes request for A, removing all sessions except A.
  5. Server completes request B, however the session was modified, so this triggers the middleware to update the session in the database
  6. Will it realise that there is no session and accept no further requests? Or will it store the session as a new record in the database, effectively by-passing the forced log-out?

Maybe this could be reproduced by artificially introducing a delay in a view (time.sleep(10)) and would also require a multi-threaded server.

@sdann
Copy link
Author

sdann commented Jan 19, 2017

Yeah, that sounds like a possible race in this scenario. The race exists even deleting a single session, if there's another ongoing request for that session, and session_security middleware is in use.

You'd need to mark each session as "dying" in the DB, so as to fail the save() on request B. Though, in practice this won't occur often, and is detectable by the user refreshing the /account/sessions/ page.

@Lionqueen94
Copy link

Lionqueen94 commented Dec 6, 2018

Hi there, this is an old bug, but I noticed this behaviour today.
When ending all other sessions and then your current session, this error is returned:

TypeError at /account/sessions/svxqrcy1ofr0yiyyoa8hukee6elmu6pt/delete/
argument of type 'NoneType' is not iterable

I also use the combination of session_security and user_sessions.
Somehow the logout is not triggered but the session gets removed.

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

No branches or pull requests

3 participants