Commit b06dfad8 authored by Tim Graham's avatar Tim Graham
Browse files

Fixed #23939 -- Moved session verification out of SessionAuthenticationMiddleware.

Thanks andrewbadr for the report and Carl Meyer for the review.
parent 26dd518b
Loading
Loading
Loading
Loading
+14 −1
Original line number Diff line number Diff line
@@ -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()


+7 −13
Original line number Diff line number Diff line
@@ -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):
+32 −18
Original line number Diff line number Diff line
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

    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
        request.user.set_password('new_password')
        request.user.save()
        verification_middleware.process_request(request)
        self.assertIsNotNone(request.user)
        self.assertTrue(request.user.is_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)
+4 −0
Original line number Diff line number Diff line
@@ -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.
+2 −2
Original line number Diff line number Diff line
@@ -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: