yourlabs / django-session-security

A little javascript and middleware work together to ensure that the user was active during the past X minutes in any tab he has open. Otherwise, display a warning leaving a couple of minutes to show any kind of activity like moving the mouse. Otherwise, logout the user.
http://django-session-security.rtfd.org
MIT License
309 stars 142 forks source link

Check if session expired on activity #113

Closed rbntimes closed 6 years ago

rbntimes commented 6 years ago

The issue

We were first facing this problem when our customers experienced problems w/ our application inside Citrix. I assume this is because Citrix does not handle the javascript counting when it is idle. The session expired on the backend but any activity (i.e. scrolling, mouseEvents) extended the time (approximately 30-60s) until the user was redirected to the logout. We were also able to reproduce this (on osx at least) by putting it asleep.

Reproducing

To reproduce this you can use the following:

SESSION_SECURITY_EXPIRE_AFTER = 180
SESSION_SECURITY_WARN_AFTER = 120

After putting your macbook to sleep for more then 180s your session should be expired and the user should be redirected by the expire function. Instead the nextping is recalculated over the remaining time and until then the user won't be redirected but will be able to continue browsing until a hard refresh is done. So any ajax calls won't be triggered as the user isn't signed in but will still be able to be pressed by the user.

Hoping the issue is clear.

rbntimes commented 6 years ago

Is there any chance that this pr can be reviewed once more?

The issue still exists and is quite frustrating for our users since there is a form of data loss.

What happens is that when users unlock there computer when the timer has expired under water, the javascript timer won't know this and will continue to reset if there is any user movement. So if the user is active between unlocking the computer and the 'apply' function that fires after approximately 45 seconds of inactivity after unlocking, e.g. filling in a form, the data cannot be saved because the user is logged out already a 401 is returned. The user does not see what's wrong and will get logged out once it clicks on a redirecting link or after the 45 seconds of inactivity. And that results in the user getting logged out without a warning and data loss.

I could make a gif of what happens if necessary? Unsure if the issue is clear enough

Any update would be great!

jpic commented 6 years ago

If no objection, I'll merge next week (please ping again !)

claytondaley commented 6 years ago

Why isn't the idleFor check prioritized over is(:visible)? Consider the following sequence:

It seems to me that we'll follow the is(:visible) branch even though the user has idled-out. I have not tested so maybe you've run into of a case where this makes sense.

rbntimes commented 6 years ago

You are right, if the idle time expires with the "warning" box open, it does not need to hide the warning only to be redirected to /login about a second later.

And since the warning box won't even be shown when the computer is put to sleep it should prioritise checking the idleFor time to see if the user's session has expired.

I've also moved the hideWarning function so it does not keep trying to hide a warning that does not exist. It was being called on every activity, that seemed unnecessary.

rbntimes commented 6 years ago

@claytondaley @jpic

What do you think? Any suggestions?

claytondaley commented 6 years ago

Haven't tested it, but otherwise LGTM

rbntimes commented 6 years ago

@jpic Did you maybe have the opportunity to check it out :)?

rbntimes commented 6 years ago

Sorry not trying to spam or be pushy, but was wondering if anyone had the chance to look at this. Kind of wish to be able to use this as some user data still gets lost at this time (not all the time but it happens more often than it should). Thanks anyways!

jpic commented 6 years ago

At the :beach: please bump again next week !

Le lun. 6 août 2018 à 08:56, rbntimes notifications@github.com a écrit :

Sorry not trying to spam or be pushy, but was wondering if anyone had the chance to look at this. Kind of wish to be able to use this as some user data still gets lost at this time (not all the time but it happens more often than it should). Thanks anyways!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yourlabs/django-session-security/pull/113#issuecomment-410607302, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFxrC2YSuClwVzgYtG8QO__dzKID5G0ks5uN-iagaJpZM4UDBgD .

rbntimes commented 6 years ago

Hope you had a great one!

Any chance to still look at this :)?

rbntimes commented 6 years ago

Just leaving this here, hope anyone here has the time today or this week to look at it. Would be really useful!

jpic commented 6 years ago

@rbntimes if you'll be available in the following weeks to support the change in case of user feedback i can release now

rbntimes commented 6 years ago

@jpic I most definitely am :)

jpic commented 6 years ago

Live on 2.6.1 release, have fun !