chx noticed this this morning and asked me to file an issue:

$info = array(
        '#theme' => 'field',
        '#weight' => $this->weight,
        '#title' => $instance['label'],
        '#access' => field_access($field, 'view', $entity_type, $entity),
        '#label_display' => $this->label,
        '#view_mode' => $this->viewMode,
        '#language' => $langcode,
        '#field_name' => $field['field_name'],
        '#field_type' => $field['type'],
        '#field_translatable' => $field['translatable'],
        '#entity_type' => $entity_type,
        '#bundle' => $entity->bundle(),
        '#object' => $entity,
        '#items' => $items,
        '#formatter' => $this->getPluginId(),
      );

field_access() is being called with the wrong order of arguments, so clearly we need more test coverage for field access restrictions.

Also, it concerns me a little that field_access($garbage, $garbage) is TRUE. ;)

CommentFileSizeAuthor
#14 1814418-14.patch4.95 KBswentel
#14 interdiff.txt896 bytesswentel
#10 1814418-10.patch4.89 KBswentel
#9 1814418-9.patch4.89 KBswentel
#9 interdiff.txt2.83 KBswentel
#7 1814418-7-test-only.patch3.58 KBswentel
#7 1814418-7.patch4.39 KBswentel
#5 1814418-2-test-only.patch2.98 KBswentel
#5 1814418-2.patch3.79 KBswentel
#3 1814418-2-test-only.patch1.59 KBswentel
#3 1814418-2.patch1.59 KBswentel
#1 1814418-1.patch832 bytesswentel

Comments

swentel’s picture

swentel commented
StatusFileSize
new 1814418-1.patch832 bytes

So weird, and there are tests re: access, but apparently they don't test well enough. I'll have a look at it later.

swentel’s picture

swentel commented
Status: Active » Needs review

Let's see if things fail now, that would be ironic :)

swentel’s picture

swentel commented
StatusFileSize
new 1814418-2.patch1.59 KB
new 1814418-2-test-only.patch1.59 KB

Ok, with tests now.

swentel’s picture

swentel commented

aargh, ignore those.

swentel’s picture

swentel commented
StatusFileSize
new 1814418-2.patch3.79 KB
new 1814418-2-test-only.patch2.98 KB

This should be better.

Status: Needs review » Needs work

The last submitted patch, 1814418-2.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 1814418-7.patch4.39 KB
new 1814418-7-test-only.patch3.58 KB

Ok, that was a bit too optimistic to use an extra permission, turns out other modules use the field_test as well. Different permission, a completely different field and a specific check in field_test_field_access().

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs tests

Thanks @swentel, that looks great. A couple minor notes about the test:

  1. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,75 @@
+class FieldAccessTest extends FieldTestBase {
...
+  function testFieldAccess() {

    Missing docblocks here. 

  2. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,75 @@
+    $web_user = $this->drupalCreateUser(array('bypass node access', 'view test_view_field content'));

    Minor, but bypass node access shouldn't be needed if there's no node access module enabled? It might be better to not mix node and field access in the test, so better to use the create type content permission (or, maybe better, just create the node with drupalCreateNode()). 

  3. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,75 @@
+      'test_view_field[und][0][value]' => $field_value

    Should be LANGUAGE_NOT_SPECIFIED rather than und I think. 

  4. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,75 @@
+    // Assert the text value is visible.
+    $this->assertText($field_value);
...
+    // Assert the text value is not visible for anonymous users.

    I'd expand these comments a little here (or somewhere in the method) to explain that field_test.module implements access control for this field name. 

  5. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,75 @@
+    $this->drupalGet('node/1');

    Minor, but we should probably use $node->nid here rather than hardcoding 1. 

  6. +++ b/core/modules/field/tests/modules/field_test/field_test.field.incundefined
@@ -188,5 +188,10 @@ function field_test_field_access($op, $field, $entity_type, $entity, $account) {
+  if ($field['field_name'] == 'test_view_field' && $op == 'view' && !user_access('view test_view_field content')) {
+    return FALSE;
+  }

    I'd add an inline comment here indicating what test this permission is used in, and how. Edit: Or really just what it does, "Only grant view access to test_view_field fields when the user has 'view test_view_field content' permission."

swentel’s picture

swentel commented
StatusFileSize
new interdiff.txt2.83 KB
new 1814418-9.patch4.89 KB

New patch
- use drupalCreateNode() so we don't need the bypass node access permission
- more comments

swentel’s picture

swentel commented
StatusFileSize
new 1814418-10.patch4.89 KB

Seriously, 'This is the some text', sigh, changed to 'This is some text'.

Stalski’s picture

Stalski commented
Status: Needs review » Reviewed & tested by the community

Look good and ready to go

xjm’s picture

xjm
she/her
Primary language English
commented

+1, looks great.

xjm’s picture

xjm
she/her
Primary language English
commented 
+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,82 @@
+class FieldAccessTest extends FieldTestBase {

Oh, the class still needs a docblock.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAccessTest.phpundefined
@@ -0,0 +1,82 @@
+    // specificaly target the 'test_view_field' field.

Oops, "specifically" mispelled here. :)

swentel’s picture

swentel commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff.txt896 bytes
new 1814418-14.patch4.95 KB

Here goes.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Yep!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice catch, and nice tests! Reminded me a little of #139921: Add "View any %nodetype% content" and "View own %nodetype% content" permissions. Oh wouldn't it be lovely if someone could implement that lovely feature before feature freeze..? ;) ;)

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

.