Problem/Motivation

#2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error uncovered that it can be very difficult even for a Drupal expert to figure out why you're getting a 403 for a particular REST resource.

Yet … we want Drupal to be adopted as the storage back-end by JavaScript developers — i.e. by non-Drupalists. How are they going to figure this out?

Proposed resolution

Our 403 responses for REST routes need to list a reason for this 403. And in fact, #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response introduced that very infrastructure! We just need to use it more broadly now.

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.
  3. Test coverage.

User interface changes

None.

API changes

None. (AccessResultNeutral now implements AccessResultReasonInterface just like AccessResultForbidden already does, but this breaks no APIs.)

Data model changes

None.

CommentFileSizeAuthor
#122 2808233-122.patch49.32 KBWim Leers
#122 interdiff.txt1.65 KBWim Leers
#117 2808233-117.patch49.32 KBdawehner
#110 interdiff.txt1.54 KBdawehner
#110 2808233-110.patch49.24 KBdawehner
#106 interdiff.txt14.51 KBdawehner
#106 2808233-106.patch49.32 KBdawehner
#102 interdiff-102-98.txt1.2 KBChi
#102 rest_http_responses-2808233-102.patch51.31 KBChi
#98 interdiff.txt1.83 KBdawehner
#98 2808233-98.patch50.11 KBdawehner
#95 interdiff.txt10.19 KBdawehner
#95 2808233-95.patch48.86 KBdawehner
#94 2808233-93.patch42.25 KBgnuget
#94 interdiff.txt24.15 KBgnuget
#90 2808233-90.patch24.07 KBgnuget
#90 interdiff.txt14.85 KBgnuget
#85 interdiff.txt1.64 KBWim Leers
#85 2808233-85.patch19.16 KBWim Leers
#84 interdiff.txt2.63 KBWim Leers
#84 2808233-84.patch18.34 KBWim Leers
#82 interdiff.txt1.76 KBWim Leers
#82 2808233-82.patch16.37 KBWim Leers
#71 2808233-71.patch16.33 KBgnuget
#71 2808233-interdiff-69-71.txt1.81 KBgnuget
#69 2808233-69.patch16.13 KBgnuget
#69 2808233-interdiff-67-69.txt10.87 KBgnuget
#67 2808233-62-67-interdiff.txt4.26 KBgnuget
#67 2808233-67.patch10.52 KBgnuget
#62 2808233-62.patch9.43 KBgnuget
#62 2808233-61-62-interdiff.txt6.69 KBgnuget
#61 2808233-61.patch8.38 KBgnuget
#52 2808233-52.patch9.72 KBgnuget
#52 2808233-interdiff-50-52.txt1.23 KBgnuget
#50 2808233-interdiff-48-50.txt1.51 KBgnuget
#50 2808233-50.patch10.95 KBgnuget
#48 2808233-interdiff-45-48.txt914 bytesgnuget
#48 2808233-48.patch11.46 KBgnuget
#45 2808233-45.patch11.57 KBgnuget
#45 2808233-interdiff-43-45.txt2.11 KBgnuget
#43 interdiff.txt2.18 KBdawehner
#43 2808233-43.patch12.05 KBdawehner
#39 2808233-indeterdiff-37-39.txt899 bytesgnuget
#39 better_403_responses-28082330-39.patch10.98 KBgnuget
#37 2808233-33-37-interdiff.txt664 bytesgnuget
#37 better_403_responses-28082330-37.patch10.1 KBgnuget
#33 2808233-29-33-interdiff.txt14.96 KBgnuget
#33 better_403_responses-28082330-33.patch9.45 KBgnuget
#29 better_403_responses-28082330-29.patch13.06 KBgnuget
#29 better_403_responses-28082330-interdiff-24-29.txt4.04 KBgnuget
#24 interdiff-16-24.txt12.81 KBtedbow
#24 2808233-24.patch13.01 KBtedbow
#16 better_403_responses-2808233-16.patch6.3 KBgnuget
#16 2808233-5-16-interdiff.txt5.59 KBgnuget
#14 access_denied.png13.26 KBgnuget
#5 better_403_responses-2808233-5.patch2.05 KBdysrama
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Title: REST 403 responses don't tell the user *why* permission is not granted: requires deep Drupal understanding to figure out » REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out
dawehner’s picture

One thing we might should also look into is to pass along the reason when merging access results together

Wim Leers’s picture

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.

