EntityResource: Provide comprehensive test coverage for Shortcut entity

Took the help of https://www.drupal.org/node/2832859 MenuLinkContent entity and putting up a patch. If this works, planning on patches for other entities as well.

Resolved a silly error. NR to trigger test.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Shortcut/ShortcutResourceTestBase.php
@@ -0,0 +1,132 @@
+  protected function setUpAuthorization($method) {
+	switch ($method) {
+	  case 'GET':
+		$this->grantPermissionsToTestedRole(['access shortcuts']);
+		break;
+	  case 'POST':
+	  case 'PATCH':
+	  case 'DELETE':
+		$this->grantPermissionsToTestedRole(['administer shortcuts']);
...
+	$shortcut = Shortcut::create(array(
+	  'shortcut_set' => 'default',
+	  'title' => t('Comments'),
+	  'weight' => -20,
+	  'link' => array(
+		'uri' => 'internal:/admin/content/comment',
+	  ),
+	));

Please convert that to spaces, sorry :(

@dawehner will remove.

Need help in resolving following.

1) Drupal\Tests\hal\Functional\EntityResource\Shortcut\ShortcutHalJsonAnonTest::testGet
Failed asserting that 403 is identical to 200.

2) Drupal\Tests\hal\Functional\EntityResource\Shortcut\ShortcutHalJsonAnonTest::testPost
Failed asserting that 400 is identical to 403.

3) Drupal\Tests\hal\Functional\EntityResource\Shortcut\ShortcutHalJsonAnonTest::testPatch
Failed asserting that 400 is identical to 403.

should i catch error and assert this??

some hal+json fixes.

Please provide interdiffs in the future. Otherwise it's impossible to review differences between previous versions of the patch. For example, it's very hard to tell what changed between #6 and #12. See https://www.drupal.org/documentation/git/interdiff.

The problems you ran into in #9 are all due to bugs:

1. Not all necessary permissions are granted: users also need the customize shortcut links permission.
2. ShortcutResourceTestBase::getNormalizedPostEntity() is wrong in two significant ways: it's not sending a bundle (this was causing the 400), plus it's sending a non-existing name field, which should be the title field.
3. Same root cause as 2.

I fixed just those problems. This patch will still come back red, because there are more problems to solve. But at least you can now continue :)

Finally: you're still using that work-around trait, which means this is 8.2.x-specific. This likely won't get committed to 8.2.x anymore, since 8.4.x was just branched. So, it's best to just remove that altogether.

Back to you, @sumanthkumarc!

@wimleers Thanks. This is my first time in writing tests and learned a lot already. Also, Thanks for the help there.

I'll provide a new patch and interdiff with above points. :)

Great, thanks!

@sumanthkumarc Any progress on this? Do you mind if i take a shot at this?

@himanshu-dixit sure, i'm just busy for sometime. Unassigning myself.

It looks like the tests are passing without 'administer shortcuts' permission, I do not know how good this news?

That's how \Drupal\shortcut\ShortcutAccessControlHandler (well, \shortcut_set_edit_access()) is designed to work. So this is fine.

1. +++ b/core/modules/shortcut/shortcut.routing.yml
   @@ -58,9 +58,9 @@ entity.shortcut.canonical:
      path: '/admin/config/user-interface/shortcut/link/{shortcut}'
      defaults:
        _entity_form: 'shortcut.default'
   -    _title: 'Edit'
   +    _title: 'View'
      requirements:
   -    _entity_access: 'shortcut.update'
   +    _entity_access: 'shortcut.view'
        shortcut: \d+
   

   Why would we be changing this? This is a form to edit a shortcut entity. Requiring view access seems very wrong.

2. +++ b/core/modules/shortcut/src/ShortcutAccessControlHandler.php
   @@ -52,7 +52,12 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +      if ($access->isNeutral()) {
   +        $message = sprintf("The 'access shortcuts' AND 'customize shortcut links' permissions is required to %s this shortcut entity of bundle %s.", $operation, $entity->bundle());
   +        $access = AccessResult::neutral($message)->cachePerPermissions()->addCacheableDependency($shortcut_set);
   +      }
   
   @@ -64,7 +69,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +      if ($access->isNeutral()) {
   +        $access = AccessResult::neutral("The 'access shortcuts' AND 'customize shortcut links' permissions is required to create this shortcut entity of bundle $entity_bundle.")->cachePerPermissions()->addCacheableDependency($shortcut_set);
   +      }
   

   Why not move this logic directly into shortcut_set_edit_access()?

