REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out

Yep, absolutely! But we already do that. At least for AccessResult::andIf(). We still need to do that for ::orIf(). This issue will probably need to add that.

Further confirmation we need this: #2810603: PATCH taxonomy term.

Awesome, thanks @gnuget! I'll provide reviews :)

The test result is a HTML response. That's suspicious.

There are three possible explanations that I can think of:

1. You don't have verbose error logging on during tests. But that'd be hard to do actually, since \Drupal\Tests\BrowserTestBase::installDrupal() enables verbose error logging by default.
2. You're somehow failing to get HTML because you're failing to specify a correct ?_format. In fact, I see you have ?_format=hal+json in the curl request. Which is wrong, it must be ?_format=hal_json.
3. You must specify an Accept header in addition to the ?_format query string. I've seen a similar problem in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

Hope this helps unblock you!

#16: I think it may be better to postpone this issue on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method then, because that issue is introducing all-new test coverage. Or, rather, spend minimal effort on updating the existing tests, instead just ensure that all 403 responses return useful information. So then when #2737719 lands, this issue will just have to update the existing test coverage :)

#17: Yes, exactly! That's definitely intended to be solved by this issue as well.

Ok, then I'm marking this postponed for now. Thanks for your help so far!

An issue was opened precisely about the lacking information in 403 responses: #2831251: Trying to create nodes via REST with ajax POST request returns 403 response.

#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed!

