Views access plugins alter route's access requirements, altered routes must have associated cache tags, because site builders can modify the view (the config entity), causing different access alterations. Unless the responses for this route always have corresponding cache tags (i.e. the View config entity), it is possible for access to be denied once to the anonymous user, and then for the resulting page to be cached forever. The View config entity's cache tag does not get set because the route's controller is never executed, because access checking happens at the routing level, not at the controller level.

Quoting the patch in #606840: Enable internal page cache by default:

diff --git a/core/modules/views/src/Tests/Plugin/AccessTest.php b/core/modules/views/src/Tests/Plugin/AccessTest.php
index 4bc77db..c756706 100644
--- a/core/modules/views/src/Tests/Plugin/AccessTest.php
+++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\views\Tests\Plugin;
 
+use Drupal\Core\Cache\Cache;
 use Drupal\views\Tests\ViewTestData;
 use Drupal\views\Views;
 
@@ -101,6 +102,13 @@ function testStaticAccessPlugin() {
     // termination event fires. Simulate that here.
     $this->container->get('router.builder')->rebuildIfNeeded();
 
+    // Clear the page cache.
+    // @todo Remove this. The root cause is that the access plugins alters the
+    //   route's access requirements. That means that the 403 from above does
+    //   not have any cache tags, so modifying the View entity does not cause
+    //   the cached 403 page to be invalidated.
+    Cache::invalidateTags(['rendered']);
+
     $this->assertTrue($access_plugin->access($this->normalUser));
 
     $this->drupalGet('test_access_static');

Comments

amateescu’s picture

Assigned: Unassigned » amateescu
Issue tags: +D8 Accelerate Dev Days

Working on this.

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Active » Needs review
StatusFileSize
new7.86 KB

This should do it.

Status: Needs review » Needs work

The last submitted patch, 3: 2464657.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new8.27 KB
new1.12 KB

Hopefully better this time :)

wim leers’s picture

Status: Needs review » Needs work

This looks wonderfully simple so far :)

+++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
@@ -102,14 +102,6 @@ function testStaticAccessPlugin() {
-    Cache::invalidateTags(['rendered']);

Can we add an assertCacheTag($view->getCacheTags()[0]) assertion?

Also, I don't think the current code works for multiple cache tags when cache_tags: … is set in a *.routing.yml file (EDIT: nope, that does look fine, but that is still worth having a tiny simple test for). We'll want a test asserting that that is supported. Look at 996eb16/#2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag for a similar set of changes, where you can probably c/p a bunch to build a test.

The last submitted patch, 5: 2464657-5.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new8.87 KB
new3.78 KB

Thanks for reviewing :)

Fixed both points and the test fails from above.

amateescu’s picture

Status: Needs work » Needs review

Go bot!

amateescu’s picture

StatusFileSize
new8.87 KB

Trying once more, same patch as #8.

Status: Needs review » Needs work

The last submitted patch, 10: 2464657-10.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
StatusFileSize
new10.8 KB
new3.15 KB

Better fix for the failures in #5, it seems the test is the one that needs to be fixed.

wim leers’s picture

Title: Views access plugins alter route's access requirements, altered routes must have associated cache tags » Non-declarative (configurable) routes must be able to associate cache tags (was: Views access plugins alter route's access requirements, altered routes must have associated cache tags)
Component: views.module » routing system
Assigned: Unassigned » fabianx
Related issues: +#2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created

Looks great. I had too much of a hand in this issue to be able to RTBC it. Assigning to Fabianx for review.

Related: #2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created.

fabianx’s picture

Assigned: fabianx » Unassigned
Status: Needs review » Reviewed & tested by the community

RTBC, I can't wait for #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers to make this API simpler, so you can just call addCacheTags() like normally, but for now this should suffice.

Not sure assertTrue(isset()) is a common pattern, but don't know any better, so leaving for a core committer to take a look.

Just one thing, not sure it affects RTBC state:

+++ b/core/modules/views/src/Plugin/views/access/AccessPluginBase.php
@@ -59,7 +59,9 @@ public function summaryTitle() {
+  public function alterRouteDefinition(Route $route) {
+    $route->setOption('cache_tags', $this->view->storage->getCacheTags());
+  }

Maybe we add a comment here, why we have to do that?

amateescu’s picture

StatusFileSize
new11.22 KB
new970 bytes

Is this comment helpful or too "wordy"? :)

fabianx’s picture

Nope, this is great!

wim leers’s picture

Great work! :)

amateescu’s picture

StatusFileSize
new965 bytes

So #2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created happened in the meantime which fixed our problem in a more generic way, the only thing we need to do here now is to remove the manual cache invalidation from the test.

wim leers’s picture

Looks great!

fabianx’s picture

Status: Reviewed & tested by the community » Needs review

Are we sure the additional view tags are not helpful anymore?

What happens if something was 'okay', but is 404 now?

This patch takes care of that the, 40x patch does not - as far as I can see.

wim leers’s picture

If something "was okay" and now is 404 due to changes in the access plugin configuration for a view, then the response will have the View's cache tag already anyway, because it contains the rendered View, and therefore the bubbled cache tag.

fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Right that is the same for nodes, therefore back to RTBC for #18.

catch’s picture

Title: Non-declarative (configurable) routes must be able to associate cache tags (was: Views access plugins alter route's access requirements, altered routes must have associated cache tags) » Remove unnecessary cache clear in Views tests
Status: Reviewed & tested by the community » Fixed

Yes the 40X patch was really nifty, and this issue shows why.

  • catch committed 95545a8 on 8.0.x
    Issue #2464657 by amateescu: Remove unnecessary cache clear in Views...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.