Merge pull request #11743 from edx/PERF-272

Send 'Vary: Origin' so XHR/CORS requests can work properly.

cms/envs/common.py

@@ -311,7 +311,7 @@ simplefilter('ignore')
 
 MIDDLEWARE_CLASSES = (
     'request_cache.middleware.RequestCache',
-    'clean_headers.middleware.CleanHeadersMiddleware',
+    'header_control.middleware.HeaderControlMiddleware',
     'django.middleware.cache.UpdateCacheMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.middleware.csrf.CsrfViewMiddleware',

common/djangoapps/clean_headers/__init__.py

-"""
-This middleware is used for cleaning headers from a response before it is sent to the end user.
-
-Due to the nature of how middleware runs, a piece of middleware high in the chain cannot ensure
-that response headers won't be present on the final response body, as middleware further down
-the chain could be adding them.
-
-This middleware is intended to sit as close as possible to the top of the list, so that it has
-a chance on the reponse going out to strip the intended headers.
-"""
-
-
-def remove_headers_from_response(response, *headers):
-    """Removes the given headers from the response using the clean_headers middleware."""
-    response.clean_headers = headers

common/djangoapps/clean_headers/tests/test_decorators.py

-"""Tests for clean_headers decorator. """
-from django.http import HttpResponse, HttpRequest
-from django.test import TestCase
-from clean_headers.decorators import clean_headers
-
-
-def fake_view(_request):
-    """Fake view that returns an empty response."""
-    return HttpResponse()
-
-
-class TestCleanHeaders(TestCase):
-    """Test the `clean_headers` decorator."""
-
-    def test_clean_headers(self):
-        request = HttpRequest()
-        wrapper = clean_headers('Vary', 'Accept-Encoding')
-        wrapped_view = wrapper(fake_view)
-        response = wrapped_view(request)
-        self.assertEqual(len(response.clean_headers), 2)

common/djangoapps/clean_headers/tests/test_middleware.py

-"""Tests for clean_headers middleware."""
-from django.http import HttpResponse, HttpRequest
-from django.test import TestCase
-from clean_headers.middleware import CleanHeadersMiddleware
-
-
-class TestCleanHeadersMiddlewareProcessResponse(TestCase):
-    """Test the `clean_headers` middleware. """
-    def setUp(self):
-        super(TestCleanHeadersMiddlewareProcessResponse, self).setUp()
-        self.middleware = CleanHeadersMiddleware()
-
-    def test_cleans_intended_headers(self):
-        fake_request = HttpRequest()
-
-        fake_response = HttpResponse()
-        fake_response['Vary'] = 'Cookie'
-        fake_response['Accept-Encoding'] = 'gzip'
-        fake_response.clean_headers = ['Vary']
-
-        result = self.middleware.process_response(fake_request, fake_response)
-        self.assertNotIn('Vary', result)
-        self.assertEquals('gzip', result['Accept-Encoding'])
-
-    def test_does_not_mangle_undecorated_response(self):
-        fake_request = HttpRequest()
-
-        fake_response = HttpResponse()
-        fake_response['Vary'] = 'Cookie'
-        fake_response['Accept-Encoding'] = 'gzip'
-
-        result = self.middleware.process_response(fake_request, fake_response)
-        self.assertEquals('Cookie', result['Vary'])
-        self.assertEquals('gzip', result['Accept-Encoding'])

common/djangoapps/contentserver/middleware.py

@@ -11,7 +11,7 @@ from django.http import (
 from student.models import CourseEnrollment
 from contentserver.models import CourseAssetCacheTtlConfig
 
-from clean_headers import remove_headers_from_response
+from header_control import force_header_for_response
 from xmodule.assetstore.assetmgr import AssetManager
 from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG
 from xmodule.modulestore import InvalidLocationError
@@ -153,7 +153,10 @@ class StaticContentServer(object):
 
         response['Last-Modified'] = content.last_modified_at.strftime(HTTP_DATE_FORMAT)
 
-        remove_headers_from_response(response, "Vary")
+        # Force the Vary header to only vary responses on Origin, so that XHR and browser requests get cached
+        # separately and don't screw over one another. i.e. a browser request that doesn't send Origin, and
+        # caches a version of the response without CORS headers, in turn breaking XHR requests.
+        force_header_for_response(response, 'Vary', 'Origin')
 
     @staticmethod
     def get_expiration_value(now, cache_ttl):

common/djangoapps/contentserver/test/test_contentserver.py

@@ -16,12 +16,13 @@ from mock import patch
 
 from xmodule.contentstore.django import contentstore
 from xmodule.modulestore.django import modulestore
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.xml_importer import import_course_from_xml
 
 from contentserver.middleware import parse_range_header, HTTP_DATE_FORMAT, StaticContentServer
 from student.models import CourseEnrollment
+from student.tests.factories import UserFactory, AdminFactory
 
 log = logging.getLogger(__name__)
 
@@ -33,39 +34,44 @@ TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT
 
 @ddt.ddt
 @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE)
-class ContentStoreToyCourseTest(ModuleStoreTestCase):
+class ContentStoreToyCourseTest(SharedModuleStoreTestCase):
     """
     Tests that use the toy course.
     """
 
-    def setUp(self):
-        """
-        Create user and login.
-        """
-        self.staff_pwd = super(ContentStoreToyCourseTest, self).setUp()
-        self.staff_usr = self.user
-        self.non_staff_usr, self.non_staff_pwd = self.create_non_staff_user()
+    @classmethod
+    def setUpClass(cls):
+        super(ContentStoreToyCourseTest, cls).setUpClass()
 
-        self.client = Client()
-        self.contentstore = contentstore()
-        store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo)  # pylint: disable=protected-access
+        cls.contentstore = contentstore()
+        cls.modulestore = modulestore()
 
-        self.course_key = store.make_course_key('edX', 'toy', '2012_Fall')
+        cls.course_key = cls.modulestore.make_course_key('edX', 'toy', '2012_Fall')
 
         import_course_from_xml(
-            store, self.user.id, TEST_DATA_DIR, ['toy'],
-            static_content_store=self.contentstore, verbose=True
+            cls.modulestore, 1, TEST_DATA_DIR, ['toy'],
+            static_content_store=cls.contentstore, verbose=True
         )
 
         # A locked asset
-        self.locked_asset = self.course_key.make_asset_key('asset', 'sample_static.txt')
-        self.url_locked = unicode(self.locked_asset)
-        self.contentstore.set_attr(self.locked_asset, 'locked', True)
+        cls.locked_asset = cls.course_key.make_asset_key('asset', 'sample_static.txt')
+        cls.url_locked = unicode(cls.locked_asset)
+        cls.contentstore.set_attr(cls.locked_asset, 'locked', True)
 
         # An unlocked asset
-        self.unlocked_asset = self.course_key.make_asset_key('asset', 'another_static.txt')
-        self.url_unlocked = unicode(self.unlocked_asset)
-        self.length_unlocked = self.contentstore.get_attr(self.unlocked_asset, 'length')
+        cls.unlocked_asset = cls.course_key.make_asset_key('asset', 'another_static.txt')
+        cls.url_unlocked = unicode(cls.unlocked_asset)
+        cls.length_unlocked = cls.contentstore.get_attr(cls.unlocked_asset, 'length')
+
+    def setUp(self):
+        """
+        Create user and login.
+        """
+        super(ContentStoreToyCourseTest, self).setUp()
+        self.staff_usr = AdminFactory.create()
+        self.non_staff_usr = UserFactory.create()
+
+        self.client = Client()
 
     def test_unlocked_asset(self):
         """
@@ -89,7 +95,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         Test that locked assets behave appropriately in case user is logged in
         in but not registered for the course.
         """
-        self.client.login(username=self.non_staff_usr, password=self.non_staff_pwd)
+        self.client.login(username=self.non_staff_usr, password='test')
         resp = self.client.get(self.url_locked)
         self.assertEqual(resp.status_code, 403)
 
