diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 53357cb83f..a6de5e45cc 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -13,6 +13,7 @@ Changelog * Add the ability to restrict what types of requests a Pages supports via `allowed_http_methods` (Andy Babic) * Allow plain strings in panel definitions as shorthand for `FieldPanel` / `InlinePanel` (Matt Westcott) * Only allow selection of valid new parents within the copy Page view (Mauro Soche) + * Add `on_serve_page` hook to modify the serving chain of pages (Krystian Magdziarz, Dawid Bugajewski) * Fix: Improve handling of translations for bulk page action confirmation messages (Matt Westcott) * Fix: Ensure custom rich text feature icons are correctly handled when provided as a list of SVG paths (Temidayo Azeez, Joel William, LB (Ben) Johnston) * Fix: Ensure manual edits to `StreamField` values do not throw an error (Stefan Hammer) @@ -32,6 +33,7 @@ Changelog * Fix: Ensure `MultipleChooserPanel` using images or documents work when nested within an `InlinePanel` when no other choosers are in use within the model (Elhussein Almasri) * Fix: Ensure `MultipleChooserPanel` works after doing a search in the page chooser modal (Matt Westcott) * Fix: Ensure new `ListBlock` instances get created with unique IDs in the admin client for accessibility and mini-map element references (Srishti Jaiswal) + * Fix: Return never-cache HTTP headers when serving pages and documents with view restrictions (Krystian Magdziarz, Dawid Bugajewski) * Docs: Move the model reference page from reference/pages to the references section as it covers all Wagtail core models (Srishti Jaiswal) * Docs: Move the panels reference page from references/pages to the references section as panels are available for any model editing, merge panels API into this page (Srishti Jaiswal) * Docs: Move the tags documentation to standalone advanced topic, instead of being inside the reference/pages section (Srishti Jaiswal) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index e2d1a4eb05..8699bd687d 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -860,6 +860,7 @@ * Alex Fulcher * Harsh Dange * Mauro Soche +* Krystian Magdziarz ## Translators diff --git a/docs/reference/hooks.md b/docs/reference/hooks.md index a45c3354e3..741804bf15 100644 --- a/docs/reference/hooks.md +++ b/docs/reference/hooks.md @@ -1131,6 +1131,40 @@ def block_googlebot(page, request, serve_args, serve_kwargs): return HttpResponse("

bad googlebot no cookie

