search

tjcsl / ion

TJ Intranet 3
https://ion.tjhsst.edu
GNU General Public License v2.0
99 stars 91 forks source link

Consider adjusting default Ion session length #771

Closed theo-o closed 5 years ago

theo-o commented 5 years ago

Topic of discussion

Currently, https://github.com/tjcsl/ion/blob/fdd9b6f62275c15240470bda68b360b8118cfa47/intranet/settings/__init__.py#L437 our session lifetime is set to two hours. This setting was chosen rather arbitrarily at the beginning of Ion development and

Proposed options/solutions

I see the following as possible solutions:

Personal opinion (if applicable)

The main question at issue is to what extent do we prioritize the small convenience of long-lived sessions versus protecting against the dangers of long-lived sessions? In my opinion, dropping the lifetime to below an hour would only annoy users while increasing it above lets say eight hours would not be terribly good for security. I believe that the right approach is keeping the lifetime as it is. This is because it satisfies the most prominent use case of long-lived sessions while preserving security. This use case is a hypothetical student logging in to check their eighth period schedules at the end of 7th period (2:15 PM) and then wanting access to Ion until a bit after dismissal. During this time, this hypothetical student would not want to re-authenticate himself/herself, so the student would like a session that lasts two hours from 2:15 PM to 4:15 PM. Hence, a two hour long session would make perfect sense for this usecase. Thoughts?

ghost commented 5 years ago

I mostly agree that we should keep the lifetime at 2 hours. However, I lean towards longer session lifetimes simply for convenience, so I think we should also implement the session management view/Trust this device checkbox as discussed in #559.

With regards to picking a "trusted device" lifetime, I see three options:

  1. The student stays logged in for a long period of time (for example, 30 days), but their session does not renew on every visit (so they have to log in every 30 days no matter what).
  2. The student stays logged in for a shorter period of time (for example, 1 week), but their session is renewed every time they visit Ion.
  3. A hybrid -- the student's session expires after 1 week and is renewed every time they visit, but it expires 30 days after their initial login.

Option 3 is the most secure, but it will probably be hardest to implement, and option 2 is the least secure but will probably require the least modification to existing code. I personally favor option 3. Thoughts on this topic?

theo-o commented 5 years ago

Option 3 sounds good when combined with the changes in #559.

ghost commented 5 years ago

This (I'm lumping in the #559 changes) is proving to be more difficult than previously thought.

Django uses database-backed sessions by default, but in production (not in development environments) we use django-redis-sessions to store sessions in Redis. There appears to be no way to list sessions using django-redis-sessions, unlike there is for sessions stored in the database. This unserstandably presents a serious obstacle to creating a session management page.

Even worse, database sessions do not store any kind of reference to the associated User, so we would have to create another object to associate them -- or else loop through all of the hundreds (perhaps thousands) of sessions looking for one that belongs to the given user every type they open the session management page.

I see several options.

  1. Switch to database-backed sessions and add a UserSession or similar model that associates users with sessions. This would have serious performance issues because we set SESSION_SAVE_EVERY_REQUEST=True, so switching to database sessions would massively increase the number of SQL queries performed on the database.
  2. Add a new TrustedSession model that stores the session key and marks it as trusted. With this implementation, we would not be able to create a general "session management" page. We could have a page with:
    • A list of trusted devices, each with an "Revoke" button
    • A "Trust this device" button that trusts the current device
    • A "Log me out everywhere" button. This could be implemented by saving in each user's session the time they logged in, adding a DateTimeField to users.User that is set to the current time every time this button is pressed, and then requiring the session login time to be newer than the last "logout everywhere time" (and also by revoking all trusted devices when this button is pressed).
ghost commented 5 years ago

@theo-o Thoughts?

theo-o commented 5 years ago

Sorry for the late reply, it fell off my radar. One possible alternative I want to put out there is django-user-sessions with SESSION_ENGINE set to django.contrib.sessions.backends.cached_db (see https://docs.djangoproject.com/en/1.11/topics/http/sessions/).

ghost commented 5 years ago

Unfortunately, I'm pretty sure that using cached_db would have almost the exact same performance issues as option 1 -- SESSION_SAVE_EVERY_REQUEST=True combined with a write-through cache would result in updating a row in the database on every single request. It's probably not a good idea to store sessions in the database at all.