bug 1431259: Don't cache the "create page" redirect

kuma/core/decorators.py

@@ -7,33 +7,49 @@
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponseForbidden, HttpResponseRedirect
 from django.shortcuts import redirect, render
+from django.utils.cache import patch_cache_control
 from django.utils.decorators import available_attrs
 from django.utils.http import urlquote
-from django.views.decorators.cache import cache_control
 
 from .jobs import BannedIPsJob
 from .urlresolvers import reverse
 
 
 def shared_cache_control(func=None, **kwargs):
     """
-    Decorator for view functions that defines the "Cache-Control" header
-    for shared caches like CDN's. It's simply a thin wrapper around
-    Django's cache-control that sets some defaults. By default, it does not
-    permit use of the browser's cache without validation ("max-age=0"), and
-    sets the caching period for shared caches ("s-maxage") based on the
-    CACHE_CONTROL_DEFAULT_SHARED_MAX_AGE setting. All of the defaults can be
-    overridden or extended via keyword arguments.
+    Decorator to set Cache-Control header for shared caches like CDNs.
+
+    This duplicates Django's cache-control decorators, but avoids changing the
+    header if a "no-cache" header has already been applied. The cache-control
+    decorator changes in Django 2.0 to remove Python 2 workarounds.
+
+    Default settings (which can be overridden or extended):
+    - max-age=0 - Don't use browser cache without asking if still valid
+    - s-maxage=CACHE_CONTROL_DEFAULT_SHARED_MAX_AGE - Cache in the shared
+      cache for the default perioid of time
+    - public - Allow intermediate proxies to cache response
     """
     # Set the default values.
     cc_kwargs = dict(public=True, max_age=0,
                      s_maxage=settings.CACHE_CONTROL_DEFAULT_SHARED_MAX_AGE)
     # Override the default values and/or add new ones.
     cc_kwargs.update(kwargs)
-    decorator = cache_control(**cc_kwargs)
-    if not func:
-        return decorator
-    return decorator(func)
+
+    def _shared_cache_controller(viewfunc):
+        @wraps(viewfunc, assigned=available_attrs(viewfunc))
+        def _cache_controlled(request, *args, **kw):
+            response = viewfunc(request, *args, **kw)
+            nocache = (response.has_header('Cache-Control') and
+                       'no-cache' in response['Cache-Control'] and
+                       'no-store' in response['Cache-Control'])
+            if not nocache:
+                patch_cache_control(response, **cc_kwargs)
+            return response
+        return _cache_controlled
+
+    if func:
+        return _shared_cache_controller(func)
+    return _shared_cache_controller
 
 
 def user_access_decorator(redirect_func, redirect_url_func, deny_func=None,

kuma/core/tests/test_decorators.py

@@ -3,6 +3,7 @@
 from django.contrib.auth.models import AnonymousUser
 from django.http import HttpResponse
 from django.test import RequestFactory
+from django.views.decorators.cache import never_cache
 
 from kuma.users.tests import UserTestCase
 
@@ -208,3 +209,16 @@ def test_shared_cache_control_decorator_with_overrides(rf, settings):
     assert 'public' in response['Cache-Control']
     assert 'max-age=999' in response['Cache-Control']
     assert 's-maxage=0' in response['Cache-Control']
+
+
+def test_shared_cache_control_decorator_keeps_no_cache(rf, settings):
+    request = rf.get('/foo')
+    response = shared_cache_control(never_cache(simple_view))(request)
+    assert response.status_code == 200
+    assert 'Cache-Control' in response
+    assert 'public' not in response['Cache-Control']
+    assert 's-maxage' not in response['Cache-Control']
+    assert 'max-age=0' in response['Cache-Control']
+    assert 'no-cache' in response['Cache-Control']
+    assert 'no-store' in response['Cache-Control']
+    assert 'must-revalidate' in response['Cache-Control']

kuma/wiki/tests/test_views.py

@@ -779,8 +779,12 @@ def test_create_on_404(self):
         # child page.
         resp = self.client.get(url)
         assert resp.status_code == 302
-        assert 'public' in resp['Cache-Control']
-        assert 's-maxage' in resp['Cache-Control']
+        assert 'max-age=0' in resp['Cache-Control']
+        assert 'no-cache' in resp['Cache-Control']
+        assert 'no-store' in resp['Cache-Control']
+        assert 'must-revalidate' in resp['Cache-Control']
+        assert 'public' not in resp['Cache-Control']
+        assert 's-maxage' not in resp['Cache-Control']
         assert 'docs/new' in resp['Location']
         assert ('?slug=%s' % local_slug) in resp['Location']
 
@@ -795,15 +799,17 @@ def test_create_on_404(self):
         response = self.client.get(reverse('wiki.document',
                                            args=['noExist'], locale=locale))
         assert response.status_code == 302
-        assert 'public' in response['Cache-Control']
-        assert 's-maxage' in response['Cache-Control']
+        assert 'public' not in response['Cache-Control']
+        assert 'no-cache' in resp['Cache-Control']
+        assert 'docs/new' in response['Location']
 
         response = self.client.get(reverse('wiki.document',
                                            args=['Template:NoExist'],
                                            locale=locale))
         assert response.status_code == 302
-        assert 'public' in response['Cache-Control']
-        assert 's-maxage' in response['Cache-Control']
+        assert 'public' not in response['Cache-Control']
+        assert 'no-cache' in resp['Cache-Control']
+        assert 'docs/new' in response['Location']
 
     def test_creating_child_of_redirect(self):
         """
@@ -822,8 +828,9 @@ def test_creating_child_of_redirect(self):
         url = reverse('wiki.document', locale='en-US', args=[child_full_slug])
         response = self.client.get(url)
         assert response.status_code == 302
-        assert 'public' in response['Cache-Control']
-        assert 's-maxage' in response['Cache-Control']
+        assert 'public' not in response['Cache-Control']
+        assert 'no-cache' in response['Cache-Control']
+        assert 'docs/new' in response['Location']
         # The parent id of the query should be same because while moving,
         # a new document is created with old slug and make redirect to the
         # old document
@@ -848,8 +855,9 @@ def test_creating_child_of_redirect_zoned_document(self):
         zoned_child_full_slug = zoned_doc.url_root + "/" + "children_document"
         response = self.client.get(zoned_child_full_slug)
         assert response.status_code == 302
-        assert 'public' in response['Cache-Control']
-        assert 's-maxage' in response['Cache-Control']
+        assert 'public' not in response['Cache-Control']
+        assert 'no-cache' in response['Cache-Control']
+        assert 'docs/new' in response['Location']
         # The parent id of the query should be same because while moving,
         # a new document is created with old slug and make redirect to the
         # old document

kuma/wiki/views/document.py

@@ -15,7 +15,7 @@
                          JsonResponse)
 from django.http.multipartparser import MultiPartParser
 from django.shortcuts import get_object_or_404, redirect, render
-from django.utils.cache import patch_vary_headers
+from django.utils.cache import add_never_cache_headers, patch_vary_headers
 from django.utils.http import parse_etags
 from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext
@@ -659,7 +659,9 @@ def document(request, document_slug, document_locale):
             create_url = _document_redirect_to_create(document_slug,
                                                       document_locale,
                                                       slug_dict)
-            return redirect(create_url)
+            response = redirect(create_url)
+            add_never_cache_headers(response)
+            return response
 
     # We found a Document. Now we need to figure out how we're going
     # to display it.