EntityResource: Remove ViewAccessControlHandler and provide comprehensive test coverage for View entity

Please see my note on https://www.drupal.org/node/2843756#comment-11983203.

I'm keen to help with these issues but I'm having some trouble using the FieldStorageConfig entity, so I'm going to switch over to the views entity, which I can get to work with the REST module. Hopefully on the way I'll learn enough to be able to go back to the FieldStorageConfig entity, but for now I'm going to put that one on hold and try to write tests for the view entity.

Great! :)

Hi, @vaplas - yes I do. I'm really still learning about how functional testing works, but if there's something I can do let me know. Would you like me to review it?

@vaplas #7 is testing Menu, not View… so the fact that #7 passes tests doesn't help you here :)

I've applied both patches to a fresh 8.3 install, and I'm still getting an error on the three testGets. It's entirely possible this is something I'm doing wrong, though. I've got to go work now, but I'll run them again a bit later on and see if I still get the same result.

for reference, the errors are as follows:

Failed asserting that 200 is identical to 403.
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:385
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:378

Failed asserting that 200 is identical to 401.
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:385
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php:35
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:356

Failed asserting that 200 is identical to 403.
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/ResourceTestBase.php:385
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php:98
/Users/james/Sites/d8dev/docroot/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:356

That's because the patch in #7 is wrong, like I said in #10.

So I tried adding

return AccessResult::allowedIfHasPermission($account, 'access content');

...to ViewsAccessControlHandler.

I've also added that permission to the grantPermissionsToTestedRole function in setUpAuthoriation in the test base file. I get a new error (which is nearly as good a successful result, IMHO), saying:

Failed asserting that two strings are identical.
Expected :{"message":"The \u0027administer views\u0027 permission is required."}
Actual :{"message":"The \u0027access content\u0027 permission is required."}

So I guess I have two questions:

1) Should the permission on on ViewsAccessControlHandler by administer views, rather than access content? (I've tried changing it, but I get the same error I've posted above)
2) I don't really understand why I'm getting an error saying the user needs a permission that I've explicitly assigned in the setup.

So it looks as though, with the following changes, the tests pass:

ViewAccessControlHandler needs the permission condition added to the check access function, as follows:

  public function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'view') {
      return AccessResult::allowedIfHasPermission($account, 'access content');
    }
    else {
      return parent::checkAccess($entity, $operation, $account);
    }
  }

ViewResourceTestBase needs both the administer views and the check access permission:

  protected function setUpAuthorization($method) {
    $this->grantPermissionsToTestedRole([
      'administer views',
      'access content'
    ]);
  }

And finally the getExpectedAuthorizedMessage function on EntityResourceTestBase needs a switch in it to return the correct error on GET:

    $permission = $this->entity->getEntityType()->getAdminPermission();
    if ($permission !== FALSE) {
      switch ($method) {
        case 'GET':
          return "The 'access content' permission is required.";

        default:
          return parent::getExpectedUnauthorizedAccessMessage($method);
      }
    }

The only thing I'm wondering is, will the change to EntityResourcTestBase mess up other people's tests? Should I be extending the ViewsResourceTestBase instead?

OK, right, that seems to work. I've added the following function to ViewResourceTestBase:

  /**
   * {@inheritdoc}
   */
  protected function getExpectedUnauthorizedAccessMessage($method) {

    if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) {
      return $this->getExpectedBCUnauthorizedAccessMessage($method);
    }

    $permission = $this->entity->getEntityType()->getAdminPermission();
    if ($permission !== FALSE) {
      switch ($method) {
        case 'GET':
          return "The 'access content' permission is required.";

        default:
          return parent::getExpectedUnauthorizedAccessMessage($method);
      }
    }
  }

So I should put this all into a patch, I guess?

...which I guess should look like this?

Thanks @vaplas. You're right, it makes sense to switch the permissions like that. I've put both patches together and added the condition on the http verb.

Thanks for all your help with this - I've really learned a lot!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/View/ViewResourceTestBase.php
@@ -0,0 +1,125 @@
+  protected function getExpectedUnauthorizedAccessMessage($method) {
+
+    if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) {

Just one nit! One extraneous \n in here.

Then this is RTBC :)

