Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources

So then why don't we only add permissions for non-entity resources?

I.e. why don't we allow the @RestResource annotation to opt out by specifying needs_permissions = FALSE?

But you first have to expose a certain REST resource in the first place. Per verb even.

On top of that, you must also use the authentication mechanism specified.

But isn't that an edge case that should be addressed by contrib? Right now, actually doing a single REST request requires so much non-obvious, Drupal-y steps.

If you only want to grant POST access to a special group of API users… then implement entity access hooks! That's what they're here for.

Sounds good!

Wow, look what I just found: @klausi at #1816354-1: Add a REST module, starting with DELETE:

Access control is also a problem right now, we don't have that in the routing system yet (see #1793520: Add access control mechanism for new router system for work on that). And we don't have something like entity_access() or access controllers in core (see #1696660: Add an entity access API for single entity access for that). I think I will just expose my own permissions right now ("read resourceXY over REST API", "create resourceXY over REST API" etc.) and check for that.

That is the issue that brought REST to core. It was not at all discussed further. No follow-up was created. In other words: very little scrutiny was applied to the REST module before it could be committed to Drupal 8. I'm certain such a patch would not have been able to land in the past few years (say, since 2014) — it looks like we were far more lax in 2012.

It's totally possible that access handling in general and permission handling in particular were discussed elsewhere, but if so, I could not find it (I looked at all these issues).

The only other issue that was remotely related is #1866908: Honor entity and field access control in REST Services. In there, two commits happened:
the first marked \Drupal\rest\Plugin\rest\resource\EntityResource::permissions()'s permissions as "restricted" (this method no longer exists), as a stopgap. The second commit removed that, and instead added proper entity access checking. But … it did in fact NOT remove the permissions for entities! It just kept them!

In other words, the only reason permissions still exist, at least for EntityResource, is simply that this was overlooked. No follow-up issue was created, and thus it was simply forgotten. It makes sense to allow @RestResource to opt in to having a permission, for example in the case of DbLogResource. But it does not make sense to always have this, without even the ability to opt out!

We (which includes dawehner, Crell, I, and others) also discussed this at DrupalCon New Orleans and unanimously agreed that:

1. it does not make sense for Drupal 8 to continue to have permissions enabled by default for EntityResource
2. we should not break BC
3. we should therefore make sure that newly installed Drupal 8.2 sites don't need to enable these permissions for EntityResource, but existing Drupal 8 sites will continue to use that

I therefore propose that :

1. we keep permissions enabled by default for @RestResource plugins: this guarantees BC for all custom/contrib REST resources
2. we remove permissions from EntityResource in Drupal 8.2.x by default, but keep them enabled for existing Drupal installations
3. we document A) why we have permissions, B) how you can opt out from permissions for a particular REST resource

Attached patch does all this.

How can a site break if a permission is just gone?

They may start to expose data that wasn't exposed before.

Yep, there is some risk, and having this BC flag set by default for existing sites means we avoid all that risk.

#12:

1. Happy to change it if need be, keeping as-is for now since you say this is fine.
2. Indeed somewhat strange. I mostly just find it strange that this lives on ResourceInterface.
3. Right, that makes much more sense than that comment :P

#17:

We could create a new base class BetterResourceBase or EightTwoResourceBase, which is also weird. And that's why you should not write base classes for plugins and always use traits instead. ResourceTrait would have been the way to go.

Indeed!

#18: also true. Done!

Next reroll will address #12.3.

Wow, this took ages to debug. The reason for the test failures in PageCacheTest::testQueryParameterFormatRequests()… is that the current patch allows the routes for EntityResource to not have any requirements. And without requirements, no access checks. Without access checks, our routing system assumes you forgot to specify access checks, and hence helpfully denies access. That's what's happening.

The fix is trivial, if somewhat confusing.

This removes the stopgap that #11 added to allow all RESTTestBase-based tests to still pass. In other words: it updates the tests to test the new reality. We have one test that verifies the old behavior still works.

So, this fixes #12.3 because that code is now gone, but we still need upgrade path tests.

