Commit 6c3303dd authored by alexpott's avatar alexpott

Issue #2458349 by Wim Leers: Route's access result's cacheability not applied...

Issue #2458349 by Wim Leers: Route's access result's cacheability not applied to the response's cacheability
parent 25753561
......@@ -950,7 +950,7 @@ services:
class: Drupal\Core\EventSubscriber\FinishResponseSubscriber
tags:
- { name: event_subscriber }
arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy']
arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy', '@cache_contexts']
redirect_response_subscriber:
class: Drupal\Core\EventSubscriber\RedirectResponseSubscriber
arguments: ['@url_generator', '@router.request_context']
......
......@@ -7,6 +7,7 @@
namespace Drupal\Core\EventSubscriber;
use Drupal\Core\Routing\AccessAwareRouterInterface;
use Drupal\Core\Url;
use Drupal\Core\Utility\Error;
use Psr\Log\LoggerInterface;
......@@ -113,6 +114,9 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
try {
// Persist the 'exception' attribute to the subrequest.
$sub_request->attributes->set('exception', $request->attributes->get('exception'));
// Persist the access result attribute to the subrequest, so that the
// error page inherits the access result of the master request.
$sub_request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT));
// Carry over the session to the subrequest.
if ($session = $request->getSession()) {
......
......@@ -8,11 +8,15 @@
namespace Drupal\Core\EventSubscriber;
use Drupal\Component\Datetime\DateTimePlus;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Cache\CacheableInterface;
use Drupal\Core\Cache\CacheContexts;
use Drupal\Core\Config\Config;
use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\Core\PageCache\RequestPolicyInterface;
use Drupal\Core\PageCache\ResponsePolicyInterface;
use Drupal\Core\Routing\AccessAwareRouterInterface;
use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\Request;
......@@ -56,6 +60,13 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
*/
protected $responsePolicy;
/**
* The cache contexts service.
*
* @var \Drupal\Core\Cache\CacheContexts
*/
protected $cacheContexts;
/**
* Constructs a FinishResponseSubscriber object.
*
......@@ -67,12 +78,15 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
* A policy rule determining the cacheability of a request.
* @param \Drupal\Core\PageCache\ResponsePolicyInterface $response_policy
* A policy rule determining the cacheability of a response.
* @param \Drupal\Core\Cache\CacheContexts $cache_contexts
* The cache contexts service.
*/
public function __construct(LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, RequestPolicyInterface $request_policy, ResponsePolicyInterface $response_policy) {
public function __construct(LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, RequestPolicyInterface $request_policy, ResponsePolicyInterface $response_policy, CacheContexts $cache_contexts) {
$this->languageManager = $language_manager;
$this->config = $config_factory->get('system.performance');
$this->requestPolicy = $request_policy;
$this->responsePolicy = $response_policy;
$this->cacheContexts = $cache_contexts;
}
/**
......@@ -119,6 +133,12 @@ public function onRespond(FilterResponseEvent $event) {
$response->headers->set($name, $value, FALSE);
}
// Apply the request's access result cacheability metadata, if it has any.
$access_result = $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT);
if ($access_result instanceof CacheableInterface) {
$this->updateDrupalCacheHeaders($response, $access_result);
}
$is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
// Add headers necessary to specify whether the response should be cached by
......@@ -138,6 +158,30 @@ public function onRespond(FilterResponseEvent $event) {
}
}
/**
* Updates Drupal's cache headers using the route's cacheable access result.
*
* @param Response $response
* @param CacheableInterface $cacheable_access_result
*/
protected function updateDrupalCacheHeaders(Response $response, CacheableInterface $cacheable_access_result) {
// X-Drupal-Cache-Tags
$cache_tags = $cacheable_access_result->getCacheTags();
if ($response->headers->has('X-Drupal-Cache-Tags')) {
$existing_cache_tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
$cache_tags = Cache::mergeTags($existing_cache_tags, $cache_tags);
}
$response->headers->set('X-Drupal-Cache-Tags', implode(' ', $cache_tags));
// X-Drupal-Cache-Contexts
$cache_contexts = $cacheable_access_result->getCacheContexts();
if ($response->headers->has('X-Drupal-Cache-Contexts')) {
$existing_cache_contexts = explode(' ', $response->headers->get('X-Drupal-Cache-Contexts'));
$cache_contexts = Cache::mergeContexts($existing_cache_contexts, $cache_contexts);
}
$response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContexts->optimizeTokens($cache_contexts)));
}
/**
* Determine whether the given response has a custom Cache-Control header.
*
......
......@@ -101,7 +101,15 @@ public function matchRequest(Request $request) {
* The request to access check.
*/
protected function checkAccess(Request $request) {
if (!$this->accessManager->checkRequest($request, $this->account)) {
// The cacheability (if any) of this request's access check result must be
// applied to the response.
$access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
// Allow a master request to set the access result for a subrequest: if an
// access result attribute is already set, don't overwrite it.
if (!$request->attributes->has(AccessAwareRouterInterface::ACCESS_RESULT)) {
$request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $access_result);
}
if (!$access_result->isAllowed()) {
throw new AccessDeniedHttpException();
}
}
......
......@@ -15,6 +15,11 @@
*/
interface AccessAwareRouterInterface extends RouterInterface, RequestMatcherInterface {
/**
* Attribute name of the access result for the request..
*/
const ACCESS_RESULT = '_access_result';
/**
* {@inheritdoc}
*
......
......@@ -246,6 +246,8 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) {
// Cache contexts associated with the view.
'user.node_grants:view',
'languages:' . LanguageInterface::TYPE_INTERFACE,
// Cache contexts associated with the route's access checking.
'user.permissions',
// Default cache contexts of the renderer.
'theme',
];
......
......@@ -76,6 +76,7 @@ function testPageCacheTags() {
'route.menu_active_trails:tools',
'theme',
'timezone',
'user.permissions',
'user.roles',
];
......
......@@ -7,6 +7,8 @@
namespace Drupal\system\Tests\Routing;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Language\LanguageInterface;
use Drupal\simpletest\WebTestBase;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
......@@ -22,32 +24,60 @@ class RouterTest extends WebTestBase {
*
* @var array
*/
public static $modules = array('block', 'router_test');
public static $modules = array('router_test');
/**
* Confirms that our default controller logic works properly.
* Confirms that our FinishResponseSubscriber logic works properly.
*/
public function testDefaultController() {
public function testFinishResponseSubscriber() {
$renderer_required_cache_contexts = ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme'];
// Confirm that the router can get to a controller.
$this->drupalGet('router_test/test1');
$this->assertRaw('test1', 'The correct string was returned because the route was successful.');
// Check expected headers from FinishResponseSubscriber.
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-ua-compatible'], 'IE=edge');
$this->assertEqual($headers['content-language'], 'en');
$this->assertEqual($headers['x-content-type-options'], 'nosniff');
$this->drupalGet('router_test/test2');
$this->assertRaw('test2', 'The correct string was returned because the route was successful.');
// Check expected headers from FinishResponseSubscriber.
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-drupal-cache-contexts'], implode(' ', $renderer_required_cache_contexts));
$this->assertEqual($headers['x-drupal-cache-tags'], 'rendered');
// Confirm that the page wrapping is being added, so we're not getting a
// raw body returned.
$this->assertRaw('</html>', 'Page markup was found.');
// In some instances, the subrequest handling may get confused and render
// a page inception style. This test verifies that is not happening.
$this->assertNoPattern('#</body>.*</body>#s', 'There was no double-page effect from a misrendered subrequest.');
// Confirm that route-level access check's cacheability is applied to the
// X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags headers.
// 1. controller result: render array, globally cacheable route access.
$this->drupalGet('router_test/test18');
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-drupal-cache-contexts'], implode(' ', Cache::mergeContexts($renderer_required_cache_contexts, ['url'])));
$this->assertEqual($headers['x-drupal-cache-tags'], 'foo rendered');
// 2. controller result: render array, per-role cacheable route access.
$this->drupalGet('router_test/test19');
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-drupal-cache-contexts'], implode(' ', Cache::mergeContexts($renderer_required_cache_contexts, ['url', 'user.roles'])));
$this->assertEqual($headers['x-drupal-cache-tags'], 'foo rendered');
// 3. controller result: Response object, globally cacheable route access.
$this->drupalGet('router_test/test1');
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-drupal-cache-contexts'], '');
$this->assertEqual($headers['x-drupal-cache-tags'], '');
// 4. controller result: Response object, per-role cacheable route access.
$this->drupalGet('router_test/test20');
$headers = $this->drupalGetHeaders();
$this->assertEqual($headers['x-drupal-cache-contexts'], 'user.roles');
$this->assertEqual($headers['x-drupal-cache-tags'], '');
}
/**
......
......@@ -106,6 +106,27 @@ router_test.17:
requirements:
_access: 'TRUE'
router_test.18:
path: '/router_test/test18'
defaults:
_controller: '\Drupal\router_test\TestControllers::test18'
requirements:
_access: 'TRUE'
router_test.19:
path: '/router_test/test19'
defaults:
_controller: '\Drupal\router_test\TestControllers::test18'
requirements:
_role: 'anonymous'
router_test.20:
path: '/router_test/test20'
defaults:
_controller: '\Drupal\router_test\TestControllers::test1'
requirements:
_role: 'anonymous'
router_test.hierarchy_parent:
path: '/menu-test/parent'
defaults:
......
......@@ -84,6 +84,19 @@ public function test10() {
$this->throwException('<script>alert(\'xss\')</script>');
}
public function test18() {
return [
'#cache' => [
'contexts' => ['url'],
'tags' => ['foo'],
'max-age' => 60,
],
'content' => [
'#markup' => 'test18',
],
];
}
/**
* Throws an exception.
*
......
......@@ -88,7 +88,7 @@ public function testGlossaryView() {
// Verify cache tags.
$this->enablePageCaching();
$this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['languages', 'theme', 'url', 'user.node_grants:view'], [
$this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['languages', 'theme', 'url', 'user.node_grants:view', 'user.permissions'], [
'config:views.view.glossary',
'node:' . $nodes_by_char['a'][0]->id(), 'node:' . $nodes_by_char['a'][1]->id(), 'node:' . $nodes_by_char['a'][2]->id(),
'node_list',
......
......@@ -7,7 +7,9 @@
namespace Drupal\Tests\Core\Routing;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Routing\AccessAwareRouter;
use Drupal\Core\Routing\AccessAwareRouterInterface;
use Drupal\Tests\UnitTestCase;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpFoundation\Request;
......@@ -73,13 +75,18 @@ protected function setupRouter() {
public function testMatchRequestAllowed() {
$this->setupRouter();
$request = new Request();
$access_result = AccessResult::allowed();
$this->accessManager->expects($this->once())
->method('checkRequest')
->with($request)
->will($this->returnValue(TRUE));
->willReturn($access_result);
$parameters = $this->router->matchRequest($request);
$this->assertSame($request->attributes->all(), array(RouteObjectInterface::ROUTE_OBJECT => $this->route));
$this->assertSame($parameters, array(RouteObjectInterface::ROUTE_OBJECT => $this->route));
$expected = [
RouteObjectInterface::ROUTE_OBJECT => $this->route,
AccessAwareRouterInterface::ACCESS_RESULT => $access_result,
];
$this->assertSame($expected, $request->attributes->all());
$this->assertSame($expected, $parameters);
}
/**
......@@ -90,11 +97,17 @@ public function testMatchRequestAllowed() {
public function testMatchRequestDenied() {
$this->setupRouter();
$request = new Request();
$access_result = AccessResult::forbidden();
$this->accessManager->expects($this->once())
->method('checkRequest')
->with($request)
->will($this->returnValue(FALSE));
$this->router->matchRequest($request);
->willReturn($access_result);
$parameters = $this->router->matchRequest($request);
$expected = [
AccessAwareRouterInterface::ACCESS_RESULT => $access_result,
];
$this->assertSame($expected, $request->attributes->all());
$this->assertSame($expected, $parameters);
}
/**
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment