EntityResource should add _entity_access requirement to REST routes

I remember articulating exactly this concern/complaint, trying to make it work, and then there being a good reason not to have that. Let me dig that up.

The design for the current approach was introduced in #1866908: Honor entity and field access control in REST Services, then the missing stuff was added in #1947432: Add a generic EntityAccessCheck to replace entity_page_access(), and then eventually I worked to get it cleaned up in #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.

Specifically see #2664780-28: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources, and this particular portion of it:

3. add _entity_access: '<entity type>:<operation>' route requirements, e.g. _entity_access: 'node.view' — this will result in the access checking happening before EntityResource::get() is called. This was simply not considered in #1866908: Honor entity and field access control in REST Services because _entity_access did not yet exist then: it was only added many months later, in #1947432: Add a generic EntityAccessCheck to replace entity_page_access().
   However… upon closer inspection, this causes failures for the REST routes that allow users to create entities: _entity_access only actually works for routes that have a route parameter that is that entity, which obviously can't work for POST routes. Patch attached for completeness, but expect failures.

So, there are not two possible solutions, but just one: #2 in the list above. (Even though I would have very much preferred the third option that uses _entity_access, it feels the most elegant to me personally.) We have variants A and B to choose from, and variant B seems to be the most reliable one.

Then see #46, #47, #48.

However… if you see a way to make it work though, be my guest! I'd love for things to work this way! It's more feasible to introduce that now, because we now have solid test coverage, we had close to zero REST test coverage back then.

But, given that things are working correctly, this would qualify as a minor task.

You know, let's try it out :)

You beat me to it :)

Details seem to matter ...

Patch review:

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -378,6 +340,23 @@ public function permissions() {
+        $route->setRequirement('_entity', $this->entityType->id() . '.delete');

#9 didn't update this one yet.

The failing tests show this:

--- Expected
+++ Actual
@@ @@
-{"message":"The used authentication method is not allowed on this route."}
+{"message":"The \u0027administer actions\u0027 permission is required."}

IOW: this change does seem to work, but it seems to prevent \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider() from firing as before. This sounds like a bug in the request processing system…

Patch no longer applied. Straight reroll.

Fixed #13's first remark.

This should reduce the number of failures.

Because testbot only reports number of test classes failed and not number of test methods failed, #15 doesn't seem to fix anything. But it really does reduce the number of fails.

#14:

Testing Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest
FFFF

Time: 55.75 seconds, Memory: 4.00MB

There were 4 failures:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"The used authentication method is not allowed on this route."}
+{"message":"The \u0027view test entity\u0027 permission is required."}

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:361

2) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPost
Failed asserting that 403 is identical to 400.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:764

3) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPatch
Failed asserting that 403 is identical to 400.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:997

4) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testDelete
Failed asserting that 204 is identical to 403.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1206

FAILURES!
Tests: 4, Assertions: 52, Failures: 4.

vs #15:

Testing Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest
FFF.

Time: 55.06 seconds, Memory: 4.00MB

There were 3 failures:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"The used authentication method is not allowed on this route."}
+{"message":"The \u0027view test entity\u0027 permission is required."}

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:361

2) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPost
Failed asserting that 403 is identical to 400.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:764

3) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPatch
Failed asserting that 403 is identical to 400.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:997

FAILURES!
Tests: 4, Assertions: 63, Failures: 3.

Strangely, testbot didn't mark this NW.

Let me have a look at the remaining failures

Here is a quick anaylsis of this problem:

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"The used authentication method is not allowed on this route."}
+{"message":"The \u0027view test entity\u0027 permission is required."}

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:361

2) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDateonlyTest::testPost
Failed asserting that 403 is identical to 400.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.

Before the flow looked like the following:

- Routing
  - The empty access checking
- \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider
- controller
  - The entity access checking

Afterwards the flow is the following:

- Routing
  - The actual access checking, which fails early!
- \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider
- controller

Given that the entity access checking will always be executed before the authentication subscriber and as such its impossible to produce the authentication checking. I'm wondering though whether we could call the logic in \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onKernelRequestFilterProvider in an exception subscriber as well?

Great analysis! Thanks @dawehner :) 👍 Totally makes sense now!

I also like the sound of your proposed solution, I'm curious to see what the code would look like.

Let's give it a try

Nice, from 216 fails to only 132!

