Code

Opened 11 months ago

Closed 2 months ago

#20936 closed Cleanup/optimization (fixed)

When logging out/ending a session, don't create a new, empty session

Reported by: mattrobenolt Owned by: mattrobenolt
Component: contrib.sessions Version: master
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.

See: https://github.com/django/django/pull/1487

Attachments (0)

Change History (9)

comment:1 Changed 11 months ago by ptone

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 11 months ago by mattrobenolt

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 Changed 11 months ago by ptone

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 Changed 11 months ago by mattrobenolt

  • Owner changed from nobody to mattrobenolt
  • Patch needs improvement set
  • Status changed from new to 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 Changed 11 months ago by aaugustin

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 Changed 11 months ago by mattrobenolt

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 Changed 10 months ago by ptone

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 Changed 10 months ago by mattrobenolt

@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 Changed 2 months ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 393c0e24223c701edeb8ce7dc9d0f852f0c081ad:

Fixed #20936 -- When logging out/ending a session, don't create a new, empty session.

Previously, when logging out, the existing session was overwritten by a
new sessionid instead of deleting the session altogether.

This behavior added overhead by creating a new session record in
whichever backend was in use: db, cache, etc.

This extra session is unnecessary at the time since no session data is
meant to be preserved when explicitly logging out.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.