Merge pull request #12373 from edx/peter-fogg/password-change-session-invalidation

Expire sessions after a password change.

cms/envs/common.py

@@ -326,6 +326,10 @@ MIDDLEWARE_CLASSES = (
 
     # Instead of AuthenticationMiddleware, we use a cache-backed version
     'cache_toolbox.middleware.CacheBackedAuthenticationMiddleware',
+    # Enable SessionAuthenticationMiddleware in order to invalidate
+    # user sessions after a password change.
+    'django.contrib.auth.middleware.SessionAuthenticationMiddleware',
+
     'student.middleware.UserStandingMiddleware',
     'contentserver.middleware.StaticContentServer',
     'crum.CurrentRequestUserMiddleware',

common/djangoapps/cache_toolbox/middleware.py

@@ -78,8 +78,11 @@ choice for most environments but you may be happy with the trade-offs of the
 
 """
 
-from django.contrib.auth.models import User
+from django.conf import settings
+from django.contrib.auth import HASH_SESSION_KEY
+from django.contrib.auth.models import User, AnonymousUser
 from django.contrib.auth.middleware import AuthenticationMiddleware
+from django.utils.crypto import constant_time_compare
 from logging import getLogger
 
 from openedx.core.djangoapps.safe_sessions.middleware import SafeSessionMiddleware
@@ -106,6 +109,29 @@ class CacheBackedAuthenticationMiddleware(AuthenticationMiddleware):
                 )
                 # Raise an exception to fall through to the except clause below.
                 raise Exception
+            self._verify_session_auth(request)
         except:
             # Fallback to constructing the User from the database.
             super(CacheBackedAuthenticationMiddleware, self).process_request(request)
+
+    def _verify_session_auth(self, request):
+        """
+        Ensure that the user's session hash hasn't changed. We check that
+        SessionAuthenticationMiddleware is enabled in order to match Django's
+        behavior.
+        """
+        session_auth_class = 'django.contrib.auth.middleware.SessionAuthenticationMiddleware'
+        session_auth_enabled = session_auth_class in settings.MIDDLEWARE_CLASSES
+        # Auto-auth causes issues in Bok Choy tests because it resets
+        # the requesting user. Since session verification is a
+        # security feature, we can turn it off when auto-auth is
+        # enabled since auto-auth is highly insecure and only for
+        # tests.
+        auto_auth_enabled = settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING', False)
+        if not auto_auth_enabled and session_auth_enabled and hasattr(request.user, 'get_session_auth_hash'):
+            session_hash = request.session.get(HASH_SESSION_KEY)
+            if not (session_hash and constant_time_compare(session_hash, request.user.get_session_auth_hash())):
+                # The session hash has changed due to a password
+                # change. Log the user out.
+                request.session.flush()
+                request.user = AnonymousUser()

common/djangoapps/cache_toolbox/tests/test_middleware.py

@@ -0,0 +1,46 @@
+"""Tests for cached authentication middleware."""
+import unittest
+
+from django.conf import settings
+from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
+from django.test import TestCase
+from mock import patch
+
+from student.tests.factories import UserFactory
+
+
+class CachedAuthMiddlewareTestCase(TestCase):
+    """Tests for CacheBackedAuthenticationMiddleware class."""
+
+    def setUp(self):
+        super(CachedAuthMiddlewareTestCase, self).setUp()
+        password = 'test-password'
+        self.user = UserFactory(password=password)
+        self.client.login(username=self.user.username, password=password)
+
+    def _test_change_session_hash(self, test_url, redirect_url):
+        """
+        Verify that if a user's session auth hash and the request's hash
+        differ, the user is logged out. The URL to test and the
+        expected redirect are passed in, since we want to test this
+        behavior in both LMS and CMS, but the two systems have
+        different URLconfs.
+        """
+        response = self.client.get(test_url)
+        self.assertEqual(response.status_code, 200)
+        with patch.object(User, 'get_session_auth_hash', return_value='abc123'):
+            response = self.client.get(test_url)
+            self.assertRedirects(response, redirect_url)
+
+    @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+    def test_session_change_lms(self):
+        """Test session verification with LMS-specific URLs."""
+        dashboard_url = reverse('dashboard')
+        self._test_change_session_hash(dashboard_url, reverse('signin_user') + '?next=' + dashboard_url)
+
+    @unittest.skipUnless(settings.ROOT_URLCONF == 'cms.urls', 'Test only valid in cms')
+    def test_session_change_cms(self):
+        """Test session verification with CMS-specific URLs."""
+        home_url = reverse('home')
+        self._test_change_session_hash(home_url, reverse('login') + '?next=' + home_url)

lms/envs/common.py

@@ -1106,6 +1106,9 @@ MIDDLEWARE_CLASSES = (
     # Instead of AuthenticationMiddleware, we use a cached backed version
     #'django.contrib.auth.middleware.AuthenticationMiddleware',
     'cache_toolbox.middleware.CacheBackedAuthenticationMiddleware',
+    # Enable SessionAuthenticationMiddleware in order to invalidate
+    # user sessions after a password change.
+    'django.contrib.auth.middleware.SessionAuthenticationMiddleware',
 
     'student.middleware.UserStandingMiddleware',
     'contentserver.middleware.StaticContentServer',