Missing access control for user base fields

Thanks! I'm re-title the issue a bit to be consistent with the other.

So this issue is for implementing default access for all base fields of a user. We need to go through them and look for existing access restrictions and make sure they are covered as part of the default Access of their field's item classes.

Example: We've got a permission for editing user names, that needs to be covered.

What to do: For each field that has a default access being not TRUE, we
- have to add a custom FieldItem class extending the field type's one
- implement defaultAccess() for it
- write a unit test case to prove it works as it should

Note: The list of fields we have to care have doesn't seem to be complete (I've not checked either), at least user name is missing.

First step, just to show where this is going, get comments, and see if it breaks something: implement the default access for the user name base field.

Now with unit tests.

The same test passes locally with the rerolled patch over the most recent commits. Manually performing the test also works as it should, so retrying with rerolled patch.

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+ * @file
+ * Definition of Drupal\user\FieldType\NameField.

Should be Contains and have a new line before the docblock

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+ * Represents a user name entity field items list. Unlike basic strings, its
+ * access rules depend on the underlying user entity.

Summary should not exceed one line, i.e. a line break and a new line after the first sentence.

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+ * The entity type is not checked because this base field is only defined on
+ * User entities.

I don'T think we need to document this here as that's the case for every field item list.

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+      $ret = $account->hasPermission('access user profiles');

Should just return TRUE as discussed.

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+      $ret = ($account->id() === $this->getEntity()->id())
+          && $account->hasPermission('change own username');

Either use $return as variable name or just return it directly.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserNameAccessTest.php
@@ -0,0 +1,80 @@
+      'create' => FALSE,
+      'update' => FALSE,
+      'delete' => FALSE,
+      'view' => TRUE,

HAving all those ops here does not make sense. Let's revisit this once we've figured $ops out at #1994140: Unify entity field access and Field API access.

+++ b/core/modules/user/lib/Drupal/user/FieldType/UserNameFieldItemList.php
@@ -0,0 +1,34 @@
+ * Represents a user name entity field items list. Unlike basic strings, its
+ * access rules depend on the underlying user entity.

I'd say the user name field has custom access rules.

Rerolled accordingly. Adding another one while the bot tests.

Just noticed a typo in the file-level phpdoc. Will include in the next version of the patch.

New version. This refactors logic and provides access for most base fields:

• name
• langcode
• preferred_langcode
• preferred_admin_langcode
• signature
• signatures_format
• theme
• timezone
• created
• access
• login
• init

There are probably policy decisions to be made regarding them, though, because for most of those we didn't have clear access policies regarding them until now.

Remaining to implement:

• uid
• uuid
• pass
• mail
• status
• roles

It also makes the $account parameter on FieldItemList::defaultAccess() non-optional, as seen with yched and fago.

Updated issue summary.

11: 2029855-default_user_field_access-11.patch queued for re-testing.

Just a straight re-roll, there were a few conflicts so see what the bot says.

FieldItemList had moved.

FieldItemListInterface had also changed. This time I tested the installer so hopefully we can get past that.

Just docs gate and small nitpicks

1. +++ b/core/modules/user/lib/Drupal/user/FieldType/UserFieldItemListBase.php
   @@ -0,0 +1,42 @@
   +   * @param AccountInterface $account
   ...
   +   * @param AccountInterface $account
   

   should be full namespace

2. +++ b/core/modules/user/lib/Drupal/user/FieldType/UserFieldItemListBase.php
   @@ -0,0 +1,42 @@
   +   * @return bool
   ...
   +   * @return bool
   

   needs description of return walue

3. +++ b/core/modules/user/lib/Drupal/user/FieldType/UserFieldItemListBase.php
   @@ -0,0 +1,42 @@
   \ No newline at end of file
   

   needs fix

4. +++ b/core/modules/user/lib/Drupal/user/FieldType/UserRestrictedFieldItemList.php
   @@ -0,0 +1,34 @@
   +  ¶
   
   +++ b/core/modules/user/lib/Drupal/user/FieldType/UserUnrestrictedFieldItemList.php
   @@ -0,0 +1,33 @@
   +  ¶
   

   trailing space

5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserNameAccessTest.php
   @@ -0,0 +1,85 @@
   +  function testUserNameAccess() {
   ...
   +  }
   +}
   

   needs empty line

Doc stuff from #19 fixed plus a few extras.

21: 2029855_20.patch queued for re-testing.

klausi opened a new pull request for this issue.

