Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral'

Testing same patch against 8.3.x

The patch works fine for me on 8.2.x and 8.3.x. I would very much like to see it committed in 8.3.x as it's a very important fix for access control.

Thanks for proposing this patch.

To consider this change for inclusion in Drupal 8, we need to add some automated test coverage.

Also tagging for a subsystem maintainer review since this is potentially a risky change that might result in information disclosure where a site or module was previously relying on the access being forbidden when combined with some other access result. We'll need to consider it carefully.

Thanks!

Commented on wrong issue.

I've added a patch that includes the patch #7 and a test.
The test includes a view of users and some fields (name, status, mail, init, changed), and checks whether the fields (columns) are present. It includes a hook_entity_field_access() for the mentioned fields.
Before adding the patch, the view only shows the name and created columns, after the patch they're all visible.

I have included screenshots of the view and test results before/after patch (the view was imported for the screenshots).

View before patch:

View after patch:

Test results before patch:

Test results after patch:

Any Update on this issue ?

It needs a review done.

I have reviewed the patch #13 and found the below status :
5 passes, 10 fails, 0 exceptions, 2 debug messages. PFA screengrab for the tests.
Also, after applying the patch I can see two checkboxes for user in Testing .PFA screengrab.
Seems to be something wrong or help me if i did some mistake.
Thanks.

@AdamPS,
Below are the steps which shows the problem :
1) Install Drupal 8.4.x-dev
2) Enable the Testing Module
3) Apply the patch #13
4) Run the test UserFieldAccessChangeTest

Let me know if I have done something wrong.
Thanks.

1. --- a/core/modules/user/src/UserAccessControlHandler.php
   +++ b/core/modules/user/src/UserAccessControlHandler.php
   

   I agree that changing these from "forbidden" to "neutral" makes sense: it allows contrib/custom code to add new permissions for example, to still allow these things to become allowed.

   "forbidden" should only be used for cases where a hard "NOPE NOT ALLOWED" is necessary. For example, in \Drupal\Core\Access\CsrfAccessCheck::access().

