Unrecognised entity operation passed to Menu Link Content throws exceptions

reroll for 8.6.10

Fix is great. Solves a fatal error on some sites where I have entity_clone enabled. Code is simple, clean and proper.

Not sure what test(s) we should write for this. Seems kinda silly to put a test directly in menu_link_content about this particular bug. Yes, it was an oversight in this module, but it seems like it'd be better to have a (series of) test(s) that automatically try custom access operations for all core entity types. I'm just not clear on where those test(s) should live and exactly how they should work.

Otherwise, I'd RTBC this. ;)

Thanks,
-Derek

Nice, thanks!

1. +++ b/core/modules/block_content/tests/src/Unit/BlockContentEntityAccessControlHandlerTest.php
   @@ -0,0 +1,63 @@
   + * Tests block_content entity access.
   + *
   + * @coversDefaultClass \Drupal\block_content\BlockContentAccessControlHandler
   

   Makes sense to add these as general test classes to flesh out unit coverage for these access control handlers. But they currently *only* test an unrecognized operation. Should we open a meta/plan issue for adding/moving test coverage for the other operations into these? If so, can we add @todo and @see comments about it?

2. +++ b/core/modules/block_content/tests/src/Unit/BlockContentEntityAccessControlHandlerTest.php
   @@ -0,0 +1,63 @@
   +   * Tests the an operation not implemented by the access control handler.
   
   +++ b/core/modules/comment/tests/src/Unit/CommentEntityAccessControlHandlerTest.php
   @@ -0,0 +1,59 @@
   +   * Tests the an operation not implemented by the access control handler.
   
   +++ b/core/modules/contact/tests/src/Unit/ContactMessageEntityAccessControlHandlerTest.php
   @@ -0,0 +1,48 @@
   +   * Tests the an operation not implemented by the access control handler.
   
   +++ b/core/modules/file/tests/src/Unit/FileEntityAccessControlHandlerTest.php
   @@ -0,0 +1,60 @@
   +   * Tests the an operation not implemented by the access control handler.
   

   /Tests the/Tests/

   (global search/replace, this comment is copied to all the new test classes).

3. +++ b/core/modules/block_content/tests/src/Unit/BlockContentEntityAccessControlHandlerTest.php
   @@ -0,0 +1,63 @@
   +    $accessControl = new BlockContentAccessControlHandler($entityType, $eventDispatcher);
   +    $accessControl->setModuleHandler($moduleHandler);
   

   Does it make sense to move these down to the bottom of this method so we can do all the mocking up front? Might be easier to read these tests if they all end with:

   ...
     $account = $this->createMock(AccountInterface::class);
   
     // Done mocking, create and call the access handler.
     $accessControl = new BlockContentAccessControlHandler($entityType, $eventDispatcher);
     $accessControl->setModuleHandler($moduleHandler);
     $access = $accessControl->access($entity, $this->randomMachineName(), $account, TRUE);
     $this->assertInstanceOf(AccessResultInterface::class, $access);
   

4. +++ b/core/modules/block_content/tests/src/Unit/BlockContentEntityAccessControlHandlerTest.php
   @@ -0,0 +1,63 @@
   +    $this->assertInstanceOf(AccessResultInterface::class, $access);
   

   I might be wrong, but don't we want to assert this is actually AccessResultNeutral, not just any AccessResult? On quick pondering, I can't imagine why any core entity type should do anything other than ignore operations they don't understand.

5. +++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
   @@ -415,4 +420,27 @@ class UserAccessControlHandlerTest extends UnitTestCase {
   +  public function testUnrecognisedOperation() {
   +    $moduleHandler = $this->createMock(ModuleHandlerInterface::class);
   +    $moduleHandler->expects($this->exactly(2))
   +      ->method('invokeAll')
   +      ->willReturn([]);
   +    $this->accessControlHandler->setModuleHandler($moduleHandler);
   +
   +    $language = $this->createMock(LanguageInterface::class);
   +    $language->expects($this->any())
   +      ->method('getId')
   +      ->will($this->returnValue('de'));
   +    $entity = $this->createMock(UserInterface::class);
   +    $entity->expects($this->any())
   +      ->method('language')
   +      ->willReturn($language);
   +    $account = $this->createMock(AccountInterface::class);
   +    $access = $this->accessControlHandler->access($entity, $this->randomMachineName(), $account, TRUE);
   +    $this->assertInstanceOf(AccessResultInterface::class, $access);
   +  }
   

   In spite of some other differences, this entire function is almost exactly replicated in every other test class you added, except "UserInterface::class" itself.

   Makes me think these should all either extend the same shared base class, or at least a test trait. This whole thing is too much copy/paste, not just the comment. ;)

