Drupal\field\Plugin\views\field\Field::access returns an object instead of the expected boolean

Call fieldAccess() in Drupal\field\Plugin\views\field\Field::accesswith $return_as_object = FALSE instead of TRUE.

That will allow access for 'neutral' access. Here are two patches. One just changes $return_as_object to FALSE. The other leaves it as TRUE and then asks the object for explicit access. Let's see what the testbot thinks.

So fields don't require explicit access?

Ahh, never mind. The bool solution just short-cuts isAllowed(). Yay security as a side-effect!

FALSE is the default value, so you can remove the last two arguments...

Learning to generate a fixture for a views integration test isn't something I'm going to be able to get to in a timely manner, so if someone wants to take this up, go right ahead.

adding tests

So I wrote a test, and scratched the surface, picked at the wallpaper and accidentally discovered that any plugin that uses the standard views field plugin *ignores entity access controllers*

Here's a test to demonstrate that.

The issue is

  /**
   * {@inheritdoc}
   */
  public function access(AccountInterface $account) {
    if (isset($this->definition['access callback']) && function_exists($this->definition['access callback'])) {
      if (isset($this->definition['access arguments']) && is_array($this->definition['access arguments'])) {
        return call_user_func_array($this->definition['access callback'], array($account) + $this->definition['access arguments']);
      }
      return $this->definition['access callback']($account);
    }

    return TRUE;
  }

in HandlerBase isn't overridden by FieldPluginBase

tagging

Spoke to @chx on irc - this is a new issue, opening separate issue.

Added #2385443: Test that base entity fields on views respect field level access for base field issue.

Here's a WIP towards a test for this, unfortunately it passes.

Going to have a look as part of the DA core sprint in Ghent.

The test currently works because the field is tested as part of an entity. When ViewExecutable::_initHandler() checks access during $view->preExecute() the failed access is ignored because the object is passed instead of the boolean. Later on in the test a second access check is performed in EntityViewDisplay::buildMultiple() which is called as part of $view->style_plugin->getField(). This one is handling the access correctly and sets '#access' to FALSE on the field.

So in order to prove that this bug is definitively fixed we need to check that the field is removed from the view after the preExecute() step. I would also leave the current test in place because that is a good functional test: it checks that the field is actually not being rendered.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed fb40bd6 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php b/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
index 3c2956f..f3bd30a 100644
--- a/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
+++ b/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
@@ -155,7 +155,7 @@ public function _testInaccessibleFieldRender() {
     $this->executeView($view);
 
     // Check that the field handler for the hidden field is correctly removed
-    // from the display. This is a regression test for issue #2382931.
+    // from the display.
     // @see https://www.drupal.org/node/2382931
     $this->assertFalse(array_key_exists('field_no_view_access', $view->field));

Fixed duplication in comment.

+++ b/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
@@ -127,6 +149,24 @@ public function _testSimpleFieldRender() {
 
+  public function _testInaccessibleFieldRender() {

this needs follow-up to add docblocks