I took a look at one of the more frequent remaining test failures. It's in the GET test coverage for Dynamic Page Cache that is breaking. But only for HAL+JSON responses. The patch appears to somehow prevent the url.site cache context from being associated with the response, which means there's no cache redirect in the response cached in Dynamic Page Cache, which causes the failure.

Root cause: not yet determined, but likely the different execution order.

Note that #2864816: HAL LinkManager doesn't add 'url.site' cache context when needed could fix this.

#2864816: HAL LinkManager doesn't add 'url.site' cache context when needed just landed. Let's reroll this. #24 still applies. Retesting it. It was at 132 fails before retesting. Let's see…

So now it's at 148. (Probably also because more REST tests were committed in the mean time.)

In any case, #2864816: HAL LinkManager doesn't add 'url.site' cache context when needed didn't help.

I did some actual debugging this time. In HEAD, for Editor config entity REST responses, there is the user.permissions cache context. With this patch, that's no longer the case.

The root cause for this is that the access checking is moved out of the controller, into the routing system. The routing system's access result is stored in $request->attributes[AccessAwareRouterInterface::ACCESS_RESULT]. But it's not being merged back into the response object before Dynamic Page Cache processes it, because RouteAccessResponseSubscriber runs after Dynamic Page Cache's KernelEvents::RESPONSE subscriber. That's easy enough to fix.

But then we encounter another problem: the access result is uncacheable, because \Drupal\Core\Access\CsrfRequestHeaderAccessCheck::access() runs for every single REST route match. This is one of those long-standing bugs in the REST module's routing architecture. I highlighted it in #2737751: Refactor REST routing to address its brittleness about 1.5 year ago:

8. ResourceRoutes::alter() sets _csrf_request_header_token on all routes, including GET routes. This is harmless, but it causes extra, unnecessary computations on those requests. And it makes the route more confusing to debug.

Well, I was overly optimistic: it is harmful. That was demonstrated here.

So this patch makes two changes: updates the priority of RouteAccessResponseSubscriber, and changes ResourceRoutes to only set the _csrf_request_header_token route requirement when actually necessary.

Now lots of tests should start passing :)

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php
@@ -50,7 +50,7 @@ public function onRespond(FilterResponseEvent $event) {
     // Priority 10, so that it runs before FinishResponseSubscriber, which will
     // expose the cacheability metadata in the form of headers.
-    $events[KernelEvents::RESPONSE][] = ['onRespond', 10];
+    $events[KernelEvents::RESPONSE][] = ['onRespond', 110];
     return $events;

I could be wrong, but I remember that we once discussed that it is by design that route access cacheability is *not* considered by dynamic page cache because dynamic page cache runs *after* route access checking.

But maybe I misunderstood something here.

Many of those failures are of the kind Failed asserting that 403 is identical to 400. — those occur because of the reason given by @dawehner in #21. Those checks now happen after logging in. That's a soft behavior change (only in case of invalid requests). Essentially, this changes the "request validation order". So just moving some of the existing checks until after the request contains authentication and until the user is authorized to perform the operation, is enough to fix those test failures.

#31 updated testPatch(). I should also have updated testPost().

In 42 cases, the GET or DELETE test is failing on asserting the expected error message in case of a 403: the expected message is some string, the actual one … is empty. In all of these cases, it's the fallback message generated by
EntityResource::generateFallbackAccessDeniedMessage(), which is what was used in case the access result didn't contain a reason.

This can and should be done in a separate issue, on which this issue should be blocked. For now, doing it here, so we can prove it works. Attached is a patch that fixes it for Media and User (which should fix 12 of the failures, but not all).

+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -94,8 +94,6 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
-        $route->setRequirement('_csrf_request_header_token', 'TRUE');

@@ -120,6 +118,7 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
+          $route->setRequirement('_csrf_request_header_token', 'TRUE');

This change in #28 was mostly right, but not entirely right. I forgot about DELETE, which is why there are now 30 test failures like this:

Failed asserting that 204 is identical to 403.

The good

Continuing #33: fixed Block + Comment + ConfigTest + Media (#33 only took care of the delete operation, this does the same for the update operation) + MenuLinkContent + ShortcutSet + User.

The Bad

There were some I could not fix easily: EntityTestLabel + EntityTest + Node +
Shortcut + Term. What's special about these?
All of them have access logic that is bundle-dependent. And apparently the _entity_access access check is not
have the same root cause $entity_bundle in the create access check is missing. Because this patch does:

+        $route->setRequirement('_entity_create_access', $this->entityType->id());

But that isn't yet setting the bundle type like \Drupal\Core\Entity\EntityCreateAccessCheck::access() expects not just node, but node:{node_type}, or shortcut:{shortcut_set}. Where that latter portion is then referring to the bundle type that should be passed to the create access check, and is in fact a placeholder for a request argument.

The Ugly

We can fix that route requirement.

But it still will fail. Because REST's POST routes are not bundle-specific. Which means the only options we have are:

1. stop using _entity_create_access
2. update \Drupal\Core\Entity\EntityCreateAccessCheck::access() to somehow receive the bundle from the request body…?

To understand this, see the following hunks:

diff --git a/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
index 9ceac52..d5bc236 100644
--- a/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
@@ -62,7 +62,7 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
       }
       // If we were unable to replace all placeholders, deny access.
       if (strpos($bundle, '{') !== FALSE) {
-        return AccessResult::neutral();
+        return AccessResult::neutral(sprintf("Could not find '%s' request argument, therefore cannot check create access.", $bundle));
       }
     }
     return $this->entityManager->getAccessControlHandler($entity_type)->createAccess($bundle, $account, [], TRUE);
diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index a945b83..be77872 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -346,7 +346,12 @@ protected function getBaseRoute($canonical_path, $method) {
         $route->setRequirement('_entity_access', $this->entityType->id() . '.view');
         break;
       case 'POST':
-        $route->setRequirement('_entity_create_access', $this->entityType->id());
+        if ($this->entityType->getBundleEntityType()) {
+          $route->setRequirement('_entity_create_access', $this->entityType->id() . ':{' . $this->entityType->getBundleEntityType() . '}');
+        }
+        else {
+          $route->setRequirement('_entity_create_access', $this->entityType->id());
+        }
         break;
       case 'PATCH':
         $route->setRequirement('_entity_access', $this->entityType->id() . '.update');

The only failures now should be of the kind "Could not find {blah} request argument…"

Pinged @dawehner on IRC and @tstoeckler on Twitter: https://twitter.com/wimleers/status/953325138312908801.

Is it true that JSON API and GraphQL have actually chosen per-bundle resources instead of per-entity-type resources?

I don't know about GraphQL, but yes, JSON API has chosen that. So, yes, JSON API should be able to use that :)

Alright, rerolling this patch to use _entity_access for all cases except POST then!

First, rebasing on top of HEAD. Sadly, #2862422: Add per-media type creation permissions for media made Media's access control handler worse than it was before. I just dropped all changes for it.

Per #44, reverted the _entity_access_create-related bits. This should pass tests, because it was the sole reason some tests failed to pass in #39. Of course, things could have changed in the mean time…

As far as I can tell this is modelled after AuthenticationSubscriber::onKernelRequestFilterProvider(). I think we should add the isset($this->filter) check to the outer if here, as well.

Nice catch!

Would be nice to pass $exception as $previous into the constructor of the new exception.

👍

Should this be split up to use e.g. allowedIfHasPermission(), andIf(), etc.

After changing it, this got way more readable.

Shouldn't this mention that the user must the comment author?

Sure, why not

would be a bit more concise?

Same for ShortcutSetResourceTestBase

I went with not changing this because I thought these might need additional steps once we add POST/UPDATE support.

This doesn't yet resolve all the test failures of the rest tests.

1. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -37,7 +37,10 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          ->cachePerPermissions()
   

   This can be removed, \Drupal\Core\Access\allowedIfHasPermission() already does this for us :) @tstoeckler already said that in #51.

   Also, a small bug snuck in there, causing the number of test failures to go up.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php
   @@ -74,6 +74,9 @@ public function testAccess($entity_bundle, $requirement, $access, $expected, $ex
   +    if (!$entity_bundle && !$expect_permission_context) {
   +      $expected_access_result->setReason("Could not find '{bundle_argument}' request argument, therefore cannot check create access.");
   +    }
   

   ❤️

I investigated the remaining failures. Most of them are due to a 4xx response not containing X-Drupal-Cache-Tags, X-Drupal-Cache-Contexts, X-Drupal-Dynamic-Cache and X-Drupal-Cache headers. IOW: the responses are not cacheable, and are no longer cached by Dynamic Page Cache nor Page Cache!

Why is that?

Well, first: why are those 4xx responses even cacheable? Because not that long ago we made them cacheable, in #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage. The entire reason they are cacheable is because we changed the throw new AccessDeniedHttpException(…) to throw new CacheableAccessDeniedHttpException($entity_access, …) in \Drupal\rest\Plugin\rest\resource\EntityResource::get().

But this patch is removing the access checking from \Drupal\rest\Plugin\rest\resource\EntityResource::get(), and is moving that into the routing system. So now it's \Drupal\Core\Routing\AccessAwareRouter::checkAccess() that is throwing throw new AccessDeniedHttpException().

We can change that to throw new CacheableAccessDeniedHttpException($access_result, …) in the appropriate circumstances.

Once that's done, the tags and contexts assertions do pass. But the Dynamic Page Cache assertion does not yet pass: the 4xx responses are no longer cached by Dynamic Page Cache. (They now are cached by Page Cache!)

Why?

Because of this in \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber::onResponse():

    // Don't cache the response if Dynamic Page Cache's request subscriber did
    // not fire, because that means it is impossible to have a Dynamic Page
    // Cache hit. This can happen when the master request is for example a 403
    // or 404, in which case a subrequest is performed by the router. In that
    // case, it is the subrequest's response that is cached by Dynamic Page
    // Cache, because the routing happens in a request subscriber earlier than
    // Dynamic Page Cache's and immediately sets a response, i.e. the one
    // returned by the subrequest, and thus causes Dynamic Page Cache's request
    // subscriber to not fire for the master request.
    // @see \Drupal\Core\Routing\AccessAwareRouter::checkAccess()
    // @see \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::on403()
    $request = $event->getRequest();
    if (!isset($this->requestPolicyResults[$request])) {
      return;
    }

So is there harm in losing Dynamic Page Cache support? Or do we win more by having access checking happening at the routing level, i.e. long before a controller is even instantiated? Benchmarking has the answer:

Patch (403 not cached by Dynamic Page Cache, but generated on every request, much earlier than in HEAD)

Requests per second:    59.70 [#/sec] (mean)
Time per request:       16.750 [ms] (mean)
Time per request:       16.750 [ms] (mean, across all concurrent requests)
Transfer rate:          40.52 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    14   17   1.5     16      30
Waiting:       14   17   1.5     16      29
Total:         14   17   1.6     16      30

HEAD (403 cached in Dynamic Page Cache)

Requests per second:    62.02 [#/sec] (mean)
Time per request:       16.124 [ms] (mean)
Time per request:       16.124 [ms] (mean, across all concurrent requests)
Transfer rate:          43.85 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    14   16   1.6     16      28
Waiting:       14   16   1.6     16      28
Total:         14   16   1.6     16      28

Pretty much identical performance to having the responses served from Dynamic Page Cache. With one crucial difference: no storage space consumed, no I/O!

Performance equal to Dynamic Page Cache's, but none of the costs? Yes, please! That's a nice scalability win :)

To clarify the patch in #53, here are some comments that should help:

1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -111,7 +113,12 @@ protected function checkAccess(Request $request) {
   -      throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
   +      if ($access_result instanceof CacheableDependencyInterface) {
   +        throw new CacheableAccessDeniedHttpException($access_result, $access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
   +      }
   +      else {
   +        throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
   +      }
   

   This is the change explained at length in the previous comment.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -475,7 +475,7 @@ public function testGet() {
   -    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
   +    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE);
   

   This is changing from "cached by DPC" to "not cached by DPC".

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -653,7 +653,7 @@ public function testGet() {
   -    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response);
   +    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', FALSE);
   

   This is changing from "not cacheable at all" to "cacheable", by Page Cache.

   (This is the test coverage for D8.0/8.1 sites that use the restful get entity:node-style permissions. Those always were checked at the route level, but never were cacheable, until today!)

Missed two occurrences of the thing mentioned in #54.3.

Some more 403 assertions that were not cacheable, and now are. Yay for strict testing.

This should fix most of the testPatch() and testDelete() failures.

And this should fix all the testPost() failures.

This reverts #32, which made changes per the analysis that @dawehner made in #21. That analysis is still accurate, but no longer applies to POST, because we concluded in #39 that the REST module cannot use _entity_create_access, hence we stopped using it, and hence the changes to testPost() are no longer necessary.

The high-level explanation for most of the test failures is that the cache tags of the 403 responses are now different. Because \Drupal\Core\Access\AccessManager::check() does:

$checks = $route->getOption('_access_checks') ?: [];

And it then proceeds to check each of these in the order given. And that order happens to be:

[
  'access_check.default',
  'access_check.permission',
  'access_check.entity',
]

Which means the entity access check happens after the restful get entity:node permission check. This is also the fastest (entity access checks are inherently slower than permission access checks).

Great.

So, updated the existing getExpectedUnauthorizedAccessCacheability() code to match the new (improved!) behavior. Special care was necessary for Block entities, because #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity", which has been a known issue for a very long time.

This took many hours to figure out, so glad this is done! Hopefully I didn't miss any more edge cases… (I'm relieved this at least this required only test coverage changes, and no changes in the request processing system!)

Yay, from 87 to 54!

This set of next failures is simpler to fix, fortunately.

This round is even simpler: the BasicAuthResourceWithInterfaceTranslationTestTrait alternative to BasicAuthResourceTestTrait that was added in #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage to accommodate the ConfigurableLanguage and ContentLanguageSettings config entity types' testing is no longer necessary! 🎉

The failures in Comment + File + Media are related to changes we were making in earlier patch iterations to their access control handlers. But rather than doing that here, I think we should improve those in a follow-up issue, and just accept that right now, they go from a somewhat-but-not-truly-helpful default fallback message as the reason for access not being granted, to no message at all. Otherwise, we make this patch even harder to review!

I already lost the MediaAccessControlHandler changes in the rebase last week (solving the conflict was far from trivial), and in this patch I'm explicitly reverting the changes to CommentAccessControlHandler.

Created 3 Novice follow-up issues:

1. #2950125: Add helpful reason for 'update' and 'delete' access not being allowed to CommentAccessControlHandler
2. #2950127: Add helpful reason for 'update' and 'delete' access not being allowed to FileAccessControlHandler
3. #2950129: Add helpful reason for 'update' and 'delete' access not being allowed to MediaAccessControlHandler

With some luck, #71 will be green! 😮

This issue really needs an updated summary though!

IT IS GREEN! 🍻

+++ b/core/modules/rest/tests/modules/config_test_rest/config_test_rest.module
@@ -26,5 +27,9 @@ function config_test_rest_config_test_access(EntityInterface $entity, $operation
+  if (!$access_result->isAllowed() && $access_result instanceof AccessResultReasonInterface) {

Why are we not using $access_result->isForbidden instead of !$access_results->isAllowed ?

We could do that in this one particular case, but in general using isForbidden() is a bad practice. Because it could also be isNeutral()! Checking !isAllowed() covers both. Both are a way of disallowing access. One is still overridable (isNeutral()), one is final (isForbidden()).

Awesome, thanks for that answer @Wim Leers! That makes sense. That was the only thing I could find. This issue still needs a summary update.

That was the only thing I could find.

Cool!

This issue still needs a summary update.

Great coincidence — I had begun working on doing exactly that! Will post it later today — hopefully you'll be able to RTBC then :)

Still working on that IS update, but this patch badly needs to be rebased too. It was green in #71, 2 months ago. It no longer applies.

Conflicts with at least the following:

1. #2910883: Move all entity type REST tests to the providing modules
2. #2950125: Add helpful reason for 'update' and 'delete' access not being allowed to CommentAccessControlHandler
3. #2950127: Add helpful reason for 'update' and 'delete' access not being allowed to FileAccessControlHandler
4. #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't

Fixed all conflicts, hopefully didn't make too many mistakes.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php
   @@ -50,7 +50,7 @@ public function onRespond(FilterResponseEvent $event) {
        // Priority 10, so that it runs before FinishResponseSubscriber, which will
        // expose the cacheability metadata in the form of headers.
   -    $events[KernelEvents::RESPONSE][] = ['onRespond', 10];
   +    $events[KernelEvents::RESPONSE][] = ['onRespond', 110];
   

   Comment is not yet updated.

2. +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   @@ -375,12 +375,13 @@ public function testPostSkipCommentApproval() {
   +
   

   Nit: Unnecessary change, reverted.

This was very hard to write. It took … much longer than I would have liked (HOURS!). I'm sure it can be better still, but this should still be a huge step forward. Hopefully it's good enough.

One typo in HTML and it looks like 💩

2. EntityCreateAccessCheck now provides a useful error message, to prevent anybody else from going through the pain of #39+#43+ #45 again.

This can be moved to a separate issue. So did that: #2973347: EntityCreateAccessCheck should provide a useful error message when no bundle is specified.

Yay for slightly smaller patch!

While updating the IS for #80, I also noticed nobody ever replied to @Berdir's review/question in #29. Sorry, @Berdir! 😟

Of course, @Berdir being @Berdir (is that enough @Berdir for you in less than 3 sentences? 😀), he asks hard yet excellent questions. I even went back to the issue that introduced Dynamic Page Cache (#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)) to check what the outcome of the discussion there was.

#29:

I could be wrong, but I remember that we once discussed that it is by design that route access cacheability is *not* considered by dynamic page cache because dynamic page cache runs *after* route access checking.

But maybe I misunderstood something here.

To my surprise 😲 and fear 😱, this was NEVER DISCUSSED! Zero mentions of route access. #2429617-219: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) mentions it. But doesn't dive in deeper. So I instead looked at every occurrence of access. Again nothing mentioned.

But I think @Berdir is right. It's just unfortunate that despite Dynamic Page Cache having such detailed test coverage, this is not something that is explicitly tested. (If it were, then the change made to \Drupal\Core\EventSubscriber\RouteAccessResponseSubscriber::getSubscribedEvents() should trigger a test failure.)

And I think the reason this is only coming up now and not months or years ago, is that the REST module's responses are atypical. Unlike pretty much all other responses, the controller in question (\Drupal\rest\Plugin\rest\resource\EntityResource::get()) has been:

1. Doing its own route/response-level access checking.
2. Significantly varying the contents of the response based on that access checking.

(The closest analogies are block and entity rendering, but those are not route/response-level, they're intra-response-level.)

So, given that REST was doing something exceptional and this is a change that merits broader discussion … let's revert those changes. And let's instead let EntityResource::get() continue to do what it does today wrt cacheability, but not wrt access checking. This minimizes change.

Created #2973356: Evaluate making DynamicPageCacheSubscriber::onResponse() run after RouteAccessResponseSubscriber::onRespond(), to take into account route access cacheability for evaluating this.

(IS already up-to-date, I wrote it with this in mind.)

Ha, that's a very dramatic comment ;)

I'll comment over there.

The updated IS makes this a lot easier to grok. Great work @Wim Leers!

Thanks @borisson_!

Nice: #2973347: EntityCreateAccessCheck should provide a useful error message when no bundle is specified already landed, meaning that nobody else in >=8.6 should need to debug for a long while to figure out why/how _entity_create_access is not working for them :)

Opened a sibling issue for JSON API: #2973784: JSON API should check entity access during routing, not in controller, for the "individual" route.

Unrelated failure.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -137,6 +152,7 @@ public static function getSubscribedEvents() {
        $events[KernelEvents::EXCEPTION][] = ['onExceptionSendChallenge', 75];
   +    $events[KernelEvents::EXCEPTION][] = ['onExceptionAccessDenied', 75];
   

   Are we sure we want two events with exactly the same weight? I think that needs a comment as to why. Also I think onExceptionSendChallenge should always come before onExceptionAccessDenied - so can we make the new listener on 80 or thereabouts?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteAccessResponseSubscriber.php
   @@ -50,7 +50,7 @@ public function onRespond(FilterResponseEvent $event) {
        // Priority 10, so that it runs before FinishResponseSubscriber, which will
        // expose the cacheability metadata in the form of headers.
   -    $events[KernelEvents::RESPONSE][] = ['onRespond', 10];
   +    $events[KernelEvents::RESPONSE][] = ['onRespond', 110];
   

   The comment is being made out-of-date by the patch.

3. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -97,7 +97,9 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
   -        $route->setRequirement('_csrf_request_header_token', 'TRUE');
   +        if (!in_array($method, ['GET', 'HEAD'], TRUE)) {
   +          $route->setRequirement('_csrf_request_header_token', 'TRUE');
   +        }
   

   Why are we making this change? Ah I see this is described in the issue summary. It feels like a comment belongs here. Also looking at \Drupal\Core\Access\CsrfRequestHeaderAccessCheck::applies() it looks like this list should be ['GET', 'HEAD', 'OPTIONS', 'TRACE']. I wish we had a version of \Symfony\Component\HttpFoundation\Request::isMethodSafe(), ::isMethodIdempotent(), ::isMethodCacheable() where we didn't need a request first.

1. Done.
2. This change is no longer present in the latest patches.
3. I share that wish! This also made me realize that rather than requiring routes to only set this requirement when necessary, we can just update \Drupal\Core\Access\CsrfRequestHeaderAccessCheck::access() to return a cacheable AccessResult::allowed() when just the HTTP method already shows CSRF checking is unnecessary. It's only when the user is authenticated and there is a session cookie on the request that we need max-age = 0.

1. +++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
   @@ -89,12 +89,16 @@ public function applies(Route $route) {
   +    // Read-only operations are always allowed.
   +    if (in_array($method, ['GET', 'HEAD', 'OPTIONS', 'TRACE'])) {
   +      return AccessResult::allowed();
   +    }
   

   Let's use a strict in_array here, you never know :(

2. +++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
   @@ -89,12 +89,16 @@ public function applies(Route $route) {
        // 1. this is a write operation
   ...
   -    if (!in_array($method, ['GET', 'HEAD', 'OPTIONS', 'TRACE'])
   -      && $account->isAuthenticated()
   +    if ($account->isAuthenticated()
          && $this->sessionConfiguration->hasSession($request)
        ) {
   

   This bit of the comment seems to no longer apply, maybe need to move this documentation up.

I tried to write a test in the routing tests for the added 403 cacheability in core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php:49. Sadly \Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod makes that impossible right now. I am wondering though whether it is "enough" to test it just in REST itself.
@Wim Leers Maybe you have an opinion about it?

#93:

1. 👍
2. 👍

Rebased. Zero changes compared to #93.

I tried to write a test in the routing tests for the added 403 cacheability in core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php:49. Sadly \Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod makes that impossible right now. I am wondering though whether it is "enough" to test it just in REST itself.
@Wim Leers Maybe you have an opinion about it?

What about this? :)

@Wim Leers
Good idea!

+++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
@@ -337,8 +337,8 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
+      case 'DELETE':
+        return '';

This change is surprising and I think needs a comment

+++ /dev/null
@@ -1,28 +0,0 @@
-trait BasicAuthResourceWithInterfaceTranslationTestTrait {

Let's deprecate this rather than delete - something in contrib or custom could be using it - no? Yeah this went into 8.5.0 so we have to remove it more carefully.

#101

• ✔️
• I could leave it and deprecate it, but any test using it will fail. See #68's interdiff and comment — comment repeated here verbatim:
  

  This round is even simpler: the BasicAuthResourceWithInterfaceTranslationTestTrait alternative to BasicAuthResourceTestTrait that was added in #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage to accommodate the ConfigurableLanguage and ContentLanguageSettings config entity types' testing is no longer necessary! 🎉

  (Because access checking is now happening earlier.)

  IOW: only EntityResourceTestBase subclasses that installed the language module needed it. That does mean that in hindsight, BasicAuthResourceWithInterfaceTranslationTestTrait should have been marked @internal. But breaking risk is extremely small. Which entity types require the language module to installed?

Random (infra?) fail in Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest.

#2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used. caused this to fail tests.

Also, this is now against 8.7, and 8.7 changed some things. So this patch needs to adapt (some changes introduced in #65 need to be reverted + use ->isMasterRequest() now). Interdiff provided.

Hoping this passes tests again! 🤞

#2981887: Add a publishing status to taxonomy terms also caused this to fail tests, but that didn't show up yet in #102, because it was committed a day later.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -120,15 +121,11 @@ public static function create(ContainerInterface $container, array $configuratio
   -    $entity_access = $entity->access('view', NULL, TRUE);
   -    if (!$entity_access->isAllowed()) {
   -      throw new CacheableAccessDeniedHttpException($entity_access, $entity_access->getReason() ?: $this->generateFallbackAccessDeniedMessage($entity, 'view'));
   -    }
   

   So what happens if someone has extended from this controller. I realise controllers aren't included in the API. But in theory someone might do it. They would also need to update their routing to avoid a security issue?

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -120,15 +121,11 @@ public static function create(ContainerInterface $container, array $configuratio
   +    // @todo Either remove the line below or remove this todo in https://www.drupal.org/project/drupal/issues/2973356
   

   i think we need to wrap here, can be fixed on commit

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -375,6 +364,19 @@ public function permissions() {
   +    switch ($method) {
   +      case 'GET':
   +        $route->setRequirement('_entity_access', $this->entityType->id() . '.view');
   

   Re #1 - i guess we hope they also call this parent method?

4. +++ /dev/null
   @@ -1,28 +0,0 @@
   -trait BasicAuthResourceWithInterfaceTranslationTestTrait {
   -
   -  use BasicAuthResourceTestTrait;
   

   do we need to keep this around and deprecate it for BC?

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -379,6 +379,20 @@ protected function getExpectedUnauthorizedAccessCacheability() {
   +  protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) {
   

   note to self, not an api break because 1:1 rule

6. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -64,11 +65,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          $access_result->setReason("Users can only update their own account, unless they have the 'administer users' permission.");
   

   nice

1. Note this (EntityResource::get()) is not a controller, but a @RestResource plugin. \Drupal\rest\RequestHandler::handle() is a controller. That's the controller that eventually invokes a @RestResource plugin method (get(), post(), patch() or delete()).

   It is somewhat conceivable that somebody would provide a different implementation for a particular entity type. They'd need to implement hook_rest_resource_alter() and override the class for the @RestResource plugin derivative for that particular entity type. Like the example in rest.api.php shows, something like:

   function hook_rest_resource_alter(&$definitions) {
     if (isset($definitions['entity:node'])) {
       // We want to handle REST requests regarding nodes with our own plugin
       // class.
       $definitions['entity:node']['class'] = 'Drupal\mymodule\Plugin\rest\resource\NodeResource';
     }
   }
   

   Such as class may very well override the get() method of the parent class. But if they do that, then the route definitions would still get the _entity_access route requirements. And even if they go so far as overriding getBaseRoute() (where that route requirement is added), they'd still be wanting to call parent::getBaseRoute() first. If they don't, they would already be opting out of everything that the REST module provides for them.

   In other words: overrides are unlikely, but if they happen, then unless it's implemented extremely poorly, they'll still get the same access control as is being specified in this patch!

2. Ok.
3. See 1.
4. @alexpott also asked this 3 months ago in #101, I already explained why we want to do it this way in #102.
5. 👍
6. 🙂

1. Needs a reroll. The patch looks great - all the committer concerns have been answered - the reroll could address #110.2 too.

2. +++ b/core/modules/block/src/BlockAccessControlHandler.php
   @@ -136,7 +136,10 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   -        $access = AccessResult::forbidden();
   +        $reason = count($conditions) > 1
   +          ? "One of the block visibility conditions ('%s') denied access."
   +          : "The block visibility condition '%s' denied access.";
   +        $access = AccessResult::forbidden(sprintf($reason, implode("', '", array_keys($conditions))));
   
   +++ b/core/modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php
   @@ -135,7 +136,7 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
   -        return "You are not authorized to view this block entity.";
   +        return "The block visibility condition 'user_role' denied access.";
   

   <3 AccessResultReasonInterface FTW

Adding issue credit for @larowlan, @tstoeckler, @Berdir and myself for comments that influenced the patch.

YAY! :D

Reroll entirely automatic thanks to git ❤️q

I think we should have a change record and a release notes section to detail the change here - ie. that rest routes now have the _entity_access requirement automatically added and that this should not affect any runtime code in a negative way.

Also the issue summary says there are two behaviour changes and then proceeds to list one. I'm guessing that the missing one is the _entity_access route requirement - but that is just a guess.

Good points. Done!

1. Change record: https://www.drupal.org/node/3021135
2. Release notes section covering the route requirement change.
3. Fixed the "two behavior changes" bit: one of the two behavior changes was moved out of this issue in #82, into #2973347: EntityCreateAccessCheck should provide a useful error message when no bundle is specified which was committed 7 months ago today, but I forgot to update the sentence before the <ol> in the issue summary.

HTML mistake 😭

Committed 701be77 and pushed to 8.7.x. Thanks!

So the release note and CR here don't mention any disruptive changes; does this belong in the release notes? As a reminder release notes should only be for things that might have some disruptive impact on existing modules or sites.

The only possible disruption I'm getting from the IS is:
AccessAwareRouter now generates a cacheable 403 exception when the access result is cacheable, rather than always throwing an uncacheable one.

...which is only disruptive if your cache invalidation is wrong, I think.

So this probably should be 8.7 highlights instead I think!