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

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Contributor

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.

@ptone ptone and 1 other commented on an outdated diff Aug 19, 2013
django/contrib/auth/__init__.py
@@ -101,7 +101,7 @@ def logout(request):
user = None
user_logged_out.send(sender=user.__class__, request=request, user=user)
- request.session.flush()
+ request.session.end()
ptone
ptone Aug 19, 2013 Member

since flush is still a thing - need a tiny tweak to the docstring for this method

mattrobenolt
mattrobenolt Aug 26, 2013 Contributor

__doc__ has been updated.

Contributor

@ptone bump. :)

Member
ptone commented Sep 3, 2013

I think the only thing remaining is a very quick mention of the change in the release notes

Contributor

@ptone, would you like me to add that as well? Are there relevant docs that should be updated to reflect this behavior?

Member
ptone commented Sep 3, 2013

Yes - the end() method probably needs to be added to https://docs.djangoproject.com/en/dev/topics/http/sessions/

I can do those two items if you want

Contributor

@ptone, adjusted based on our previous IRC conversation. Removes the new end() method and just changed the behavior of flush() so it doesn't create.

Which release notes should I add this to?

Owner

It would be the 1.7 release notes. It could be useful to mention it in docs/topics/http/sessions.txt as well with a .. versionchanged:: 1.7 marker.

@mattrobenolt mattrobenolt Fixed #20936: When logging out/ending a session, don't create a new, …
…empty session

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.
5864ccc
Contributor

@timgraham How does that look?

@timgraham timgraham commented on the diff Sep 7, 2013
docs/releases/1.7.txt
@@ -214,6 +214,10 @@ Minor features
* The :ttag:`widthratio` template tag now accepts an "as" parameter to capture
the result in a variable.
+* Session cookie now gets deleted after calling
+ :meth:`~django.contrib.sessions.backends.base.flush()`. Also, a new
+ session is created lazily.
timgraham
timgraham Sep 7, 2013 Owner

clarify what "created lazily" means?

@timgraham timgraham commented on the diff Sep 7, 2013
docs/releases/1.7.txt
@@ -214,6 +214,10 @@ Minor features
* The :ttag:`widthratio` template tag now accepts an "as" parameter to capture
the result in a variable.
+* Session cookie now gets deleted after calling
timgraham
timgraham Sep 7, 2013 Owner

I'd rephrase: The session cookie is now deleted after flush() is called.

@timgraham timgraham commented on the diff Sep 7, 2013
docs/topics/http/sessions.txt
@@ -222,8 +222,9 @@ You can edit it multiple times.
.. method:: flush
- Delete the current session data from the session and regenerate the
- session key value that is sent back to the user in the cookie. This is
+ .. versionchanged:: 1.7
timgraham
timgraham Sep 7, 2013 Owner

put .. versionchanged:: 1.7 at the bottom of the section and add a sentence under it to clarify what exactly changed

@timgraham timgraham commented on the diff Sep 7, 2013
docs/topics/http/sessions.txt
@@ -222,8 +222,9 @@ You can edit it multiple times.
.. method:: flush
- Delete the current session data from the session and regenerate the
- session key value that is sent back to the user in the cookie. This is
+ .. versionchanged:: 1.7
+
+ Delete the current session data from the session and deletes the session cookie entirely. This is
timgraham
timgraham Sep 7, 2013 Owner

wrap line at 80 chars. "entirely" seems uncessary

Owner

Please rebase the branch so there isn't a merge conflict. Also the release notes are broken into sections so the note should go under "django.contrib.sessions" instead of "templates." Thanks!

Owner

Hi Matt, would you have a chance to update this PR so we can merge it?

Contributor

Oh derp, I just need to add documentation for the change, right?

Owner

Yes, it may also need to be rebased to merge cleanly.

Contributor

Cool, will take care of that today. Thanks for the reminder. :)

Member
ramiro commented May 12, 2014

Merged in 393c0e2.

Thank you very much for your work on this.

@ramiro ramiro closed this May 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment