From b06dfad88fb12a927c86a1eb23064201c9560fb1 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Wed, 3 Dec 2014 07:33:44 -0500 Subject: [PATCH] Fixed #23939 -- Moved session verification out of SessionAuthenticationMiddleware. Thanks andrewbadr for the report and Carl Meyer for the review. --- django/contrib/auth/__init__.py | 15 +++++- django/contrib/auth/middleware.py | 20 +++----- django/contrib/auth/tests/test_middleware.py | 50 +++++++++++++------- docs/releases/1.7.2.txt | 4 ++ docs/releases/1.7.txt | 4 +- 5 files changed, 59 insertions(+), 34 deletions(-) diff --git a/django/contrib/auth/__init__.py b/django/contrib/auth/__init__.py index 550b60d14e..a9eb41a609 100644 --- a/django/contrib/auth/__init__.py +++ b/django/contrib/auth/__init__.py @@ -4,9 +4,10 @@ import re from django.apps import apps as django_apps from django.conf import settings from django.core.exceptions import ImproperlyConfigured, PermissionDenied +from django.middleware.csrf import rotate_token +from django.utils.crypto import constant_time_compare from django.utils.module_loading import import_string from django.utils.translation import LANGUAGE_SESSION_KEY -from django.middleware.csrf import rotate_token from .signals import user_logged_in, user_logged_out, user_login_failed @@ -165,6 +166,18 @@ def get_user(request): if backend_path in settings.AUTHENTICATION_BACKENDS: backend = load_backend(backend_path) user = backend.get_user(user_id) + # Verify the session + if ('django.contrib.auth.middleware.SessionAuthenticationMiddleware' + in settings.MIDDLEWARE_CLASSES and hasattr(user, 'get_session_auth_hash')): + session_hash = request.session.get(HASH_SESSION_KEY) + session_hash_verified = session_hash and constant_time_compare( + session_hash, + user.get_session_auth_hash() + ) + if not session_hash_verified: + request.session.flush() + user = None + return user or AnonymousUser() diff --git a/django/contrib/auth/middleware.py b/django/contrib/auth/middleware.py index b34da60299..02c7b26298 100644 --- a/django/contrib/auth/middleware.py +++ b/django/contrib/auth/middleware.py @@ -2,7 +2,6 @@ from django.contrib import auth from django.contrib.auth import load_backend from django.contrib.auth.backends import RemoteUserBackend from django.core.exceptions import ImproperlyConfigured -from django.utils.crypto import constant_time_compare from django.utils.functional import SimpleLazyObject @@ -25,20 +24,15 @@ class AuthenticationMiddleware(object): class SessionAuthenticationMiddleware(object): """ - Middleware for invalidating a user's sessions that don't correspond to the - user's current session authentication hash (generated based on the user's - password for AbstractUser). + Formerly, a middleware for invalidating a user's sessions that don't + correspond to the user's current session authentication hash. However, it + caused the "Vary: Cookie" header on all responses. + + Now a backwards compatibility shim that enables session verification in + auth.get_user() if this middleware is in MIDDLEWARE_CLASSES. """ def process_request(self, request): - user = request.user - if user and hasattr(user, 'get_session_auth_hash'): - session_hash = request.session.get(auth.HASH_SESSION_KEY) - session_hash_verified = session_hash and constant_time_compare( - session_hash, - user.get_session_auth_hash() - ) - if not session_hash_verified: - auth.logout(request) + pass class RemoteUserMiddleware(object): diff --git a/django/contrib/auth/tests/test_middleware.py b/django/contrib/auth/tests/test_middleware.py index 2bdc77a16e..44372bce01 100644 --- a/django/contrib/auth/tests/test_middleware.py +++ b/django/contrib/auth/tests/test_middleware.py @@ -1,4 +1,4 @@ -from django.contrib.auth.middleware import SessionAuthenticationMiddleware +from django.contrib.auth.middleware import AuthenticationMiddleware from django.contrib.auth.models import User from django.http import HttpRequest from django.test import TestCase @@ -11,25 +11,39 @@ class TestSessionAuthenticationMiddleware(TestCase): 'test@example.com', self.user_password) - def test_changed_password_invalidates_session(self): - """ - Tests that changing a user's password invalidates the session. - """ - verification_middleware = SessionAuthenticationMiddleware() + self.middleware = AuthenticationMiddleware() self.assertTrue(self.client.login( username=self.user.username, password=self.user_password, )) - request = HttpRequest() - request.session = self.client.session - request.user = self.user - verification_middleware.process_request(request) - self.assertIsNotNone(request.user) - self.assertFalse(request.user.is_anonymous()) + self.request = HttpRequest() + self.request.session = self.client.session - # After password change, user should be anonymous - request.user.set_password('new_password') - request.user.save() - verification_middleware.process_request(request) - self.assertIsNotNone(request.user) - self.assertTrue(request.user.is_anonymous()) + def test_changed_password_doesnt_invalidate_session(self): + """ + Changing a user's password shouldn't invalidate the session if session + verification isn't activated. + """ + session_key = self.request.session.session_key + self.middleware.process_request(self.request) + self.assertIsNotNone(self.request.user) + self.assertFalse(self.request.user.is_anonymous()) + + # After password change, user should remain logged in. + self.user.set_password('new_password') + self.user.save() + self.middleware.process_request(self.request) + self.assertIsNotNone(self.request.user) + self.assertFalse(self.request.user.is_anonymous()) + self.assertEqual(session_key, self.request.session.session_key) + + def test_changed_password_invalidates_session_with_middleware(self): + with self.modify_settings(MIDDLEWARE_CLASSES={'append': ['django.contrib.auth.middleware.SessionAuthenticationMiddleware']}): + # After password change, user should be anonymous + self.user.set_password('new_password') + self.user.save() + self.middleware.process_request(self.request) + self.assertIsNotNone(self.request.user) + self.assertTrue(self.request.user.is_anonymous()) + # session should be flushed + self.assertIsNone(self.request.session.session_key) diff --git a/docs/releases/1.7.2.txt b/docs/releases/1.7.2.txt index a537207924..771e353818 100644 --- a/docs/releases/1.7.2.txt +++ b/docs/releases/1.7.2.txt @@ -104,3 +104,7 @@ Bugfixes * Fixed serialization of ``type`` when adding a ``deconstruct()`` method (:ticket:`23950`). + +* Prevented the + :class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` from + setting a ``"Vary: Cookie"`` header on all responses. diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index e14fd8bc48..c6b31074ad 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -1461,8 +1461,8 @@ Miscellaneous * With the addition of the :class:`~django.contrib.auth.middleware.SessionAuthenticationMiddleware` to - the default project template, a database must be created before accessing - a page using :djadmin:`runserver`. + the default project template (pre-1.7.2 only), a database must be created + before accessing a page using :djadmin:`runserver`. .. _deprecated-features-1.7: