Spec compliance: `related` routes should return 200, not 403, if field access is allowed but the related resources are forbidden

When requesting a related route like: /jsonapi/node/article/{uuid}/field_related, access should be dependent on the parent entity and field access result. Not any of the access results for the related entities.

IOW, the related route needs to be treated just like a collection... since it's just a collection that is filtered using "referenced by" semantics.

I don't fully grok this.

Can you explain this using a concrete example?

Thanks, that helps a lot 👌

This bit from the IS now makes perfect sense, and is a perfect summary:

IOW, the related route needs to be treated just like a collection... since it's just a collection that is filtered using "referenced by" semantics.

Thanks, and please proceed!

+++ b/src/LabelOnlyEntity.php
@@ -19,6 +24,7 @@ class LabelOnlyEntity {
+    $this->setCacheability(CacheableMetadata::createFromObject($entity));

This can just be $this->setCacheability($entity);

#28:

Thanks especially for points 4, 5 and 6!

Point 11 sounds like an absolute nightmare to track down! Well done! 👏

This patch is mostly just moving existing code around. Logic in EntityResource::getCollection() and EntityResource::respondWithCollection() is being moved around to allow for code reuse by EntityResource::getRelated().

Makes sense.

1. +++ b/src/Controller/EntityResource.php
   @@ -212,9 +213,9 @@ class EntityResource {
   +      // by the user. Field access makes no difference between 'create' and
   

   s/difference/distinction/ if we're changing these lines anyway.

2. +++ b/src/Controller/EntityResource.php
   @@ -423,46 +444,35 @@ class EntityResource {
   -    $referenced_entities = array_filter(
   -      $field_list->referencedEntities(),
   ...
   +    $target_entity_ids = array_map(function (EntityInterface $target_entity) {
   +      return $target_entity->id();
   +    }, $field_list->filterEmptyItems()->referencedEntities());
   +
   +    $query_cacheability = new CacheableMetadata();
   +    if (!empty($target_entity_ids)) {
   +      $query = $this->getCollectionQuery($entity_type_id, $params);
   +      $query->condition($this->entityTypeManager->getDefinition($entity_type_id)->getKey('id'), $target_entity_ids, 'IN');
   

   We went from using ::referencedEntities() to still using that, but using it to perform a query.

   We didn't do that query before.

   I'm not sure how to feel about that. Can you explain your rationale?

3. +++ b/src/Exception/EntityAccessDeniedHttpException.php
   @@ -13,8 +16,9 @@ use Symfony\Component\HttpKernel\Exception\HttpException;
   -class EntityAccessDeniedHttpException extends HttpException {
   +class EntityAccessDeniedHttpException extends HttpException implements CacheableDependencyInterface {
   

   Then let's extends \Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException instead of extends HttpException implements CacheableDependencyInterface. That's what #2929428: [>=8.5] Convert "throw new *HttpException" into "throw new Cacheable*HttpException" where possible is about, but this is then at least already one tiny step :)

4. +++ b/src/Exception/EntityAccessDeniedHttpException.php
   @@ -51,6 +55,7 @@ class EntityAccessDeniedHttpException extends HttpException {
   +    $this->setCacheability(CacheableMetadata::createFromObject($entity_access));
   

   Same remark as in #20.

5. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -21,6 +21,13 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +   * Whether this collection can have more than one resource.
   

   Let's document when/why this would not be the case, because "collection" pretty much implies "can contain multiple" for any reader.

6. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -40,15 +47,20 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +    assert($is_multiple || count($entities) <= 1);
   

   👍

7. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -118,4 +130,14 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +   * Gets the cardinality of this collection.
   ...
   +  public function isMultiple() {
   

   "cardinality" vs "multiple".

   I like "cardinality" better.

8. +++ b/src/LabelOnlyEntity.php
   @@ -9,7 +11,9 @@ use Drupal\Core\Entity\EntityInterface;
   -class LabelOnlyEntity {
   +class LabelOnlyEntity implements CacheableDependencyInterface {
   

   👍

9. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
   @@ -203,7 +203,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
   -    return new JsonApiDocumentTopLevelNormalizerValue($normalizer_values, $context, $link_context, $is_collection);
   +    return new JsonApiDocumentTopLevelNormalizerValue($normalizer_values, $context, $link_context, $is_collection && $data->isMultiple());
   

   This is confusing. We're telling this constructor that the values do not represent a collection if it's a single-cardinality collection.

   So really, at that point, EntityCollection is more like an EntityContainer?

10. +++ b/src/Normalizer/RelationshipItemNormalizer.php
    @@ -59,8 +59,8 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
    -      $entity_and_access = EntityResource::getEntityAndAccess($target_entity);
    ...
    +      $entity = EntityResource::getEntityAndAccess($target_entity);
    

    It's confusing that the variable name was changed, but the method name remains the same.

11. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
    @@ -500,4 +475,27 @@ trait ResourceResponseTestTrait {
    +  /**
    +   * Gets a generic empty collection response.
    +   *
    +   * @return \Drupal\jsonapi\ResourceResponse
    +   *   The empty collection ResourceResponse.
    +   */
    +  protected static function getEmptyCollectionResponse($is_multiple, $self_link) {
    

    Nit: incomplete docblock.

12. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
    @@ -238,7 +238,11 @@ class EntityResourceTest extends JsonapiKernelTestBase {
    -    $this->assertEquals(['config:node_type_list'], $response->getCacheableMetadata()->getCacheTags());
    +    $expected_cache_tags = [
    +      'config:node.type.article',
    +      'config:node_type_list',
    +    ];
    +    $this->assertSame($expected_cache_tags, $response->getCacheableMetadata()->getCacheTags());
    
    @@ -275,7 +279,12 @@ class EntityResourceTest extends JsonapiKernelTestBase {
    -    $this->assertEquals(['config:node_type_list'], $response->getCacheableMetadata()->getCacheTags());
    +    $expected_cache_tags = [
    +      'config:node.type.article',
    +      'config:node.type.lorem',
    +      'config:node_type_list',
    +    ];
    +    $this->assertSame($expected_cache_tags, $response->getCacheableMetadata()->getCacheTags());
    

    Oh, these were missing before? Nice catch! 👏

Doesn't this also solve #2965056: Support `include` parameter on relationship routes (turns out it accidentally works on cold caches: fix + test this)?

• 2. I hate to say this but … I think it'd be much easier to understand this issue and the next if those query changes happened in the next issue.
• 4. Ahhh, good point, I missed that! 👍
• 5. Hmmmm … that sort of feels out of scope for this issue, but I guess it's silly to do that in a separate issue too. I do worry about how this impacts JSON API Extras. So I checked. Zero mentions of EntityCollection … meaning no impact! Then I can live with this change happening here.
• 10. Yes!

Interdiff review:

1. +++ b/src/Controller/EntityResource.php
   @@ -444,12 +444,11 @@ class EntityResource {
   -    $is_multiple = $field_list->getDataDefinition()->getFieldStorageDefinition()->isMultiple();
   
   @@ -458,13 +457,15 @@ class EntityResource {
   +      $resource_collection = new ResourceCollection($this->doCollectionQuery($query, $query_cacheability)->toArray(), $field_storage_definition->getCardinality());
   

   Oh, hah, Drupal core apparently also confuses "multiple" and "cardinality" :)

2. +++ b/src/JsonApiResource/ResourceCollection.php
   @@ -22,11 +22,11 @@ class ResourceCollection implements \IteratorAggregate, \Countable {
   -   * Whether this collection can have more than one resource.
   +   * The number of resources permitted in this collection.
       *
       * @var bool
       */
   -  protected $isMultiple;
   +  protected $cardinality;
   

   s/bool/int/

3. +++ b/src/JsonApiResource/ResourceCollection.php
   @@ -43,24 +43,26 @@ class ResourceCollection implements \IteratorAggregate, \Countable {
   -   * Instantiates a EntityCollection object.
   +   * Instantiates a ResourceCollection object.
   ...
   +   * @param \Drupal\Core\Entity\EntityInterface|null[] $resources
   +   *   The resources for the collection.
   

   "resource collection" but it contains "entity" objects.

   I don't think it's worth renaming this class right now.

4. +++ b/src/JsonApiResource/ResourceCollection.php
   @@ -43,24 +43,26 @@ class ResourceCollection implements \IteratorAggregate, \Countable {
   +  public function __construct(array $resources, $cardinality = -1) {
   
   +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -69,10 +69,13 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
   +  public function __construct(array $values, array $context, array $link_context, $cardinality = 1) {
   

   Why different defaults?

   Or, why even have defaults at all? Why not require explicitness instead?

5. +++ b/src/JsonApiResource/ResourceCollection.php
   @@ -43,24 +43,26 @@ class ResourceCollection implements \IteratorAggregate, \Countable {
   +    assert($cardinality < 0 || count($resources) <= $cardinality);
   

   Shouldn't we also assert that the cardinality is non-zero *and* not smaller than minus 1?
   Only valid values are: -1, 1, 2, 3 … N

• #35.3: I probably wrote that comment in two sittings. I did check impact on JSON API Extras and from that POV I can live with this change. But I think renaming it is also very much out of scope here. Such a rename merits its own separate issue. Also: I totally missed that interdiff-rename wasn't part of the patch yet 😳 Sorry!
• #35.4: I indeed did not anticipate that! 😮 I'm fine with not fixing a separate can of worms here (because: scope creep). Thanks for making cardinality a required argument. Explicitness FTW.
• #42: I've fixed the same problem in core REST's test coverage a long time ago 👍
• #43: 🙏 Thanks for making this patch more reviewable.
• #44: Waiting to review until the blocker is in. Note that I asked for that blocker (#2986987) to be more tightly scoped itself. It's generally better to have a larger number of issues but that are each more tightly scoped. Then each individual patch can be committed much quicker, because it's far easier to understand :)

Marking postponed.

I just committed #2986987: Convert EntityAccessDeniedHttpException into cacheable exception.

#2987206: Refactor `getEntityAndAccess` to return cacheable objects with access cacheability rather than an entity/access pair. also just landed. Retesting #47's non-combined patch…

#47 doesn't apply because I asked in #2986987-11: Convert EntityAccessDeniedHttpException into cacheable exception.2 to descope it, and you did. I think we can do this patch without those changes.

Sorry for the long silence — I was on vacation since the 24th!

This is now basically down to nits and making the set of changes as minimal as possible, to make it simpler for future readers to understand this change:

1. +++ b/src/Controller/EntityResource.php
   @@ -401,51 +402,33 @@ class EntityResource {
   -    $referenced_entities = array_filter(
   -      $field_list->referencedEntities(),
   -      function (EntityInterface $entity) {
   -        return (bool) $this->resourceTypeRepository->get(
   -          $entity->getEntityTypeId(),
   -          $entity->bundle()
   -        );
   -      }
   -    );
   ...
   +    $referenced_entities = array_filter($field_list->filterEmptyItems()->referencedEntities(), function (EntityInterface $entity) {
   +      return (bool) $this->resourceTypeRepository->get($entity->getEntityTypeId(), $entity->bundle());
   +    });
   

   This is 99% just reformatting code.

   The only actual change here is ->filterEmptyItems(). Is this a necessary change? If so, do we have test coverage for it?

   If not essential, should we defer this to a separate minor bugfix issue?

2. +++ b/src/Controller/EntityResource.php
   @@ -401,51 +402,33 @@ class EntityResource {
   -    foreach ($referenced_entities as $referenced_entity) {
   -      $collection_data[$referenced_entity->id()] = static::getAccessCheckedEntity($referenced_entity);
   ...
   -    $entity_collection = new EntityCollection($collection_data);
   ...
   +    // Check access to each of the referenced entities.
   +    $access_checked_entities = array_map(function (EntityInterface $entity) {
   +      return static::getAccessCheckedEntity($entity);
   +    }, $referenced_entities);
   +
   +    $entity_collection = new EntityCollection($access_checked_entities, $field_list->getFieldDefinition()->getFieldStorageDefinition()->getCardinality());
   

   This again seems to be 100% identical. It'd be simpler to understand and hence commit this if these changes were omitted.

3. +++ b/src/Controller/EntityResource.php
   @@ -401,51 +402,33 @@ class EntityResource {
   -      $cacheable_metadata->addCacheableDependency($referenced_entity);
   ...
   -    $response->addCacheableDependency($cacheable_metadata);
   +    $response->addCacheableDependency($entity);
   
   +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -393,11 +391,7 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -      ['node:1', 'node:2', 'node:3', 'node:4'],
   ...
   +    $this->assertEquals(['node:4'], $response->getCacheableMetadata()->getCacheTags());
   

   This is a bugfix 👍

4. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -36,19 +43,27 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   -   * Instantiates a EntityCollection object.
   +   * Instantiates a ResourceCollection object.
   

   Nit: Leftover that needs to be reverted.

Note: I fully expect to be able to commit this after just one more reroll! 🙏👌

MUCH better! Because much less change :)

1. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -38,17 +45,25 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +   *   to-one relationship should have a cardinality of 1. Use -1 is for
   

   Übernit: s/is for/for/

2. +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -147,7 +153,7 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
          // If this is a collection we need to append the pager data.
   

   This comment is outdated. Can just be removed.

Other than those two doc nitpicks, this is RTBC! I fixed those docs nitpicks myself. Committing… 🎉

+use Drupal\Core\Entity\Entity;

This is actually unused. Removing this.

(There is one other CS violation, but that one is pre-existing.)

Committed!

(I don't think @e0ipso would have wanted to give his blessing for this first, since this is a pure bugfix issue/patch, without significant architectural changes. The most significant changes are in the test coverage!)

This also unblocked #2853066: Spec Compliance: Inaccessible collection/related resources surface errors: should be 200 with hypermedia + metadata, one of the last few spec compliance bugs! 🎉 As well as #2986169: [PP-1] Support sortable, paginated and filterable related resources., a feature request.