dysrama’s picture

Status: Active » Needs review
FileSize
2.05 KB

Added is a patch for better error messages. This is the first work I've ever done on D8, so I hope I haven't misunderstood the issue.
I tried to look at the AccessResultReasonInterface, but the reason seems always to be empty for the entity access checks?

dawehner’s picture

This is certainly adding the messages in all various places. This is really nice. It is a small incremental improvement.

Grayside’s picture

Not sure if it belongs here, but a standardized approach to thinking about contextualized error messages might look at the Problem Details RFC. One thing I absolutely love is when error messages are directly affiliated with a URL to "canonical" troubleshooting documentation. These are probably second-order considerations, but what should be decided is whether there is a point at which changing the error responses themselves constitute a breaking change.

dawehner’s picture

@Grayside
Nice research! I'm totally in favour of adopting that, maybe we could do in its own issue. Just improving the exception message itself is worth doing so.

Grayside’s picture

dawehner’s picture

Status: Needs review » Needs work

IMHO expanding the tests would still make sort of sense.

Wim Leers’s picture

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

gnuget’s picture

I will work on this.

Wim Leers’s picture

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

gnuget’s picture

FileSize
13.26 KB

I'm stuck on this.

While I in my local this is working:

curl --include --request POST --user something:something --header 'Content-type: application/hal+json' http://d8.dev/entity/node\?_format\=hal+json --data-binary '{"_links":{"type":{"href":"http://d8.dev/rest/type/node/page"}}, "title":[{"value":"My first page"}]}'
HTTP/1.1 403 Forbidden
Server: nginx/1.10.1
Content-Type: text/plain; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/7.0.6
Cache-Control: must-revalidate, no-cache, private
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Date: Fri, 04 Nov 2016 14:22:14 GMT

Entity creation for bundle page is not allowed for the current user.%

In the tests I got this error instead:

I will continue working on this today.

Wim Leers’s picture

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!

gnuget’s picture

Hi Wim

Thanks for your help, I was missing the query string (duh!)

And now I was able to test: delete, update and create but I think found a problem with READ:

The Drupal\rest\Tests\ReadTest class tests the following entities:

  • entity_test
  • node
  • config_test
  • taxonomy_vocabulary
  • block
  • user_role

And config_test, taxonomy_vocabulary, block and user_role still return an empty body when the user hasn't permissions:
Response body: {"message":""}

So there is still a place where we need to add a better error message.

I also changed hal+json to hal_json in a few places (sorry if this is out of the scope but it was just 1 character! I can revert this change if necessary).

I attached my progress so far, I will continue working on this.

Thanks!

Chi’s picture

In some cases REST 403 responses come from outside REST module. For instance I just spent some time on debugging 403 error that was thrown in the onKernelRequestFilterProvider. The error message was as follows: {message: ""}. Can we fix this as well in this ticket?

Wim Leers’s picture

#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.

gnuget’s picture

Wim Leers’s picture

Title: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out » [PP-1] REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out
Status: Needs work » Postponed

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

Wim Leers’s picture

Wim Leers’s picture

Title: [PP-1] REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out » REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out
Status: Postponed » Needs work
tedbow’s picture

Assigned: Unassigned » tedbow

Re-rolling

tedbow’s picture

Status: Needs work » Needs review
FileSize
13.01 KB
12.81 KB

Ok here is re-roll and update

The only changes from the previous patch still applicable are in \Drupal\rest\Plugin\rest\resource\EntityResource

Since a lot of files removed in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

I did update the errors to also include the entity type since it probably would make debugging easier because you could bundle names shared across entity types.

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

I also added a couple more @todo point this issue in places where we have an empty string message in the tests haven't figured out where they come from yet but should be addressed here.

Status: Needs review » Needs work

The last submitted patch, 24: 2808233-24.patch, failed testing.

Chi’s picture

@tedbow, can you also update onKernelRequestFilterProvider mentioned in #17.

Wim Leers’s picture

  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.

