Opened 7 years ago

Last modified 2 years ago

#11506 new Bug

session.flush should not delete the old session

Reported by: Glenn Maynard Owned by: nobody
Component: contrib.sessions Version: master
Severity: Normal Keywords:
Cc: glenn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Flushing and cycling the session should empty the data in the session and create a new key, but should not delete the old key.

Scenario:

1: JS kicks off a periodic AJAX request to update something, which is delayed in transit.

2: User submits an AJAX login form, which calls auth.login, calling session.flush or session.cycle_key. The AJAX response sets a new session cookie for the user.

3: The async request from #1 makes it to the server. This still has the old cookie, since it started before #2 finished. contrib.session doesn't recognize the cookie, since the previous request deleted it. It thinks it's an expired or corrupt session cookie, and flushes the session again.

#2 logs the user in, then #3 logs the user back out. (I've seen this happen even without AJAX logins, when using django.views.static.serve in development.)

session.flush should leave the old session in the database, and just clear its data. That way, when #3 comes around, it won't be an unrecognized session, and it won't trigger a session flush. Let the old session row expire on its own, like any idle session.

This doesn't change the definition of the function: "Removes the current session data from the database and regenerates the key."

This patch also fixes and tests session.cycle_key() raising an error if no session already existed; accessing self._session_cache raises AttributeError. This was triggering while I was writing the main test.

Attachments (1)

django-session-flush.diff (5.5 KB) - added by Glenn Maynard 7 years ago.

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by Glenn Maynard

Attachment: django-session-flush.diff added

comment:1 Changed 7 years ago by Glenn Maynard

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

As a followup: this will still fail if the delayed request modifies the session, since it'll refresh the cookie.

A fix would be to update the cookie only when the session hasn't been updated in over some timeout (say, a minute); this is long enough to avoid this race condition. This would have the nice side benefit of not sending a Set-Cookie header for each and every request that modifies the session.

It's harder to implement cleanly, though, since the session rows hold an expiry date, not a last-saved date. You can't reliably derive one from the other after the fact, since the session expiry setting might have changed. I'll leave this for further discussion.

comment:2 Changed 7 years ago by Glenn Maynard

Cc: glenn@… added

The test case should probably use django.test.client, but until there's some indication of interest, I'll hold off on further updates.

comment:3 Changed 7 years ago by Alex Gaynor

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:5 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

comment:8 Changed 2 years ago by Tomek Paczkowski

Patch needs improvement: set

Patch no longer applies. As the ticket is 5 years old it would be worth checking that the problems still occurs.

Note: See TracTickets for help on using tickets.
Back to Top