Anonymous user label can't be viewed and auth user labels are only accessible with 'access user profiles' permission

The same problem with me. Someone have any idea?

This look good for me.

Yes, I had not seen it. Thanks.

I'm not really happy with the hardcoded fix...
It will cause a lot of issues where other (contrib) modules will run into the same issue...

Yeah, as discussed in IRC, I think we need to move that method into the selection handler plugins, so that user can override it.

Adding a new method on the selection handler is just an API addition so I think it's pretty safe to do in the beta cycle :)

Hm, LinkWidget::getUriAsDisplayableString() is tricky indeed. Although, in EntityAutocomplete::getEntityLabels() we can always instantiate a "minimal" selection plugin based only on the target type of the passed-in entities, it would probably be a bit too heavy on the performance side.

I'm ok with the 'view_label' solution if everyone else likes it too.

I'm in DrupalCon LA. I'm applying the patch :-)

I couldn't duplicate this issue (before applying the patch).

I followed the steps to reproduce.

Before patching:

After patching:

I also read the code and I've seen nothing strange. I checked that we still had tests for other restricted entities and we do.
Thanks for your work!

Let's try and get some entity system maintainer(s) to chime in here since it's not a very straightforward change.

Labels *can* give away a lot of sensitive information. Even if we don't show the label, matching user input and then returning a result will tell users that some entity with that label exists.

