Support from Acquia helps fund testing for Drupal Acquia logo

Comments

harings_rob created an issue. See original summary.

harings_rob’s picture

Assigned: Unassigned » harings_rob
harings_rob’s picture

Issue summary: View changes
harings_rob’s picture

Hi,

Attached I have added the patch. But I am running into a knowledge issue here.

The test seems pretty straight forward, so I guess that I either:
- Completely misunderstand the logic,
- Menu link content permissions don't make sense/Menu link content should be a config entity
CheckAccess seems to always require 'administer menu': \Drupal\menu_link_content\MenuLinkContentAccessControlHandler::checkAccess

Some hints/guidance would be welcome here.

Wim Leers’s picture

Status: Active » Needs work

Woot, thanks Rob!

To answer your questions regarding access handling: you're right that it's strange right now. We have similar strangeness for vocabulary & term entities, we have #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission + #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission to fix that. You could open a similar issue for menu_link_content entities (but please don't do it in this issue, that would be out of scope). I think it'd make sense to add a more granular permission for viewing Menu Link content entities.

  1. This is missing a JSON + cookie test.
  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonAnonTest.php
    @@ -0,0 +1,46 @@
    + * MenuLinkContent anonymous hal json test.
    
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonBasicAuthTest.php
    @@ -0,0 +1,33 @@
    + * MenuLinkContent basic auth hal json test.
    

    The other EntityResourceTest subclasses also don't have these comments, so let's not start doing so here.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
    @@ -0,0 +1,185 @@
    +  protected function setUpAuthorization($method) {
    +    switch ($method) {
    +      case 'GET':
    +        $this->grantPermissionsToTestedRole(['administer menu']);
    +        break;
    +
    +      case 'POST':
    +      case 'PATCH':
    +      case 'DELETE':
    +        $this->grantPermissionsToTestedRole(['administer menu']);
    +        break;
    +    }
    +  }
    

    All HTTP methods require the same permission for now, so we can simplify this.

webflo’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
10.22 KB

Status: Needs review » Needs work

The last submitted patch, 6: 2832859-6.patch, failed testing.

Wim Leers’s picture

Woah, @webflo is in the house! I didn't know you were working on REST stuff :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

webflo’s picture

I work on it during SprintWeekend.

sumanthkumarc’s picture

@webflo, i'm also working on similar issue, https://www.drupal.org/node/2843750 and following you on this. Any directions for me??

webflo’s picture

Status: Needs work » Needs review
FileSize
12.47 KB
8.44 KB

Some progress on hal+json.

webflo’s picture

Version: 8.4.x-dev » 8.3.x-dev
FileSize
12.59 KB

Status: Needs review » Needs work

The last submitted patch, 13: 2832859-13.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 2832859-13.patch, failed testing.

webflo’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -561,7 +561,7 @@ public function testPost() {
-    $label_field_capitalized = ucfirst($label_field);
+    $label_field_capitalized = $this->entity->getFieldDefinition($label_field)->getLabel();

@@ -751,7 +751,7 @@ public function testPatch() {
-    $label_field_capitalized = ucfirst($label_field);
+    $label_field_capitalized = $this->entity->getFieldDefinition($label_field)->getLabel();

Nice improvements to the base test class! :)

This looks perfect, thanks! :)

The last submitted patch, 12: 2832859-12.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ef0a872 and pushed to 8.4.x. Thanks!
Committed 48ec098 and pushed to 8.3.x. Thanks!

diff --git a/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonBasicAuthTest.php b/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonBasicAuthTest.php
index eb5e705..3af8362 100644
--- a/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonBasicAuthTest.php
+++ b/core/modules/hal/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentHalJsonBasicAuthTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\hal\Functional\EntityResource\MenuLinkContent;
 
-use Drupal\Tests\hal\Functional\EntityResource\MenuLinkContent\MenuLinkContentHalJsonAnonTest;
 use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait;
 
 /**
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonBasicAuthTest.php b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonBasicAuthTest.php
index 9be9e55..952088b 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonBasicAuthTest.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonBasicAuthTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\rest\Functional\EntityResource\MenuLinkContent;
 
 use Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait;
-use Drupal\Tests\rest\Functional\JsonBasicAuthWorkaroundFor2805281Trait;
 
 /**
  * @group rest
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonCookieTest.php b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonCookieTest.php
index 155b853..73b4567 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonCookieTest.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentJsonCookieTest.php
@@ -1,10 +1,5 @@
 <?php
 
-/**
- * @file
- * Contains \Drupal\Tests\rest\Functional\EntityResource\MenuLinkContent\MenuLinkContentJsonCookieTest.
- */
-
 namespace Drupal\Tests\rest\Functional\EntityResource\MenuLinkContent;
 
 use Drupal\Tests\rest\Functional\CookieResourceTestTrait;
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
index 37117eb..ee886c0 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
@@ -170,7 +170,7 @@ protected function getExpectedNormalizedEntity() {
            'value' => '1',
          ],
        ],
-      'parent' => [],
+       'parent' => [],
      ];
   }
 

Coding standards fixed on commit.

  • alexpott committed ef0a872 on 8.4.x
    Issue #2832859 by webflo, harings_rob, Wim Leers: Write...

  • alexpott committed 48ec098 on 8.3.x
    Issue #2832859 by webflo, harings_rob, Wim Leers: Write...

  • xjm committed 730bb1a on 8.3.x
    Revert "Issue #2832859 by webflo, harings_rob, Wim Leers: Write...

  • xjm committed 5f178e3 on 8.4.x
    Revert "Issue #2832859 by webflo, harings_rob, Wim Leers: Write...