@@ -101,7 +107,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         CourseEnrollment.enroll(self.non_staff_usr, self.course_key)
         self.assertTrue(CourseEnrollment.is_enrolled(self.non_staff_usr, self.course_key))
 
-        self.client.login(username=self.non_staff_usr, password=self.non_staff_pwd)
+        self.client.login(username=self.non_staff_usr, password='test')
         resp = self.client.get(self.url_locked)
         self.assertEqual(resp.status_code, 200)
 
@@ -109,7 +115,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         """
         Test that locked assets behave appropriately in case user is staff.
         """
-        self.client.login(username=self.staff_usr, password=self.staff_pwd)
+        self.client.login(username=self.staff_usr, password='test')
         resp = self.client.get(self.url_locked)
         self.assertEqual(resp.status_code, 200)
 
@@ -191,6 +197,15 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
             first=(self.length_unlocked), last=(self.length_unlocked)))
         self.assertEqual(resp.status_code, 416)
 
+    def test_vary_header_sent(self):
+        """
+        Tests that we're properly setting the Vary header to ensure browser requests don't get
+        cached in a way that breaks XHR requests to the same asset.
+        """
+        resp = self.client.get(self.url_unlocked)
+        self.assertEqual(resp.status_code, 200)
+        self.assertEquals('Origin', resp['Vary'])
+
     @patch('contentserver.models.CourseAssetCacheTtlConfig.get_cache_ttl')
     def test_cache_headers_with_ttl_unlocked(self, mock_get_cache_ttl):
         """
@@ -215,7 +230,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         CourseEnrollment.enroll(self.non_staff_usr, self.course_key)
         self.assertTrue(CourseEnrollment.is_enrolled(self.non_staff_usr, self.course_key))
 
-        self.client.login(username=self.non_staff_usr, password=self.non_staff_pwd)
+        self.client.login(username=self.non_staff_usr, password='test')
         resp = self.client.get(self.url_locked)
         self.assertEqual(resp.status_code, 200)
         self.assertNotIn('Expires', resp)
@@ -245,7 +260,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         CourseEnrollment.enroll(self.non_staff_usr, self.course_key)
         self.assertTrue(CourseEnrollment.is_enrolled(self.non_staff_usr, self.course_key))
 
-        self.client.login(username=self.non_staff_usr, password=self.non_staff_pwd)
+        self.client.login(username=self.non_staff_usr, password='test')
         resp = self.client.get(self.url_locked)
         self.assertEqual(resp.status_code, 200)
         self.assertNotIn('Expires', resp)
@@ -256,20 +271,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         near_expire_dt = StaticContentServer.get_expiration_value(start_dt, 55)
         self.assertEqual("Thu, 01 Dec 1983 20:00:55 GMT", near_expire_dt)
 
-    def test_response_no_vary_header_unlocked(self):
-        resp = self.client.get(self.url_unlocked)
-        self.assertEqual(resp.status_code, 200)
-        self.assertNotIn('Vary', resp)
-
-    def test_response_no_vary_header_locked(self):
-        CourseEnrollment.enroll(self.non_staff_usr, self.course_key)
-        self.assertTrue(CourseEnrollment.is_enrolled(self.non_staff_usr, self.course_key))
-
-        self.client.login(username=self.non_staff_usr, password=self.non_staff_pwd)
-        resp = self.client.get(self.url_locked)
-        self.assertEqual(resp.status_code, 200)
-        self.assertNotIn('Vary', resp)
-
 
 @ddt.ddt
 class ParseRangeHeaderTestCase(unittest.TestCase):

common/djangoapps/header_control/__init__.py

+"""
+This middleware is used for adjusting the headers in a response before it is sent to the end user.
+
+This middleware is intended to sit as close as possible to the top of the middleare list as possible,
+so that it is one of the last pieces of middleware to touch the response, and thus can most accurately
+adjust/control the headers of the response.
+"""
+
+
+def remove_headers_from_response(response, *headers):
+    """Removes the given headers from the response using the header_control middleware."""
+    response.remove_headers = headers
+
+
+def force_header_for_response(response, header, value):
+    """Forces the given header for the given response using the header_control middleware."""
+    force_headers = {}
+    if hasattr(response, 'force_headers'):
+        force_headers = response.force_headers
+    force_headers[header] = value
+
+    response.force_headers = force_headers

common/djangoapps/clean_headers/decorators.py → common/djangoapps/header_control/decorators.py

@@ -3,17 +3,18 @@ Middleware decorator for removing headers.
 """
 
 from functools import wraps