Yes, the Shortcut module is a mess. Not something we need to solve here though :)

This patch is perfect now! 99% test coverage. 1% surgical modification to shortcut module.

+++ b/core/modules/shortcut/shortcut.module
@@ -65,7 +65,10 @@ function shortcut_set_edit_access(ShortcutSetInterface $shortcut_set = NULL) {
+  if (!$may_edit_current_shortcut_set) {
+    return AccessResult::neutral("The 'access shortcuts' AND 'customize shortcut links' permissions is required.")->cachePerPermissions();
+  }
+  return AccessResult::allowed()->cachePerPermissions();

This is fine!

However, if the committer doesn't like this, it could be changed to this alternative:

$result =  AccessResult::allowedIf($may_edit_current_shortcut_set)->cachePerPermissions();
if (!$result->isAllowed()) {
  $result->setReason(…);
}
return $result;

Thanks! :)

Thanks, great work @vaplas! :)

Looks great!

+++ b/core/modules/shortcut/shortcut.module
@@ -65,7 +65,11 @@ function shortcut_set_edit_access(ShortcutSetInterface $shortcut_set = NULL) {
   // Sufficiently-privileged users can edit their currently displayed shortcut
   // set, but not other sets. They must also be able to access shortcuts.
   $may_edit_current_shortcut_set = $account->hasPermission('customize shortcut links') && (!isset($shortcut_set) || $shortcut_set == shortcut_current_displayed_set()) && $account->hasPermission('access shortcuts');
-  return AccessResult::allowedIf($may_edit_current_shortcut_set)->cachePerPermissions();
+  $result = AccessResult::allowedIf($may_edit_current_shortcut_set)->cachePerPermissions();
+  if (!$result->isAllowed()) {
+    $result->setReason("The 'access shortcuts' AND 'customize shortcut links' permissions is required.");
+  }
+  return $result;

The words here are not the same as the logic defining $may_edit_current_shortcut_set. There is something about it being the currently displayed set too. This also makes me wonder if the we should also be doing ->addCacheableDependency($shortcut_set) if the shortcut_set is provided. The caching is out-of-scope here but should have a separate issue to discuss. What is in scope is this message. I'm not sure of what it should be but I know it needs to correctly describe the logic... which is described by:

  // Sufficiently-privileged users can edit their currently displayed shortcut
  // set, but not other sets. They must also be able to access shortcuts.

So maybe something like:
"The shortcut set must be the currently displayed set for the user and the user must have 'access shortcuts' AND 'customize shortcut links' permissions."

Not sure.

Good catch!

I forgot for a moment that unfortunately the "reason" message for denying access must reflect the actual complexity of the access checking, regardless of how insane it is (yes, I think shortcut access checking is insane, and I think most would agree).

I think your proposal makes sense. It's complex and confusing, but again… that is only because the access checking is complex and confusing. We must convey all information/requirements, otherwise the "reason" still is incomplete.

Thanks for your diligence.

@alexpott pointed out in IRC that we actually want/need addCacheableDependency() in shortcut_set_edit_access(). But fixing that is out of scope here.

I was fairly certain this problem had been reported before. And in fact… \Drupal\shortcut\ShortcutSetAccessControlHandler::checkAccess() gets it right! So the problem is more that shortcut_set_edit_access() is a pointless/stale/wrong duplicate. We already have #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions to fix that. So the follow-up already exists :)

@Wim Leers so should we be adding the setReason to \Drupal\shortcut\ShortcutSetAccessControlHandler::checkAccess() to avoid surprises when we do deprecate? Not sure.

#35: when we remove shortcut_set_edit_access(), we'll get test failures for the test coverage added here, because the expected reason will not be present in the 403 response. So I think it's safe to say we won't forget to move this over.

Committed and pushed e1524ff to 8.4.x and 60f5d00 to 8.3.x. Thanks!

The change to shortcut_set_edit_access() is purely informational so there aren't any BC implications.