dawehner’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -106,7 +106,7 @@ public static function create(ContainerInterface $container, array $configuratio
     if (!$entity_access->isAllowed()) {
-      throw new AccessDeniedHttpException();
+      throw new AccessDeniedHttpException('The current user does not have access to view the requested entity.');
     }

@@ -145,7 +145,7 @@ public function post(EntityInterface $entity = NULL) {
 
     if (!$entity->access('create')) {
-      throw new AccessDeniedHttpException();
+      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
     }
     if (!$original_entity->access('update')) {
-      throw new AccessDeniedHttpException();
+      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
     if (!$entity->access('delete')) {
-      throw new AccessDeniedHttpException();
+      // @todo Handle message for no bundle entities https://www.drupal.org/node/2300677.
+      throw new AccessDeniedHttpException("Entity deletion for entity type {$entity->getEntityTypeId()} of bundle {$entity->bundle()} is not allowed for the current user.");
     }

Is it just me that its weird that 3 of the messages mention the bundle but one doesn't? Is there a specific reason for that?

gnuget’s picture

I will comeback to work on this.

My progress so far:

  • #17 done (my english is not the best of the world, but I did my best, please if you think in a better message let me know and I will update the patch)
  • #27.1 done
  • #27.2 done (the same as #17, if you think in a better message let me know)
  • #27.2.2 (related with the CookieResourceTestTrait::assertResponseWhenMissingAuthentication() to it shouldn't assert the response body. not sure to understand what I need to do there. so this is not done.
  • #27.3 done
  • #28 done

Lets see what the bot says and in my next patch will try to fix any broken test.

Regards.

Chi’s picture

+++ 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.');

I would just copy a sentence from the method description.

Authentication provider is not allowed on this route.

Status: Needs review » Needs work

The last submitted patch, 29: better_403_responses-28082330-29.patch, failed testing.

Wim Leers’s picture

#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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
14.96 KB

I think covered all the comments in this new patch.

Let's see what the bot says.

Wim Leers’s picture

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

Status: Needs review » Needs work

The last submitted patch, 33: better_403_responses-28082330-33.patch, failed testing.

Wim Leers’s picture

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);
  }
gnuget’s picture

Status: Needs work » Needs review
FileSize
10.1 KB
664 bytes

Duh!

I attach a new patch with this change.

Also, it's curious how the message is still empty under some circunstances.

1) Drupal\Tests\hal\Functional\EntityResource\Block\BlockHalJsonAnonTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"You are not authorized to view this block entity of bundle block."}
+{"message":""}

Wonder if this change will fix those, if not I will look more in deep today.

Status: Needs review » Needs work

The last submitted patch, 37: better_403_responses-28082330-37.patch, failed testing.

gnuget’s picture

I've been trying to figure it out why the messages return empty without luck, In my manual tests the response is sent correctly, the message is there but in the tests it is empty.

And it's hard debugging the tests because it seems to once the request to the endpoint is made none of my print_r() calls are printed, so not sure where the things went wrong (unless my debug messages aren't printed because the empty messages don't come from EntityResource.php and Drupal send the empty messages before to touch the REST module.

Will continue digging on this, it result harder than expected.

In the meanwhile I fixed the test related with the CookieResourceTestTrait

gnuget’s picture

Status: Needs work » Needs review

The last submitted patch, 16: better_403_responses-2808233-16.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 39: better_403_responses-28082330-39.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.05 KB
2.18 KB

A lot of those failures are caused by a limit due to the BC layer we have. The BC layer of having specific rest permissions causes a worse error message, which honestly is totally fine, as long we document it. This change should fix a good amount of the remaining test failures.

Status: Needs review » Needs work

The last submitted patch, 43: 2808233-43.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
11.57 KB

Great dawehner!!!

I've spent a good amount of time trying to figure it out why it was returning an empty response.

I just apply the same change as you did for GET but for the other verbs.

Hoping to this is going to fix all the tests.

dawehner’s picture

@gnuget
Yeah I primarily tried to enable you to continue to work on this issue. I hope this helped :)

Status: Needs review » Needs work

The last submitted patch, 45: 2808233-45.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
11.46 KB
914 bytes

Ups! I forgot one.

New patch attached.

Status: Needs review » Needs work

The last submitted patch, 48: 2808233-48.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
10.95 KB
1.51 KB

Let's try again. (I added the fix in the wrong line in my previous patch -_-)

Status: Needs review » Needs work

The last submitted patch, 50: 2808233-50.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
9.72 KB

I might need help explaining this but this is what I learned from my last patch:

1) Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonAnonTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":""}
+{"message":"You are not authorized to view this node entity of bundle camelids."}

That error comes from:

    // DX: 404 when resource not provisioned, 403 if canonical route. Non-HTML
    // response because ?_format query string is present.
    $response = $this->request('GET', $url, $request_options);
    if ($has_canonical_url) {
      $this->assertResourceErrorResponse(403, '', $response);
    }
    else {
      $this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);
    }

