Merge pull request #13777 from edx/douglashall/fix_session_cookie_domain_middleware

Refactored middleware that supports SESSION_COOKIE_DOMAIN overrides.

common/djangoapps/microsite_configuration/middleware.py

@@ -36,50 +36,3 @@ class MicrositeMiddleware(object):
         """
         microsite.clear()
         return response
-
-
-class MicrositeSessionCookieDomainMiddleware(object):
-    """
-    Special case middleware which should be at the very end of the MIDDLEWARE list (so that it runs first
-    on the process_response chain). This middleware will define a wrapper function for the set_cookie() function
-    on the HttpResponse object, if the request is running in a middleware.
-
-    This wrapped set_cookie will change the SESSION_COOKIE_DOMAIN setting so that the cookie can be bound to a
-    fully customized URL.
-    """
-
-    def process_response(self, request, response):
-        """
-        Standard Middleware entry point
-        """
-
-        # See if we are running in a Microsite *AND* we have a custom SESSION_COOKIE_DOMAIN defined
-        # in configuration
-        if microsite.has_override_value('SESSION_COOKIE_DOMAIN'):
-
-            # define wrapper function for the standard set_cookie()
-            def _set_cookie_wrapper(key, value='', max_age=None, expires=None, path='/', domain=None, secure=None, httponly=False):
-
-                # only override if we are setting the cookie name to be the one the Django Session Middleware uses
-                # as defined in settings.SESSION_COOKIE_NAME
-                if key == settings.SESSION_COOKIE_NAME:
-                    domain = microsite.get_value('SESSION_COOKIE_DOMAIN', domain)
-
-                # then call down into the normal Django set_cookie method
-                return response.set_cookie_wrapped_func(
-                    key,
-                    value,
-                    max_age=max_age,
-                    expires=expires,
-                    path=path,
-                    domain=domain,
-                    secure=secure,
-                    httponly=httponly
-                )
-
-            # then point the HttpResponse.set_cookie to point to the wrapper and keep
-            # the original around
-            response.set_cookie_wrapped_func = response.set_cookie
-            response.set_cookie = _set_cookie_wrapper
-
-        return response

lms/djangoapps/django_comment_client/base/tests.py

@@ -370,8 +370,8 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet
         return inner
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 4, 32),
-        (ModuleStoreEnum.Type.split, 3, 13, 32),
+        (ModuleStoreEnum.Type.mongo, 3, 4, 33),
+        (ModuleStoreEnum.Type.split, 3, 13, 33),
     )
     @ddt.unpack
     @count_queries
@@ -379,8 +379,8 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet
         self.create_thread_helper(mock_request)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 3, 26),
-        (ModuleStoreEnum.Type.split, 3, 10, 26),
+        (ModuleStoreEnum.Type.mongo, 3, 3, 27),
+        (ModuleStoreEnum.Type.split, 3, 10, 27),
     )
     @ddt.unpack
     @count_queries

lms/envs/common.py

@@ -1174,7 +1174,7 @@ MIDDLEWARE_CLASSES = (
     'openedx.core.djangoapps.theming.middleware.CurrentSiteThemeMiddleware',
 
     # This must be last
-    'microsite_configuration.middleware.MicrositeSessionCookieDomainMiddleware',
+    'openedx.core.djangoapps.site_configuration.middleware.SessionCookieDomainOverrideMiddleware',
 )
 
 # Clickjacking protection can be enabled by setting this to 'DENY'

openedx/core/djangoapps/site_configuration/middleware.py

+"""
+This file contains Django middleware related to the site_configuration app.
+"""
+
+from django.conf import settings
+from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
+
+
+class SessionCookieDomainOverrideMiddleware(object):
+    """
+    Special case middleware which should be at the very end of the MIDDLEWARE list (so that it runs first
+    on the process_response chain). This middleware will define a wrapper function for the set_cookie() function
+    on the HttpResponse object, if the request is running in a middleware.
+
+    This wrapped set_cookie will change the SESSION_COOKIE_DOMAIN setting so that the cookie can be bound to a
+    fully customized URL.
+    """
+
+    def process_response(self, __, response):
+        """
+        Django middleware hook for process responses
+        """
+
+        # Check for SESSION_COOKIE_DOMAIN setting override
+        session_cookie_domain = configuration_helpers.get_value('SESSION_COOKIE_DOMAIN')
+        if session_cookie_domain:
+            def _set_cookie_wrapper(key, value='', max_age=None, expires=None, path='/', domain=None, secure=None,
+                                    httponly=False):
+                """
+                Wrapper function for set_cookie() which applies SESSION_COOKIE_DOMAIN override
+                """
+
+                # only override if we are setting the cookie name to be the one the Django Session Middleware uses
+                # as defined in settings.SESSION_COOKIE_NAME
+                if key == configuration_helpers.get_value('SESSION_COOKIE_NAME', settings.SESSION_COOKIE_NAME):
+                    domain = session_cookie_domain
+
+                # then call down into the normal Django set_cookie method
+                return response.set_cookie_wrapped_func(
+                    key,
+                    value,
+                    max_age=max_age,
+                    expires=expires,
+                    path=path,
+                    domain=domain,
+                    secure=secure,
+                    httponly=httponly
+                )
+
+            # then point the HttpResponse.set_cookie to point to the wrapper and keep
+            # the original around
+            response.set_cookie_wrapped_func = response.set_cookie
+            response.set_cookie = _set_cookie_wrapper
+
+        return response

openedx/core/djangoapps/site_configuration/tests/mixins.py

@@ -28,6 +28,7 @@ class SiteMixin(object):
             site=self.site_other,
             values={
                 "SITE_NAME": self.site_other.domain,
+                "SESSION_COOKIE_DOMAIN": self.site_other.domain,
                 "course_org_filter": "fakeOtherX",
                 "ENABLE_MKTG_SITE": True,
                 "SHOW_ECOMMERCE_REPORTS": True,

common/djangoapps/microsite_configuration/tests/test_middleware.py → openedx/core/djangoapps/site_configuration/tests/test_middleware.py

 # -*- coding: utf-8 -*-
 """
