mbr / flask-kvsession

A drop-in replacement for Flask's session handling using server-side sessions.
http://pythonhosted.org/Flask-KVSession/
MIT License
168 stars 53 forks source link

Invalidating sessions based on time inactive instead of time created #23

Closed vimalloc closed 10 years ago

vimalloc commented 10 years ago

I want to set this up so that each time a user loads a page, it keeps track of when that was, and if a timeout (PERMANENT_SESSION_LIFETIME probably) is greater then that, clear the session.

As it stands, it looks like the created timestamp is encoded in they key for this entry. Thus, to update the timestamp when the user accesses a page, it would need to get the data from that kv map, create a new entry with a new timestamp encoded in the key, delete the old key, and get the new cookie to the client. Yuck.

I was thinking that I would do something instead where there were two fields in the kv map.

Where the first one has the session data and the second one mapped to a timestamp for the last time a page was accessed. Then I can just add a "update timestamp" call in app.before_requeset() to keep the timestamp up to date, and have clear_sessions() remove sessions accordingly.

What are your thoughts on this? Does this seem like a good way to go about it? Would you be interested in a patch that enables this as an option, or should I make this just for me?

vimalloc commented 10 years ago

Turns out I can update the timestamp in the open/save session functions. I don't need to do this in the app.before_requeset() like I initially thought.

mbr commented 10 years ago

It may just be simpler to store a "last activity" timestamp independant of session lifetime inside the session. Everytime a user visits a page, you can record that.

The session will keep its timestamp, but the contents will be updated, which is just normal session operation.

Do you want to refresh a session on each page-load? This does require a write each time someone visits a page and that write cannot be avoided in that case. A compromise could be: Check how old the session is on each request (a read). If it's older than half its lifetime, use regenerate() to create a new session.

Other than that, I'd love to know what you're trying to achieve with this functionality first, to give some better advice.

Note: I've also released 0.5 a few hours ago with more time-to-live features built-in. Maybe it does what you need.

vimalloc commented 10 years ago

The big picture for what I'm trying to do is setup a site where if a user doesn't access a page for 30 minutes, he gets logged out. If he does access a page, then the 30 minute 'countdown' starts over. Basically, logging users out after 30 minutes of inactivity.

In a nutshell, this patch adds another field to the kvmap, a redis db in my case, that has the timestamp for the session (so that it isn't encoded in the in key with _). It also provides an option to update that timestamp every time a user accesses a page. In my case, this site isn't going to get a lot of traffic, so the write on page access an acceptable cost. I do like your idea about regenerating if over half the lifetime is up though.

Thanks for the release by the way, this code is awesome :). I'll look through it later to see if it can provide functionality along these lines. If it doesn't already, and you are interested in a patch for this, I would be happy to update it for the 5.0 release and submit a pull request. Otherwise I can just keep it for my project.

Cheers!

mbr commented 10 years ago

Frankly, if your site does not get a lot of traffic, this is fairly straightforward to handle with the existing functionality:

  1. Set the session lifetime to 30 minutes.
  2. On a before_request (unsure about this one, but in a hook that gets called every time when the session is already loaded), just call session.regenerate(). You'll have about the same cost (writing a small session + a new redis key) and it will cause sessions to get a "fresh" 30 minutes on each request.

Closing this for now, let me know if that doesn't work for you for some reason!

vimalloc commented 9 years ago

Hey mbr,

Sorry to bring this back up again. I've been using your idea of calling session.regenerate() on a before requiest, and it has worked pretty well. However, I've discovered that if you refresh a page to quickly you will lose the session on your browser (ie, get logged out). This has caused a couple minor issues for me.

I've thrown together some code to deminstrate this (you can run it and hold ctrl-r in a browser to see the session data is lost). If you remove the session_regenerate() call then the session data is never lost:

from flask import Flask, session, url_for, redirect
from datetime import timedelta
from simplekv.memory import DictStore
from flask_kvsession import KVSessionExtension

app = Flask(__name__)
app.secret_key = 'foobar'

@app.route('/')
def hello_world():
    if 'foo' not in session:
        session['foo'] = 'bar'
    return redirect(url_for('session_set'))

@app.route('/session_set')
def session_set():
    if 'foo' in session:
        return "session has previously been set"
    else:
        return "session is now unset :/"

@app.before_request
def regenerate_session():
    session.regenerate()

if __name__ == '__main__':
    store = DictStore()
    app.permanent_session_lifetime = timedelta(minutes=30)
    KVSessionExtension(store, app)
    app.run()

Do you have any ideas for getting this working using the existing code base, or alternately would you be interested in seeing a patch for this behaivor? I would be happy to write something up to fix this, but wanted to touch base with you before doing anything, as I know most people using KVSessions aren't using it in quiet the same way I am here.

Cheers.

ln2khanal commented 7 years ago

@vimalloc, I am suffering from the similar type of problem. I wonder why nobody has replied to your query yet. Permanent session life time & inactivity timeout need to work together which is not with flask_kvsession. Hope to hear any sort of solution from anyone else.

Thanks!

mbr commented 7 years ago

I've ran the example code and I can reproduce the problem. Debugging this shows me that occasionally sessions are "forgotten", i.e. I can get four requests in a row that will start off of the same dictionary of sessions, add theirs. E.g. if the new session IDs are [A, B, C, D] and before those four requests the sessions X and Y existed, I'd end up with [X, Y, A], [X, Y, B], [X, Y, C] and [X, Y, D].

This seems like a race condition in Flask (which may be indicating that I am using the Session API wrong). The issue here is that DictStorage is not thread-safe. The only thread_safe storages I would recommend are redis and possibly Postgres. The file-backed storage might work as well, but do not count on it. Do these problems disappear when using Redis?

SvenFestersen commented 6 years ago

@ln2khanal I'm with you on this. I recently started to use flask_kvsession as a drop-in replacement for the normal session interface. It turns out that this problem makes it less of a simple "drop-in" because the behaviour is different from the original, as SESSION_REFRESH_EACH_REQUEST is ignored.

@mbr As far as I understand, the reason for this is that the validity of the session is checked by comparing the session creation time that is encoded in the session key to the current time. Since the session id is usually never updated, this results in invalidation of the session after creation_time + life_time instead of last_activity_time + life_time.

Of course this can be circumvented by calling session.regenerate() on each request, but wouldn't it make more sense to store the expiration time in the backend and check session validity against that (if SESSION_REFRESH_EACH_REQUEST is set)?

If such a feature would have the chance to be accepted, I'm more than willing to implement it.