In theory at this point the resource hasn't even been provisioned, so the answer shouldn't come from the REST module and that is why it is empty (that is what Drupal returns when a non-provisioned resource is requested).

But for some tests it actually returns:

{"message":"You are not authorized to view this node entity of bundle camelids."}

And that is because of Dawehner addition at #43 in the EntityAccessCheck.php file.

-        $result = $entity->access($operation, $account, TRUE);
-        if ($result->isForbidden()) {
-          return AccessResultForbidden::forbidden("You are not authorized to view this {$entity->getEntityTypeId()} entity of bundle {$entity->bundle()}.")
-            ->mergeCacheMaxAge($result);
-        }
-        return $result;

But what I cannot explain is why this answer is not returned for all the tested resources which haven't been provisioned otherwise the patch I made on #45 should work.

What I will try with this patch is undo the changes made on EntityAccessCheck.php file. and check if in this way the answer is consistent between all the tests.

gnuget’s picture

And the answer was consistent, not sure what is next, is this ready? I need to re-add the change added By #34?

Should create a follow up to make drupal return a useful answer when the endpoint hasn't been enabled in the rest module?

:-) let me know.

Regards and thanks.

dawehner’s picture

What I will try with this patch is undo the changes made on EntityAccessCheck.php file. and check if in this way the answer is consistent between all the tests.

I'm happy if you revert them, conceptually though it makes sense to have this message in EntityAccessCheck ...

