REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error

Hi! Patches are peer-reviewed and this one lacks a test.

1. The analysis in #7 is wrong. HEAD is correct. You're making up your own interpretation of AccessResultInterface values. Read the code and its docs. And perhaps read the relevant change record (https://www.drupal.org/node/2337377).
2. Because of this, the patch in this issue introduces an access bypass security vulnerability.
3. Never self-RTBC patches, as @cilefen said in #20, you need peer review.
4. Every bug fix needs a test to prove that it's actually fixing what's perceived as a bug.
5. This is not remotely a critical bug. See https://www.drupal.org/core/issue-priority#critical-bug.

Are you perhaps new to open source?

Please post your rest.settings.yml.

Also marked #2772537: REST Views override existing REST GET routes as a duplicate for now, because it's not yet clear that this issue is a bug, let alone that we need yet another, almost-duplicate issue.

Thank you for your patch

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -90,7 +90,7 @@ public static function create(ContainerInterface $container, array $configuratio
-    if (!$entity_access->isAllowed()) {
+    if ($entity_access->isForbidden()) {

Note: \Drupal\Core\Access\AccessManager::check uses also isAllowed which is the canonical place for checking access on routes.

Let me also quote AccessResultInterface:

 * IMPORTANT NOTE: You have to call isAllowed() when you want to know whether
 * someone has access. Just using

. Given that I think the problem is rather the vocabulary access check.

If this is just about access checking on vocabularies, I also can't really believe this is critical, but feel free to disagree with it :)

i marked this as critical because without the taxonomy endpoints working, the REST service is not ready for production.

That may very well be, but it still doesn't meet the "critical" definition.

 'entity:taxonomy_vocabulary':
  …

Okay, great. You have enabled the taxonomy vocabulary REST resource. Looking at \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess(), you need the administer taxonomy permission to be allowed to GET Taxonomy Vocabulary config entities. So, does the user that's doing REST requests have that permission?

Note that vocabularies are config entities. Config entity GET support was added in Drupal 8.2, in #2724823: EntityResource: read-only (GET) support for configuration entities. This is not supported in Drupal 8.1.

Also note that you won't be able to PATCH/POST/DELETE vocabulary entities at all, for that we'll need #2300677: JSON:API POST/PATCH support for fully validatable config entities to happen first.

That being said, the error message you're seeing is extremely unhelpful. That's unacceptable. To fix that, we have #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response and #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response. But those only improve error messages for PATCH/POST/DELETE requests. I don't yet know what is causing this particular error. Investigating…

Cool, but the patch is definitely wrong. You'll need to debug this response, to figure out why it's getting a "neutral" access result and not an "allowed" one.

Enable xdebug and put a breakpoint in \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess(), that should lead you to the root cause.

Does this user have the administer taxonomy permission?

EDIT: to clarify: you say this in the IS:

Access GET on Taxonomy term resource
Access GET on Taxonomy vocabulary resource

But that's not enough. That's just the default 8.1.x way of allowing access to a REST resource in the first place (that's gone by default in 8.2). You then also need access to perform this operation on that entity: you need to be allowed to do this by $entity->access($operation) too!

so yes, it looks like administer taxonomy permissions is required for GET to work, which is technically “dangerous” if you are looking to only open READ ONLY / GET access.

Yes it is. But the solution then is to add additional permissions for viewing/updating/… Vocabulary. For now, there is not a separate "view vocabulary" permission.

imho, the fix here should be to respect the RESTful Web Services permissions over the administer taxonomy permissions, because we may not want to necessarily allow full admin access to the api user.

This would be at least equally dangerous, because it means the Entity Access API would need to be bypassed. Furthermore, in Drupal 8.2 and beyond, the restful get <entity type> permission is no longer enabled by default (existing sites will continue to have that, and new sites can opt in to it). Precisely because it's very confusing that there's a separate permission on top of the Entity Access API that must already grant you access anyway.

So, two follow-ups must be solved:

1. saner default access to Vocabulary entities: #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission (patch posted!)
2. better error responses, that tell you why you're getting a 403: #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out