-Test Microsite middleware.
+Test site_configuration middleware.
 """
 import ddt
 import unittest
@@ -20,6 +20,7 @@ from microsite_configuration.tests.tests import (
     side_effect_for_get_value,
     MICROSITE_BACKENDS,
 )
+from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory
 
 
 # NOTE: We set SESSION_SAVE_EVERY_REQUEST to True in order to make sure
@@ -28,13 +29,13 @@ from microsite_configuration.tests.tests import (
 @ddt.ddt
 @override_settings(SESSION_SAVE_EVERY_REQUEST=True)
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class MicrositeSessionCookieTests(DatabaseMicrositeTestCase):
+class SessionCookieDomainOverrideTests(DatabaseMicrositeTestCase):
     """
     Tests regarding the session cookie management in the middlware for Microsites
     """
 
     def setUp(self):
-        super(MicrositeSessionCookieTests, self).setUp()
+        super(SessionCookieDomainOverrideTests, self).setUp()
         # Create a test client, and log it in so that it will save some session
         # data.
         self.user = UserFactory.create()
@@ -43,10 +44,21 @@ class MicrositeSessionCookieTests(DatabaseMicrositeTestCase):
         self.client = Client()
         self.client.login(username=self.user.username, password="password")
 
+        self.site = SiteFactory.create(
+            domain='testserver.fake',
+            name='testserver.fake'
+        )
+        self.site_configuration = SiteConfigurationFactory.create(
+            site=self.site,
+            values={
+                "SESSION_COOKIE_DOMAIN": self.site.domain,
+            }
+        )
+
     @ddt.data(*MICROSITE_BACKENDS)
-    def test_session_cookie_domain_no_microsite(self, site_backend):
+    def test_session_cookie_domain_no_override(self, site_backend):
         """
-        Tests that non-microsite behaves according to default behavior
+        Test sessionid cookie when no override is set
         """
         with patch('microsite_configuration.microsite.BACKEND',
                    get_backend(site_backend, BaseMicrositeBackend)):
@@ -55,7 +67,7 @@ class MicrositeSessionCookieTests(DatabaseMicrositeTestCase):
             self.assertNotIn('Domain', str(response.cookies['sessionid']))
 
     @ddt.data(*MICROSITE_BACKENDS)
-    def test_session_cookie_domain(self, site_backend):
+    def test_session_cookie_domain_with_microsite_override(self, site_backend):
         """
         Makes sure that the cookie being set in a Microsite
         is the one specially overridden in configuration
@@ -71,7 +83,6 @@ class MicrositeSessionCookieTests(DatabaseMicrositeTestCase):
         Tests to make sure that a Microsite that specifies None for 'SESSION_COOKIE_DOMAIN' does not
         set a domain on the session cookie
         """
-
         with patch('microsite_configuration.microsite.get_value') as mock_get_value:
             mock_get_value.side_effect = side_effect_for_get_value('SESSION_COOKIE_DOMAIN', None)
             with patch('microsite_configuration.microsite.BACKEND',
@@ -79,3 +90,10 @@ class MicrositeSessionCookieTests(DatabaseMicrositeTestCase):
                 response = self.client.get('/', HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME)
                 self.assertNotIn('test_site.localhost', str(response.cookies['sessionid']))
                 self.assertNotIn('Domain', str(response.cookies['sessionid']))
+
+    def test_session_cookie_domain_with_site_configuration_override(self):
+        """
+        Makes sure that the cookie being set is for the overridden domain
+        """
+        response = self.client.get('/', HTTP_HOST=self.site.domain)
+        self.assertIn(self.site.domain, str(response.cookies['sessionid']))

openedx/core/djangoapps/user_api/accounts/tests/test_views.py

@@ -248,7 +248,7 @@ class TestAccountAPI(CacheIsolationTestCase, UserAPITestCase):
         """
         self.different_client.login(username=self.different_user.username, password=self.test_password)
         self.create_mock_profile(self.user)
-        with self.assertNumQueries(18):
+        with self.assertNumQueries(19):
             response = self.send_get(self.different_client)
         self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY)
 
@@ -263,7 +263,7 @@ class TestAccountAPI(CacheIsolationTestCase, UserAPITestCase):
         """
         self.different_client.login(username=self.different_user.username, password=self.test_password)
         self.create_mock_profile(self.user)
-        with self.assertNumQueries(18):
+        with self.assertNumQueries(19):
             response = self.send_get(self.different_client)
         self._verify_private_account_response(response, account_privacy=PRIVATE_VISIBILITY)