From 524e2292456de9175990adaea32ab6ac99d7e9e8 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Thu, 5 May 2016 10:07:30 -0400 Subject: [PATCH] Expire sessions after a password change. This is slightly more complicated than it should be since we're using custom authentication middleware (i.e., not Django's standard middleware class). We have to check that the session auth hash we have stored is equal to the request's session auth hash (since the stored hash is a function of the password). Normally this gets handled in `django.contrib.auth.get_user`, but due to our caching we don't go through that function, even in the cache miss case. ECOM-4288 --- cms/envs/common.py | 4 ++ common/djangoapps/cache_toolbox/middleware.py | 28 ++++++++++- .../cache_toolbox/tests/test_middleware.py | 46 +++++++++++++++++++ lms/envs/common.py | 3 ++ 4 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 common/djangoapps/cache_toolbox/tests/test_middleware.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 5e17b93aa7..651f64a58f 100644 --- a/cms/envs/common.py +++ b/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', diff --git a/common/djangoapps/cache_toolbox/middleware.py b/common/djangoapps/cache_toolbox/middleware.py index eba90210de..7c7839b9a5 100644 --- a/common/djangoapps/cache_toolbox/middleware.py +++ b/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() diff --git a/common/djangoapps/cache_toolbox/tests/test_middleware.py b/common/djangoapps/cache_toolbox/tests/test_middleware.py new file mode 100644 index 0000000000..2e6744e0ad --- /dev/null +++ b/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) diff --git a/lms/envs/common.py b/lms/envs/common.py index bf87fdc0f6..067678eb8a 100644 --- a/lms/envs/common.py +++ b/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',