#20936 closed Cleanup/optimization (fixed)
When logging out/ending a session, don't create a new, empty session
Reported by: | Matt Robenolt | Owned by: | Matt Robenolt |
---|---|---|---|
Component: | contrib.sessions | Version: | dev |
Severity: | Normal | Keywords: | session, logout, auth |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Previously, when logging out, the existing session is overwritten by a new sessionid instead of deleting the session all together.
This behavior adds overhead by creating a new session record in whichever backend being used, db, cache, etc.
This extra session is unnecessary at the time since no session data is meant to be preserved when explicitly logging out.
Change History (11)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
ptone, do you have an opinion about modifying SessionStore.flush() to take over the behavior of what I introduced as SessionStore.end? That was my original intent, and inside Django, logout is the only place this use exists.
Not sure if this would break for others in the wild since this is technically a documented API and may have some expected behavior. In my opinion, doing this end/flush thing is wrong, and flush should just do the job.
comment:3 by , 11 years ago
mattrobenolt looking at this, and if I'm reading things right - won't the session middleware just try to save a new session on process_response?
session.clear() sets modified=True (inside end or flush)
and process_response will request.session.save()
- also in your patch you set the _session_key
to ''
for the db backend, while most of the session module tests explicitly for None
this actually makes it less purposeful for auth to be creating it via flush, as if you want to use sessions in a custom way, you could use your own middleware that does something different.
not sure on changing flush vs adding end. No way to deprecate a behavior, only the whole method, always surprising how people are using expected behavior.
comment:4 by , 11 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
Status: | new → assigned |
You are right, mostly. It's desirable to set the _session_key
to ''
because that tells the browser to effectively remove the data. Then the step that I've missed, is sending it back an expiry time in the past. So basically time.time() - 1
. I hope that makes sense. The combination of those two will tell the browser to actually delete the cookie.
With these combined, the process_response
will behave correctly. I'm going to spend a little more time on this to get it to set an expiry to a date in the past. While I'm at it, I'm going to check out what Django does when you explicitly delete cookies anyways. I have a hunch that this is all behaving slightly wonky, but I don't know for sure off thet op of my head.
comment:5 by , 11 years ago
FYI I remember (but can't locate right now) a ticket about preserving the language, which is stored in the session, after logout. It's related to this discussion.
comment:6 by , 11 years ago
I've updated my patch to update some of the logic. To me, it looks good.
@aaugustin, this doesn't affect any existing logic around that. The middleware is what's keeping track of deleting the session or not. If the language was preserved, the session would not be empty, meaning it wouldn't get deleted.
comment:7 by , 11 years ago
The language question is a specific instance of a general problem of an "outer session", that is a session that transcends user login/logout.
We already have this notion established by preserving session data across login - anything added to the session while an anon user is logged in is "upgraded" into the authed session.
On logout, the session is flushed. This currently seems watertight as far as any data-leakage, so removing the cookie on the client doesn't add any security value.
Agreed that anything that would preserve "outer session" state on logout will have no issue with this patch, as the session is lazily recreated if accessed (an explicit create() is not required).
There is some small optimization in not creating a session in the DB that you are likely not to use. @mattrobenolt - is there a benefit of removing the cookie beyond just good housekeeping?
Because auth.logout is the one to call and end to a session, there could be a test added to verify that this happens. The current test just checks for the auth session key - and there was never a test to check the current flush behavior - but it would be a nice extra touch to the patch.
I can't see any possible security issue here at all, but wouldn't mind a quick glance from Donald or Paul - I'll see if I can't recruit them to have a look.
comment:8 by , 11 years ago
@ptone, the performance issue isn't a huge concern since we don't use the db for sessions, so it's not something that affects us in that regard. Was just an unexpected behavior.
The main concern here is dealing with upstream caches. Speaking from the context of Varnish, if a Cookie header is present, it wants to skip caches, which is a good thing in general since you don't want to cache too much. But when someone logs out, we have no real way of determining the difference between an empty session, and a user that actually has session data.
Again, it's just a very unexpected behavior. In my opinion, if the session is empty, and the framework is explicitly deleting the session, we should just destroy the cookie as well until someone sets new session data.
comment:9 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
this makes sense, no reason to build a new empty session on logout - PR looks good on read through, but can't review/run the tests now this late