It's so great to hear from @vaplas that he's learning a lot, then seeing @vaplas mentor @jamesdecq, and then seeing @jamesdecq thank @vaplas! Thank you both for being so helpful, so constructive, and so eager to absorb & spread knowledge!

@vaplas - I think I misunderstood the point you were making in #19 with regard to the extraneous condition. I've removed it now. It's also entirely possible I hadn't drunk enough coffee when I made the patch this morning.

@wim - I'm not sure I understand what you mean. Do you mean there's a line break between the - and > on the pointer? I can see that in the code you've quoted in #24, but I can't see it in the patch I uploaded at #21. Am I missing something?

Let me know and I'll redo the patch.

Great - thanks again!

So, this patch has the stray line ending removed from the getExpectedUnauthorizedAccessMessage function, as well as the unnecessary condition.

#25: great remark! Thanks for spotting that :)

#28: Yep, that's it! Thanks :) Next time, can you please include an interdiff? That makes it easy to see what changes you made. See https://www.drupal.org/documentation/git/interdiff.
For this time, I did that manually, and I can confirm the changes are sound.

Thanks, @jamesdesq & @vaplas!

@wim-leers - I didn't realise interdiff was a thing - but next time I'll definitely use it.

Is there anything that needs to happen, next-steps-wise? Looking at https://www.drupal.org/node/2843756, it seems like @alexpott eventually reviewed it and committed it - which I assume he can do because he's a co-maintainer? So I'm assuming that all the work here is done and we just wait for the patches to get included in a release?

Last up, thanks again to both @wim-leers and @vaplas for everything. I've had a couple of false starts with contributing in the past, but this has been a really positive experience for me, and I'm going to try to pick up another of the outstanding test tickets on the parent issue as soon as I can.

Nope, this is ready! It's RTBC now: Reviewed & Tested By the Community. The next step is that a Drupal core committer takes a look at it, and either finds problems and marks this Needs work, or (s)he commits it and marks it Fixed :)

So, yes, this is now just waiting — nothing more to do here!

I'm very glad you found this such a positive experience — looking forward to seeing you on one of the other remaining test issues on the parent issue!

+++ b/core/modules/views/src/ViewAccessControlHandler.php
@@ -19,7 +19,7 @@ class ViewAccessControlHandler extends EntityAccessControlHandler {
-      return AccessResult::allowed();
+      return AccessResult::allowedIfHasPermission($account, 'access content');

This needs more discussion. As per the other issues. See #2843772-15: EntityResource: Provide comprehensive test coverage for DateFormat entity

Also what doesn't viewing the view entity mean?

Also what doesn't viewing the view entity mean?

I'm assuming this is referring to

+++ b/core/modules/views/src/ViewAccessControlHandler.php
@@ -19,7 +19,7 @@ class ViewAccessControlHandler extends EntityAccessControlHandler {
   public function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
     if ($operation == 'view') {
-      return AccessResult::allowed();
+      return AccessResult::allowedIfHasPermission($account, 'access content');
     }

This code means it was literally impossible to configure your site to disallow access to this entity. Which means you cannot possibly ever deny access to it. Which means anonymous users can always see this.
This is at least a small step forward. May need further refinement.

That being said, this is blocked on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST reaching consensus first.

Consensus was achieved! Quoting #2870018-35: Should the 'access content' permission be used to control access to viewing configuration entities via REST:

1. config entities that do not yet have an access control handler nor an admin_permission (RdfMapping, Tour): add an admin_permission.
2. update all access control handlers for entity types that currently grant access without requiring any permission (View, Menu, File, DateFormat): change them to require the entity type's admin_permission or the entity-type specific "view" permission (yet to be added), so for example: AccessResult::allowedIfHasPermissions($account, ['view menu', 'administer menu'], 'OR').

Fixing the open access is a bug fix.

Committed c0a1c92 and pushed to 8.4.x. Thanks!

I discussed this with @catch because of the class removal. Whilst entity handlers are explicitly not API (see https://www.drupal.org/core/d8-bc-policy) removing a class in a bug fix feels off.

If we want to backport this we should deprecate it in 8.3.x and say it is going to be removed in 8.4.x.

Not sure that this is worth it but if contributors do - feel free to re-open and port the patch to 8.3.x by doing the above steps.

Yay, thanks! Updated #2824572: Write EntityResourceTestBase subclasses for every other entity type..