diff --git a/AUTHORS b/AUTHORS index 8e10fff33d..81f81e26ec 100644 --- a/AUTHORS +++ b/AUTHORS @@ -720,6 +720,7 @@ answer newbie questions, and generally made Django that much better: Tom Insam Tommy Beadle Tom Tobin + Tore Lundqvist torne-django@wolfpuppy.org.uk Travis Cline Travis Pinney diff --git a/django/contrib/sessions/backends/base.py b/django/contrib/sessions/backends/base.py index 718cc54215..bb9bcdac94 100644 --- a/django/contrib/sessions/backends/base.py +++ b/django/contrib/sessions/backends/base.py @@ -28,6 +28,13 @@ class CreateError(Exception): pass +class UpdateError(Exception): + """ + Occurs if Django tries to update a session that was deleted. + """ + pass + + class SessionBase(object): """ Base class for all Session classes. @@ -301,7 +308,8 @@ class SessionBase(object): key = self.session_key self.create() self._session_cache = data - self.delete(key) + if key: + self.delete(key) # Methods that child classes must implement. @@ -323,7 +331,8 @@ class SessionBase(object): """ Saves the session data. If 'must_create' is True, a new session object is created (otherwise a CreateError exception is raised). Otherwise, - save() can update an existing object with the same key. + save() only updates an existing object and does not create one + (an UpdateError is raised). """ raise NotImplementedError('subclasses of SessionBase must provide a save() method') diff --git a/django/contrib/sessions/backends/cache.py b/django/contrib/sessions/backends/cache.py index 268e06576a..602596073d 100644 --- a/django/contrib/sessions/backends/cache.py +++ b/django/contrib/sessions/backends/cache.py @@ -1,5 +1,7 @@ from django.conf import settings -from django.contrib.sessions.backends.base import CreateError, SessionBase +from django.contrib.sessions.backends.base import ( + CreateError, SessionBase, UpdateError, +) from django.core.cache import caches from django.utils.six.moves import range @@ -55,8 +57,10 @@ class SessionStore(SessionBase): return self.create() if must_create: func = self._cache.add - else: + elif self._cache.get(self.session_key) is not None: func = self._cache.set + else: + raise UpdateError result = func(self.cache_key, self._get_session(no_load=must_create), self.get_expiry_age()) diff --git a/django/contrib/sessions/backends/db.py b/django/contrib/sessions/backends/db.py index ae2470e120..7ed6945431 100644 --- a/django/contrib/sessions/backends/db.py +++ b/django/contrib/sessions/backends/db.py @@ -1,8 +1,10 @@ import logging -from django.contrib.sessions.backends.base import CreateError, SessionBase +from django.contrib.sessions.backends.base import ( + CreateError, SessionBase, UpdateError, +) from django.core.exceptions import SuspiciousOperation -from django.db import IntegrityError, router, transaction +from django.db import DatabaseError, IntegrityError, router, transaction from django.utils import timezone from django.utils.encoding import force_text from django.utils.functional import cached_property @@ -83,11 +85,15 @@ class SessionStore(SessionBase): using = router.db_for_write(self.model, instance=obj) try: with transaction.atomic(using=using): - obj.save(force_insert=must_create, using=using) + obj.save(force_insert=must_create, force_update=not must_create, using=using) except IntegrityError: if must_create: raise CreateError raise + except DatabaseError: + if not must_create: + raise UpdateError + raise def delete(self, session_key=None): if session_key is None: diff --git a/django/contrib/sessions/backends/file.py b/django/contrib/sessions/backends/file.py index 9170615f5a..145eeb3f6d 100644 --- a/django/contrib/sessions/backends/file.py +++ b/django/contrib/sessions/backends/file.py @@ -7,7 +7,7 @@ import tempfile from django.conf import settings from django.contrib.sessions.backends.base import ( - VALID_KEY_CHARS, CreateError, SessionBase, + VALID_KEY_CHARS, CreateError, SessionBase, UpdateError, ) from django.contrib.sessions.exceptions import InvalidSessionKey from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation @@ -129,15 +129,17 @@ class SessionStore(SessionBase): try: # Make sure the file exists. If it does not already exist, an # empty placeholder file is created. - flags = os.O_WRONLY | os.O_CREAT | getattr(os, 'O_BINARY', 0) + flags = os.O_WRONLY | getattr(os, 'O_BINARY', 0) if must_create: - flags |= os.O_EXCL + flags |= os.O_EXCL | os.O_CREAT fd = os.open(session_file_name, flags) os.close(fd) except OSError as e: if must_create and e.errno == errno.EEXIST: raise CreateError + if not must_create and e.errno == errno.ENOENT: + raise UpdateError raise # Write the session file without interfering with other threads diff --git a/django/contrib/sessions/middleware.py b/django/contrib/sessions/middleware.py index 96751dbbc1..51e0101616 100644 --- a/django/contrib/sessions/middleware.py +++ b/django/contrib/sessions/middleware.py @@ -2,6 +2,8 @@ import time from importlib import import_module from django.conf import settings +from django.contrib.sessions.backends.base import UpdateError +from django.shortcuts import redirect from django.utils.cache import patch_vary_headers from django.utils.http import cookie_date @@ -47,7 +49,13 @@ class SessionMiddleware(object): # Save the session data and refresh the client cookie. # Skip session save for 500 responses, refs #3881. if response.status_code != 500: - request.session.save() + try: + request.session.save() + except UpdateError: + # The user is now logged out; redirecting to same + # page will result in a redirect to the login page + # if required. + return redirect(request.path) response.set_cookie(settings.SESSION_COOKIE_NAME, request.session.session_key, max_age=max_age, expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, diff --git a/tests/defer_regress/tests.py b/tests/defer_regress/tests.py index ffb0b6e775..2594ffcdb8 100644 --- a/tests/defer_regress/tests.py +++ b/tests/defer_regress/tests.py @@ -131,7 +131,7 @@ class DeferRegressionTest(TestCase): s = SessionStore(SESSION_KEY) s.clear() s["item"] = item - s.save() + s.save(must_create=True) s = SessionStore(SESSION_KEY) s.modified = True diff --git a/tests/sessions_tests/tests.py b/tests/sessions_tests/tests.py index 7b3feec565..bb8ca2b635 100644 --- a/tests/sessions_tests/tests.py +++ b/tests/sessions_tests/tests.py @@ -8,6 +8,7 @@ import unittest from datetime import timedelta from django.conf import settings +from django.contrib.sessions.backends.base import UpdateError from django.contrib.sessions.backends.cache import SessionStore as CacheSession from django.contrib.sessions.backends.cached_db import \ SessionStore as CacheDBSession @@ -174,6 +175,7 @@ class SessionTestsMixin(object): prev_key = self.session.session_key prev_data = list(self.session.items()) self.session.cycle_key() + self.assertFalse(self.session.exists(prev_key)) self.assertNotEqual(self.session.session_key, prev_key) self.assertEqual(list(self.session.items()), prev_data) @@ -349,6 +351,28 @@ class SessionTestsMixin(object): # provided unknown key was cycled, not reused self.assertNotEqual(session.session_key, 'someunknownkey') + def test_session_save_does_not_resurrect_session_logged_out_in_other_context(self): + """ + Sessions shouldn't be resurrected by a concurrent request. + """ + # Create new session. + s1 = self.backend() + s1['test_data'] = 'value1' + s1.save(must_create=True) + + # Logout in another context. + s2 = self.backend(s1.session_key) + s2.delete() + + # Modify session in first context. + s1['test_data'] = 'value2' + with self.assertRaises(UpdateError): + # This should throw an exception as the session is deleted, not + # resurrect the session. + s1.save() + + self.assertEqual(s1.load(), {}) + class DatabaseSessionTests(SessionTestsMixin, TestCase): @@ -657,6 +681,25 @@ class SessionMiddlewareTests(TestCase): # Check that the value wasn't saved above. self.assertNotIn('hello', request.session.load()) + def test_session_update_error_redirect(self): + path = '/foo/' + request = RequestFactory().get(path) + response = HttpResponse() + middleware = SessionMiddleware() + + request.session = DatabaseSession() + request.session.save(must_create=True) + request.session.delete() + + # Handle the response through the middleware. It will try to save the + # deleted session which will cause an UpdateError that's caught and + # results in a redirect to the original page. + response = middleware.process_response(request, response) + + # Check that the response is a redirect. + self.assertEqual(response.status_code, 302) + self.assertEqual(response['Location'], path) + def test_session_delete_on_end(self): request = RequestFactory().get('/') response = HttpResponse('Session test') @@ -808,3 +851,7 @@ class CookieSessionTests(SessionTestsMixin, unittest.TestCase): @unittest.skip("Cookie backend doesn't have an external store to create records in.") def test_session_load_does_not_create_record(self): pass + + @unittest.skip("CookieSession is stored in the client and there is no way to query it.") + def test_session_save_does_not_resurrect_session_logged_out_in_other_context(self): + pass