Impossible to raise an error when an `include` is requested for an inaccessible relationship field.

Access can only be guaranteed at the entity level, the entity type definition is not necessarily enough. For me that's the last nudge into having each entity fail differently if necessary and succeed with the collection itself. So +1.

+1

#2853066: Spec Compliance: Inaccessible collection/related resources surface errors: should be 200 with hypermedia + metadata was pushed to 2.x. Should this be pushed to 2.x too? Or can/should we do it in 1.x, because this is something that currently just is broken/fails? Can it be done elegantly in 1.x, or would it be much easier to just do it all in 2.x?

Can you explain why #2949019: Untangle Relationship(Normalizer) and EntityReferenceFieldNormalizer is related?

Thanks! (Admittedly I don't see exactly where these issues intersect, but I do see how they're related. I'm sure that #9 will help when we work on this issue!)

I think that the patch at #2976108-5: Spec Compliance: Impossible to include related resources when relationship field is not in a request's sparse fieldset shows a pretty simple way for us to support this.

Discovered as part of #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets.

Actually, this was discovered not in #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets, but in #2956084: Impossible to raise an error when an `include` is requested for an inaccessible relationship field..

At least, that's the issue that made the @todo start pointing to this issue. Because it was actually introduced in 165974 — for the 1.14 security release!

I don't think this is postponed on any particular issue anymore?

Also, making this test/patch pass is AFAICT the goal.

@gabesullice told me in chat he thought that he thought/hoped #2976108: Spec Compliance: Impossible to include related resources when relationship field is not in a request's sparse fieldset could serve as an inspiration for how to implement this. And I apparently said something similar in #11. I'm happy to say that turned out to be true :)

The IS says:

1. Decide if this should be a 400-level bad request, or if an error should be added to meta.errors.
2. Implement the change.

I'm assuming it's the latter: an error added to meta.errors.

Assuming that, this patch is a starting point. The bubbling of meta.errors from deeper levels to the top level is a bit messy, because, to my great surprise, that is not something that is actually supported yet! :O

+++ b/src/Normalizer/HttpExceptionNormalizer.php
@@ -59,7 +59,7 @@ class HttpExceptionNormalizer extends NormalizerBase {
-      FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
+      1,

This caused those failures. Fixing…

Also working to fix the remaining failures.

This fixes most but not all failures.

This fixes NodeTest::getRelated() and NodeTest::getRelationships(). I'm not convinced this is what we want though, but there already is a pre-existing todo for that very seem reason, about the change this interdiff is making…

I actually wanted to shift my focus from this to #2955020-17: Spec Compliance: JSON API's profile/extention (Fancy Filters, Drupal sorting, Drupal pagination, relationship arity) needs to be explicitly communicated, but given that this blocks #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships, I spent some more time on it. For the first time NodeTest will pass all tests. Hopefully some others will too!

Back on this.

This fixes TermTest::testGetIndividual().

More in the morning.

Quoting #4:

Should this be pushed to 2.x too? Or can/should we do it in 1.x, because this is something that currently just is broken/fails? Can it be done elegantly in 1.x, or would it be much easier to just do it all in 2.x?

Quoting #6:

I can't see a way to do this in 1.x without wasting a ton of effort.

So #6 moved this from 1.x to 2.x.

Yet this apparently is a blocker for #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships, and that issue is against 1.x. So … I'm confused now.

I didn't link to #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships by mistake, I thought this was a blocker to that issue! Based on #2972808-8: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships. But as I explained in #2972808-22: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships, that was a misinterpretation on my side! Sorry :(

Thanks for explaining that this is a blocker, but not for #2972808, for #2946537: Test coverage: Inclusion of intermediate resources when include is a multi-part relationship path!

I am still digging into the 6 remaining failures. Man, they're hard to fix. I've fixed one more failure, I'll just post that rather than waiting for all of them to be fixed. This fixes MediaTest::testGetIndividual().

Fix FileTest::testGetIndividual() and CommentTest::testGetIndividual() in a similar way.

All this time, I was trying to get the expected pointer to match the actual one. And that got me very deep into the details of the test coverage that automatically constructs expected responses for collections, and includes for collections; those expected responses are constructed by following the web of responses. This is understandably and necessarily complex.

But then it hit me: I can just grant additional permissions. For some reason, getIncludePermissions() was only being granted for includes for the individual test coverage, not for includes for the collection test coverage. This seems like a simple oversight.

This should make all tests pass except UserTest::testCollection and EntityTest::testCollection().

Yay, down to 2 failures as expected! 🎉

EntityTest::testCollection() needed special care. I expected the fix to be similar to those in #39. But that didn't work out. I found the reason: its owner is user zero, the anonymous user. view access to the anonymous user is always forbidden by UserAccessControlHandler. If #2843922: Show label of inaccessible entities ('view' access denied) when 'view label' access is allowed were already fixed, this could've worked.

But the solution is simple: just don't make user zero the owner! That was a non-essential detail of the test coverage anyway.

The last one: UserTest::testCollection(). Not only are the pointers wrong, there's also a difference in error count! 7 versus 9.

Applying the same pattern as #39 and #41 fixes the pointers problem and in doing so, it reduces the number of errors, but the delta remains: 4 versus 6.

The count delta is due to the sorted collection containing user zero, and the way the expected collection response is built; it is fetching related data (to fetch the expected includes) for every entity in the collection, even if the entity is not accessible. But if the entity is not accessible, then neither can its includes be accessible; otherwise that'd be information disclosure. In other words: things are working correctly and securely, but the expectations generated by the tests are wrong :) A simple if-test in the right place fixes this!

This should be green 🙏

In #41, UserTest::testCollection() failed. In #43, this is fixed, but now UserTest::testGetIndividual() is failing. Why?

+++ b/tests/src/Functional/UserTest.php
@@ -461,4 +461,13 @@ class UserTest extends ResourceTestBase {
+      'roles' => ['administer users'],

This causes many additional fields to show up in testGetIndividual(), which then causes the doTestIncluded() →
getExpectedIncludeResponse() → getExpectedGetIndividualResourceResponse() → getExpectedDocument() call chain to get a set of fields that is only a subset of what is now returned.

Gah! 😭

Turns out we didn't even that administer users permission; the errors match exactly, they're just sorted differently! That's hopefully fixable by updating \Drupal\Tests\jsonapi\Functional\ResourceTestBase::sortErrors() :)

While working on this, I spotted a fair amount of duplication that we've accumulated, and which seriously complicated the process of getting to a green patch. This works at least for UserTest::testGetIndividual(), let's see if it works for everything!

I think there's more to be done still, but the functional overlap between getExpectedIncludeResponse() and decorateExpectedResponseForIncludedFields() is significant.

I suspect getExpectedRelatedResponses() and getExpectedIncludedResourceResponse() are similarly overlapping.

Note that this probably should happen in a separate issue, but having done so much digging in this issue, I didn't want this insight to get lost, and posting a separate patch will mean less (since this issue is improving a fair bit of our test coverage). If this is green, it should probably be omitted from this patch and spun off into its own issue. Unless @gabesullice thinks it belongs here.

+++ b/src/Normalizer/RelationshipItemNormalizer.php
@@ -82,6 +83,9 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
+        $entity_and_access['entity'] = new EntityAccessDeniedHttpException($target_entity, $entity_and_access['access'], "/data/relationships/$host_field_name", $entity_and_access['entity']->getMessage());

I'm actually not at all convinced that /data/relationships/… is the right pointer here.

If it's for an include, the pointer should probably be /includes?

That being said… I don't think it makes sense to fix that here. That's going to be another massive undertaking.

Addressing all the reviews now!

#24:

1. 👍 You're right! Re-reading \Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer::expandContext() seems to confirm your suspicion. Because the variable is called $public_includes. But re-reading \Drupal\jsonapi\Context\FieldResolver::resolveInternalIncludePath() indicates the opposite! Therefore $context['includes'] lists internal, resolved includes. This is also the only sensible thing here; otherwise we'd continuously be translating back and forth everywhere. It makes sense that expandContext() did this for us.

   Renamed the variable in expandContext() to avoid future confusion.

2. 🙂
3. 👎 That'd cause nested arrays rather than a single array of errors I think? Both sides are lists of errors. Pushing won't work; that'd I copy/pasted this from \Drupal\jsonapi\Normalizer\Value\JsonApiDocumentTopLevelNormalizerValue::rasterizeValue(). Seems to work fine? Perhaps I'm misunderstanding what you wrote?
4. 🦅👀 It's to ensure that we never end up omitting data under meta that is not meta.errors. For example meta.openapi. If/when we start doing that, this assertion will fail, which will remind us to update this logic, and we'll be thankful to not have to figure this out all from scratch! It's a safeguard for our future selves :)
5. See above.
6. ❤️
7. ❤️

#47:

1. ✔️
2. 👍 BTW, you were right about the reason.
3. See above.
4. 👍 Indeed!
5. Agreed. Which is why I'm not convinced this patch is ready to go as-is. The better solution would be to first assert expected error responses, and then grant the permission for a particular include, then observing that error disappearing. That's how we test field access.

   Perhaps you could take that on? :)

composer/packagist are having a bad day:

Updating dependencies (including require-dev)

                                                                                                                                                                                     
  [Composer\Downloader\TransportException]                                                                                                                                           
  The "http://packagist.org/p/symfony/polyfill-iconv%2445594a6036fb36f5574e98f855e18f60cf7bf3e1007cac2b001a9111d12b4c90.json" file could not be downloaded (HTTP/1.1 404 Not Found)  

Retesting.

🤞 → 🤘

#55: 👍

#56 + #57:

1. +++ b/tests/src/Functional/BlockContentTest.php
   @@ -262,4 +262,12 @@ class BlockContentTest extends ResourceTestBase {
   +  protected static function getIncludePermissions() {
   +    return [
   +      'field_jsonapi_test_entity_ref' => ['field_jsonapi_test_entity_ref view access']
   +    ];
   +  }
   

   Why only do this for this entity type? Why not do this in ResourceTestBase?

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1124,17 +1124,10 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $expected_response = static::decorateExpectedResponseForIncludedFields($expected_response, $related_responses);
   +    $expected_response =  $this->getExpectedCollectionResponse($filtered_entity_collection, $filtered_collection_include_url->toString(), $request_options, $relationship_field_names);
   
   @@ -1155,14 +1163,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $expected_response = static::decorateExpectedResponseForIncludedFields($expected_response, $related_responses);
   +    $expected_response = $this->getExpectedCollectionResponse($sorted_entity_collection, $sorted_collection_include_url->toString(), $request_options, $relationship_field_names);
   
   @@ -1180,13 +1181,16 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected function getExpectedCollectionResponse(array $collection, $self_link, array $request_options) {
   +  protected function getExpectedCollectionResponse(array $collection, $self_link, array $request_options, array $included_paths = NULL) {
   

   Refactoring/cleanup: 👍

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1142,6 +1135,21 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // If the response should vary by a user's authorizations, grant permissions
   +    // for the included resources and execute another request.
   +    if (!empty($relationship_field_names) && !empty(array_intersect($expected_cacheability->getCacheContexts(), ['user', 'user.permissions', 'user.roles']))) {
   

   So this is new test coverage:

   Then, we introduce the code that will test an include collection without authentication. If it should be differ by user/permissions, we then test it again with appropriate permissions added.

   Great!

4. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1142,6 +1135,21 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $expected_response =  $this->getExpectedCollectionResponse($filtered_entity_collection, $filtered_collection_include_url->toString(), $request_options, $relationship_field_names);
   

   But where is this causing the request to be unauthenticated, like the comment said?

##5:

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1560,11 +1560,13 @@ abstract class ResourceTestBase extends BrowserTestBase {
+      // @todo: is this the right pointer?
+      $pointer = "/data/relationships/$relationship_field_name";

I made the same remark in #49. I think we can leave that for a future issue? This is still a big step forward.

#66:

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -2867,4 +2859,18 @@ abstract class ResourceTestBase extends BrowserTestBase {
+  protected function grantIncludedPermissions(array $include_paths = []) {

Nit: "included" vs "include"

Overall patch review:

1. +++ b/src/Controller/EntityResource.php
   @@ -819,7 +819,7 @@ class EntityResource {
          // @todo Is this really the right path?
   
   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1549,11 +1562,13 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      // @todo: is this the right pointer?
   

   We should replace these @todos with an issue link.

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -86,6 +87,13 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +      $field_name = $field->getName();
   +      // Allow inaccessible includes to raise an error.
   +      if (isset($context['include']) && in_array($field_name, $context['include'], TRUE)) {
   +        $exception = new EntityAccessDeniedHttpException($field->getEntity(), $field_access, "/data/relationships/$field_name", 'The current user is not allowed to view this relationship.');
   +        return $this->serializer->normalize($exception, $format, $context);
   +      }
   

   This is the key/central change that triggered all other changes.

3. +++ b/src/Normalizer/RelationshipItemNormalizer.php
   @@ -82,6 +83,9 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
   +      if ($entity_and_access['entity'] instanceof EntityAccessDeniedHttpException) {
   +        $entity_and_access['entity']->setPointer("/data/relationships/$host_field_name");
   +      }
   

   Could also use a link to that same issue.

4. +++ b/src/Normalizer/RelationshipItemNormalizer.php
   --- a/src/Normalizer/Value/EntityNormalizerValue.php
   +++ b/src/Normalizer/Value/EntityNormalizerValue.php
   

   In here, bugfixes to how includes are rasterized; necessary to let includes bubble up meta errors!

5. +++ b/tests/src/Functional/FileTest.php
   @@ -230,4 +230,14 @@ class FileTest extends ResourceTestBase {
   +  protected static function getIncludePermissions() {
   +    return [
   +      'uid' => ['access user profiles'],
   +      'uid.roles' => ['administer users'],
   +    ];
   +  }
   
   +++ b/tests/src/Functional/MediaTest.php
   @@ -374,9 +374,21 @@ class MediaTest extends ResourceTestBase {
   +  protected static function getIncludePermissions() {
   +    return [
   +      'bundle' => ['administer media types'],
   +      'field_media_file' => ['access content'],
   +      'thumbnail' => ['access content'],
   +      'uid.roles' => ['administer users'],
   +    ];
      }
   
   +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1127,24 +1127,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +    // If the response should vary by a user's authorizations, grant permissions
   +    // for the included resources and execute another request.
   +    $varies_by_access = !empty(array_intersect($expected_cacheability->getCacheContexts(), ['user', 'user.permissions', 'user.roles']));
   +    if (!empty($relationship_field_names) && $varies_by_access) {
   +      $applicable_permissions = array_intersect_key(static::getIncludePermissions(), array_flip($relationship_field_names));
   +      $flattened_permissions = array_unique(array_reduce($applicable_permissions, 'array_merge', []));
   +      $this->grantPermissionsToTestedRole($flattened_permissions);
   +      $expected_response =  $this->getExpectedCollectionResponse($filtered_entity_collection, $filtered_collection_include_url->toString(), $request_options, $relationship_field_names);
   +      $expected_cacheability = $expected_response->getCacheableMetadata();
   +      $expected_document = $expected_response->getResponseData();
   +      $response = $this->request('GET', $filtered_collection_include_url, $request_options);
   +      $requires_include_only_permissions = !empty($flattened_permissions);
   +      $cacheable = $expected_cacheability->getCacheMaxAge() !== 0;
   +      $dynamic_cache = $cacheable ? $requires_include_only_permissions ? 'MISS' : 'HIT' : 'UNCACHEABLE';
   +      $this->assertResourceResponse(200, $expected_document, $response, $expected_cacheability->getCacheTags(), $expected_cacheability->getCacheContexts(), FALSE, $dynamic_cache);
   +    }
   

   I wrote:

   Which is why I'm not convinced this patch is ready to go as-is. The better solution would be to first assert expected error responses, and then grant the permission for a particular include, then observing that error disappearing.... Perhaps you could take that on? :)

   And I think you added the cited new test code to test that. But does it actually test this? I don't think it does.

6. +++ b/tests/src/Functional/ResourceResponseTestTrait.php
   @@ -491,7 +514,7 @@ trait ResourceResponseTestTrait {
   -      $error['source']['pointer'] = ($pointer) ? $pointer : $relationship_field_name;
   +      $error['source']['pointer'] = ($pointer) ? $pointer : "/data/relationships/$relationship_field_name";
   

   Again point to that same issue.

Rebased #66. Two conflicts had to be resolved (one with #2962461: JsonApiDocumentTopLevelNormalizer is SerializerAware but doesn't get the serializer injected, one with #2948666: Remove JSON API's use of $context['cacheable_metadata'], both got committed today).

#69 still needs to be addressed.

It'd be great to get this done, so we can also close #2946537: Test coverage: Inclusion of intermediate resources when include is a multi-part relationship path.

Both of those issue should have made this significantly easier

👍

but it also means that #70 is no longer applicable and will be practically impossible to reroll.

Makes sense.

#77 actually doespass tests, but fails on a single CS violation!

Fixing that.

+++ b/src/IncludeResolver.php
@@ -103,7 +104,11 @@ class IncludeResolver {
-        if (!$field_list->access('view')) {
+        $field_access = $field_list->access('view', NULL, TRUE);
+        if (!$field_access->isAllowed()) {
+          $message = 'The current user is not allowed to view this relationship.';
+          $exception = new EntityAccessDeniedHttpException($entity, $field_access, '', $message);
+          $includes = EntityCollection::merge($includes, new EntityCollection([$exception]));
           continue;

What can I say? This is now all that's necessary.

Compare that to #70. It's night and day!

But in #70, we're also adding getIncludePermissions() and adding test coverage:

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1127,24 +1127,31 @@ abstract class ResourceTestBase extends BrowserTestBase {
+    // If the response should vary by a user's authorizations, grant permissions
+    // for the included resources and execute another request.
+    $varies_by_access = !empty(array_intersect($expected_cacheability->getCacheContexts(), ['user', 'user.permissions', 'user.roles']));
+    if (!empty($relationship_field_names) && $varies_by_access) {
…

Shouldn't we retain those things?

Oops, I misinterpreted #77 — I thought it was passing tests, apparently it wasn't!

super subtle and frustrating to debug 🙃

I believe you!

Down to 2 failures :) Patch looks good! Just one nit for now:

+++ b/src/Exception/EntityAccessDeniedHttpException.php
@@ -46,18 +46,22 @@ class EntityAccessDeniedHttpException extends CacheableAccessDeniedHttpException
+   *   (Optional) A relationship field name if access was denied for a

s/for a/for an entity included via a/

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1131,11 +1132,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $expected_response = $this->getExpectedCollectionResponse($filtered_entity_collection, $filtered_collection_include_url->toString(), $request_options);
   -    $related_responses = array_reduce($filtered_entity_collection, function ($related_responses, $entity) use ($relationship_field_names, $request_options) {
   -      return array_merge($related_responses, array_values($this->getExpectedRelatedResponses($relationship_field_names, $request_options, $entity)));
   -    }, []);
   -    $expected_response = static::decorateExpectedResponseForIncludedFields($expected_response, $related_responses);
   +    $expected_response = $this->getExpectedCollectionResponse($filtered_entity_collection, $filtered_collection_include_url->toString(), $request_options, $relationship_field_names);
   
   @@ -1157,11 +1175,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -    $expected_response = $this->getExpectedCollectionResponse($sorted_entity_collection, $sorted_collection_include_url->toString(), $request_options);
   -    $related_responses = array_reduce($sorted_entity_collection, function ($related_responses, $entity) use ($relationship_field_names, $request_options) {
   -      return array_merge($related_responses, array_values($this->getExpectedRelatedResponses($relationship_field_names, $request_options, $entity)));
   -    }, []);
   -    $expected_response = static::decorateExpectedResponseForIncludedFields($expected_response, $related_responses);
   +    $expected_response = $this->getExpectedCollectionResponse($sorted_entity_collection, $sorted_collection_include_url->toString(), $request_options, $relationship_field_names);
   
   @@ -1180,13 +1194,16 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected function getExpectedCollectionResponse(array $collection, $self_link, array $request_options) {
   +  protected function getExpectedCollectionResponse(array $collection, $self_link, array $request_options, array $included_paths = NULL) {
   

   Makes sense to move this logic into the helper, because the logic became more nuanced.

2. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1144,6 +1141,27 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +      $dynamic_cache = $cacheable ? $requires_include_only_permissions ? 'MISS' : 'HIT' : 'UNCACHEABLE';
   

   Wow 😵

3. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -2825,4 +2806,18 @@ abstract class ResourceTestBase extends BrowserTestBase {
   +   * Grant authorization to view includes.
   

   Übernit: s/Grant/Grants/. Will fix on commit. Interdiff already attached.

4. +++ b/tests/src/Functional/TermTest.php
   @@ -496,4 +496,13 @@ class TermTest extends ResourceTestBase {
   +  protected static function getIncludePermissions() {
   +    return [
   +      'vid' => ['access taxonomy overview'],
   +    ];
   

   Interesting that we only needed this for taxonomy_term. In #66, we needed this for many more fields. Why is that?

Point 3 is the only reason that this isn't RTBC and committed yet.

I still want to verify this in detail before committing. Just to be 100% sure, and confirm my own understanding.

I expect to commit this tomorrow :)

Reverted the README changes that accidentally sneaked in to #100.

+++ b/tests/src/Functional/NodeTest.php
@@ -346,7 +346,7 @@ class NodeTest extends ResourceTestBase {
-      'uid.roles' => ['administer permissions'],
+      'uid.roles' => ['administer users'],

This is a fix that as in the earlier patch iterations, introduced in #28. If you check \Drupal\user\UserAccessControlHandler::checkFieldAccess(), you can see that administer users is definitely the right one.

So some change in the earlier patch iterations was causing getIncludePermissions() to be used correctly, in HEAD its' being used incorrectly. I managed to reproduce that same flaw with the current patch iteration. But also with HEAD. We shouldn't block this issue on fixing a pre-existing bug. Created a new issue for that: #3005158: Nested include test coverage is inadequate wrt "include permissions".

So this issue uncovered [#30051858]. But it also unblocked #2946537: Test coverage: Inclusion of intermediate resources when include is a multi-part relationship path!

:)