+from header_control import remove_headers_from_response, force_header_for_response
 
 
-def clean_headers(*headers):
+def remove_headers(*headers):
     """
-    Decorator that removes any headers specified from the response.
+    Decorator that removes specific headers from the response.
     Usage:
-        @clean_headers("Vary")
+        @remove_headers("Vary")
         def myview(request):
             ...
 
-    The CleanHeadersMiddleware must be used and placed as closely as possible to the top
+    The HeaderControlMiddleware must be used and placed as closely as possible to the top
     of the middleware chain, ideally after any caching middleware but before everything else.
 
     This decorator is not safe for multiple uses: each call will overwrite any previously set values.
@@ -28,7 +29,38 @@ def clean_headers(*headers):
             Alters the response.
             """
             response = func(*args, **kwargs)
-            response.clean_headers = headers
+            remove_headers_from_response(response, *headers)
+            return response
+
+        return _inner
+
+    return _decorator
+
+
+def force_header(header, value):
+    """
+    Decorator that forces a header in the response to have a specific value.
+    Usage:
+        @force_header("Vary", "Origin")
+        def myview(request):
+            ...
+
+    The HeaderControlMiddleware must be used and placed as closely as possible to the top
+    of the middleware chain, ideally after any caching middleware but before everything else.
+
+    This decorator is not safe for multiple uses: each call will overwrite any previously set values.
+    """
+    def _decorator(func):
+        """
+        Decorates the given function.
+        """
+        @wraps(func)
+        def _inner(*args, **kwargs):
+            """
+            Alters the response.
+            """
+            response = func(*args, **kwargs)
+            force_header_for_response(response, header, value)
             return response
 
         return _inner

common/djangoapps/clean_headers/middleware.py → common/djangoapps/header_control/middleware.py

 """
-Middleware used for cleaning headers from a response before it is sent to the end user.
+Middleware used for adjusting headers in a response before it is sent to the end user.
 """
 
 
-class CleanHeadersMiddleware(object):
+class HeaderControlMiddleware(object):
     """
-    Middleware that can drop headers present in a response.
+    Middleware that can modify/remove headers in a response.
 
     This can be used, for example, to remove headers i.e. drop any Vary headers to improve cache performance.
     """
 
     def process_response(self, _request, response):
         """
-        Processes the given response, potentially stripping out any unwanted headers.
+        Processes the given response, potentially remove or modifying headers.
         """
 
-        if len(getattr(response, 'clean_headers', [])) > 0:
-            for header in response.clean_headers:
-                try:
-                    del response[header]
-                except KeyError:
-                    pass
+        for header in getattr(response, 'remove_headers', []):
+            del response[header]
+
+        for header, value in getattr(response, 'force_headers', {}).iteritems():
+            response[header] = value
 
         return response

common/djangoapps/header_control/tests/test_decorators.py

+"""Tests for remove_headers and force_header decorator. """
+from django.http import HttpResponse, HttpRequest
+from django.test import TestCase
+from header_control.decorators import remove_headers, force_header
+
+
+def fake_view(_request):
+    """Fake view that returns an empty response."""
+    return HttpResponse()
+
+
+class TestRemoveHeaders(TestCase):
+    """Test the `remove_headers` decorator."""
+
+    def test_remove_headers(self):
+        request = HttpRequest()
+        wrapper = remove_headers('Vary', 'Accept-Encoding')
+        wrapped_view = wrapper(fake_view)
+        response = wrapped_view(request)
+        self.assertEqual(len(response.remove_headers), 2)
+
+
+class TestForceHeader(TestCase):
+    """Test the `force_header` decorator."""
+
+    def test_force_header(self):
+        request = HttpRequest()
+        wrapper = force_header('Vary', 'Origin')
+        wrapped_view = wrapper(fake_view)
+        response = wrapped_view(request)
+        self.assertEqual(len(response.force_headers), 1)
+        self.assertEqual(response.force_headers['Vary'], 'Origin')