dawehner’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -145,7 +145,7 @@ public function post(EntityInterface $entity = NULL) {
    -      throw new AccessDeniedHttpException();
    +      throw new AccessDeniedHttpException("You are not authorized to create this {$entity->getEntityTypeId()} entity of bundle {$entity->bundle()}.");
         }
    
    @@ -200,7 +200,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
         if (!$original_entity->access('update')) {
    -      throw new AccessDeniedHttpException();
    +      throw new AccessDeniedHttpException("You are not authorized to update this {$entity->getEntityTypeId()} entity of bundle {$entity->bundle()}.");
    
    @@ -264,7 +264,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
    -      throw new AccessDeniedHttpException();
    +      throw new AccessDeniedHttpException("You are not authorized to delete this {$entity->getEntityTypeId()} entity of bundle {$entity->bundle()}.");
         }
    

    I wonder whether we should take into account the case of non bundleable entities: Currently the message would look like You are not authorized to create this user entity of bundle user

  2. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
    @@ -92,7 +92,7 @@ protected function getAuthenticationRequestOptions($method) {
    +    $this->assertResourceErrorResponse(403, FALSE, $response);
    

    So FALSE means we don't compare the response body at all. Is there a reason we cannot generate the expected one? Maybe at least at a comment about it.

Wim Leers’s picture

  • #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.)
gnuget’s picture

And which would be the best way to address #55.1?

Checking if the entity is bundleable and if is not then provide a different message (and update the tests accordinly)?

dawehner’s picture

You could create a helper function which takes an entity, a message without bundles and a message with bundles and returns the appropriate message.

gnuget’s picture

Sounds great, will work on that.

Thanks.

Wim Leers’s picture

Status: Needs review » Needs work

For #55 and later.

gnuget’s picture

Status: Needs work » Needs review
FileSize
8.38 KB
gnuget’s picture

Ok, Here a patch which address #55

dawehner’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -145,7 +145,7 @@ public function post(EntityInterface $entity = NULL) {
     if (!$entity->access('create')) {

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -339,7 +339,7 @@ public function testGet() {
     $response = $this->request('GET', $url, $request_options);
-    $this->assertResourceErrorResponse(403, "You are not authorized to view this {$this->entity->getEntityTypeId()} entity of bundle {$this->entity->bundle()}.", $response);
+    $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);
 

So this means we don't have any rest test coverage for non bundleable entity types?

dawehner’s picture

The latest patch totally works for me ...

Wim Leers’s picture

Status: Needs review » Needs work

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 :)

Wim Leers’s picture

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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
10.52 KB
4.26 KB

Ok, Here another try!

This basically address:

  • 65.1
  • 65.2
  • 65.3
  • 65.4
  • 65.5
  • 65.6

Also, I tried for a couple hours to use a custom provider as 65.1 suggested but I didn't find a way to switch to TestAuth directly on EntityResourceTestBase the alternative I guess is to add this provider on every entity resource test class (eg. NodeJsonCookieTest::$auth), so I can have that provider available on the test or something like that?

For now, I just added the test directly on: \Drupal\basic_auth\Tests\Authentication\BasicAuthTest I can give it another try with some guidance if you want @Wim Leers.

For 65.7 and 66 I will provide another patch.

Regards!

(I will put the status as Needs Review so I can know to all the tests are working, I will switch back to needs work when it finished)

gnuget’s picture

Status: Needs review » Needs work
gnuget’s picture

Status: Needs work » Needs review
FileSize
10.87 KB
16.13 KB

Ok, this new patch tackle 65.7 and 66.

Status: Needs review » Needs work

The last submitted patch, 69: 2808233-69.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
16.33 KB

Ok, round 2.

Status: Needs review » Needs work

The last submitted patch, 71: 2808233-71.patch, failed testing.

tedbow’s picture

Assigned: tedbow » Unassigned

Forgot to un-assign myself.

gnuget’s picture

After to pass a few hours with this. It seems to we will need to introduce a new abstract method on ResourceTestBase called: getHttpMethodPermission (or something like that) which will return the permission necessary to access the resource, so we can match the error response with what we are expecting.

For instance:

1) Drupal\Tests\hal\Functional\EntityResource\Term\TermHalJsonBasicAuthTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":""}
+{"message":"The access content permission is required."}

/Projects/drupal_development/drupal8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:317
/Projects/drupal_development/drupal8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:333
/Projects/drupal_development/drupal8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:306

Here we explicitly need to know the user needs the "access content" permission in order to see the content and actually, we are granting that permission to the user at TermResourceTestBase::setUpAuthorization

So having getHttpMethodPermission we can update TermResourceTestBase::setUpAuthorization in this way:

protected function setUpAuthorization($method) {
    switch ($method) {
      case 'GET':
        $this->grantPermissionsToTestedRole([$this->getHttpMethodPermission('GET')]);
        break;
      case 'POST':
      case 'PATCH':
      case 'DELETE':
        // @todo Update once https://www.drupal.org/node/2824408 lands.
        $this->grantPermissionsToTestedRole([$this->getHttpMethodPermission('DELETE')]);
        break;
    }
  }

and getHttpMethodPermission will look something like:

protected function getHttpMethodPermission ($method = '') {
  switch($method) {
    case 'GET':
      return 'access content';
    case 'DELETE':
      return 'administer taxonomy';
  }

  return '';
}

And finally we can do something like this on EntityResourceTestBase (line 306) :

if ($has_canonical_url) {
      $this->assertResourceErrorResponse(403, "The {$this->getHttpMethodPermission('GET')}  permission is required", $response);
    }

In order to do this we need to update a ton of classes and tests (which I'm super happy to do it if necessary) but first I want to know your opinion so I can be sure to I'm on the correct path.

Thanks!

dawehner’s picture

  1. index f67b9f5..143361e 100644
    --- a/core/lib/Drupal/Core/Access/AccessResult.php
    
    --- a/core/lib/Drupal/Core/Access/AccessResult.php
    +++ b/core/lib/Drupal/Core/Access/AccessResult.php
    
    @@ -5,7 +5,24 @@
      */
    -class AccessResultNeutral extends AccessResult {
    

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

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -263,9 +267,12 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
    +    $entity_access = $entity->access('delete');
    +    if (!$entity_access->isAllowed()) {
    +      throw new AccessDeniedHttpException($entity_access->getReason() ?: $this->accessDeniedExceptionMessage($entity, 'delete'));
         }
    
    @@ -279,6 +286,26 @@ public function delete(EntityInterface $entity) {
    +  public function accessDeniedExceptionMessage(EntityInterface $entity, $operation) {
    

    Given that its most probably always a permission issue, I'm wondering whether we could move this additional method into a later issue. You know, its all about having small steps at a time, to actually achieve progress.

Wim Leers’s picture

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).

gnuget’s picture

So, #74 is in the correct path? or should I change something?

Thanks! and regards.

lukedekker’s picture

Running the patch from #74, I'm getting an error Call to a member function isAllowed() on a non-object in /core/modules/rest/src/Plugin/resource/EntityResource.php Line 145.

144:  $entity_access = $entity->access('create');
145:  if (!$entity_access->isAllowed()) {

->access() defaults to return_as_object = FALSE.

I believe line 144 should be changed to $entity_access = $entity->access('create', NULL, TRUE);

I've only tested with POST, but this is likely the case for all methods. (Unless I'm running into some other issue.)

gnuget’s picture

Hi lukedekker

Yes, this still has issues but I want to know if the changes to I suggested on #74 are correct before to continue fixing the rest of the tests.

Thanks for your review.

lukedekker’s picture

Gnuget,

Absolutely understand. My last comment was meant as more of an FYI/when you get there sort of thing.

Thanks everyone for all of their work on this. I'm a noob to REST in D8 and this patch saved me hours of banging my head against the wall.

tedbow’s picture

@gnuget #74 sounds like a good idea to me.

We could even have EntityResourceTestBase::assertResource403ResponseForMethod($method, $response)
Maybe something like

funciton assertResource403ResponseForMethod($method, $response) {
  $this->assertResourceErrorResponse(403, "The {$this->getHttpMethodPermission($method')}  permission is required", $response);
}

To avoid having "The {$this->getHttpMethodPermission($method')} permission is required" in bunch of places.

But I like the idea in general.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
16.37 KB
1.76 KB

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 :)

Status: Needs review » Needs work

The last submitted patch, 82: 2808233-82.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.34 KB
2.63 KB

#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.

Wim Leers’s picture

#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.

Wim Leers’s picture

Status: Needs review » Needs 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 :)

Wim Leers’s picture

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.

gnuget’s picture

Ok, I will focus on this today, I will do my best so we can include it on 8.3.x

Thanks.

Wim Leers’s picture

Wooot! Thanks, @gnuget :)

gnuget’s picture

Status: Needs work » Needs review
FileSize
14.85 KB
24.07 KB

This still is work in progress.

This patch almost addresses everything but I need the testbot's help so I can know which entities require overwriting the getExpectedUnauthorizedAccessMessage method, so I will let the bot to fail in those cases.

(And 85.8 is still pending)

Regards.

Status: Needs review » Needs work

The last submitted patch, 90: 2808233-90.patch, failed testing.

Wim Leers’s picture

That looks like another big step in the right direction!

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gnuget’s picture

Status: Needs work » Needs review
FileSize
24.15 KB
42.25 KB

The only pending thing is #85.8 (not sure where put these tests)

I found that drupal was still sending several empty responses on different places I update all of them.

Let's see what the testbot says.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Access/AccessResult.php
@@ -147,7 +157,24 @@ public static function allowedIfHasPermissions(AccountInterface $account, array
+    if ($access_result instanceof AccessResultReasonInterface) {
+      if (count($permissions) === 1) {
+        $access_result->setReason("The '$permission' permission is required.");
+      }
+      elseif (count($permissions) > 1) {
+        $quote = function ($s) {
+          return "'$s'";
+        };
+        $access_result->setReason(sprintf("The following permissions are required: %s.", implode(" $conjunction ", array_map($quote, $permissions))));
+      }
+      else {
+        $access_result->setReason("Access denied.");
+      }

IMHO the additional complexity of one vs. many should not be handled here. From my point of view this is not worth it.

I extended the test coverage and I think I found a bug, we don't merge message correctly on OR. See the failing test.

Status: Needs review » Needs work

The last submitted patch, 95: 2808233-95.patch, failed testing.

gnuget’s picture

Thanks for your review dawehner.

I won't be able to work on this during the weekend. If there is someone participating on the weekend sprint and want to help finishing this issue feel free to take over it I would love to see this on 8.3.x

Thanks again.

dawehner’s picture

Status: Needs work » Needs review
FileSize
50.11 KB
1.83 KB

This fixes the additional test I wrote in #95. We still need to talk about the 1 vs. many IMHO.

gnuget’s picture

@Wim Leers added that part he can explain better why it was added there.

Regards and thanks.

mradcliffe’s picture

This probably needs a security review to confirm that we are not falling into a information disclosure vulnerability.

It may not be good to leak what exactly caused the user to be denied access, but it is very useful when debugging. For instance, Drupal's current Access Denied page, which does not show why a user was denied access.

400 messages for validation are desirable, but 403 and 500 should be fairly limited as a malicious actor should not get much information as to what to target next. I do not know if mentioning permission names or entity type names is information disclosure or not, but this should be assessed before committing.

dawehner’s picture

This probably needs a security review to confirm that we are not falling into a information disclosure vulnerability.

This is indeed a good point. Maybe this should just be enabled somehow but we should not expose the information by default.

Chi’s picture

This adds error level condition to ExceptionJsonSubscriber. I am not sure if we can fix this globally, not only for JSON HTTP errors.

Status: Needs review » Needs work

The last submitted patch, 102: rest_http_responses-2808233-102.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -35,7 +36,9 @@ protected static function getPriority() {
+    $data = error_displayable($error) ? ['message' => $event->getException()->getMessage()] : NULL;

I'm not entirely convinced that this is the right way to solve it. error_displayable is really about php errors and exceptions (aka. server side errors), rather than what we deal with here which are client side errors. I'm wondering whether this should be actually a configuration for the serialization exception subscriber. Alternative this might be a container parameter, much like cacheability cache headers.

Wim Leers’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.32 KB
14.51 KB

Thank you for the great review wim!

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.

I 100% agree, good point. By doing what is in the patch right now, we effectively throw away information.]
]

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

+1

#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.

If someone also cares about that level of security, they can implement their own exception rendering.

Status: Needs review » Needs work

The last submitted patch, 106: 2808233-106.patch, failed testing.

Wim Leers’s picture

Version: 8.4.x-dev » 8.3.x-dev

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

mradcliffe’s picture

Thanks for the review, Wim.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.24 KB
1.54 KB

Here are the remaining test failures

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks!

Wim Leers’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: 2808233-110.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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

gnuget’s picture

This looks great!

Just want to say THANKS to all of you for helping me to work on this, I've learned a lot about how the REST module work and the access system.

Regards.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This patch looks sensible. Imo we should consider making the reason required but that is a D9 change.

git ac https://www.drupal.org/files/issues/2808233-110.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 50426  100 50426    0     0   184k      0 --:--:-- --:--:-- --:--:--  207k
error: patch failed: core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:334
error: core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php: patch does not apply

Needs a reroll.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.32 KB

Thank you for pointing it out. Here is a quick reroll. (There has been just one conflict).

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev

Note that issues like this should be targeted against 8.4.x, but can be considered for backport once they are committed to that branch. See the updated alpha release policy. Thanks!

Wim Leers’s picture

#119: apologies!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

I'm sorry I missed that there was no change record for this issue. We totally need one - especially if we're pointing to this in the release notes. If people don't know to set a reason the contrib projects which use neutral access results won't make the necessary change.

One thing this change has me thinking about is how reasons are merged.

>>> $a = new \Drupal\Core\Access\AccessResultNeutral();
=> Drupal\Core\Access\AccessResultNeutral {#10781}
>>> $a->setReason('One');
=> Drupal\Core\Access\AccessResultNeutral {#10781}
>>> $b = new \Drupal\Core\Access\AccessResultNeutral();
=> Drupal\Core\Access\AccessResultNeutral {#10775}
>>> $b->setReason('Two');
=> Drupal\Core\Access\AccessResultNeutral {#10775}
>>> $c = $a->andIf($b);
=> Drupal\Core\Access\AccessResultNeutral {#10792}
>>> $c->getReason();
=> "One"

In this example, doesn't the end user actually want both the reasons? Setting back to needs review to get an answer to this.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
@@ -124,4 +124,22 @@ protected function getNormalizedPostEntity() {
+      default:
+      return parent::getExpectedUnauthorizedAccessMessage($method);

Noticed a minor coding standards issue that should be addressed prior to commit.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
FileSize
1.65 KB
49.32 KB

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.

dawehner’s picture

Works for me

xjm’s picture

This a contrib soft blocker as a bad DX issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 7f59f9c to 8.4.x and 448e8c2 to 8.3.x. Thanks!

@Wim Leers thanks for answering the reason issue. I think what you suggest makes sense.

  • alexpott committed 1fcd3d5 on 8.4.x
    Issue #2808233 by gnuget, dawehner, Wim Leers, tedbow, Chi, dysrama:...

  • alexpott committed 92f5624 on 8.3.x
    Issue #2808233 by gnuget, dawehner, Wim Leers, tedbow, Chi, dysrama:...
Wim Leers’s picture

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

Wim Leers’s picture

jacov’s picture

Thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

effulgentsia’s picture

Version: 8.4.x-dev » 8.3.x-dev

Per #127, this was committed to 8.3.x, so retroactively updating the version attribute.