Finally, this will have a fail in AuthTest. Analysis for that to follow.

Also fails in ResponseGeneratorTest, but that's not something tricky, it's just something I overlooked. Fixing that first, then addressing AuthTest.

This should now have a single fail, in AuthTest.

#24 had a fail in AuthTest, because:

BC route requirements:

requirements:
  _access: 'TRUE'
  _permission: 'restful entity:node GET'

Non-BC route requirements:

requirements:
  _access: 'TRUE'

In the BC case, AuthTest will throw a 403 HTTP exception because the anonymous user does not have the permission, which \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onExceptionSendChallenge() catches, which in turn causes \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException() to be called, which converts that into a 401 exception.

In the non-BC case, AuthTest will NOT throw a 403 HTTP exception because the anonymous user is not denied access, since the permission is no longer required. However, the anonymous user still cannot access the entity, because EntityResource::get() calls $entity_access = $entity->access('view', NULL, TRUE);, which calls \Drupal\entity_test\EntityTestAccessControlHandler::checkAccess(), which denies access, which results in EntityResource::get() throwing a 403 HTTP exception. Sounds like we're good? No, we're not! Because this time, the 403 exception does not bubble up to the HTTP kernel level, but is caught much earlier, in RequestHandler::handle(): since #1865594: Better REST error messages in case of exceptions, we catch any HTTP exceptions there and generate a response with an error message. Which means we still send a 403 response, but not a 403 exception. Since there is no 403 HTTP exception, the BasicAuth authentication provider (called by the HTTP kernel) also can't convert it into a 401 exception, which means … the anonymous user gets to access our response without being authenticated!!!

There are three two possible solutions:

1. remove the "better error messages" stuff that #1865594: Better REST error messages in case of exceptions added, but then we lose our helpful error messages
2. make the "better error messages" stuff that [#2865594] added more specific: either only convert 400 and 422, or convert everything except 403
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.

P.S.: this is only somewhat related, but I want to point out that this seems like a very fragile design: we don't actually require authentication: _auth is a route option, not a route requirement. We only send a 401 if a 403 happens to occur, and that 403 is actually an unhandled exception: a 403 response won't be converted into a 401. This weakness was introduced in #1865594: Better REST error messages in case of exceptions.

Forgot the interdiff for rest_permissions-2664780-28-3.patch.

This also needs a change record. Created one: https://www.drupal.org/node/2733435.

Also updated https://www.drupal.org/developing/api/8/rest, changes: https://www.drupal.org/node/2667736/revisions/view/9386550/9713245.

So, running with option #28.2.b. Added documentation.

Interdiff relative to #26.

#23:

In an ideal world we would move this requirement building into its own subclass, so entityBase could simply override the behaviour. Here we just add more complexity to the already existing method.

I disagree. The whole point of doing this in ResourceBase rather than in EntityResource is to allow any resource plugin that extends ResourceBase to override permissions() to not specify any permissions, and then have it work.

The alternative is to specifically document ResourceBase to only be used for resources that need to have permissions generated, that don't have their own access control mechanism. But doing so would require EntityResource to no longer extend ResourceBase, which means it'd need to duplicate all of the complexity in ResourceBase::routes().

Given that ::permissions() is in fact defined by the ResourceInterface, and thus every REST resource plugin must implement it, it seems more appropriate to let ResourceBase::getBaseRoute() be smart enough to deal with a permissions() implementation that simply returns no permissions. And that's exactly what that code that you cited does.

After fighting my way through many of the WTFs that one encounters when writing an upgrade test, here is what I believe to be a very solid update test.

IMHO this patch is ready.

Updated IS to summarize the entire issue.

Thanks for the review!

#36: I think we should keep it as it is, precisely to make it the least dynamic possible. Every developer should learn that all access checks must grant access to be allowed access a route. Hopefully, if a developer sees both, they'll learn that. Furthermore, if they see those two requirements, then they can also see the route name. Given the route name, they will surely find their way back here. And then they'll find the comment documenting this. That's exactly why I have that comment in the first place: to make sure all developers implementing or debugging @RestResource plugins based on ResourceBase are explicitly aware of this.

#37: Ohhh! That's very interesting :) But it also makes the BC-vs-new behavior far more difficult to understand: all new sites have it, none of the existing sites, EXCEPT …. Also, however much I like your very clever idea, I'm afraid we cannot actually rely on it: users with the administrator role are automatically granted every permission, yet the user.role.administrator configuration lists no permissions at all. So, it's perfectly possible that a site actually is relying on permission-based REST access, even if no EntityResource permissions are listed explicitly!
So, AFAICT your suggestion is:

1. undesirable for clarity
2. impossible due to roles having is_admin not listing which permissions they have

Oh, a helper method. that is totally reasonable.

Done. Extracted that logic from getBaseRoute() into getBaseRouteRequirements().

Hah, #2626298: REST module must cache only responses to GET requests just landed, causing this to fail. Rebased. One tiny conflict to resolve.

#2626298: REST module must cache only responses to GET requests also expanded some test coverage in a non-conflicting way, which explains the fail in #43. Fixed.

#46:

[…] _entity_access […]

No, that doesn't work. I tried! See #28, option 3 for the details + patch.

Would it make more sense to catch AccessDeniedHttpExceptions separately?

I'd love to! But REST has lots of throw new HttpException(STATUSCODE, …), which means it's more likely than not that contrib plugins will do the same. Which means we can't 100% rely on catching that particular exception. If you think we can, that'd be fine for me.

Arbitrary code could still execute so we want to have a special behaviour for it.

Exactly!

(Thanks for the pointer though, I should have found that myself.)

So … what's left here?

Yep, one of the recent commits introduced another usage of this permission. I'll fix this.

Confirmed, #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it introduced another use of the permission that this patch now disables by default. Simply removing that permission should do the trick.

So, it does the trick in that it solves the fail we saw above, but … then we start to see new fails (i.e. this patch will fail), triggered by this hunk in the patch:

+++ b/core/modules/rest/src/RequestHandler.php
@@ -91,6 +91,16 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+      // In case a response is forbidden for the current user, we must not
+      // convert it into a response, because that would rob
+      // \Drupal\Core\EventSubscriber\AuthenticationSubscriber::onExceptionSendChallenge()
+      // from the opportunity to convert it into a 401 response, to challenge
+      // the user to authenticate.
+      // @see \Drupal\Core\Authentication\AuthenticationProviderChallengeInterface
+      if ($e->getStatusCode() == 403) {
+        throw $e;
+      }
+
       $error['error'] = $e->getMessage();
       $content = $serializer->serialize($error, $format);
       // Add the default content type, but only if the headers from the

Those additions mean that a 403 exception no longer gets such an {error: "ERROR MESSAGE"} response. Which was fine so far, but #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it added test coverage for that. And so now it turns out this solution isn't good enough anymore.

This is just yet another example for #2737751: Refactor REST routing to address its brittleness. :(

Clearly, the real problem is that this catching of all exceptions in the controller and transforming it into a response is something that:

1. works against Symfony's design, where you should really use a subscriber to the KernelEvents::EXCEPTION event to address this
2. works against Drupal's design, which builds on top of the aforementioned Symfony design, and provides a base class: \Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase

So, let's get rid of all that logic in RequestHandler and instead use that established pattern. That means we need a subclass of HttpExceptionSubscriberBase that can handle all serialization formats. The RTBC patch at #2403307: RPC endpoints for user authentication: log in, check login status, log out already has that, so we can import that from there (and when that lands, we can remove it from this patch).

That's what this patch does.

So, we made progress in #54, but we're still far from where we need to be… there's still lots of fails :/

The first problem is that we're now getting error (HTTP 4xx) responses in HTML, not in JSON/HAL+JSON/… Why? Because none of the PATCH or POST tests actually specify ?_format=SOME_FORMAT in the URL. Which means we don't really request any format, and hence \Drupal\Core\StackMiddleware\NegotiationMiddleware::getContentType() defaults to html.

Possible solutions:

1. Add ?_format=SOME_FORMAT to all POST and PATCH requests.
2. Do #2662284-33: Return complete entity after successful PATCH, -35, -36, but that would be hugely out of scope.

So, I went with solution 1.

After the above is solved, the second problem is that in HEAD, the error message lives in an error key, but as of #55, it lives in a message key. Is this a BC break? I don't think so: it's only intended for developers anyway. So I went with that for now.
EDIT: Alternatively, we could change the new exception subscriber to put the message at 'error' instead of at 'message'.

And a third problem is that HttpExceptionSubscriberBase itself is poorly designed: it requires every HTTP status to have its own callback, and it's missing the on422() callback by default. So, added that on422() method.

Thanks for opening #2739617: Make it easier to write on4xx() exception subscribers!

Does that mean we commit this issue after the issue on #2662284: Return complete entity after successful PATCH and call it a day?

Yes! Committing this patch after that one will make this patch a fair bit simpler. I just went with option 1 for now, to prove that this patch can in fact be green.

And in fact, if #2403307: RPC endpoints for user authentication: log in, check login status, log out lands first, we'll also be able to remove most of what #55 changed.

So, let's postpone this on both #2662284 and #2403307.

#2662284: Return complete entity after successful PATCH landed, so one less blocker!

I agree. But that's due to the unfortunate state that the rest.module codebase is in. The fix in #55 (see the interdiff) is about 20% of the entire patch size. There's been many more eyeballs on #2403307: RPC endpoints for user authentication: log in, check login status, log out, so I'd rather see that hunk landing there than here.

Furthermore, #2403307: RPC endpoints for user authentication: log in, check login status, log out has been ready for almost 2 months at this point, so I expected it to be committed a long time ago. Hopefully any day now.

The changes made in #55 actually aren't being introduced in #2403307: RPC endpoints for user authentication: log in, check login status, log out anymore, they already were introduced in #2308745: Remove rest.settings.yml, use rest_resource config entities! So, no need for this to be postponed anymore.

Rerolling…

rest_update_8201() now exists already, so renamed the update function in this patch to rest_update_8203() (this is what the interdiff shows). Updated the patch + IS accordingly. The CR is still accurate.

After #59 was rolled, #2308745: Remove rest.settings.yml, use rest_resource config entities landed, which added a administer rest resources permission. Hence the assumption that was made in this issue/patch that \Drupal\rest\Tests\Update\EntityResourcePermissionsUpdateTest makes about REST module having no permissions by default is no longer true.

This reroll/interdiff makes the minor changes necessary to no longer have that assumption, so this patch should come back green.

(I also noticed that the test method name was silly/wrong, it was a copy/paste remnant. Fixed that too.)

but it is also an important patch because the last piece of a jigsaw that we want to complete in 8.2.0.

Nice way of putting it! :)

Conflicted with #2403307: RPC endpoints for user authentication: log in, check login status, log out, trivial resolution.

(Rebased from #67, because #72 lost the upgrade path test.)

there's no coverage of whether or not when this layer is on the permissions work

This fixes that.

there's no test coverage that the old permissions still exist when this layer is on

Actually, we do have test coverage for that, in \Drupal\rest\Tests\Update\EntityResourcePermissionsUpdateTest::testBcEntityResourcePermissionSettingAdded():

    $this->runUpdates();

    // Make sure we have the expected values after the update.
    $rest_settings = $this->config('rest.settings');
    $this->assertTrue(array_key_exists('bc_entity_resource_permissions', $rest_settings->getRawData()));
    $this->assertTrue($rest_settings->get('bc_entity_resource_permissions'));
    $rest_permissions = array_keys(array_filter($permission_handler->getPermissions(), $is_rest_resource_permission));
    $this->assertEqual(['restful delete entity:node', 'restful get entity:node', 'restful patch entity:node', 'restful post entity:node'], $rest_permissions);

So, AFAICT we were really just missing that one bit?

Woot!

Thanks :)