common/djangoapps/header_control/tests/test_middleware.py

+"""Tests for header_control middleware."""
+from django.http import HttpResponse, HttpRequest
+from django.test import TestCase
+from header_control import remove_headers_from_response, force_header_for_response
+from header_control.middleware import HeaderControlMiddleware
+
+
+class TestHeaderControlMiddlewareProcessResponse(TestCase):
+    """Test the `header_control` middleware. """
+    def setUp(self):
+        super(TestHeaderControlMiddlewareProcessResponse, self).setUp()
+        self.middleware = HeaderControlMiddleware()
+
+    def test_doesnt_barf_if_not_modifying_anything(self):
+        fake_request = HttpRequest()
+
+        fake_response = HttpResponse()
+        fake_response['Vary'] = 'Cookie'
+        fake_response['Accept-Encoding'] = 'gzip'
+
+        result = self.middleware.process_response(fake_request, fake_response)
+        self.assertEquals('Cookie', result['Vary'])
+        self.assertEquals('gzip', result['Accept-Encoding'])
+
+    def test_doesnt_barf_removing_nonexistent_headers(self):
+        fake_request = HttpRequest()
+
+        fake_response = HttpResponse()
+        fake_response['Vary'] = 'Cookie'
+        fake_response['Accept-Encoding'] = 'gzip'
+        remove_headers_from_response(fake_response, 'Vary', 'FakeHeaderWeeee')
+
+        result = self.middleware.process_response(fake_request, fake_response)
+        self.assertNotIn('Vary', result)
+        self.assertEquals('gzip', result['Accept-Encoding'])
+
+    def test_removes_intended_headers(self):
+        fake_request = HttpRequest()
+
+        fake_response = HttpResponse()
+        fake_response['Vary'] = 'Cookie'
+        fake_response['Accept-Encoding'] = 'gzip'
+        remove_headers_from_response(fake_response, 'Vary')
+
+        result = self.middleware.process_response(fake_request, fake_response)
+        self.assertNotIn('Vary', result)
+        self.assertEquals('gzip', result['Accept-Encoding'])
+
+    def test_forces_intended_header(self):
+        fake_request = HttpRequest()
+
+        fake_response = HttpResponse()
+        fake_response['Vary'] = 'Cookie'
+        fake_response['Accept-Encoding'] = 'gzip'
+        force_header_for_response(fake_response, 'Vary', 'Origin')
+
+        result = self.middleware.process_response(fake_request, fake_response)
+        self.assertEquals('Origin', result['Vary'])
+        self.assertEquals('gzip', result['Accept-Encoding'])
+
+    def test_does_not_mangle_undecorated_response(self):
+        fake_request = HttpRequest()
+
+        fake_response = HttpResponse()
+        fake_response['Vary'] = 'Cookie'
+        fake_response['Accept-Encoding'] = 'gzip'
+
+        result = self.middleware.process_response(fake_request, fake_response)
+        self.assertEquals('Cookie', result['Vary'])
+        self.assertEquals('gzip', result['Accept-Encoding'])

lms/envs/common.py

@@ -1084,7 +1084,7 @@ simplefilter('ignore')
 
 MIDDLEWARE_CLASSES = (
     'request_cache.middleware.RequestCache',
-    'clean_headers.middleware.CleanHeadersMiddleware',
+    'header_control.middleware.HeaderControlMiddleware',
     'microsite_configuration.middleware.MicrositeMiddleware',
     'django_comment_client.middleware.AjaxExceptionMiddleware',
     'django.middleware.common.CommonMiddleware',