EntityResource: Provide comprehensive test coverage for Tour entity

This patch adds an access handler to Tour entity. Let's see what fails.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Tour/TourResourceTestBase.php
   @@ -0,0 +1,128 @@
   +/**
   + * ResourceTestBase for Tour entity.
   + */
   ...
   +   * The Tour entity.
   

   Let's drop these comments for consistency with the other test coverage.

2. +++ b/core/modules/tour/src/TourAccessControlHandler.php
   @@ -0,0 +1,24 @@
   +  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
   +    return AccessResult::allowedIfHasPermission($account, 'access tour');
   +  }
   

   This is wrong, an dangerous!

   This grants access to all operations on tours based just on 'access tour'.

   What we want instead is to add a admin_permission to the Tour entity annotation, and then make this call the parent method except for the view operation. For the view operation, we want the logic you've written here.

Will something like this work? (see patch).

If we add an admin_permission to the Tour entity, do we need a new administer tours permission as well?

Thanks for the review Wim. Learning a lot while working on these tests.

Thanks for the review Wim. Learning a lot while working on these tests.

Great :) That's what I was hoping!

If we add an admin_permission to the Tour entity, do we need a new administer tours permission as well?

That's a great question. I didn't know the answer myself, so I looked it up: FilterFormat has an admin_permission… so let's see if that permission is also listed in filter.permissions.yml… it is! So, the answer is yes :)

OK. Ready for the bots.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Tour/TourResourceTestBase.php
   @@ -0,0 +1,123 @@
   +          'body' => 'Who handle the awesomeness of llamas?',
   

   :)

2. +++ b/core/modules/tour/src/TourAccessControlHandler.php
   @@ -0,0 +1,28 @@
   +    if ($operation == 'view') {
   

   ===

Once point 2 is addressed, this is RTBC.

Here is the new patch.

Random fail in Drupal\Tests\inline_form_errors\FunctionalJavascript\FormErrorHandlerCKEditorTest.

Committed 431461b and pushed to 8.4.x. Thanks!

Committed to 8.4.x - going to discuss with other committers before backporting to 8.3.x. The new permission and access control handler require a bit more discussion.

Discussed with @xjm. We agreed that there is an issue with issue scope here. We need a better issue title and issue summary because this is introducing non test changes and fixing missing things. Like the lack of the admin permission - is that a bug, security hole (I don;'t think so because I don't think that by enabling rest on tour entities you can create them in HEAD) or a task because we adding missing stuff?

#17 is identical to #2843767-22: EntityResource: Provide comprehensive test coverage for BaseFieldOverride entity + add missing access control handler, so repeating my answer from there:

a task because we adding missing stuff?

This.

It'a a task, because many (most) config entity types do not specify an access control handler, nor an admin_permission.

See #2870018-6: Should the 'access content' permission be used to control access to viewing configuration entities via REST for more detail.

---

This is blocked on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST reaching consensus.

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

Turns out the patch in #12 that was committed to 8.4.x already changed it in the necessary way! :D

I think this should not be backported to 8.3.x, because it'd introduce A) a new permission, B) would introduce an access control handler. Disruption is likely close to zero, but then again very few people are likely to want to access tours via REST. So no need to do even remotely risky things in 8.3.x. They can wait for 8.4.x. If they really want this faster, they can apply this patch themselves to 8.3.x. and then stop using it once they update to 8.4.x.

So marking Fixed.

Actually, no, there's a tiny difference between what the patch in #12 does compared to what the consensus agreed on.

Also, the commit was never actually pushed — HAH!

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Tour/TourResourceTestBase.php
   @@ -0,0 +1,123 @@
   +          'body' => 'Who handle the awesomeness of llamas?',
   

   handle*s* ? :)

2. +++ b/core/modules/tour/tour.permissions.yml
   @@ -1,2 +1,6 @@
   +  title: 'Administer tours'
   

   This is a UI addition, lets make sure to add screenshots each time a permission is added in this multi-issue effort. It's a bit weird to expose a persmission for something that does not also provide a UI for it. I had a look at the permissions page and didn't find another example of this. Together with the "give to trusted users only" warning this is scary: enabling a permission to do something without a UI for it, what kind of unkonwn door does this open then?

   Put another way: when would I want/need to enable this permission? Since there's no UI for it, should we maybe explain that a bit in the permission description?

#21:

1. LOL, wonderful nitpick!
2. Ugh… that makes me wonder whether we should make this use the administer site configuration permission instead?

Needs yoroy's feedback.

I can't say because it's not clear to me why the permission is needed in the first place.

I think going with administer site configuration makes sense — we did the same for DateFormat and RdfMapping. We did that in #2843771: EntityResource: Add an admin permission to RdfMapping entity and provide comprehensive test coverage, which landed a week after #21–#24.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Tour/TourResourceTestBase.php
   @@ -0,0 +1,123 @@
   +    return "The following permissions are required: 'access tour' OR 'administer tour'.";
   

   The 'administer tour' is not longer in tour.permissions.yml file. Why doesn't this fail?

2. +++ b/core/modules/tour/src/TourAccessControlHandler.php
   @@ -0,0 +1,28 @@
   +      return AccessResult::allowedIfHasPermissions($account, ['access tour', 'administer tour'], 'OR');
   

   The 'administer tour' is not longer in tour.permissions.yml file. It was removed in this patch.

:O

Oh wait, this is normal.

Permission checking never validates that the passed in permissions actually exist. Yes, it is stupid. Yes, it is infuriating. Yes, it is a bad DX. Yes, this is why I insisted we have validation for cache tags & contexts, to prevent exactly that problem — see \Drupal\Core\Cache\Cache::mergeContexts().

However, fixing that for permissions is vastly out of scope here.

The cause is much more innocent here: the expected error message is correct, because I forgot to update the access control handler. And since permissions are not validated, it just runs/works.

Ok 2 points in #26 are fixed

RTBC!

Committed/pushed to 8.4.x, thanks!

'administer site configuration' seems like the right choice here.

Thanks!