Allow DisplayPluginInterface::access() to return an AccessResultInterface object with cacheability metadata

dawehner++

This looks great already.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2234,11 +2235,13 @@ public function access(AccountInterface $account = NULL) {
   +      assert('$result instanceof \Drupal\Core\Access\AccessResultInterface');
   

   Nice :)

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1617,17 +1618,38 @@ public function attachDisplays() {
   +   * @param bool $return_as_object
   +   * @param bool $return_as_object
   

   :)

3. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1635,15 +1657,18 @@ public function access($displays = NULL, $account = NULL) {
   +          // @fixme This is certainly not the right thing to do. We want to not
   +          // combine all of them but rather just match one of them.
   +          $access = $access->orIf($display->access($account));
   

   This can use other complex cases (such as entity/field access) as examples to follow.

4. +++ b/core/modules/views/tests/modules/views_test_data/src/Plugin/views/access/StaticTest.php
   @@ -33,7 +34,7 @@ protected function defineOptions() {
   -    return !empty($this->options['access']);
   +    return !empty($this->options['access']) ? AccessResult::allowed() : AccessResult::forbidden();
   

   Hrm, isn't this technically then dependent on the View's configuration, and shouldn't it therefore add the View as a cacheable dependency?

   Similar remark for all other places that use $this->options.

I think this issue needs to be critical. Without this, HEAD may for example end up render caching a view across all users, even if access was only granted to a specific user (because the cacheability metadata of the used Views access plugins does not bubble up, hence the necessary cache contexts are missing).

Well, do we need to take that into account for $access or isn't all the other cache tags which are exposed with a view already good enough?

Yeah, that's why I said technically. In practice, it won't make a big difference because of what you said.

Except… for ViewsBlock should — just like any other block — actually do this access checking in BlockPluginInterface::access(). That's the correct semantical thing to do. If/when we do that, this will actually matter, because access to the block does depend on the underlying view's configuration.

We don't cache an access call directly, but we might be caching a whole page that contains blocks and therefore need to be aware of their access cacheability.

For example with smartcache or eventually also with page_manager.

#2464427-203: Replace CacheablePluginInterface with CacheableDependencyInterface found good reasons to still do this:

+    // @todo Fix this in https://www.drupal.org/node/2551037,
+    // DisplayPluginBase::applyDisplayCachablityMetadata() is not invoked when
+    // using buildBasicRenderable() and a Views access plugin returns FALSE.

So this API change won't be possible anymore.

But I think we can at least get the cacheability metadata of the used access plugin, something like this above the actual access check:

    if (!isset($view->current_display)) {
      $view->initDisplay();
    }

    if (($display = $view->displayHandlers->get($element['#display_id'])) && $display->getPlugin('access')) {
      $cacheablity_metadata = CacheableMetadata::createFromRenderArray($element);
      $cacheablity_metadata = $cacheablity_metadata->merge(CacheableMetadata::createFromObject($display->getPlugin('access')));
      $cacheablity_metadata->applyTo($element);
    }

Also, views access checks actually are render-cached, atleast for rest displays.

core/modules/user/src/Tests/Views/AccessRoleTest.php contains a @todo about this issue, which doesn't seem to be addressed in #9.

I think this issue needs to be critical. Without this, HEAD may for example end up render caching a view across all users, even if access was only granted to a specific user

I don't see a pathway to that, at least not any more (maybe there was when #8 was written). Each access plugin can/should implement CacheableDependencyInterface, and if it does so, then that makes it into the cacheability information of the display and its renderings. Therefore, removing the "security" tag from this issue.

However, this issue might still make sense as a security improvement / extra protection for cases where a contrib/custom access plugin fails to implement its own CacheableDependencyInterface correctly. Tentatively tagging with "Security improvements" accordingly, but I'm demoting this to Normal priority until a case can be made (via an issue summary update) for why such an improvement should be higher priority than that.

So I think this is still major, just it is not a critical security issue.

So now we've got to do this with a BC shim

I guess we can introduce a new interface CacheabilityMetadataAwareAccessPlugin with a new method accessWithCacheability or something

And then in the display we can juggle based on instanceof, triggering an error if something didn't return cacheability and we had to call the old access method.

And in all the existing access methods, trigger error.

And then similar changes in the displays.

Its a long road, but I think we can do it with deprecations and BC