xjm’s picture

Status: Fixed » Needs work

Unfortunately, this seems to be failing in HEAD, so I reverted it. I'll add retests on the patch.

E.g.: https://www.drupal.org/pift-ci-job/596766

Wim Leers’s picture

Issue tags: +Needs reroll

Ah yes, the failing tests make sense: this needs a reroll after #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.

Apparently the RTBC queue is so long that testbot didn't actually do that yet, despite it landing about 2 days before this got committed.

jofitz’s picture

Issue tags: -Needs reroll

Patch applies successfully on both 8.3.x and 8.4.x so does not appear to be a re-roll, but still Needs Work to address test failures.

harings_rob’s picture

Are the failures happening due to normalization?

Wanted to take a look, but not sure where to start

1) Drupal\Tests\hal\Functional\EntityResource\MenuLinkContent\MenuLinkContentHalJsonAnonTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"The \u0027administer menu\u0027 permission is required."}
+{"message":""}

This seems like an actual bug in the implementation (Meaning test is ok). Needs a new issue?

TestPatch:
Failed asserting that two strings are identical.
Expected :{"message":"No route found for \u0022PATCH admin\/structure\/menu\/item\/1\/edit\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}
Actual   :{"message":"No route found for \u0022PATCH \/admin\/structure\/menu\/item\/1\/edit\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}

This looks to me a formatting issue, might be due to my environment.. The same happens on Post and Delete..

Can we trigger testbot again?

Wim Leers’s picture

#28: no that means this entity type has unauthorized access error messages that deviate from the default ones. That's fine.

It means we just need to override getExpectedUnauthorizedAccessMessage(). For an example, see \Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::getExpectedUnauthorizedAccessMessage().

harings_rob’s picture

Ok so what about the other one?

Wim Leers’s picture

All three errors are about that AFAICT :)

Anonymous’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
3.4 KB

#20 coding standards + #29 getExpectedUnauthorizedAccessMessage + cheat.

About cheat:

+++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
@@ -56,7 +56,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-          return AccessResult::neutral()->cachePerPermissions();
+          return AccessResult::neutral("The 'administer menu' permission is required.")->cachePerPermissions();

It seems neutral() still don't tell anything. But maybe we need bit more refactoring this part of code. Example:

protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
  $access = AccessResult::allowedIfHasPermission($account, 'administer menu');
  switch ($operation) {
    case 'view':
      // There is no direct viewing of a menu link, but still for purposes of
      // content_translation we need a generic way to check access.
      return $access;
    case 'update':
      if ($access->isAllowed()) {
        if (($url_object = $entity->getUrlObject()) && $url_object->isRouted()) {
          $link_access = $this->accessManager->checkNamedRoute($url_object->getRouteName(), $url_object->getRouteParameters(), $account, TRUE);
          $access = $access->andIf($link_access);
        }
      }
      else {
        return $access;
      }
    case 'delete':
      return AccessResult::allowedIf(!$entity->isNew() && $access->isAllowed())->cachePerPermissions()->addCacheableDependency($entity);
  }
}

:)

Anonymous’s picture

one of fix not included :(

shadcn’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
@@ -0,0 +1,193 @@
+       'uuid' => [

Nit: indentation. Thanks :)

Anonymous’s picture

let's fix it + few commas.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.67 KB
5.69 KB

I compared #17 and #35 by applying both locally, and diffing them. Because it also fixes an indentation issue, I also had to use --word-diff.

This looks perfect.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2832859-34.patch, failed testing.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community

@Wim Leers, thank you for review! Come back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2832859-34.patch, failed testing.

Anonymous’s picture

The last submitted patch, 35: 2832859-34.patch, failed testing.

The last submitted patch, 35: 2832859-34.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Wim Leers’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed e4461e6 to 8.4.x and 0dcbf65 to 8.3.x. Thanks!

I've backported this to 8.3.x as it 99.9% additional test coverage and the remainer just helps with REST adoption and has no BC concerns.

+++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
@@ -56,7 +56,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-          return AccessResult::neutral()->cachePerPermissions();
+          return AccessResult::neutral("The 'administer menu' permission is required.")->cachePerPermissions();

This is the one non test change in the patch and it's helpful and totally non-controversial.

  • alexpott committed e4461e6 on 8.4.x
    Issue #2832859 by vaplas, webflo, harings_rob, Wim Leers: Write...

  • alexpott committed 0dcbf65 on 8.3.x
    Issue #2832859 by vaplas, webflo, harings_rob, Wim Leers: Write...
Wim Leers’s picture

Awesome :) Thanks, @alexpott, for your thoughtfulness!

Anonymous’s picture

Woot!)

+ return AccessResult::neutral("The 'administer menu' permission is required.")->cachePerPermissions();
This change has test coverage too. Without it we have fail:

# /core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:325
Expected :{"message":"The \u0027administer menu\u0027 permission is required."}
Actual   :{"message":""}

But if we adding code:

  protected function getExpectedUnauthorizedAccessMessage($method) {
..
    switch ($method) {
      case 'GET':
        return "";

we have other fail:

# /core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:371
Expected :{"message":""}
Actual   :{"message":"The \u0027administer menu\u0027 permission is required."}

So change MenuLinkContentAccessControlHandler::checkAccess() did not come for fun)

EntityResourceTestBase is a very cool, because it makes necessary to correct such places :)

Wim Leers’s picture

@vaplas: thanks for explaining the reason for that change so clearly! I'm sure it'll be helpful for future readers.

EntityResourceTestBase is a very cool, because it makes necessary to correct such places :)

I'm glad you feel that way :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.