p.s. When you upload patch and test only (either separately or in the same comment), please upload the test-only first, then the full patch. Otherwise, our test bots get confused. You always want the full working thing to be "the last submitted patch".

Thanks again!
-Derek

Closed #3070337: MenuLinkContent access check may not return an AccessResult as a duplicate.

@dpi re: #14: Thanks for moving this forward again!

Thanks for all the linked issues. You forgot to link #3103262: Improve entity access test coverage, the parent, which is useful. Adding that here as a related issue.

> /Tests the/Tests/

Got it!

Alas, you did /Tests the/Test/ but core conventions are that doc blocks start with action verbs ending in 's'.

So now we need: s/Test/Tests/ ;) Sorry about NW for this, but it'll save time in the end so core committers don't complain about this.

I dont think it matters what result we get back. We are not asserting that no other alters impact the result.

I think it matters a great deal. ;) Anything dealing with access control should be rigorously tested, since bugs usually will count as security holes (access bypass, etc). I don't think it's enough to assert "we got some kind of access result". I think we should assert "we got the correct access result". ;) It's not up to me. But if I were a core committer, that's what I'd think. Whether we should fix this now or later is left as an exercise for the interested reader. ;)

> Makes me think these should all either extend the same shared base class

Yeh perhaps. Further optimisation can be made in the future with abstracts and traits. Because these are Unit tests, each does the minimum required logic for each implementation to get it working, I'm hesitant to refactor these into a common class.

Hrm. I agree further optimization can be a follow-up. However, I don't buy the argument that Unit tests change the nature of the basic desire to put duplicate code in a shared place. DRY. ;) Anyway, I think all this can happen as part of #3103262 and not hold this up anymore.

Thanks,
-Derek

Coming back to this issue after so long, I'm somewhat rethinking whether all these tests are necessary for this issue. Its a bit out of scope.

I agree the scope seems a bit out of hand. Why not fix MenuLinkContentAccessControlHandler, introduce the test coverage and file follow-ups to prevent regressions on other entity types?

FWIW, the code itself looks great, it's just given this has a significant negative impact on sites using common contribs, it would be good to commit this asap.

I agree the scope seems a bit out of hand. Why not fix MenuLinkContentAccessControlHandler, introduce the test coverage and file follow-ups to prevent regressions on other entity types?

FWIW, the code itself looks great, it's just given this has a significant negative impact on sites using common contribs, it would be good to commit this asap.

Agree

Agree with larowlan. Code changes are looking good we should move this issue forward.

Looking for consensus on how to proceed with this. I'm happy to prune the patch back to just the MenuLinkContentAccessControlHandler changes and associated test, and move the tests for the other entity types into the issues raised by @dpi. Just want to make sure that's what everyone thinks is the correct approach :)

Here's the cut down patch for now.

I think it should call the parent's checkAccess() rather than returning neutral as the result directly.
In a scenario where a custom operation is allowed if the user has the default administrative permission.
In this case, it should return AccessResult::allowedIfHasPermission($account, $admin_permission) rather than AccessResult::neutral()

So following codes:

  default:
      return AccessResult::neutral();

should be replaced with:

    default:
        return parent::checkAccess($entity, $operation, $account);

I used patch #14 for 8.8.8 version and its working +1

Re #21 I'm not sure that's necessary. Other access control handlers in core do a similar thing such as FileAccessControlHandler (i.e just return neutral).

1. Looking for consensus on how to proceed with this. I'm happy to prune the patch back to just the MenuLinkContentAccessControlHandler changes and associated test, and move the tests for the other entity types into the issues raised by @dpi. Just want to make sure that's what everyone thinks is the correct approach :)

   Agree with this in #21, focusing on MenuLinkContentAccessControlHandler relevant.

2. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
   @@ -74,6 +74,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        return AccessResult::neutral();
   

   I agree with the suggestion in #21

   return parent::checkAccess($entity, $operation, $account);
   

   , even though @acbramley replied in #25

   Re #21 I'm not sure that's necessary. Other access control handlers in core do a similar thing such as FileAccessControlHandler (i.e just return neutral).

   Calling parent by default makes sense as the default behavior. And \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess() returns return AccessResult::neutral(); by default too if no opinion.

3. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +   * Test an operation not implemented by the access control handler.
   

   Should start with a verb. "Test" -> "Tests"

Otherwise it's good to go to me.

Thanks for the review @jungle! I've fixed those 2 things up.

Looks good to me. I just have a few suggestions, but no complaints.

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
   @@ -74,6 +74,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +      default:
   +        return parent::checkAccess($entity, $operation, $account);
   

   This is a really elegant fix.

2. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +  /**
   +   * Tests an operation not implemented by the access control handler.
   +   */
   +  public function testUnrecognisedOperation() {
   

   The test has @coversDefaultClass, so this method should have a @covers ::checkAccess annotation.

3. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +    $accessManager = $this->createMock(AccessManagerInterface::class);
   +    $moduleHandler = $this->createMock(ModuleHandlerInterface::class);
   +    $moduleHandler->expects($this->any())
   +      ->method('invokeAll')
   +      ->willReturn([]);
   +
   +    $language = $this->createMock(LanguageInterface::class);
   +    $language->expects($this->any())
   +      ->method('getId')
   +      ->will($this->returnValue('de'));
   +
   +    $entity = $this->createMock(ContentEntityInterface::class);
   +    $entity->expects($this->any())
   +      ->method('language')
   +      ->willReturn($language);
   +
   +    $account = $this->createMock(AccountInterface::class);
   

   Nit: This all works fine, but it might be a little easier to read if we use Prophecy instead. Compare this:

   $entity->expects($this->any())->method('language')->willReturn($language);
   

   to this:

   $entity->language()->willReturn($language)

   See what I mean?

1. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +  public function testUnrecognisedOperation() {
   

   British spelling. We want testUnrecognizedOperation().

2. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +    $access = $accessControl->access($entity, $this->randomMachineName(), $account, TRUE);
   

   Potential false positive if randomMachineName() randomly returns something valid? ;) Should we hard-code something we know is invalid, instead?

3. +++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
   @@ -0,0 +1,52 @@
   +    $this->assertInstanceOf(AccessResultInterface::class, $access);
   

   As above (#10.4), I still think this isn't great, and we should assert we got the result we expect, not that we got any possible AccessResult. But I'm willing to be out-voted on this. ;)

x-post, but agreed with #28.1 -- I almost flagged the same lines and wrote something similar! ;)

we should assert we got the result we expect, not that we got any possible AccessResult.

I respectfully disagree :) In this case, the problem isn't that the wrong access result is coming back -- it's that nothing is. I think this test is perfectly covering the scope of this issue.

Fixes 28.2, 29.1, and 29.2

I tried 28.3 but got
Error: Call to a member function willReturn() on null

Will leave as is for now.

All child issues created in #14 now have patches on them with the test coverage provided by @dpi + the changes from reviews.

👍 Ship it!

+++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkContentEntityAccessTest.php
@@ -0,0 +1,54 @@
+    $entityType = $this->createMock(EntityTypeInterface::class);
+    $accessManager = $this->createMock(AccessManagerInterface::class);
+    $moduleHandler = $this->createMock(ModuleHandlerInterface::class);
+    $moduleHandler->expects($this->any())
...
+    $accessControl = new MenuLinkContentAccessControlHandler($entityType, $accessManager);
+    $accessControl->setModuleHandler($moduleHandler);

should not use camelCase naming variables. Thanks!

If I remember correctly, the coding standards do allow camel case variables, as long as it's kept consistent within the same file. So I don't think we actually need to change anything here; restoring RTBC. (Correct me if I'm wrong!)

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.

From https://www.drupal.org/docs/develop/standards/coding-standards#s-functio...

saving issue credits

Committed 5920411 and pushed to 9.1.x. Thanks!

c/p to 9.0.x

Waiting for 8.9.x test result

Thanks for committing, but still uploading the rewrote Test here using Prophecy.

Because this is a bug, involves a fatal error, impacts contrib-projects and is non-disruptive, backported to 8.9.x

Discussed the existing follow-up with @jungle in slack as best place for moving to prophecy