However, if that is no concern among the other reviewers, then here's a more technical review of mine:

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -316,6 +316,11 @@ public function uriRelationships() {
   +    // By default, use the same permission schema for viewing a label as
   +    // viewing an entity.
   +    if ($operation == 'view label') {
   +      $operation = 'view';
   +    }
   

   This MUST be done in entity types' access handlers.

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -82,6 +83,18 @@ public function isNew() {
   +  public function access($operation, \Drupal\Core\Session\AccountInterface $account = NULL, $return_as_object = FALSE) {
   +    // We don't treat the label as priviledged information.
   +    if ($operation == 'view label') {
   +      $result = AccessResult::allowed();
   +      return $return_as_object ? $result : $result->isAllowed();
   +    }
   +    return parent::access($operation, $account, $return_as_object);
   +  }
   

   This MUST be done in the user entity type's access control handler.

3. +++ b/core/modules/user/src/Entity/User.php
   @@ -82,6 +83,18 @@ public function isNew() {
   +  public function access($operation, \Drupal\Core\Session\AccountInterface $account = NULL, $return_as_object = FALSE) {
   

   Use a use statement instead of the FQN in a type hint.

This addresses the feedback I gave in #27.

The reason I rolled this patch myself, is that I wanted to show why the location of the original access code was wrong:
1) All access control logic MUST reside in entity access control handlers. EntityInterface::access() is merely a convenience wrapper.
2) By changing the operation in EntityAccessControlHandler we pretended we're not actually checking access for the view label operation anymore. This prevented anyone from responding to hook invocations for this operation, for instance. I moved this check to EntityAccessControlHandler::checkAccess(), where is simply returns the access for the view operation. This makes sure that all code can still respond to the view label access check and implement the hooks for this specific operation. The fact that we call $this->access() for the view operation, means the full stack of access control checks are executed for that operation as well.

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -217,7 +217,9 @@ public static function validateEntityAutocomplete(array &$element, FormStateInte
      public static function getEntityLabels(array $entities) {
   

   The doc block on this method needs to be fixed, still talks about the "view" access.

2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -133,6 +133,11 @@ protected function processAccessHookResults(array $access) {
   +    // By default, use "view" access for viewing the entity label.
   +    if ($operation == 'view label') {
   +      return $this->access($entity, 'view', $langcode, $account);
   +    }
   

   So you are inventing a new operation "view label" here? Then that needs to be documented as one of the usual operation strings at EntityAccessControlHandlerInterface.

3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -97,6 +97,9 @@ function testURLValidation() {
   +    // Get the user 1 account.
   +    $admin_account = entity_load('user', 1);
   

   entity_load() is deprecated, let's not add more calls to the code base. Use User::load() instead.

So you are inventing a new operation "view label" here? Then that needs to be documented as one of the usual operation strings at EntityAccessControlHandlerInterface.

We shouldn't document implementation details on interfaces. If anything, this must be documented on EntityAccessControlHandler.

I wonder if this is not related to #2322525: Add separate SelectionHandler for entity_reference fields using user entity used in Author context.

29.1) Changed to view label.
30 added view label to EntityAccessControlHandler.
3. Updated to user load.

Some more small changes i noticed.

Added fix for the tests. Hope this is the right way.

Should address the last failing test.

+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -105,6 +105,9 @@ protected function checkAccess(EntityInterface $node, $operation, $langcode, Acc
 
+    if ($operation === 'view label' && $account->hasPermission('access content')) {
+      return AccessResult::allowed();
+    }

I don't think this is correct, if this doesn't have any test fails, then we are missing test coverage. Unlike users, we always considered it to be a (security) bug if it is possible to view node titles of nodes you do not have access to, e.g. due to using the node grant system.

I'm not sure why we need to add special cases to any access control handlers except user with the default fallback that's in place now? Are they not calling the parent if they don't have something to say?

I can't say I really like the approach but I also don't have a better idea. But we need to document this view label thing somewhere. Possibly on EntityAccessControlHandlerInterface.

I think we're also missing tests.

Note: This problem doesn't just affect widgets, it also affects formatters. If you are using user reference fields, then the default entity reference formatters won't show the reference at all if you don't have the access user profiles permission. The only one that works is "Author" because that doesn't do access checking but that doesn't allow to configure the output (e.g. link or not link).

Document view label on EntityAccessControlHandlerInterface.
Added new var in EntityAccessControlHandler to use when we want label to be shown without access check.

Change now seems to be non-BC breaking which is good. I don't know if we want to add additional test coverage for things @Berdir mentioned in #41.

1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -138,9 +145,9 @@ function testURLValidation() {
        $invalid_external_entries = array(
   -      // Invalid protocol
   +      // Invalid protocol.
          'invalid://not-a-valid-protocol' => $validation_error_1,
   -      // Missing host name
   
   @@ -215,7 +222,7 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
      /**
       * Tests the link title settings of a link field.
       */
   -  function testLinkTitle() {
   +  protected function testLinkTitle() {
        $field_name = Unicode::strtolower($this->randomMachineName());
   

   You should not make unrelated code style cleanups unless they are on the very lines you are already changing anyway. This makes it harder to review and makes clashes with other issues more likely.

2. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -103,6 +103,15 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
   +    if ($operation == 'view label') {
   +      if ($this->viewLabelOperation == TRUE) {
   +        return AccessResult::allowed();
   +      }
   +      else {
   +        $operation = 'view';
   +      }
   +    }
   +
   

   This is not correct and requires that specific implementations have to do something.

   What that flag should do is automatically map $op view label to view unless viewLabelOperation is set to TRUE. No access control handler except the default class and user must be changed in this issue.

Tried to remove all unrelated changes and made changes accordion to #45.2

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -40,6 +40,13 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce
+   * Allow access to entity label.
+   *
+   * @var bool
+   */
+  protected $viewLabelOperation = FALSE;

This should say something like this:

Allows to grant access to just the labels.

By default, the "view label" operation falls back to view. Set this to TRUE to allow to return different access when just listing entity labels.

Other than that, looks clean to me now. As said, some tests would be great that test this a bit more explicitly.

1. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -81,6 +88,11 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
   +      if ($operation == 'view label' && $this->viewLabelOperation == FALSE) {
   +        $operation = 'view';
   +      }
   

   Shouldn't this be moved to the beginning of the method? With the current code, when the 'if' condition is TRUE and we change the operation to 'view', that's what will be stored in the static cache, and, on subsequent 'view label' checks, the static cache will not be used because we stored the info only for the 'view' operation.

2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -125,8 +129,8 @@ function testURLValidation() {
   -      // URI for an entity that exists, but is not accessible by the user.
   -      'entity:user/1' => '- Restricted access - (1)',
   +      // Account labels are not treated as confidential information.
   +      'entity:user/1' => $admin_account->label() . ' (1)',
   

   This defeats the purpose of the test, which was to check an entity with no 'view' or 'view label' access.

Here's a patch that adds dedicated test coverage and fixes 2. above. I didn't fix 1. as well because I might be missing something.. it would be good to get some confirmation on that.

Hm, let's restrict testing for the 'view label' operation to the entity_test_label entity type only.

The question in #48.1 still remains though :)

Applied the patch in comment #50 and verified the fix, but still 'Restricted Access' is displayed in Authored By field.Attached snapshot for reference.

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php
@@ -37,7 +45,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+    elseif (in_array($operation, array('view', 'view label'))) {

I'm a bit concerned about the naming, in that sense that it might be mis-used in contrib or custom code. Maybe be more explicit, and be something like 'view admin label' ?

Confirm as already stated in #51, applying patch #50 still doesn't solve the issue.

Re #52: Why admin label and why do you think it would be mis-used? Note that this doesn't just apply to the widget, it applies in exactly the same way when using a formatter too. There it is even worse as the user name is never displayed. The only reason this isn't visible out of the box is because node uses a special author formatter that doesn't check permissions.

@Berdir hmm yeah, didn't know about AuthorFormatter implementing checkAccess(). Also didn't know that viewing the label of users is considered ok (which I can live by though).

I'm confused by a change in the test though:

+++ b/core/modules/link/src/Tests/LinkFieldTest.php
@@ -126,7 +133,7 @@ function testURLValidation() {
-      'entity:user/1' => '- Restricted access - (1)',
+      'entity:entity_test/' . $entity_test_no_label_access->id() => '- Restricted access - (' . $entity_test_no_label_access->id() . ')',

Shouldn't the check for user stay and now actually show the label then ? Or am I missing this completely ? Maybe I should actually test out the patch :)

Also, maybe rename viewLabelOperation to viewLabelAccess ? Just a suggestion, it sounds better in my head for some reason, but then again, I'm not a native English speaker.

The reason that it doesn't work is because UserAccessControlHandler has an explicit check for the anonymous user. We need to move view label above that and check that first, not in the switch.

Shows that we're still missing test coverage for the user case.

Also didn't know that viewing the label of users is considered ok (which I can live by though).

That's what we always did. You can always view the username, but you can't always visit the user profile page. The view/view label access operation thing doesn't fully match that, but I guess it's as close as we can get. In a better world, the user profile would possibly be a separate entity type (profile.module) that you'd view, not the user entity itself. Or something like that I guess. But we have to deal with what we have.

I will move the label access check above the anonymous user check.

Also, I will improve test coverage.

I have no ideea why the test fails. It asserts that a regular user has no access to create/update/delete/view users, and it fails for each of theese assertions.

However, as far as I know, a regular user shouldn't have access to these operations for user objects.

You're falling into the uid 1 trap :)

The first user that you're creating gets the UID 1, because this is a kernel test where there is no user by default. So the first one that you create is uid 1 and can do everything.

Explicitly pass ['uid' => 2] to the first createUser() call that you set as the current user. Then the test passes for me.

Did you test manually with the node author widget? Wondering if we have any existing tests that we should be able to update to cover this, if not, I think we should also have an integration/UI test that makes sure editing a node with uid 0 as author is shown correctly.

I tested manually with the node author widget, and it works now. I know of no existing tests for this.

Also, I think this should be changed to 8.1.x. I wrote the patches based on 8.1.x and they apply cleanly on 8.0.x. I think the relevant files have no difference between 8.0.x and 8.1.x.

testbot runs against 8.0.x, so that's fine :)

The test code in NodeEditForm::checkVariousAuthoredByValues() would be very easy to extend to check for this. It sets the author to the anonymous user (in a way that shouldn't be supported IMHO but that's a different topic). And later on it goes back to the edit form. You just have to refactor that a bit to use drupalGet() to got the edit form and check the value in there.

Then also provide that as a test-only patch (possibly with the other new tests too but they are mostly specific to the new operation and won't really do anything without the new code anyway)

Here's a more specific test of the widget value, as suggested by #73.

Looks perfect to me but the real patch is 0 bytes :(

Now with more patch!

It would be great to get this into 8.0.3.. it's quite an embarrassing bug :/

Indeed.

do we need a short CR to explain the new operation/flag?

A short CR can't hurt :)

But I just remembered that #48.1 was not answered yet, I still think that the new code there should be moved to the top of the method. Any thoughts on that?

Wrote a (not so) short CR in the meantime: https://www.drupal.org/node/2661092

Missed #48.1: Yes, you're right, that should move up, agreed.

We need the CR first, as a draft. Doesn't need much, just explain the new operation/use case and how to customize it based on the user example.

Alright, here we go. The CR is already written, see #81 :P

Ok, RTBC if green. Thanks!

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -302,7 +302,9 @@ protected static function matchEntityByTitle(SelectionInterface $handler, $input
   +      // Use the special view label, since some entites allow the label to be
   

   entites.

2. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -22,11 +22,24 @@
   +    if ($operation === 'view label') {
   

   Why does this not allow access to authenticated user labels to users that can't view them? It looks like it does reviewing visually.

   I also can't see that tested explicitly - we should specifically assert that users that don't have access to view an authenticated user's account still are unable to see their username.

Talked to @catch about the second point.

We should rewrite the comment to clarify that this check is where it is so that labels for all users, including the special anon user can be viewed.

It's too focused on anon user right now, as it tries to explain why we moved it up.

Talked to Berdir in irc, I got confused because of the following:

- the issue title, description, and comments in the patch all talk about the label for the anonymous user only, but auth user labels are only accessible to users with 'access user profiles' permission at the moment.

- Also I was trying to think of a situation where a site might rely on this behaviour, but you'd only not see user labels anywhere if you disable 'submitted' from node types and don't use comments. If you do that, this patch doesn't change things for you anyway, so it shouldn't really be possible to rely on the buggy behaviour.

Updated the title but the comment could use updating too.

Fixed #87.1 and improved the comment from UserAccessControlHandler::checkAccess().

entites, classic typo :)

Looks good to me, back to @catch.

Needs a re-roll for 8.1.x

Rerolled for 8.1.x. The patch in #91 still applies cleanly for 8.0.x :)

Committed/pushed to 8.1.x, thanks!

The protected property added to the base class is something we'd normally avoid in a patch release, but given this is an actual bug it might be worth making the change, but leaving RTBC for 8.0.x a bit longer.

Thanks @catch! Re-uploading #91 for 8.0.x, just so it's clear what can be committed :)

How did this got into 8.1.x even without a secteam nod?

Because we've established that this is/was currently a buggy, broken behavior and fixing it is absolutely in line with how this is handled everywhere else and we can not imagine a scenario where this would make anything visible that wasn't already? It's a new operation that nothing except autocomplete calls.

also, "Needs security review" is a pretty good way to put an issue to sleep for.... a very long time.

FYI: this issue allows a very simple fix for #2634158: Referencing a block that has conditions leads to 'Unrestricted access' for label and there's also a small followup for this patch: #2692091: Use the new 'view label' entity access check in the entity reference label formatter

@chx I gave it an in-depth review from a security standpoint and discussed it at length with Berdir in irc, as is documented above. Also klausi reviewed this a while back. Who else did you have in mind to review it?

Per #101 and #103, I think we can move this back to RTBC.

8.0.x is now frozen for its final patch release, and this is already fixed in 8.1.x so will be coming out in two weeks! Hooray!

The change record for this issue was still a draft, pending the commit of this patch to 8.0x. Since that won't happen anymore per #105, I just published it: https://www.drupal.org/node/2661092