2. +++ b/core/modules/user/tests/modules/user_access_test/user_access_test.module
   @@ -22,3 +25,20 @@ function user_access_test_user_access(User $entity, $operation, $account) {
   +  // Allow view access for status, init and mail fields.
   +  if ($field_definition->getName() == 'status' && $operation == 'view') {
   +    return AccessResult::allowed();
   +  }
   +  if ($field_definition->getName() == 'init' && $operation == 'view') {
   +    return AccessResult::allowed();
   +  }
   +  if ($field_definition->getName() == 'mail' && $operation == 'view') {
   +    return AccessResult::allowed();
   +  }
   

   I think it'd be better to show a realistic use case: allow access to these in case the user has a certain permission.

   Also: === instead of ==.

   And:

   if ($operation === 'view' &&
    in_array(…

   would be much more elegant.

NW for point 2, and for writing a change record.

• The Contributed project blocker is justified: this is blocking https://www.drupal.org/project/administerusersbyrole.

  (Although in theory, this is only a soft blocker, because it's possible to override \Drupal\user\UserAccessControlHandler completely, but that'd be far more dangerous of course.)

• I don't think this qualifies as a major bug per https://www.drupal.org/core/issue-priority#major-bug. I don't even think this is a bug. It's a task, because we're making a new capability possible. And it affects only a small fraction, so it's normal, not major.
• Title clarified.
• I'm not a maintainer of the User module, but I'm kind of a maintainer of the access result system. Still keeping the Needs subsystem maintainer review tag for now though.

Summarizing #22 + #23:

• Issue metadata updated.
• Marked "needs work" for missing change record and #22.2.

Once #22.2 is addressed, this can go back to RTBC.

there was recently a support question and we figured out that it is possible when using the alter hook, but that's complicated.

field access is a common problem because the default is allowed, unlike entity access, so you usually have to use forbidden in hook implementations and many of our helper methods don't work (there is no allowedIfPermission() counterpart, forbiddenIf doesn't work)

Your change record looked good! :) I made only minor changes. Well done!

#22.2 still needs to be addressed, so back to NW.

Thanks!

Here is an updated patch which implemented #22.2 and interdiff

Here is an updated patch which implemented #22.2 and interdiff.
Changing the status to Needs Review

+++ b/core/modules/user/tests/modules/user_access_test/user_access_test.module
@@ -22,3 +25,22 @@ function user_access_test_user_access(User $entity, $operation, $account) {
+  $access_fields = array('status','init','mail');
+  if ($operation === 'view' && in_array($field_definition_name, $access_fields)) {
+    return AccessResult::allowed();
+  }
+  if ($operation === 'view' && in_array($field_definition_name, $access_fields)) {
+    return AccessResult::allowed();
+  }
+  if ($operation === 'view' && in_array($field_definition_name, $access_fields)) {
+    return AccessResult::allowed();
+  }
+  return AccessResult::neutral();

should use short array syntax.

Since this was now changed to use an in_array() check, you now have 3x the same code. That means the second and third if are not needed and you can remove it.

In fact, you could even do a return AccessResult::allowedIf($operation == 'view' && in_array(...)) and shorten it all to a single line.

Here is an updated patch and interdiff.txt implements #31.

I think the feedback from #22.2 is not really being addressed in those changes, what Wim probably means is that a more natural usage of this checks is permission based, something like this:

if ($field_definition->getName() == 'status' &&  $account->hasPermission('view status field') && $operation == 'view') {
    return AccessResult::allowed();
}

Instead of

if ($field_definition->getName() == 'status' && $operation == 'view') {
    return AccessResult::allowed();
}

No, I meant

if ($operation === 'view' && in_array($field_definition->getName(), ['status', 'init', 'mail'])) {

The short array and coding style on the line above is not fixed yet, and there's a question of using local variables or not (so leaving needs work for that), but that's what we have now, except that it is an allowedIf()?

It seems allowedIf() is better to use because it creates an allowed or neutral access result. Here is the final code with allowedIf().
return AccessResult::allowedIf($operation === 'view' && in_array($field_definition->getName(), ['status', 'init', 'mail']));
Kindly review it so that I can create the patch for it.

Oh, so it is already implemented! Sorry, I missed that.

Re-reviewing…

1. +++ b/core/modules/user/src/Tests/Views/UserFieldsAccessChangeTest.php
   @@ -0,0 +1,58 @@
   +  public static $modules = ['views_ui', 'user_test_views', 'user_access_test'];
   

   Why is this installing views_ui?

2. +++ b/core/modules/user/src/Tests/Views/UserFieldsAccessChangeTest.php
   @@ -0,0 +1,58 @@
   +    // User has access to name and created date by default.
   +    $this->assertText(t('Name'));
   +    $this->assertText(t('Created'));
   +
   +    // Access for init, mail and status is added in hook_entity_field_access().
   +    $this->assertText(t('Init'));
   +    $this->assertText(t('Email'));
   +    $this->assertText(t('Status'));
   

   This should:

   1. test that init/email/status are NOT accessible
   2. install user_access_test
   3. test that init/email/status are now indeed accessible

Hi all, do I understand correctly that even with this patch, you still need to create your own module to override access permissions to the fields? So the example of #13 was including a separate module that implements hook_entity_field_access to allow access to those fields in the screenshots? Or is there a way with this patch to directly display the email address in a view without additional module development?

OK, I applied the patch and implemented hook_entity_field_access in a custom module that assumes field_permissions module is installed. It's a tiny custom module, and it works like a charm with patch #39 applied.

I reported this also to the field_permissions module so hopefully the functionality can be included there directly.

This is my module for anyone who runs into the same issue:

use \Drupal\Core\Field\FieldDefinitionInterface;
use \Drupal\Core\Field\FieldItemListInterface;
use \Drupal\Core\Session\AccountInterface;
use Drupal\Core\Access\AccessResult;

function user_mail_entity_field_access($operation, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL){
    if ($field_definition->getName() == 'mail' && $operation == 'view'){
        return AccessResult::allowedIfHasPermission($account, 'access private fields');
    }
    return AccessResult::neutral();
}    

+1 for RTBC

Good idea.. I'm using this patch already for a couple of weeks on a production site

still works and looks good :)

1. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -106,7 +106,7 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
              return AccessResult::allowed()->cachePerPermissions()->cachePerUser();
            }
            else {
   -          return AccessResult::forbidden();
   +          return AccessResult::neutral();
            }
   

   This if/else statement only exists because allowedIf() returns "allowed" or "neutral", but we needed "forbidden" to do exactly the same as what was there before.

   If we're changing this from "forbidden" to "neutral", we can remove the if/else.

2. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -116,7 +116,7 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   -          return $is_own_account ? AccessResult::allowed()->cachePerUser() : AccessResult::forbidden();
   +          return $is_own_account ? AccessResult::allowed()->cachePerUser() : AccessResult::neutral();
   

   Similarly here, can become:

   return AccessResult::allowedIf($is_own_account)->cachePerUser();
   

3. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -127,14 +127,14 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   -        return ($operation == 'view') ? AccessResult::allowed() : AccessResult::forbidden();
   +        return ($operation == 'view') ? AccessResult::allowed() : AccessResult::neutral();
   

   Same here:
   return AccessResult::allowedIf($operation == 'view');

+++ b/core/modules/user/src/UserAccessControlHandler.php
@@ -102,12 +102,7 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
         // satisfied.
-        if ($is_own_account && $account->hasPermission('change own username')) {
-          return AccessResult::allowed()->cachePerPermissions()->cachePerUser();
-        }
-        else {
-          return AccessResult::forbidden();
-        }
+        return AccessResult::allowedIf($is_own_account && $account->hasPermission('change own username'))->cachePerPermissions()->cachePerUser();
 

It's not about being cached or not, at least not the first test fail but by what it is cached.

Before, it was only cached by user if you had access. Now always.

The new behavior is actually correct, but it's the same problem as elsewhere.. per-user/owner checks are practically uncacheable. But that's not really related to neutral/forbidden, so I guess we could move that to a separate issue.

Here's an interesting thing, though. I was actually confused why this worked, because as I mentioned before, field access is different to entity access because it defaults to allowed. That means to deny access in hook_entity_field_access(), you *must* use forbidden, neutral doesn't work. But as the test added here shows, it does indeed work in the checkFieldAccess() method. The reason for that is that unlike access(), fieldAccess() does this:

> $default = $default->andIf($entity_default);

Any other check is done with an orIf(), meaning the logic cited in the issue summary is TRUE. However, andIf() is different, it means both access checks must explicitly be allowed, not neutral. That's why this works. Considering how field access works, I think we should have done the same with the field access hooks, but then we would also need to implement and document them accordingly (that they must return allowed by default, not neutral). So this is not a change we can make in a BC-compatible way, ever. I also still think that we should allow empty responses instead of neutral and filter them out. Field access checks are a considerably performance issue and because it is a global hook, any hook implementation for any entity type and field always returns a result that needs to be merged together.

Fine with me, although I don't really agree with it being a blocker. as commented already in #25, there is an alter hook, using that is a bit more complicated, but it definitely allows to do whatever you want, including replacing the default access check completely.

I tested patch user-access.2773645-74.patch from #75 on Drupal 8.5.1 and confirm that it works.

How can a 1.38 KB patch without tests be RTBC if we previously had a 18 KB patch that included test coverage (in #62)?

Oh …

I guess I could load up #75 again as #83 if that would make it clearer.

Yes, please do! Thanks :)

I'll re-RTBC once you've done that.

This is asking for a subsystem maintainer review, but while Berdir isn't listed, I think he counts enough. Moshe to my knowledge hasn't worked on user module for a while.

#65 is an unfortunate discrepancy, we should consider a 9.x issue to see if it's something we can change in a way that while it breaks bc, allows modules to account for both the old and new behaviour maybe?

Committed e7f061d and pushed to 8.6.x. Thanks!