Test that base entity fields on views respect field level access

Tagging

What a wonder, views did not got an update while the entity world changed.

1. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_hostname_access.yml
   @@ -0,0 +1,207 @@
   diff --git a/core/modules/views/src/EntityViewsData.php b/core/modules/views/src/EntityViewsData.php
   
   diff --git a/core/modules/views/src/EntityViewsData.php b/core/modules/views/src/EntityViewsData.php
   index 4bb38fe..3bee3ac 100644
   
   index 4bb38fe..3bee3ac 100644
   --- a/core/modules/views/src/EntityViewsData.php
   
   --- a/core/modules/views/src/EntityViewsData.php
   +++ b/core/modules/views/src/EntityViewsData.php
   
   +++ b/core/modules/views/src/EntityViewsData.php
   +++ b/core/modules/views/src/EntityViewsData.php
   @@ -368,6 +368,12 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
   
   @@ -368,6 +368,12 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
        if (method_exists($this, $process_method)) {
          $this->{$process_method}($table, $field_definition, $views_field, $column_name);
   

   Let's expand the unit test coverage of EntityViewsData all the time

2. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -368,6 +368,12 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
   +    $views_field['field']['access callback'] = 'views_entity_base_field_access';
   

   Does this need to be a function or can it be any kind of callable?

3. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -368,6 +368,12 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
   +    $views_field['field']['access arguments'] = [
   +      'entity_type_id' => $this->entityType->id(),
   +      'field_name' => $field_definition->getName(),
   +    ];
    
   

   Note: This all is really related to #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency which basically would tackle the problem from the other side

Why don't we postpone this issue, or mark it as a duplicate? It seems like we need to have the base fields using Field UI anyway on #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency, which is already critical and should fix this problem, so this is just a stop-gap fix. I would be in favor of making the title of that issue "... and entity access is not respected" and combining this into it, since the real fix would be the same and we already need to fix that other issue. Whatever is being done here will just be discarded.

I sure hope there is no chance #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is demoted to major, it is impossible to make a multilingual view where both base fields and configurable fields exist now, so...

+++ b/core/modules/views/src/EntityViewsData.php
@@ -368,6 +368,8 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
diff --git a/core/modules/views/src/Plugin/views/field/Standard.php b/core/modules/views/src/Plugin/views/field/Standard.php

diff --git a/core/modules/views/src/Plugin/views/field/Standard.php b/core/modules/views/src/Plugin/views/field/Standard.php
index 11ecc6a..472667b 100644

index 11ecc6a..472667b 100644
--- a/core/modules/views/src/Plugin/views/field/Standard.php

--- a/core/modules/views/src/Plugin/views/field/Standard.php
+++ b/core/modules/views/src/Plugin/views/field/Standard.php

+++ b/core/modules/views/src/Plugin/views/field/Standard.php
+++ b/core/modules/views/src/Plugin/views/field/Standard.php
@@ -6,6 +6,9 @@

@@ -6,6 +6,9 @@
  */

@@ -16,4 +19,49 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function access(AccountInterface $account) {
+    if (isset($this->definition['field access']) && strstr($this->definition['field access'], ':') !== FALSE) {
+      list($entity_type_id, $field_name) = explode(':', $this->definition['field access']);
+      $access_handler = $this->entityManager->getAccessControlHandler($entity_type_id);
+      $field_definition = $this->entityManager->getBaseFieldDefinitions($entity_type_id)[$field_name];
+      return $access_handler->fieldAccess('view', $field_definition, $account);
+    }
+
+    return parent::access($account);
+  }
+

Just to be clear, its entirely wrong that this behaviour is attached to the Standard class for multiple reasons:

a) numeric fields are using a different class already
b) why do we not use the existing 'access callback' mechanism ... as you probably know, we could also expand that one to support what you want. its a callback, you can't get more flexibility.

If this is postponed on [#9416717] then that issue can't/shouldn't be demoted to major.

What is this postponed on...? catch's link doesn't seem to be valid or even resembling any of the issue mentioned here (I even tried https://www.drupal.org/comment/9416717/view)

I think it is postponed on #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency

Yeah its exactly postpoed on that.

Shouldn't this actually just be marked as a duplicate of that other issue, rather than postponed on the other issue? Will there be anything left to do here when the other (which is already also critical) is fixed?

@jhodgdon
I'm a huge fan of using issues as bugs, not only for solutions.
... I think we could for example leverage this issue to have dedicated tests for access checking.

OK. There is no reason to postpone writing tests, which would also allow us to see if the other issue fixes the problem.

Agreed

DIscussed with @dawehner on IRC - it would be great if #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency contained the test coverage since its implementation will solve this - but we should check once that issue is in.

Since this is security-related, tagging as D8 upgrade path.

Note: I think we should have a test which uses the entity_test entity instead of just the comment entity type as @larowlan did earlier.

Well #7 honestly, but we need a proper test coverage.

Well, not sure, we could do that for sure, or just leave the test for this issue, as they are logically two separated issues, its just an implementation detail that the
other issue fixes this already.

Do what you think is the best thing to do, but yeah adding the comment test coverage should be also included. More test coverage is
not a bad idea.

So #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency solves some of the base fields. IMHO we should figure out what other base fields are critical to resolve and either elevate those issues as well and postpone this one or choose another course of action.

We should discuss that on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, which lists all of the fields, probably?

.

$issue_title--

Let's get this in then :)

+1

Nice work!

Yay, moar test coverage! :)

Committed and pushed to 8.0.x. Thanks!

Superb thanks all!

looks like this evolved into a task along the way.