EntityResource: Provide comprehensive test coverage for Menu entity

Looks perfect, thanks!

+++ b/core/modules/system/src/MenuAccessControlHandler.php
@@ -19,7 +19,7 @@ class MenuAccessControlHandler 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

@vaplas - we still need to discuss what is the right generic approach to viewing config entities on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST

Yep, 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 #8 already changed it in the necessary way! :D

+++ b/core/modules/system/src/MenuAccessControlHandler.php
@@ -18,11 +18,8 @@ class MenuAccessControlHandler extends EntityAccessControlHandler {
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
-    if ($operation === 'view') {
-      return AccessResult::allowed();
-    }
     // Locked menus could not be deleted.
-    elseif ($operation == 'delete') {
+    if ($operation === 'delete') {
       if ($entity->isLocked()) {
         return AccessResult::forbidden()->addCacheableDependency($entity);
       }

I do not see the mentioned #12

AccessResult::allowedIfHasPermissions($account, ['view menu', 'administer menu'], 'OR')

This issue is on 8.3.x. Is that correct?

Changing access control handlers means this can only be 8.4.x, good catch.

You don't see the "if has VIEW or ADMIN permission", because that'd require adding new permissions, which has yet other implications. That was the principle, but in applying the principle, we cannot add permissions in these issues; adding VIEW permissions must happen in other issues.

And per #2870018-47: Should the 'access content' permission be used to control access to viewing configuration entities via REST as well as #2843772-23: EntityResource: Provide comprehensive test coverage for DateFormat entity and subsequent comments, we must convert the view operation to the view label operation in the access control handler, to not break BC. Full test coverage added.

+++ b/core/modules/system/src/MenuAccessControlHandler.php
@@ -17,14 +17,20 @@ class MenuAccessControlHandler extends EntityAccessControlHandler {
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
-    if ($operation === 'view') {
+    // There are no restrictions on viewing the label of a date format.
+    if ($operation === 'view label') {
       return AccessResult::allowed();
     }
     // Locked menus could not be deleted.
-    elseif ($operation == 'delete') {
+    elseif ($operation === 'delete') {
       if ($entity->isLocked()) {
-        return AccessResult::forbidden()->addCacheableDependency($entity);
+        return AccessResult::forbidden('The Menu config entity is locked.')->addCacheableDependency($entity);

Looking at this access controller and #2843772: EntityResource: Provide comprehensive test coverage for DateFormat entity aren't a bunch of config entity access controllers going to be exactly the same?

Would it make sense to make

class ConfigEntityAccessController extends EntityAccessControllor

Then we could set all config entities that follow this pattern to use ConfigEntityAccessController directly?

+++ b/core/modules/system/tests/src/Kernel/MenuAccessControlHandlerTest.php
@@ -0,0 +1,149 @@
+class MenuAccessControlHandlerTest extends KernelTestBase {

If we can't do ConfigEntityAccessController could either have
ConfigEntityAccessHandlerTestBase extends KernelTestBase
or ConfigEntityAccessHandlerTrait

Looking at #2843772: EntityResource: Provide comprehensive test coverage for DateFormat entity all these tests are going to be very similar. Just a thought.

If we don't want to do any of that refactoring I think this good to RTBC!

Then we could set all config entities that follow this pattern to use ConfigEntityAccessController directly?

That's … a very good point :) Although I have to add that isLocked() is not actually on a generic interface yet. I think extracting a LockableInterface is going to be a hard requirement. I count at least 5 config entity types that'd need to use it (DateFormat, NodeType, Menu, ConfigurableLanguage, FieldStorageConfig). Then this issue would have to be blocked on that one.

On the other hand, I think many of these access control handlers will start to diverge over time. Their access control will get more refined. Particularly for complex and broadly/frequently used config entities like Menu.

So I personally think it's not worth it.

Ok. sounds good. RTBC!

Committed 5e2e4a8 and pushed to 8.4.x. Thanks!

Thanks!