Show label of inaccessible entities ('view' access denied) when 'view label' access is allowed

This is definitely a good thing to add, particularly so that the module can respond with Users' names if the rest of the object is inaccessible.

I've been running into issues related to the fact that GETing the Anonymous user always returns access denied, so I decided to see what a solution might look like for falling back to "view label" when accessing Users. This approach is not as elegant as I'd like, but it seems to work well.

Making it generic to all Entities might be possible, though there's one piece to that puzzle I haven't figured out yet: how to determine which field is the Entity's "label". It's easy to get the label value itself in a generic way, but then where do you put it in the response without knowing that?

Retesting...

Thanks @hampercm!

1. +++ b/src/Controller/EntityResource.php
   @@ -118,8 +119,17 @@ class EntityResource {
   +    if (!$entity_access->isAllowed() && $entity instanceof User) {
   ...
   +      $entity = User::create([
   
   @@ -724,6 +734,16 @@ class EntityResource {
   +    if (!$access->isAllowed() && $entity instanceof User) {
   

   All the User special case wrangling is a bit awkward. We should discuss if this principle should apply to all entities.

   Also, how is the error handling affected when we can display the label but not the entity?

2. +++ b/src/Normalizer/Value/EntityNormalizerValue.php
   @@ -68,6 +69,12 @@ class EntityNormalizerValue implements ValueExtractorInterface, RefinableCacheab
   +      $this->values = array_filter($this->values, function($value, $key) {
   +        return ($key == 'name' || $key == 'uuid');
   +      }, ARRAY_FILTER_USE_BOTH);
   

   Damn! I did not know about this ARRAY_FILTER_USE_BOTH flag.

3. +++ b/src/Normalizer/Value/EntityNormalizerValue.php
   @@ -68,6 +69,12 @@ class EntityNormalizerValue implements ValueExtractorInterface, RefinableCacheab
   +    // Remove extra User fields when access was limited to 'view label'.
   +    if ($entity instanceof User && empty($entity->mail->value)) {
   

   Maybe we should have a LabelOnlyEntityNormalizer class that handles the normalization in that scenario?

Thanks for the review! Responses to #9:

1) Yes, I'd prefer a general solution, I'm just missing one piece of the puzzle, as noted in #5. I guess in some cases the label doesn't even correspond to an actual field, as it can be generated by a callback. Should we just put the label in the attributes section as "label" in that case? If so, would it be a good idea to return the "label" in our responses for all entities, even those the user has full view access to?

2) :-)

3) That approach might be a bit cleaner. We just need to be careful there's no information disclosure along the way.

I'm still on the fence on this. I don't like the view label pattern, even if it's already used somewhere else.

Quoting @hampercm in #3

This is definitely a good thing to add, particularly so that the module can respond with Users' names if the rest of the object is inaccessible.

I disagree that we should take the liberty to disclose parts of an entity (the label) when the entity says Access Denied for the view operation.

If you want an entity to have a public label for everyone, you should make the entity accessible to everyone and the deny access based on permissions for all the fields that are not the label.

I don't feel too strongly about this. I may be convinced the other way, so I'm open for discussion.

The label will only be shown for entities that have 'view label' access granted to the user; this is a completely separate access operation. Most entities do not have 'view label' allowed by default, User entities being the main exception, since often times full access to user information is not granted to non-admin users. Following this pattern will be completely consistent with how Drupal core functions.

More specifically, the entity representation of the "Anonymous" user is NEVER accessible by 'view' access, even to user/1; only 'view label' is ever permitted. This makes handling entities that refer to the Anonymous user difficult without the 'view label' functionality.

Working on an improved implementation of this...

FYIY I'm still not sold on this idea. I feel a site owner can achieve this without requiring any extra code in the jsonapi module.

Damn! I did not know about this ARRAY_FILTER_USE_BOTH flag.

:O me neither!

If you want an entity to have a public label for everyone, you should make the entity accessible to everyone and the deny access based on permissions for all the fields that are not the label.

Philosophically, I have to say I'm with @e0ipso here. However, I think that ship has sailed. We likely do need to support this. I've personally worked around this issue for client work with custom controllers on several occasions.

Maybe we should have a LabelOnlyEntityNormalizer class that handles the normalization in that scenario?

A new normalizer for this seems excessive... but I don't have an elegant alternative. I'll try to think more about this.

A few open thoughts and questions:

1. The label needs to be under the attributes key. If so, we need to give it its own key (I assume this will be "label"). Will this appear only when the 'view label' mode is invoked or always?
2. If the answer to the above is "always", I think Drupal\Core\Entity\ContentEntityBase ought to have a computed label field. This would make our implementation much easier.
3. If it is "always" and it is not made a computed field, then we need to worry about the "label" key conflicting with other field names...
4. I'm not sure if we already support computed fields for sparse fieldsets, but if not, this would be a great use case for it. I imagine that it will often be the case that the label is the only thing needed.

However, I think that ship has sailed. We likely do need to support this.

Please elaborate.

If I provide custom module that removes view access for all entities of a type, then having a module (core or not) allowing access via a view $propertyName access check seems like as security hole for information disclosure. I don't like navigating these waters unless we absolutely need to. I don't think this is the case.

If I provide custom module that removes view access for all entities of a type, then having a module (core or not) allowing access via a view $propertyName access check seems like as security hole for information disclosure.

The reason I say that I think the ship has sailed is because the permissions have already landed in core and they established the precedent for how the operations should be interpreted.

The existence of `view label` implies that `view` should be interpreted as `view all`. We should also remember that Drupal permissions have always been grants, not rules. That is, if a user does not have the `view` permission, we don't say that the viewing the entity is forbidden, we say that the result is neutral. Thus, if another system (node grants, hook_entity_access, etc) does grant permission, it's not an information disclosure because access was never "forbidden" in the first place.

You both bring solid arguments. Count me convinced.

What should we do next?

For next steps, we will need to answer some of these when we come back around to this.

A few open thoughts and questions:

1. The label needs to be under the attributes key. If so, we need to give it its own key (I assume this will be "label"). Will this appear only when the 'view label' mode is invoked or always?
2. If the answer to the above is "always", I think Drupal\Core\Entity\ContentEntityBase ought to have a computed label field. This would make our implementation much easier.
3. If it is "always" and it is not made a computed field, then we need to worry about the "label" key conflicting with other field names...
4. I'm not sure if we already support computed fields for sparse fieldsets, but if not, this would be a great use case for it. I imagine that it will often be the case that the label is the only thing needed.

I'm not sure if we already support computed fields for sparse fieldsets, but if not, this would be a great use case for it. I imagine that it will often be the case that the label is the only thing needed.

We totally do.

The label needs to be under the attributes key. If so, we need to give it its own key (I assume this will be "label"). Will this appear only when the 'view label' mode is invoked or always?

All entities that have this access type should either have:

1. A label key.



2. A label callback.

If there is a label key, then we're already providing the label under a known field. That answers all your questions.

If there is a label callback, chances are that we're already providing it as well. In fact you can see that label_callback is being phased out #2450793: Properly deprecate support for entity type label callbacks. Which means that we can avoid the issue there and ship with special behavior for User, since it's the only core entity that uses it.

Monumental effort! Good job. This is going to make many people very happy :)

Overall, this looks really good too.

One meta question: why is this 2.x?

1. +++ b/src/Controller/EntityResource.php
   @@ -974,9 +978,13 @@ class EntityResource {
          $output['entity'] = new EntityAccessDeniedHttpException($entity, $access, '/data', 'The current user is not allowed to GET the selected resource.');
   +      $label_access = $entity->access('view label', NULL, TRUE);
   +      if ($label_access->isAllowed()) {
   +        $output['entity'] = new LabelOnlyEntity($entity);
   +      }
   

   Ubernit: can we make this an if/else or ternary to avoid reassigning the value?

2. +++ b/src/Normalizer/EntityNormalizer.php
   @@ -187,6 +187,21 @@ class EntityNormalizer extends NormalizerBase implements DenormalizerInterface {
   +      // @todo Fix Entity/Field API and User module in Drupal core so that despite
   +      // there being a label callback, we can also still figure out that "name" is
   +      // the 'label' entity key.
   

   Is there already an issue for this? If so, can we add a URL?

3. +++ b/src/Normalizer/LabelOnlyEntityNormalizer.php
   @@ -0,0 +1,98 @@
   +    // If the fields to use were specified, only output those field values.
   +    $context['resource_type'] = $this->resourceTypeRepository->get(
   

   This comment no longer makes sense.

4. +++ b/src/Normalizer/LabelOnlyEntityNormalizer.php
   @@ -0,0 +1,98 @@
   +    // Determine the (internal) label field name.
   +    $label_field_name = $entity->getEntityType()->getKey('label');
   ...
   +    if ($entity->getEntityTypeId() === 'user') {
   +      $label_field_name = 'name';
   +    }
   

   Let's move this logic to LabelOnlyEntity and expose it as a method.

5. +++ b/src/Normalizer/LabelOnlyEntityNormalizer.php
   @@ -0,0 +1,98 @@
   +    // @todo Fix Entity/Field API and User module in Drupal core so that despite
   +    // there being a label callback, we can also still figure out that "name" is
   +    // the 'label' entity key.
   

   Same as above.

6. +++ b/src/Normalizer/LabelOnlyEntityNormalizer.php
   @@ -0,0 +1,98 @@
   +    // Reconstruct an EntityNormalizerValue object, this time with only the
   +    // label field.
   +    $label_only_values = [$public_field_label_name => $all_values[$public_field_label_name]];
   

   This is dangerous territory. Looks good though.

7. +++ b/src/Resource/EntityCollection.php
   @@ -44,6 +45,7 @@ class EntityCollection implements \IteratorAggregate, \Countable {
        assert(Inspector::assertAll(function ($entity) {
          return $entity === NULL
            || $entity instanceof EntityInterface
   +        || $entity instanceof LabelOnlyEntity
            || $entity instanceof EntityAccessDeniedHttpException;
        }, $entities));
   

   This is starting to smell a little, not worth fixing here, but worth putting on the backs of our minds.

8. +++ b/tests/src/Functional/DateFormatTest.php
   @@ -29,6 +29,11 @@ class DateFormatTest extends ResourceTestBase {
   +  protected static $anonymousUsersCanViewLabels = TRUE;
   

   I like the explicitness here, over introspection 👍

9. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -156,6 +156,9 @@ trait ResourceResponseTestTrait {
              $target_access = static::entityAccess($target_entity, 'view', $this->account);
              if (!$target_access->isAllowed()) {
   +            $target_access = static::entityAccess($target_entity, 'view label', $this->account)->addCacheableDependency($target_access);
   

   Why reassign? ... Probably to reuse the code below WRT $access->getReason()?

10. +++ b/tests/src/Functional/UserTest.php
    @@ -414,7 +419,7 @@ class UserTest extends ResourceTestBase {
    -    $collection_url = Url::fromRoute('jsonapi.user--user.collection');
    +    $collection_url = Url::fromRoute('jsonapi.user--user.collection', [], ['query' => ['filter[status]' => 1]]);
    

    Why is the status necessary? Can we add a comment?

11. +++ b/tests/src/Functional/UserTest.php
    @@ -461,4 +466,21 @@ class UserTest extends ResourceTestBase {
    +    $this->assertSame(User::load(0)->uuid(), $doc['data'][0]['id']);
    +    $this->assertSame('Anonymous', $doc['data'][0]['attributes']['name']);
    

    These two lines assert the whole feature request! :)

12. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -262,19 +262,20 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
    +    $this->assertCount(1, $normalized['included'][0]['attributes']);
    

    Good assertion.

👏 We're now able to get the anonymous user.

I agree!