1. +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php
   @@ -31,7 +31,8 @@ protected function getAuthenticationRequestOptions($method) {
   +    // 401 errors always have the same message for BasicAuth
        $this->assertResourceErrorResponse(401, 'No authentication credentials provided.', $response);
   

   It's hardcoded. So that's clear. No need for this comment.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -332,7 +333,7 @@ public function testGet() {
   -      $this->assertResponseWhenMissingAuthentication($response);
   +      $this->assertResponseWhenMissingAuthentication($response, 'The current user does not have access to view the requested entity.');
   

   This is confusing "authentication" with "authorization". The message being added here is related to authorization, not authentication.

   We need to update CookieResourceTestTrait::assertResponseWhenMissingAuthentication() to not assert the response body.

   Only \Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait::assertResponseWhenMissingAuthentication should assert the response body, because it has its own authentication-related error message.

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -180,7 +180,7 @@ protected function provisionResource($resource_type, $formats = [], $authenticat
   +  abstract protected function assertResponseWhenMissingAuthentication(ResponseInterface $response, $message = '');
   

   Hence we don't even need to add this $message parameter.

4. I wasn't sure why RoleJsonBasicAuthTest wasn't using JsonBasicAuthWorkaroundFor2805281Trait so updated that b/c it deals with the message.

   That was probably just an oversight, yes.

#29: yay, thanks for picking this up again! :)

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -96,7 +96,7 @@ public function onKernelRequestFilterProvider(GetResponseEvent $event) {
   +        throw new AccessDeniedHttpException('Access denied in this route.');
   

   The used authentication method is not allowed on this route.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -106,7 +106,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +      throw new AccessDeniedHttpException('The current user does not have access to view the entity type {$entity->getEntityTypeId()} of bundle {$entity->bundle()}.');
   
   @@ -145,7 +145,7 @@ public function post(EntityInterface $entity = NULL) {
   +      throw new AccessDeniedHttpException("Entity creation for entity type {$entity->getEntityTypeId()} of bundle {$entity->bundle()} is not allowed for the current user.");
   
   @@ -200,7 +200,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      throw new AccessDeniedHttpException("Entity update for entity type {$entity->getEntityTypeId()} of bundle {$entity->bundle()} is not allowed for the current user.");
   
   @@ -264,7 +264,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      throw new AccessDeniedHttpException("Entity deletion for entity type {$entity->getEntityTypeId()} of bundle {$entity->bundle()} is not allowed for the current user.");
   

   These are inconsistent. Let's change them to:

   You are not authorized to (view|update|create|delete) this {$entity->getEntityTypeId()} entity of bundle {$entity->bundle()}.
   

3. +++ b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php
   @@ -24,8 +24,8 @@
   -  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
   -    throw new \LogicException('When testing for anonymous users, authentication cannot be missing.');
   +  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response, $message = 'When testing for anonymous users, authentication cannot be missing.') {
   +    throw new \LogicException($message);
   
   +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php
   @@ -31,7 +31,7 @@ protected function getAuthenticationRequestOptions($method) {
   -  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
   +  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response, $message = '') {
   
   +++ b/core/modules/rest/tests/src/Functional/JsonBasicAuthWorkaroundFor2805281Trait.php
   @@ -13,7 +13,7 @@
   -  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
   +  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response, $message = '') {
   

   These changes can be reverted.

4. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
   @@ -91,8 +91,8 @@ protected function getAuthenticationRequestOptions($method) {
   +  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response, $message = '') {
   +    $this->assertResourceErrorResponse(403, $message, $response);
   

   What I said in #27.2 is still accurate:

   We need to update CookieResourceTestTrait::assertResponseWhenMissingAuthentication() to not assert the response body.

   In other words: the second parameter must be FALSE, not $message.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -301,6 +301,7 @@ public function testGet() {
   +      // @todo Update message https://www.drupal.org/node/2808233
          $this->assertResourceErrorResponse(403, '', $response);
   

   Surely this can be addressed here now?

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -341,7 +342,7 @@ public function testGet() {
        // @todo Update the message in https://www.drupal.org/node/2808233.
   -    $this->assertResourceErrorResponse(403, '', $response);
   +    $this->assertResourceErrorResponse(403, 'The current user does not have access to view the requested entity.', $response);
   

   The @todo is now done and hence can be removed!

   But the expected message is incorrect; it doesn't match the actual message we will get.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -545,7 +546,11 @@ public function testPost() {
   -      $this->assertResponseWhenMissingAuthentication($response);
   ...
   +      }
   
   @@ -554,8 +559,10 @@ public function testPost() {
   -    $this->assertResourceErrorResponse(403, '', $response);
   +    if ($bundle = $this->entity->bundle()) {
   +      // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
   +      $this->assertResourceErrorResponse(403, "Entity creation for entity type {$this->entity->getEntityTypeId()} of bundle $bundle is not allowed for the current user.", $response);
   +    }
   
   @@ -745,7 +752,10 @@ public function testPatch() {
   -      $this->assertResponseWhenMissingAuthentication($response);
   +      if ($bundle = $this->entity->bundle()) {
   +        // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
   +        $this->assertResponseWhenMissingAuthentication($response, "Entity update for entity type {$this->entity->getEntityTypeId()} of bundle $bundle is not allowed for the current user.");
   +      }
   
   @@ -754,8 +764,10 @@ public function testPatch() {
   -    $this->assertResourceErrorResponse(403, '', $response);
   +    if ($bundle = $this->entity->bundle()) {
   +      // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
   +      $this->assertResourceErrorResponse(403, "Entity update for entity type {$this->entity->getEntityTypeId()} of bundle $bundle is not allowed for the current user.", $response);
   +    }
   
   @@ -902,7 +914,11 @@ public function testDelete() {
   -      $this->assertResponseWhenMissingAuthentication($response);
   +      if ($bundle = $this->entity->bundle()) {
   +        // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
   +        $this->assertResponseWhenMissingAuthentication($response, "Entity deletion for entity type {$this->entity->getEntityTypeId()} of bundle $bundle is not allowed for the current user.");
   +      }
   
   @@ -911,8 +927,11 @@ public function testDelete() {
   -    $this->assertResourceErrorResponse(403, '', $response);
   +    if ($bundle = $this->entity->bundle()) {
   +      // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
   +      $this->assertResourceErrorResponse(403, "Entity deletion for entity type {$this->entity->getEntityTypeId()} of bundle $bundle is not allowed for the current user.", $response);
   +    }
   

   These changes are wrong. They remove the assertion completely when an entity type has no bundle. Let's revert these changes.

Looks great, thanks! :) Eagerly awaiting testbot results…

Some of those failures are because you didn't update CookieResourceTestTrait yet.

You need to change it from

  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
    $this->assertResourceErrorResponse(403, '', $response);
  }

to

  protected function assertResponseWhenMissingAuthentication(ResponseInterface $response) {
    $this->assertResourceErrorResponse(403, FALSE, $response);
  }

• #55.1++
• #55.2: let's leave a comment. Something along the lines of // Requests needing cookie authentication but missing it results in a 403 response. The cookie authentication mechanism sets no response message. (Contrast this with Basic Auth, which sends a 401 response, and a particular message.)

For #55 and later.

This is getting very close now! :) Great work, @gnuget! I have mostly nits:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -96,7 +96,7 @@ public function onKernelRequestFilterProvider(GetResponseEvent $event) {
   +        throw new AccessDeniedHttpException('The used authentication method is not allowed on this route.');
   

   This has no test coverage. Let's update \Drupal\basic_auth\Tests\Authentication\BasicAuthTest to test this.

   Or alternatively (which would be even better), is if you would implement a custom authentication provider and trigger that one from EntityResourceTest, and then verify you get a 403 response with this message.

   A test auth provider would look like this:

   /**
    * Authentication provider for testing purposes.
    */
   class TestAuth implements AuthenticationProviderInterface {
   
     /**
      * {@inheritdoc}
      */
     public function applies(Request $request) {
       return $request->headers->has('test_authentication_provider');
     }
   
     /**
      * {@inheritdoc}
      */
     public function authenticate(Request $request) {
       return NULL;
     }
   
   }
   

   i.e. sending a test_authentication_provider request header would trigger this authentication provider to apply, but it wouldn't apply to the routed request, hence triggering this exception.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -106,7 +106,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, "view"));
   
   @@ -145,7 +145,7 @@ public function post(EntityInterface $entity = NULL) {
   +      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, "create"));
   
   @@ -200,7 +200,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, "update"));
   
   @@ -264,7 +264,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, "delete"));
   

   Supernit: single quotes instead of double quotes, for consistency with the other code there.

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -278,6 +278,27 @@ public function delete(EntityInterface $entity) {
      }
    
   +
   +  /**
   

   Supernit: two newlines should be one.

4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -278,6 +278,27 @@ public function delete(EntityInterface $entity) {
   +   * Return the proper message checking if the entity is bundleable.
   

   This comment is wrong.

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -278,6 +278,27 @@ public function delete(EntityInterface $entity) {
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity object.
   +   *
   +   * @param string
   +   *   The action executed before to call the exception.
   +   * @return string
   +   *   The proper message to display in the AccessDeniedHttpException.
   

   There should be no newline between the two @params, there should be one before the @return.

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -278,6 +278,27 @@ public function delete(EntityInterface $entity) {
   +  public function accessDeniedExceptionMessage(EntityInterface $entity, $action) {
   

   s/$action/$operation/

   That's consistent with \Drupal\Core\Entity\EntityAccessControlHandlerInterface::access.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -401,7 +400,8 @@ public function testGet() {
   +    // In case we have a BC layer, permissions are used, which don't have a good
   
   @@ -831,7 +830,8 @@ public function testPatch() {
   +    // In case we have a BC layer, permissions are used, which don't have a good
   
   @@ -931,7 +930,8 @@ public function testDelete() {
   +    // In case we have a BC layer, permissions are used, which don't have a good
   

   s/In case we have a BC layer,/When using the bc_entity_resource_permissions BC layer,/

Keeping the Needs tests tag for the first remark.

#63: huh? The patch before #62 always had the bundle in the message, even if it matched the entity type ID (i.e. even entity types without bundles return something for bundle: they return the entity type ID, see \Drupal\Core\Entity\Entity::bundle()). And since #62, we have more accurate messages. So, I think all is fine? Correct me if I'm wrong :)

After posting #65, I went back to the issue summary to read what I originally wrote. I wrote this in the remaining tasks:

1. Also let AccessResultNeutral implement \Drupal\Core\Access\AccessResultReasonInterface.
2. Ensure it's done for all permission checking, and \Drupal\Core\Access\AccessResult::allowedIfHasPermission(s)() in particular.

Neither of those have been done yet. Which is why the patch has this:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -624,7 +623,8 @@ public function testPost() {
+    // In case we have a BC layer, permissions are used, which don't have a good
+    // error message.
     $this->assertResourceErrorResponse(403, '', $response);

in several places.

It's also why we have

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -106,7 +106,7 @@ public static function create(ContainerInterface $container, array $configuratio
   public function get(EntityInterface $entity) {
     $entity_access = $entity->access('view', NULL, TRUE);
     if (!$entity_access->isAllowed()) {
-      throw new AccessDeniedHttpException();
+      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, "view"));

this hardcoding a certain message. Which is missing the point. It's merely giving a message, but it really doesn't help the end user in a sufficient way: the point of this issue was that the error messages should say exactly what is missing, when possible.

This message would be fine as a default. But what we really want, is that we check if $entity_access has a reason $entity_access->getReason() (see \Drupal\Core\Access\AccessResultForbidden::getReason() + \Drupal\Core\Access\AccessResultReasonInterface).

That reason should be set automatically.

So, for example, for Vocabulary config entities, what determines access is EntityAccessControlHandler::checkAccess(), which uses Vocabulary's admin_permission (administer taxonomy)
to determine whether access must be granted or not:

    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());
    }

In other words, AccessResult::allowedIfHasPermission() must be modified to call setReason() if access is not allowed. It can set a sensible reason automatically, because it knows which permission is necessary.

In other words, what should happen here is:

1. Modify AccessResultNeutral to implement \Drupal\Core\Access\AccessResultReasonInterface
2. Update \Drupal\Core\Access\AccessResult::allowedIfHasPermission() to set a sensible reason when access is not allowed.
3. Have EntityResource::get() etc. check if a reason is set and use that. So: throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->accessDeniedExceptionMessage($entity, $operation).

That way, you get what you have built so far if no reason is set, and otherwise you get a more helpful, more detailed reason.

IMHO we should get these changes done in its own issue first, and ensure we have good unit test coverage for those.

Once it's ready, yes. But let's first prove here that it actually helps solve the problem that this issue wants to solve :)

But, yes, once this is looking good, I agree it'd be good to land that separately. Although we also didn't do that for #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response (which added this to AccessResultForbidden).

First, let's get this patch to green again. The most common failure is Failed asserting that 500 is identical to 403.. On my local machine (on PHP 7), I get a more helpful error message:

Exception: Error: Call to a member function isAllowed() on boolean
Drupal\rest\Plugin\rest\resource\EntityResource->post()() (Line: 148)

The problem lies here:

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -144,9 +144,11 @@ public function post(EntityInterface $entity = NULL) {
-    if (!$entity->access('create')) {
-      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, 'create'));
+    $entity_access = $entity->access('create');
+    if (!$entity_access->isAllowed()) {

@@ -199,8 +201,10 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-    if (!$original_entity->access('update')) {
-      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, 'update'));
+
+    $entity_access = $original_entity->access('update');
+    if (!$entity_access->isAllowed()) {

@@ -263,9 +267,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-    if (!$entity->access('delete')) {
-      throw new AccessDeniedHttpException($this->accessDeniedExceptionMessage($entity, 'delete'));
+
+    $entity_access = $entity->access('delete');
+    if (!$entity_access->isAllowed()) {

The original code was returning a boolean. But now you need the access result object, to be able to access the reason.

So you need to call not:

$entity->access($op)

but:

$entity->access($op, NULL, TRUE)

Just like EntityResource::get() was already doing.

Once you do that, those failures are fixed. In other words: a tiny, tiny thing you missed :)

#67: that addressed all points of #65, except #65.7 — thanks! :)

I can help you with the TestAuth authentication provider plus EntityResourceTestBase additions. Did that in this reroll.

#69 + #71:

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -106,7 +112,12 @@ public static function forbiddenIf($condition) {
   +      $access_result->setReason("The {$permission} permission is required.");
   

   Let's put single quotes around the permission, for legibility. (Did that for you in point 6.)

2. +++ b/core/lib/Drupal/Core/Access/AccessResultNeutral.php
   @@ -5,7 +5,24 @@
   +   * Constructs a new AccessResultForbidden instance.
   

   s/Forbidden/Neutral/

3. +++ b/core/lib/Drupal/Core/Access/AccessResultNeutral.php
   --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   

   Unnecessary \n additions in several places in this file. Can you please revert those?

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -10,6 +10,7 @@
   +use Drupal\Tests\Core\Render\Element\PasswordConfirmTest;
   

   Unused, please remove this.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -556,8 +562,13 @@ public function testPost() {
   -    $this->assertResourceErrorResponse(403, "You are not authorized to create this {$this->entity->getEntityTypeId()} entity" . (($this->entity->bundle() !== $this->entity->getEntityTypeId()) ? " of bundle {$this->entity->bundle()}." : "."), $response);
   -
   +    $permission = $this->entity->getEntityType()->getAdminPermission();
   

   This kind of unnecessary \n addition in multiple places here. Please revert those.

6. You forgot to update AccessResult::allowedIfHasPermissions(). I did that for you here.
7. This is missing a AccessResultNeutralTest, look at AccessResultForbiddenTest for an example. (Which #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response added.)
8. This is missing an expansion of the test coverage for AccessResult::orIf() and AccessResult::andIf().

#74 This is interesting. This happens when it's not the admin_permission on the entity type annotation that matters, but another permission, which we cannot know, but which is determined by the logic of the entity type's access control handler. It could even be a combination of multiple permissions.

This is happening for:

1. blocks (access content for GET)
2. comments (post comments for POST)
3. entity_test entities (no message yet for GET
4. et cetera

Basically, it's happening for every entity type EXCEPT nodes, because that uses the "node access" (node grants) system.

In other words: access may be denied because of a permission which the current user does not have… but it may also be because of other reasons. This is why the approach proposed in #74 is mostly sound, but not entirely.

Interesting that it turns out this is by far the trickiest part of this patch! So, what do we need? We need to be able to specify the expected 403 message when a 403 happens due to a lack of authorization. We don't want to do an assertion; because the assertion can be automated via assertResourceErrorResponse(). What we need is the expected message parameter for that method. So I think abstract protected function getExpectedUnauthorizedAccessMessage($method) would make sense. Then EntityResourceTestBase can provide a default implementation, which can be exactly your current implementation:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -334,14 +335,28 @@ public function testGet() {
+    $permission = $this->entity->getEntityType()->getAdminPermission();
+    if ($permission !== FALSE) {
+      $this->assertResourceErrorResponse(403, "The {$permission} permission is required.", $response);
+    }
+    else {
+      $this->assertResourceErrorResponse(403, "You are not authorized to view this {$this->entity->getEntityTypeId()} entity" . (($this->entity->bundle() !== $this->entity->getEntityTypeId()) ? " of bundle {$this->entity->bundle()}." : "."), $response);
+    }

Then for example BlockResourceTestBase can override it like this:

  protected function getExpectedUnauthorizedAccessMessage($method) {
    if ($method === 'GET') {
      return 'You are not authorized to view this block entity.';
    }
    return parent::getExpectedUnauthorizedAccessMessage($method);
  }

IOW: it becomes easy to override the default behavior for just a single HTTP method, or a subset, or all. And if an entity type has the default behavior, well, then you don't need to do anything.

#78 + #79 + #80: oops, I already fixed that in #82 :)

#81: that won't work either, for the same reason #74 won't work.

NW for:

1. #85's review of #69 + #71
2. #85's answer to #74

Back to you, @gnuget, you're now fully unblocked again :)

Also, we should really get this into Drupal 8.3. Without this, developers will have to continue to run into these painful 403 responses without any idea on how to get it to work.

When it lands, it's worthy of the release notes, so tagging 8.3.0 release notes.

This also blocks e.g. JSON API from having good DX when it comes to 403 responses, so also tagging Contributed project blocker.

Wooot! Thanks, @gnuget :)

That looks like another big step in the right direction!

Awesome progress here!

I basically only have nits remaining. This patch is approaching RTBC status. It looks great (besides the few rough edges — see my review below) and delivers the huge DX improvement that this issue was created for :)

#94:

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -163,12 +163,15 @@ public static function allowedIfHasPermissions(AccountInterface $account, array
   +      else {
   +        $access_result->setReason("Access denied.");
   +      }
   

   This is not a reason. It's a useless message.

   This change should be reverted.

   Apparently @dawehner agrees in #95.

2. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -36,8 +36,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +          $access_result->setReason("The 'access comments' permission is required and the comment must be published.");
   

   Yay!

3. +++ b/core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php
   @@ -84,10 +84,17 @@ public function testUsersWithoutPermission() {
   -      $this->assertIdentical('{}', $response);
   +      if (!$user->hasPermission('access in-place editing')) {
   +        $message = "A fatal error occurred: The 'access in-place editing' permission is required.";
   +      }
   +      else {
   +        $message = "A fatal error occurred: The 'update' permission is required.";
   +      }
   +      $this->assertIdentical(Json::encode(['message' => $message]), $response);
   

   Yay!

4. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -62,10 +62,15 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
   +    if (!$result->isAllowed()) {
   +      $result->setReason("The '$operation' permission is required.");
   +    }
   

   This does not make sense. An operation is not a permission. Let's write a better message. And, in fact, let's remove this altogether, because it's impossible to determine the reason for access not being allowed here. The reason must be specified in the place that is disallowing access.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
   @@ -310,4 +310,20 @@ public function testPostDxWithoutCriticalBaseFields() {
   +    if ($method === 'GET') {
   +      return "The 'access comments' permission is required and the comment must be published.";
   +    }
   +    if ($method === 'POST') {
   +      return "The 'post comments' permission is required.";
   +    }
   +    return parent::getExpectedUnauthorizedAccessMessage($method);
   

   switch ($method) {
     case 'GET':
       return …;
     case 'POST':
       return …;
     default:
       return parent::…();
   }
   

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -249,6 +249,12 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
   +    // If it has an admin permission lets return that message.
   

   s/lets/let's/

   Or, actually, just remove this comment. The code is trivial, the comment is a unnecessary.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -338,13 +344,7 @@ public function testGet() {
   -      $permission = $this->entity->getEntityType()->getAdminPermission();
   -      if ($permission !== FALSE) {
   -        $this->assertResourceErrorResponse(403, "The '{$permission}' permission is required.", $response);
   -      }
   -      else {
   -        $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response);
   -      }
   +      $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response);
   

   YAY!

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
   @@ -124,4 +124,20 @@ protected function getNormalizedPostEntity() {
   +    if ($method === 'GET') {
   +      return "The 'view test entity' permission is required.";
   +    }
   +    if ($method === 'POST') {
   +      return "The following permissions are required: 'administer entity_test content' OR 'administer entity_test_with_bundle content' OR 'create entity_test entity_test_with_bundle entities'.";
   +    }
   +    return parent::getExpectedUnauthorizedAccessMessage($method);
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -193,4 +193,17 @@ protected function getNormalizedPostEntity() {
   +    if ($method === 'GET' || $method == 'PATCH' || $method == 'DELETE') {
   +      return "The 'access content' permission is required.";
   +    }
   +    return parent::getExpectedUnauthorizedAccessMessage($method);
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/User/UserResourceTestBase.php
   @@ -217,4 +217,24 @@ public function testPatchDxForSecuritySensitiveBaseFields() {
   +    if ($method === 'GET') {
   +      return "The 'access user profiles' permission is required and the user must be active.";
   +    }
   +    if ($method === 'PATCH') {
   +      return "You are not authorized to update this user entity.";
   +    }
   +    if ($method === 'DELETE') {
   +      return 'You are not authorized to delete this user entity.';
   +    }
   +    return parent::getExpectedUnauthorizedAccessMessage($method);
   

   All of these would also be more legible with switch.

9. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -38,7 +39,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
        // The anonymous user's profile can neither be viewed, updated nor deleted.
        if ($entity->isAnonymous()) {
   -      return AccessResult::forbidden();
   +      return AccessResult::forbidden("Access Denied");
   

   This is again an unhelpful reason.

   In this case, I think it makes sense to remove the comment and use its text as the reason.

#95++ for pointing out the lack of test coverage + bug that you found by fixing the test coverage — this is why I said in #85.8 This is missing an expansion of the test coverage for AccessResult::orIf() and AccessResult::andIf()

#100: disclosing what the requirements are to be able to access something is not information disclosure. If Drupal's security depended on this, we'd be in the questionable land of security through obscurity. Let's also not forget that 99% of Drupal sites don't use custom access control schemes; they use Drupal's permissions. So for 99% of these responses, the reason that we now send back in the response actually is something any attacker could have found by reading the Drupal source code.
Finally, let's not forget that this is only for API (REST/JSON API/GraphQL) requests, i.e. for non-HTML requests. Most routes/controllers only allow HTML. HTML error responses do not show these reasons. And even if they did, the above about security through obscurity still applies.

So this is unchanged:

Drupal's current Access Denied page, which does not show why a user was denied access.

#102: let's revert this change. For the reasons given in #104, this change does not make sense.

#98: this is the patch that I now need to review.

1. --- a/core/lib/Drupal/Core/Access/AccessResult.php
   +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   
   index ca262c7..6f18648 100644
   --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   

   Thoroughly reviewed this, the logic looks perfect. I have zero remarks.

   Well, there is one remark: the first point in my review of #94, for allowedIfHasPermissions(). But that's trivially fixed.

2. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -62,10 +62,15 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
   +      $result->setReason("The '$operation' permission is required.");
   

   Should be removed, as I said in my review of #94.

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -279,6 +282,26 @@ public function delete(EntityInterface $entity) {
   +   * Return the proper message checking if the entity has bundles.
   

   Generates a fallback access denied message, in case no specific reason is set.

4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -279,6 +282,26 @@ public function delete(EntityInterface $entity) {
   +   *   The operation executed before to call the exception.
   

   The disallowed entity operation.

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -279,6 +282,26 @@ public function delete(EntityInterface $entity) {
   +   * @return string $operation
   

   @return string

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -279,6 +282,26 @@ public function delete(EntityInterface $entity) {
   +  public function accessDeniedExceptionMessage(EntityInterface $entity, $operation) {
   

   Should be protected.

   Also, generateFallbackAccessDeniedMessage would be a better name.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -241,6 +241,50 @@ protected function getNormalizedPatchEntity() {
   +    $operation = '';
   +    if ($method === 'GET') {
   +      $operation = 'view';
   +    }
   +    if ($method === 'POST') {
   +      $operation = 'create';
   +    }
   +    if ($method === 'PATCH') {
   +      $operation = 'update';
   +    }
   +    if ($method === 'DELETE') {
   +      $operation = 'delete';
   +    }
   

   Let's change this to:

   $http_method_to_entity_operation = [
     'GET' => 'view',
     'POST' => 'create',
     'PATCH' => 'update',
     'DELETE' => 'delete',
   ];
   $operation = $http_method_to_entity_operation[$method];
   

   Much shorter, much easier to read.

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -334,18 +378,22 @@ public function testGet() {
        $this->setUpAuthorization('GET');
    
   -
        // 200 for well-formed HEAD request.
   
   @@ -551,9 +598,7 @@ public function testPost() {
   -    // @todo Update the message in https://www.drupal.org/node/2808233.
   -    $this->assertResourceErrorResponse(403, '', $response);
   -
   +    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('POST'), $response);
    
        $this->setUpAuthorization('POST');
   
   @@ -741,9 +785,7 @@ public function testPatch() {
   -    // @todo Update the message in https://www.drupal.org/node/2808233.
   -    $this->assertResourceErrorResponse(403, '', $response);
   -
   +    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('PATCH'), $response);
   
   @@ -896,9 +937,7 @@ public function testDelete() {
        $response = $this->request('DELETE', $url, $request_options);
   -    // @todo Update the message in https://www.drupal.org/node/2808233.
   -    $this->assertResourceErrorResponse(403, '', $response);
   -
   +    $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('DELETE'), $response);
    
        $this->setUpAuthorization('DELETE');
    
   

   Unnecessary whitespace changes in these places, let's not make whitespace changes!

9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
   @@ -124,4 +124,21 @@ protected function getNormalizedPostEntity() {
   +  protected function getExpectedUnauthorizedAccessMessage($method) {
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -193,4 +193,17 @@ protected function getNormalizedPostEntity() {
   +  protected function getExpectedUnauthorizedAccessMessage($method) {
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
   @@ -137,4 +137,27 @@ protected function getNormalizedPostEntity() {
   +  protected function getExpectedUnauthorizedAccessMessage($method) {
   

   As I said in my review of #94, these would be better as switch statements.

10. +++ b/core/modules/user/src/UserAccessControlHandler.php
    @@ -38,7 +39,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +      return AccessResult::forbidden("Access Denied");
    

    Again echoing what I said in my review of #94.

RTBC once it comes back green, this just needs a trivial fix now!

Thanks!

Random fail in Drupal\node\Tests\Views\FrontPageTest?

Imo we should consider making the reason required but that is a D9 change.

+1!

Reviewed #117 (and diffed it with the previous RTBC patch). Trivial conflict resolved correctly. Back to RTBC.

#119: apologies!

Change record created: https://www.drupal.org/node/2849066.

Regarding reason merging: if we go down the path of listing all reasons involved, we hugely complicate the logic (and the reason returned in the 403 response) for very little benefit. How do we present 5 reasons that have been AND-ed together in a clear manner? Just listing the first reason is causing access to not be allowed, the developer can fix that. And perhaps there will be another reason, but that's okay: the developer can then next fix that reason too. And so on.
There's one more reason, and probably it's even more important: access results are evaluated lazily. See \Drupal\Tests\Core\Access\AccessResultTest::testAndIf(). You can see that in the last several assertions:

    // FORBIDDEN && ALLOWED = FORBIDDEN
    $access = $forbidden->andif($allowed);
    $this->assertFalse($access->isAllowed());
    $this->assertTrue($access->isForbidden());
    $this->assertFalse($access->isNeutral());
    $this->assertDefaultCacheability($access);

    // FORBIDDEN && NEUTRAL = FORBIDDEN
    $access = $forbidden->andif($neutral);
    $this->assertFalse($access->isAllowed());
    $this->assertTrue($access->isForbidden());
    $this->assertFalse($access->isNeutral());
    $this->assertDefaultCacheability($access);

    // FORBIDDEN && FORBIDDEN = FORBIDDEN
    $access = $forbidden->andif($forbidden);
    $this->assertFalse($access->isAllowed());
    $this->assertTrue($access->isForbidden());
    $this->assertFalse($access->isNeutral());
    $this->assertDefaultCacheability($access);

    // FORBIDDEN && * === FORBIDDEN: lazy evaluation verification.
    $access = $forbidden->andIf($unused_access_result_due_to_lazy_evaluation);
    $this->assertFalse($access->isAllowed());
    $this->assertTrue($access->isForbidden());
    $this->assertFalse($access->isNeutral());
    $this->assertDefaultCacheability($access);

In other words: the reason in the second, third, etc access result object would not even be considered as soon as the first is disallowing access!

Fixed the coding standards issue and actually found one more.

YAYAYAYAYAYAY! This is such a big DX win! Thanks! :)

CR published: https://www.drupal.org/node/2849066

Issue to make JSON API also use this: #2930231: JSON API 403 errors don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.

What this did for entity access, #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens does for field access.

And also created #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not, which fixes a minor bug that was introduced here.