core/lib/Drupal/Core/Access/AccessResult.php | 9 ++++++++- .../DefaultExceptionHtmlSubscriber.php | 5 ++++- .../Core/Render/MainContent/HtmlRenderer.php | 9 +++++++++ core/lib/Drupal/Core/Routing/AccessAwareRouter.php | 10 +++++++++- .../Core/Routing/AccessAwareRouterInterface.php | 5 +++++ .../contact/src/Access/ContactPageAccess.php | 2 +- .../contact/src/Controller/ContactController.php | 5 +++++ .../contact/src/Tests/ContactPersonalTest.php | 2 ++ .../modules/node/src/Tests/Views/FrontPageTest.php | 7 ++++--- .../Tests/Cache/PageCacheTagsIntegrationTest.php | 2 ++ core/modules/views/src/Tests/GlossaryTest.php | 3 ++- .../Drupal/Tests/Core/Access/AccessResultTest.php | 23 +++++++++++----------- .../Tests/Core/Routing/AccessAwareRouterTest.php | 23 +++++++++++++++++----- 13 files changed, 81 insertions(+), 24 deletions(-) diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index db78d89..98d7947 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -339,7 +339,14 @@ public function setCacheMaxAge($max_age) { * @return $this */ public function cachePerRole() { - $this->addCacheContexts(array('user.roles')); + $this->addCacheContexts(['user.roles']) + // @todo this is always needed when the 'user.roles' cache context is set: + // when caching per role, and something has changed in that role (e.g. + // a permission is granted or revoked), then the results for that role + // will change! To invalidate accordingly, we need this cache tag. + // This will be handled automatically once + // https://www.drupal.org/node/2428703 lands. + ->addCacheTags(['config:user_role_list']); return $this; } diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php index a98bf57..5861e85 100644 --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php @@ -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; @@ -111,8 +112,10 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st } try { - // Persist the 'exception' attribute to the subrequest. + // Persist the 'exception' and access result attributes to the + // subrequest. $sub_request->attributes->set('exception', $request->attributes->get('exception')); + $sub_request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT)); // Carry over the session to the subrequest. if ($session = $request->getSession()) { diff --git a/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php index 269b64e..73cb2fc 100644 --- a/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php @@ -10,6 +10,7 @@ use Drupal\Component\Plugin\PluginManagerInterface; use Drupal\Component\Utility\NestedArray; use Drupal\Core\Cache\Cache; +use Drupal\Core\Cache\CacheableInterface; use Drupal\Core\Controller\TitleResolverInterface; use Drupal\Core\Display\PageVariantInterface; use Drupal\Core\Extension\ModuleHandlerInterface; @@ -17,6 +18,7 @@ use Drupal\Core\Render\Renderer; use Drupal\Core\Render\RendererInterface; use Drupal\Core\Render\RenderEvents; +use Drupal\Core\Routing\AccessAwareRouterInterface; use Drupal\Core\Routing\RouteMatchInterface; use Symfony\Component\DependencyInjection\ContainerAwareTrait; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -144,6 +146,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch } } + // Merge the request's access result cacheability metadata, if it has any. + $access_result = $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT); + if ($access_result instanceof CacheableInterface) { + $cache_contexts = Cache::mergeContexts($cache_contexts, $access_result->getCacheContexts()); + $cache_tags = Cache::mergeTags($cache_tags, $access_result->getCacheTags()); + } + // Set the generator in the HTTP header. list($version) = explode('.', \Drupal::VERSION, 2); diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php index 93a31c3..87e30a3 100644 --- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php @@ -105,7 +105,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(); } } diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php index 34fa75a..b6cfe4a 100644 --- a/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php @@ -16,6 +16,11 @@ interface AccessAwareRouterInterface extends RouterInterface, RequestMatcherInterface { /** + * Attribute name of the access result for the request.. + */ + const ACCESS_RESULT = '_access_result'; + + /** * {@inheritdoc} * * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException diff --git a/core/modules/contact/src/Access/ContactPageAccess.php b/core/modules/contact/src/Access/ContactPageAccess.php index 97d6b06..cb5630c 100644 --- a/core/modules/contact/src/Access/ContactPageAccess.php +++ b/core/modules/contact/src/Access/ContactPageAccess.php @@ -62,7 +62,7 @@ public function access(UserInterface $user, AccountInterface $account) { // Anonymous users cannot have contact forms. if ($contact_account->isAnonymous()) { - return AccessResult::forbidden(); + return AccessResult::forbidden()->cachePerRole(); } // Users may not contact themselves. diff --git a/core/modules/contact/src/Controller/ContactController.php b/core/modules/contact/src/Controller/ContactController.php index 648a064..8813cd9 100644 --- a/core/modules/contact/src/Controller/ContactController.php +++ b/core/modules/contact/src/Controller/ContactController.php @@ -106,6 +106,7 @@ public function contactSitePage(ContactFormInterface $contact_form = NULL) { $form = $this->entityFormBuilder()->getForm($message); $form['#title'] = String::checkPlain($contact_form->label()); + $form['#cache']['contexts'][] = 'user.roles'; $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [], $config->getCacheTags()); return $form; } @@ -141,6 +142,7 @@ public function contactPersonalPage(UserInterface $user) { $form = $this->entityFormBuilder()->getForm($message); $form['#title'] = $this->t('Contact @username', array('@username' => $user->getUsername())); + $form['#cache']['contexts'][] = 'user.roles'; return $form; } @@ -150,6 +152,9 @@ public function contactPersonalPage(UserInterface $user) { * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException */ protected function contactFloodControl() { + // Flood control is incompatible with page caching. + \Drupal::service('page_cache_kill_switch')->trigger(); + $limit = $this->config('contact.settings')->get('flood.limit'); $interval = $this->config('contact.settings')->get('flood.interval'); if (!$this->flood->isAllowed('contact', $limit, $interval)) { diff --git a/core/modules/contact/src/Tests/ContactPersonalTest.php b/core/modules/contact/src/Tests/ContactPersonalTest.php index 03965ad..3eaa709 100644 --- a/core/modules/contact/src/Tests/ContactPersonalTest.php +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php @@ -152,11 +152,13 @@ function testPersonalContactAccess() { // Test that anonymous users can access admin user's contact form. $this->drupalGet('user/' . $this->adminUser->id() . '/contact'); $this->assertResponse(200); + $this->assertCacheTag('config:user_role_list'); // Revoke the personal contact permission for the anonymous user. user_role_revoke_permissions(RoleInterface::ANONYMOUS_ID, array('access user contact forms')); $this->drupalGet('user/' . $this->contactUser->id() . '/contact'); $this->assertResponse(403); + $this->assertCacheTag('config:user_role_list'); $this->drupalGet('user/' . $this->adminUser->id() . '/contact'); $this->assertResponse(403); diff --git a/core/modules/node/src/Tests/Views/FrontPageTest.php b/core/modules/node/src/Tests/Views/FrontPageTest.php index 61fe5eb..43632d7 100644 --- a/core/modules/node/src/Tests/Views/FrontPageTest.php +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php @@ -241,7 +241,7 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) { $view = Views::getView('frontpage'); $view->setDisplay('page_1'); - $cache_contexts = ['node_view_grants', 'language']; + $cache_contexts = ['node_view_grants', 'language', 'user.roles']; // Test before there are any nodes. $empty_node_listing_cache_tags = [ @@ -257,7 +257,7 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) { $this->assertPageCacheContextsAndTags( Url::fromRoute('view.frontpage.page_1'), $cache_contexts, - Cache::mergeTags($empty_node_listing_cache_tags, ['rendered']) + Cache::mergeTags($empty_node_listing_cache_tags, ['config:user_role_list', 'rendered']) ); // Create some nodes on the frontpage view. Add more than 10 nodes in order @@ -317,7 +317,7 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) { $this->assertPageCacheContextsAndTags( Url::fromRoute('view.frontpage.page_1'), $cache_contexts, - Cache::mergeTags($first_page_output_cache_tags, ['rendered']) + Cache::mergeTags($first_page_output_cache_tags, ['config:user_role_list', 'rendered']) ); // Second page. @@ -329,6 +329,7 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) { 'node:4', 'node:5', // The rest. + 'config:user_role_list', 'config:filter.format.plain_text', 'config:views.view.frontpage', 'node_list', diff --git a/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php index f4d30ec..7e41e7f 100644 --- a/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php @@ -101,6 +101,7 @@ function testPageCacheTags() { 'config:system.menu.footer', 'config:system.menu.main', 'config:system.site', + 'config:user_role_list', )); // Full node page 2. @@ -130,6 +131,7 @@ function testPageCacheTags() { 'comment_list', 'node_list', 'config:views.view.comments_recent', + 'config:user_role_list', )); } diff --git a/core/modules/views/src/Tests/GlossaryTest.php b/core/modules/views/src/Tests/GlossaryTest.php index fd97978..472ba04 100644 --- a/core/modules/views/src/Tests/GlossaryTest.php +++ b/core/modules/views/src/Tests/GlossaryTest.php @@ -87,7 +87,8 @@ public function testGlossaryView() { // Verify cache tags. $this->enablePageCaching(); - $this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['cache.context.url', 'node_view_grants', 'language', 'user'], [ + $this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['cache.context.url', 'node_view_grants', 'language', 'user', 'user.roles'], [ + 'config:user_role_list', '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', diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index 42d2d2a..92d22c5 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -325,7 +325,7 @@ public function testCacheMaxAge() { * @covers ::allowedIfHasPermission */ public function testCacheContexts() { - $verify = function (AccessResult $access, array $contexts) { + $verify = function (AccessResult $access, array $contexts, array $tags = []) { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); $this->assertTrue($access->isNeutral()); @@ -333,7 +333,7 @@ public function testCacheContexts() { $this->assertSame('default', $access->getCacheBin()); $this->assertSame(Cache::PERMANENT, $access->getCacheMaxAge()); $this->assertSame($contexts, $access->getCacheContexts()); - $this->assertSame([], $access->getCacheTags()); + $this->assertSame($tags, $access->getCacheTags()); }; $access = AccessResult::neutral()->addCacheContexts(['foo']); @@ -357,10 +357,11 @@ public function testCacheContexts() { // ::cachePerRole() convenience method. $contexts = array('user.roles'); - $a = AccessResult::neutral()->addCacheContexts($contexts); - $verify($a, $contexts); + $tags = ['config:user_role_list']; + $a = AccessResult::neutral()->addCacheContexts($contexts)->addCacheTags($tags); + $verify($a, $contexts, $tags); $b = AccessResult::neutral()->cachePerRole(); - $verify($b, $contexts); + $verify($b, $contexts, $tags); $this->assertEquals($a, $b); // ::cachePerUser() convenience method. @@ -373,12 +374,12 @@ public function testCacheContexts() { // Both. $contexts = array('user', 'user.roles'); - $a = AccessResult::neutral()->addCacheContexts($contexts); - $verify($a, $contexts); + $a = AccessResult::neutral()->addCacheContexts($contexts)->addCacheTags($tags); + $verify($a, $contexts, $tags); $b = AccessResult::neutral()->cachePerRole()->cachePerUser(); - $verify($b, $contexts); + $verify($b, $contexts, $tags); $c = AccessResult::neutral()->cachePerUser()->cachePerRole(); - $verify($c, $contexts); + $verify($c, $contexts, $tags); $this->assertEquals($a, $b); $this->assertEquals($a, $c); @@ -393,7 +394,7 @@ public function testCacheContexts() { // Verify the object when using the ::allowedIfHasPermission() convenience // static method. $b = AccessResult::allowedIfHasPermission($account, 'may herd llamas'); - $verify($b, $contexts); + $verify($b, $contexts, $tags); } /** @@ -473,7 +474,7 @@ public function testInheritCacheability() { $this->assertTrue($access->inheritCacheability($other) instanceof AccessResult); $this->assertTrue($access->isCacheable()); $this->assertSame(['user.roles'], $access->getCacheContexts()); - $this->assertSame(['node:20011988'], $access->getCacheTags()); + $this->assertSame(['config:user_role_list', 'node:20011988'], $access->getCacheTags()); $this->assertSame('default', $access->getCacheBin()); $this->assertSame(1500, $access->getCacheMaxAge()); diff --git a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php index 3e8676f..da58cd9 100644 --- a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php @@ -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); } /**