Losing cacheability while normalizing EntityAccessDeniedHttpException for an inaccessible include

i.e. the cacheability of denied access on node B did not make it into the cacheability of JSON API document response.

That definitely sounds like a bug!

The patch in #2 looks correct. Great work, and thank you! 🙏

Originally, JSON:API did not handle cacheability at all. Now it obviously does. EntityAccessDeniedHttpException indeed carries cacheability. It looks like we forgot some edge case.

I'm saying "some edge case", because we do have explicit test coverage for precisely this. But we don't have such test coverage for inaccessible includes apparently. Even though ::doTestIncluded() (see https://github.com/drupal/drupal/blob/8.7.6/core/modules/jsonapi/tests/s...) does grant the necessary permissions prior to testing (see https://github.com/drupal/drupal/blob/8.7.6/core/modules/jsonapi/tests/s...), it does not test what the response is before granting those permissions.

We cannot just commit this patch, we need to add test coverage to prove this fixes the problem and to prevent this from ever regressing again. So we need to either add a single clearly focused regression test that fails on HEAD to JsonApiRegressionTest (this contains many prior examples so should be pretty easy) or we need to expand ::doTestIncluded() (this test coverage is very abstract so that's pretty hard). Do you think you could do that? :)

Awesome, thanks so much @bucefal91! 🥳

Perfect! 🥳 Thanks!

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -989,4 +989,53 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
+    $response = Json::decode((string) $response->getBody());
+    $this->assertArrayNotHasKey('included', $response, 'JSON API response does not contain "included" taxonomy term as the latter is not published, i.e not accessible.');

i think we should add a positive assertion here too, e.g. check that the omitted key is present and contains what we expect, otherwise we've got a bit of a false sense of coverage

Other than that, great work and nice sleuth work 🕵️‍♂️

Ah, of course! Great point, @larowlan! 👍

Back to you, @bucefal91, and thank you very much for the kind words :)

#18 is a zero-byte patch sadly :D Can you reupload? :)

Thanks!

No longer applies after #3048348: Denormalizing NULL for an optional @FieldType=address or @FieldType=geolocation field fails due to either no main property name or computed read-only main property

Reroll.

Thanks!

Wanted to re-RTBC, but then noticed a few things.

1. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1091,6 +1091,63 @@ public function testInvalidDataTriggersUnprocessableEntityErrorFromIssue3052954(
   +  }
   +
   +
      /**
   

   During the reroll, this added one line 😅🤓

   That can be fixed on commit.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -1091,6 +1091,63 @@ public function testInvalidDataTriggersUnprocessableEntityErrorFromIssue3052954(
   +  public function testLeakCacheMetadataInOmittedFromIssue3074710() {
   

   Oh but actually this should also still remove the FromIssue… suffix.

So, let's remove that suffix, remove the blank line and … actually, let's move this to the very end of the class too — if we're adding a new test, it should just be appended to the end, not inserted before the end.

(Edit: not so) Proper reroll with changes from #24

-

Aaaah, so close!

#25:

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -48,7 +48,7 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
-  public function testBundleSpecificTargetEntityTypeFromIssue2953207() {
+  public function testBundleSpecificTargetEntityType() {

@@ -1131,4 +1131,60 @@ public function testEmptyMapFieldTypeDenormalization() {
+  public function testLeakCacheMetadataInOmittedFromIssue3074710() {

You changed an existing method name instead of the new one 😛 I see you fixed that in #27, but not entirely:

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1131,4 +1131,60 @@ public function testEmptyMapFieldTypeDenormalization() {
+  public function testLeakCacheMetadataInOmittedFromIssue() {

The FromIssue suffix should still be removed!

One more try for proper reroll.

🥳

Committed 0e29b53 and pushed to 8.8.x. Thanks!