New approach: use a checkFieldAccess() method on the access controller same as for entity level access which uses checkAccess(). That way we can easily specify reasonable field access defaults without hacking/overriding all the field item classes. And you have field access in one place.

Unassigning myself: not working on this at the moment.

1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserFieldAccessTest.php
   @@ -0,0 +1,230 @@
   +  public function testUserNameAccess() {
   

   While much faster, DUBT and especially EntityUnitTestBase still isn't as fast as a unit test, wondering if it's worth to use a single test method + doSomething() methods pattern here, but running the test took 3 seconds for me, so not worth it.

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserFieldAccessTest.php
   @@ -0,0 +1,230 @@
   +        'target' => 'viewer',
   ...
   +      'signature',
   

   This seems very indirect, why not just pass the thing that should be access-checked into the helper method? If you want something to display for the message then you could use the username/label() for that?

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Combined the test methods in one for performance, interdiff: https://github.com/klausi/drupal/commit/b96605cdd2e2b3834ec28170d58e1a7f...

I don't want to change the invocation of the assertion because I think it is easier to read and understand in the current form, where looping and actual user accounts get out of the way as much as possible.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Now completely converted the test case to phpunit, removing one more frankenstein drupal unit test we should not use anymore.

dawehner said that we might want to group the field access method by operation, but I'm not sure about that. It would make the implementation even longer.

Yeah, I think the method is ok as it is. It's not nice looking, but the best I can think of.

1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
   @@ -312,4 +318,27 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
   +   * @param \Drupal\Core\Session\AccountInterface $account
   +   *   The user session for which to check access, or NULL to check access for
   +   *   the current user. Defaults to NULL.
   

   Should start with (optional)

2. +++ b/core/modules/user/lib/Drupal/user/UserAccessController.php
   @@ -60,4 +62,60 @@ protected function viewAccess(EntityInterface $entity, $langcode, AccountInterfa
   +        return $operation == 'edit';
   ...
   +        if ($operation == 'view') {
   +          return TRUE;
   +        }
   +        return FALSE;
   

   Should be consistent, i.e. always use the direct return of the comparison result.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Actually $account is not optional in that case, so I fixed the comment. Also fixed the comparison return logic for the created date. Interdiff: https://github.com/klausi/drupal/commit/1558228422dd2051dd912acece2284a5...

Patch looks decent now, only found the following:

1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
   @@ -312,4 +318,26 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
   +   * Default field access as determined by this access controller.
   

   Maybe we should add a paragraph documenting when this is taken into account and when not.

2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessController.php
   @@ -312,4 +318,26 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
   +   *   is checked for the field definition, without any specific value
   

   I do not think the comma here is needed.

Besides that, this needs a review of whether the access implementation for user is correct. (I've not reviewed this part yet).

1. +++ b/core/modules/user/lib/Drupal/user/UserAccessController.php
   @@ -60,4 +62,58 @@ protected function viewAccess(EntityInterface $entity, $langcode, AccountInterfa
   +    // Administrative users are allowed to edit and view all fields.
   +    if ($account->hasPermission('administer users')) {
   +      return TRUE;
   +    }
   

   Changing roles needs "administer permissions", check AccountFormController, so we should check that first.

   I'm also not sure if editing some fields like init/access/login should be prevent, as you wouldn't want that to be allowed over rest?

2. +++ b/core/modules/user/lib/Drupal/user/UserAccessController.php
   @@ -60,4 +62,58 @@ protected function viewAccess(EntityInterface $entity, $langcode, AccountInterfa
   +      case 'mail':
   +        // Allow view access to own mail address and other personalization
   +        // settings.
   +        if ($operation == 'view') {
   +          return $is_own_account;
   +        }
   +        // Anyone that can edit the user can also edit this field.
   +        return TRUE;
   

   We discussed this, and while kind of correct as we rely on entity level edit access, this still reads very weird. Not sure what to do about it.

Taking a look.

I tried to understand the comments in #38 and #39, but instead just did a simple re-roll of #36 including the PSR4 changes.

That re-roll was missing UserAccessController.php

Here goes again.

As mentioned in the call, this is not specific to rest.module, that's just one usage of this API. Another are field widgets/formatters, and the patches that are converting the custom form code there to widgets need to keep the custom access control right now, see patches and discussion in #2227381: Apply formatters and widgets to User base fields 'name' and 'email' as an example.

Those hacks only work in the normal form, if you add quickedit.module which directly edits a field using its widget, it will not get the correct access and would allow you to edit something that you aren't allowed to do, like being able to edit the username if you don't have the permisison to change the username.

+++ b/core/modules/user/src/UserAccessController.php
@@ -60,4 +62,58 @@ protected function viewAccess(EntityInterface $entity, $langcode, AccountInterfa
+    // Administrative users are allowed to edit and view all fields.
+    if ($account->hasPermission('administer users')) {
+      return TRUE;
+    }
...
+      case 'pass':
+        // Allow editing the password, but not viewing it.
+        return $operation == 'edit';

Which also means administrators can view the 'password' field. Is that wanted?

Yeah, another issue :) Agreed that should be limited as well.

The tests pass but that's only because they were written to match the implementation.

I am attaching a patch to fix the problem in #47. I understand we should update the test cases as well. Let's see how this patch goes.

Please provide an interdiff when making changes to a patch, it's very hard to see what exactly you changed otherwise.

We need to to something similar to roles and there's also some other possible cleanup necessary, see #38 and #39.

I created an interdiff from the last 2 patches.

I'm a bit lost at how to get this correct and verified, probably picked the hardest example to get the API in.

Therefore, I updated the patch in #2098355: Missing default access for all node fields so that we can get the API in an issue where the implementation is not as complex. Please help to review that.

See #2028027: [META] Missing access control for base fields, while this is still a critical security bug, it is IMHO no longer a beta target as we have the API in place, we just need to implement it.

Attempting to reroll. I will review all the comments after this patch goes.

It seems changes in EntityAccessControlHandler.php are no longer required (as hinted in #53). This is basically still from the reroll (as the reroll resulted in PHP errors). The interdiff wouldn't really give a clear picture here. The patch now contains changes only in user module.

Trying again. Moving test file in correct location and changing the namespace to match.

This needs to return AccessResult objects now, see how NodeAccessControlHandler works now.

I can at least do a reroll but I will try to address #62 as well.

Let's try this.

Reroll, revised patch, and interdiff attached.

Thanks!

+++ b/core/modules/user/src/UserAccessControlHandler.php
@@ -60,4 +62,63 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+        // Allow editing the password, but not viewing it.
+        return AccessResult::allowedIf($operation == 'edit');
+
+      case 'created':
+        // Allow viewing the created date, but not editing it.
+        return AccessResult::allowedIf($operation == 'view');

This unfortunately doesn't work like this.

Field access is allowed by default and allowedIf() is a allowedIfOrIgnore(), and not a allowedIfOrForbidden(), so this will never deny access.

You need to do $condition ? ::allowed() : ::forbidden() here.

Ah, got it. I can take care of that.

This should be better. I'm not sure if I'm doing things correctly for the caching.

This also would suggest that we're missing test coverage, right?

Looked at this briefly. There was a problem with the test data providers not returning the full set of test data. I still have to test this locally, but have to attend a meeting now.

Leaving this assigned to me for the moment, will have another look tomorrow.

There was indeed missing coverage: the case of an owner viewing or editing its own email address was not covered, causing the bug that was fixed in #67 to be undetected. I did a code coverage analysis and this indicated that the default case was also not covered.

Added all missing test coverage and put some more documentation since I was bewildered to see a test of an anonymous user not being able to view a field on a user but seemingly being able to edit it. I'm not sure if this comment is factually correct, so it would be nice if someone can confirm this:

      $access_info[] = array(
        'field' => $field,
        'viewer' => 'viewer',
        'target' => 'owner',
        'view' => FALSE,
        // Anyone with edit access to the user can also edit these fields. In
        // reality edit access will already be checked on entity level and the
        // user without view access will typically not be able to edit.
        'edit' => TRUE,
      );

That fail there was HEAD failing.

Committed 59788e3 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
index f77cfed..4548e48 100644
--- a/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
+++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
@@ -44,7 +44,7 @@ class UserAccessControlHandlerTest extends UnitTestCase {
   protected $owner;
 
   /**
-   * The mock adminstrative test user.
+   * The mock administrative test user.
    *
    * @var \Drupal\Core\Session\AccountInterface
    */
@@ -310,7 +310,7 @@ public function adminFieldAccessProvider() {
   }
 
   /**
-   * Tests that paswords cannot be viewed, just edited.
+   * Tests that passwords cannot be viewed, just edited.
    *
    * @dataProvider passwordAccessProvider
    */

Fixed spelling mistakes on commit.