") ``` +(on_serve_page)= + +### `on_serve_page` + +Called when Wagtail is serving a page, after `before_serve_page` but before the page's `serve()` method is called. Unlike `before_serve_page`, this hook allows you to modify the serving chain rather than just returning an alternative response. + +The callable passed to this hook must accept a function as its argument and return a new function that will be used in its place. The passed-in function will be the next callable in the serving chain. + +For example, to add custom cache headers to the response: + +```python +from wagtail import hooks + +@hooks.register('on_serve_page') +def add_custom_headers(next_serve_page): + def wrapper(page, request, args, kwargs): + response = next_serve_page(page, request, args, kwargs) + response['Custom-Header'] = 'value' + return response + return wrapper +``` + +Parameters passed to the function: +- `page` - the Page object being served +- `request` - the request object +- `args` - positional arguments that will be passed to the page's serve method +- `kwargs` - keyword arguments that will be passed to the page's serve method + +This hook is particularly useful for: +- Adding/modifying response headers +- Implementing access restrictions +- Modifying the response content +- Adding logging or monitoring + ## Document serving (before_serve_document)= diff --git a/docs/releases/6.4.md b/docs/releases/6.4.md index cda304e069..742b7fe8ea 100644 --- a/docs/releases/6.4.md +++ b/docs/releases/6.4.md @@ -22,6 +22,7 @@ depth: 1 * Add the ability to restrict what types of requests a Pages supports via [`allowed_http_methods`](#wagtail.models.Page.allowed_http_methods) (Andy Babic) * Allow plain strings in panel definitions as shorthand for `FieldPanel` / `InlinePanel` (Matt Westcott) * Only allow selection of valid new parents within the copy Page view (Mauro Soche) + * Add [`on_serve_page`](on_serve_page) hook to modify the serving chain of pages (Krystian Magdziarz, Dawid Bugajewski) ### Bug fixes @@ -44,6 +45,7 @@ depth: 1 * Ensure `MultipleChooserPanel` using images or documents work when nested within an `InlinePanel` when no other choosers are in use within the model (Elhussein Almasri) * Ensure `MultipleChooserPanel` works after doing a search in the page chooser modal (Matt Westcott) * Ensure new `ListBlock` instances get created with unique IDs in the admin client for accessibility and mini-map element references (Srishti Jaiswal) + * Return never-cache HTTP headers when serving pages and documents with view restrictions (Krystian Magdziarz, Dawid Bugajewski) ### Documentation diff --git a/wagtail/documents/wagtail_hooks.py b/wagtail/documents/wagtail_hooks.py index e319219b13..d394d5a842 100644 --- a/wagtail/documents/wagtail_hooks.py +++ b/wagtail/documents/wagtail_hooks.py @@ -3,6 +3,7 @@ from warnings import warn from django.conf import settings from django.template.response import TemplateResponse from django.urls import include, path, reverse, reverse_lazy +from django.utils.cache import add_never_cache_headers from django.utils.translation import gettext, ngettext from django.utils.translation import gettext_lazy as _ @@ -200,7 +201,11 @@ def check_view_restrictions(document, request): ) context = {"form": form, "action_url": action_url} - return TemplateResponse(request, password_required_template, context) + response = TemplateResponse( + request, password_required_template, context + ) + add_never_cache_headers(response) + return response elif restriction.restriction_type in [ BaseViewRestriction.LOGIN, diff --git a/wagtail/tests/test_hooks.py b/wagtail/tests/test_hooks.py index e8fac7dd29..78d763e029 100644 --- a/wagtail/tests/test_hooks.py +++ b/wagtail/tests/test_hooks.py @@ -1,7 +1,14 @@ -from django.test import TestCase +from unittest import mock + +from django.contrib.sessions.middleware import SessionMiddleware +from django.http import HttpResponse +from django.test import RequestFactory, TestCase from wagtail import hooks +from wagtail.models import Page, PageViewRestriction from wagtail.test.utils import WagtailTestUtils +from wagtail.views import serve, serve_chain +from wagtail.wagtail_hooks import check_view_restrictions def test_hook(): @@ -34,3 +41,161 @@ class TestLoginView(WagtailTestUtils, TestCase): with self.register_hook("test_hook_name", after_hook, order=1): hook_fns = hooks.get_hooks("test_hook_name") self.assertEqual(hook_fns, [test_hook, after_hook]) + + +class TestServeHooks(WagtailTestUtils, TestCase): + fixtures = ["test.json"] + + def setUp(self): + self.page = Page.objects.get(id=2) + self.request = RequestFactory().get("/test/") + middleware = SessionMiddleware(lambda x: None) + middleware.process_request(self.request) + self.request.session.save() + + def test_serve_chain_order(self): + order_calls = [] + + def hook_1(next_fn): + def wrapper(page, request, *args, **kwargs): + order_calls.append(1) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + def hook_2(next_fn): + def wrapper(page, request, *args, **kwargs): + order_calls.append(2) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + def hook_3(next_fn): + def wrapper(page, request, *args, **kwargs): + order_calls.append(3) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + with self.register_hook("on_serve_page", hook_1): + with self.register_hook("on_serve_page", hook_2): + with self.register_hook("on_serve_page", hook_3): + serve(self.request, self.page.url) + + self.assertEqual(order_calls, [1, 2, 3]) + + def test_serve_chain_modification(self): + def hook_modifier(next_fn): + def wrapper(page, request, *args, **kwargs): + response = next_fn(page, request, *args, **kwargs) + response.content = b"Modified content" + return response + + return wrapper + + with self.register_hook("on_serve_page", hook_modifier): + response = serve(self.request, self.page.url) + self.assertEqual(response.content, b"Modified content") + + def test_serve_chain_halt_execution(self): + def hook_halt(next_fn): + def wrapper(page, request, *args, **kwargs): + return HttpResponse("Halted") + + return wrapper + + with self.register_hook("on_serve_page", hook_halt): + response = serve(self.request, self.page.url) + self.assertEqual(response.content, b"Halted") + + def test_serve_chain_view_restriction(self): + restriction = PageViewRestriction.objects.create( + page=self.page, + restriction_type=PageViewRestriction.PASSWORD, + password="password", + ) + + with self.register_hook("on_serve_page", check_view_restrictions): + response = self.client.get(self.page.url) + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, "wagtailcore/password_required.html") + + restriction.delete() + + def test_serve_always_called_last(self): + hook_calls = [] + serve_called = [] + + def hook_1(next_fn): + def wrapper(page, request, *args, **kwargs): + hook_calls.append(1) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + def hook_2(next_fn): + def wrapper(page, request, *args, **kwargs): + hook_calls.append(2) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + def hook_3(next_fn): + def wrapper(page, request, *args, **kwargs): + hook_calls.append(3) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + original_serve_chain = serve_chain + + def mock_serve_chain(page, request, *args, **kwargs): + serve_called.append(True) + return original_serve_chain(page, request, *args, **kwargs) + + with mock.patch("wagtail.views.serve_chain", mock_serve_chain): + with self.register_hook("on_serve_page", hook_1): + with self.register_hook("on_serve_page", hook_2): + with self.register_hook("on_serve_page", hook_3): + serve(self.request, self.page.url) + + self.assertEqual(hook_calls, [1, 2, 3]) + self.assertEqual(len(serve_called), 1) + self.assertTrue(serve_called[0]) + + def test_check_view_restrictions_receives_correct_parameters(self): + received_params = [] + + def hook_spy(next_fn): + def wrapper(page, request, *args, **kwargs): + received_params.append( + {"page": page, "request": request, "args": args, "kwargs": kwargs} + ) + return next_fn(page, request, *args, **kwargs) + + return wrapper + + self.assertIsNotNone(self.page, "Test page should not be None") + self.assertIsNotNone(self.request, "Test request should not be None") + + with self.register_hook("on_serve_page", hook_spy): + route_result = Page.route_for_request(self.request, self.page.url) + + self.assertIsNotNone(route_result, "route_result should not be None") + + if route_result: + page, args, kwargs = route_result + serve(self.request, self.page.url) + + self.assertEqual(len(received_params), 1) + + params = received_params[0] + + self.assertIsNotNone(params["page"], "Hook received None as page") + self.assertIsNotNone(params["request"], "Hook received None as request") + + self.assertEqual(params["page"], self.page) + self.assertEqual(params["request"], self.request) + + self.assertEqual(params["args"], ([], {})) + self.assertEqual(params["kwargs"], {}) diff --git a/wagtail/tests/test_page_model.py b/wagtail/tests/test_page_model.py index 8aed66b9b7..220dc3e791 100644 --- a/wagtail/tests/test_page_model.py +++ b/wagtail/tests/test_page_model.py @@ -10,6 +10,7 @@ from django.core.exceptions import ValidationError from django.http import Http404 from django.test import Client, TestCase, override_settings from django.test.client import RequestFactory +from django.urls import reverse from django.utils import timezone, translation from freezegun import freeze_time @@ -4000,3 +4001,64 @@ class TestPageCachedParentObjExists(TestCase): "_cached_parent_obj_exists", "Page.get_parent() (treebeard) no longer uses _cached_parent_obj to cache the parent object", ) + + +class TestPageServeWithPasswordRestriction(TestCase, WagtailTestUtils): + def setUp(self): + self.root_page = Page.objects.get(id=2) + self.test_page = Page( + title="Test Page", + slug="test", + ) + self.root_page.add_child(instance=self.test_page) + + self.password_restriction = PageViewRestriction.objects.create( + page=self.test_page, + restriction_type=PageViewRestriction.PASSWORD, + password="password123", + ) + + def test_page_with_password_restriction_authenticated_has_cache_headers(self): + auth_url = reverse( + "wagtailcore_authenticate_with_password", + args=[self.password_restriction.id, self.test_page.id], + ) + + post_response = self.client.post( + auth_url, + { + "password": "password123", + "return_url": "/test/", + }, + ) + + self.assertRedirects(post_response, "/test/") + + response = self.client.get("/test/") + + self.assertTrue("Cache-Control" in response) + self.assertIn("max-age=0", response["Cache-Control"]) + self.assertIn("no-cache", response["Cache-Control"]) + self.assertIn("no-store", response["Cache-Control"]) + self.assertIn("must-revalidate", response["Cache-Control"]) + self.assertIn("private", response["Cache-Control"]) + self.assertTrue("Expires" in response) + + def test_page_with_password_restriction_has_cache_headers(self): + response = self.client.get("/test/") + + self.assertTrue("Cache-Control" in response) + self.assertIn("max-age=0", response["Cache-Control"]) + self.assertIn("no-cache", response["Cache-Control"]) + self.assertIn("no-store", response["Cache-Control"]) + self.assertIn("must-revalidate", response["Cache-Control"]) + self.assertIn("private", response["Cache-Control"]) + self.assertTrue("Expires" in response) + + def test_page_without_password_restriction_has_no_cache_headers(self): + self.password_restriction.delete() + + response = self.client.get("/test/") + + self.assertFalse("Cache-Control" in response) + self.assertFalse("Expires" in response) diff --git a/wagtail/tests/test_page_privacy.py b/wagtail/tests/test_page_privacy.py index 24c6168daa..7bb3e3c7c9 100644 --- a/wagtail/tests/test_page_privacy.py +++ b/wagtail/tests/test_page_privacy.py @@ -231,3 +231,13 @@ class TestPagePrivacy(WagtailTestUtils, TestCase): response = self.client.get("/secret-login-plans/") self.assertEqual(response.status_code, 200) self.assertContains(response, "Secret login plans") + + def test_password_protected_page_headers(self): + response = self.client.get("/secret-plans/") + self.assertEqual( + response.templates[0].name, "wagtailcore/password_required.html" + ) + self.assertIn("no-cache", response["Cache-Control"]) + self.assertIn("no-store", response["Cache-Control"]) + self.assertIn("must-revalidate", response["Cache-Control"]) + self.assertIn("max-age=0", response["Cache-Control"]) diff --git a/wagtail/views.py b/wagtail/views.py index ce83927de3..3c5c0055f6 100644 --- a/wagtail/views.py +++ b/wagtail/views.py @@ -9,6 +9,10 @@ from wagtail.forms import PasswordViewRestrictionForm from wagtail.models import Page, PageViewRestriction +def serve_chain(page, request, args, kwargs): + return page.serve(request, *args, **kwargs) + + def serve(request, path): route_result = Page.route_for_request(request, path) if route_result is None: @@ -16,12 +20,16 @@ def serve(request, path): else: page, args, kwargs = route_result + on_serve_chain = serve_chain + for fn in reversed(hooks.get_hooks("on_serve_page")): + on_serve_chain = fn(on_serve_chain) + for fn in hooks.get_hooks("before_serve_page"): result = fn(page, request, args, kwargs) if isinstance(result, HttpResponse): return result - return page.serve(request, *args, **kwargs) + return on_serve_chain(page, request, args, kwargs) def authenticate_with_password(request, page_view_restriction_id, page_id): diff --git a/wagtail/wagtail_hooks.py b/wagtail/wagtail_hooks.py index 93fbee070c..38663d88a8 100644 --- a/wagtail/wagtail_hooks.py +++ b/wagtail/wagtail_hooks.py @@ -5,6 +5,7 @@ from django.contrib.auth.models import Permission from django.contrib.auth.views import redirect_to_login from django.db import models from django.urls import reverse +from django.utils.cache import add_never_cache_headers from django.utils.text import capfirst from django.utils.translation import gettext_lazy as _ from django.utils.translation import ngettext @@ -28,35 +29,51 @@ def require_wagtail_login(next): return redirect_to_login(next, login_url) -@hooks.register("before_serve_page") -def check_view_restrictions(page, request, serve_args, serve_kwargs): - """ - Check whether there are any view restrictions on this page which are - not fulfilled by the given request object. If there are, return an - HttpResponse that will notify the user of that restriction (and possibly - include a password / login form that will allow them to proceed). If - there are no such restrictions, return None - """ - for restriction in page.get_view_restrictions(): - if not restriction.accept_request(request): - if restriction.restriction_type == PageViewRestriction.PASSWORD: - from wagtail.forms import PasswordViewRestrictionForm +@hooks.register("on_serve_page") +def check_view_restrictions(callback): + def inner(page, request, serve_args, serve_kwargs): + """ + Check whether there are any view restrictions on this page which are + not fulfilled by the given request object. If there are, return an + HttpResponse that will notify the user of that restriction (and possibly + include a password / login form that will allow them to proceed). If + there are no such restrictions, return None + """ + restrictions = page.get_view_restrictions() + response = None + for restriction in restrictions: + if not restriction.accept_request(request): + if restriction.restriction_type == PageViewRestriction.PASSWORD: + from wagtail.forms import PasswordViewRestrictionForm - form = PasswordViewRestrictionForm( - instance=restriction, - initial={"return_url": request.get_full_path()}, - ) - action_url = reverse( - "wagtailcore_authenticate_with_password", - args=[restriction.id, page.id], - ) - return page.serve_password_required_response(request, form, action_url) + form = PasswordViewRestrictionForm( + instance=restriction, + initial={"return_url": request.get_full_path()}, + ) + action_url = reverse( + "wagtailcore_authenticate_with_password", + args=[restriction.id, page.id], + ) - elif restriction.restriction_type in [ - PageViewRestriction.LOGIN, - PageViewRestriction.GROUPS, - ]: - return require_wagtail_login(next=request.get_full_path()) + response = page.serve_password_required_response( + request, form, action_url + ) + add_never_cache_headers(response) + return response + elif restriction.restriction_type in [ + PageViewRestriction.LOGIN, + PageViewRestriction.GROUPS, + ]: + response = require_wagtail_login(next=request.get_full_path()) + add_never_cache_headers(response) + return response + + response = callback(page, request, serve_args, serve_kwargs) + if restrictions: + add_never_cache_headers(response) + return response + + return inner @hooks.register("register_rich_text_features")