Cannot edit the user/login menu link as an admin

Here is a patch, let's see if it will fail any or all tests.
I removed access check for internal links. As far as I can see it doesn't need to be there.

Please correct me if I am wrong.

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

Tested the above patch menu-link-edit-access-2492513-1.patch. Its working fine. Manually verified admin user can access and edit the menu link with value "/user/login" to another one after applying the patch.

1. Patch applied successfully without errors.
2. Tested on Firefox & chrome browser (Ubuntu 14.04)
2. Added a link 'User Menu' http://localhost/drupal/admin/structure/menu/manage/account/add
3. Before applying patch: On click to edit link, giving an error "You are not authorized to access this page" as an admin user.
4. After applying patch: On click to edit link redirects the user to this url http://localhost/drupal/admin/structure/menu/item/4/edit?destination=/dr...
Result: Issue is resolved after applying the patch. Admin user can able to access and change the menu link.

Screenshot attached.

We need tests but we also need to come up with a proper solution. Removing any acces checking is here is IMHO not the right solution

Here is a patch attached. This patch doesn't remove the access check, instead it checks if access for the current user to the given link is forbidden or allowed.

.

Tested again the recent patch fixed-edit-menu-link-as-admin-2492513-8.patch. Its working fine. https://www.drupal.org/node/2492513#comment-10368139

Hi,

I have tested the issue on my side and found that the issue is with _user_is_logged_in validation. I changed the permission in user.routing.yml _user_is_logged_in: 'FALSE' to _user_is_logged_in: 'TRUE', cleared cache and then tried to edit the link and it worked. I reckon the issue is drupal is checking /user/login to be allowed for anonymous user, in our case when we try to edit the link, the drupal behavior is checking the user_logged_in permission and thus giving us 'Access Denied'.

Hope this will works.

@nitesh: it works perfectly with edit link, the issue is when a non admin user with administer menu permission creates a menu and enters /user/login as link the issue still prevails.

@shabbir, please check "link to any page" permission.

@nitesh: tested with "link to any page" permission for user with "administer menu" role and applied your patch, it works fine. For admin user i didn't gave "link to any page" permission and it worked as well. Works fine. Please someone else review and give your comment as well.

Still needs tests.

This seems very much related to #2409099: Edit access to custom menu links not granted to users with "link to any page" permission

Added a test.

Hi @JeroenT,

Just checked the patch in Drupal Version 8.2.3.

It works fine.

I also applied the patch and it's working, so far so good, I'm building a new site so I'll report back if I find any issue. For those that would like to help test this patch and are not sure how, just download the patch add to menu_link_content module and via command line ```git apply --stat cannot_edit_the-2492513-25.patch```

Thanks @jJeroenT for the patch!

@nikhilsukul or @PierreMarcel,

Thanks for the reviews. If any of you could RTBC this issue, that would be great.

Thanks for working on this!

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
@@ -50,6 +51,22 @@ public function testMenuLinkContentForm() {
+   * Tests if a user with link to any page permission has the right to edit
+   * a page he cannot access.

Two minor things to fix with this comment:

1. It should be a single line of 80 characters or fewer.
2. It should not use gendered language ("he").

I'd say: "Tests the 'link to any page' permission for a restricted page."

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
@@ -50,6 +51,22 @@ public function testMenuLinkContentForm() {
+    $this->drupalGet('/admin/structure/menu/item/' . $menu_link->id() . '/edit');
+    $this->assertResponse(200);

We could also add an inline comment above these lines that says "The user should be able to edit a menu link to the page, even though the user cannot access the page itself." That makes up for having less information in the method docblock, I think.

Here is a patch for drupal 8.2.x
Working fine for me .

Made changes as suggested in #32 and #33.

Patch #35 is working beautifully . Thanks for patch.

Tested and working great! Changing it to RTBC, also changed the priority to Major, just because I'm thinking about new people trying Drupal it would be common to add login path to a menu, and I would hate to hear bashing us just because a silly bug like this.

Great Job @JeroenT thanks!!!

Thanks everyone!

This does not meet the criteria for a major bug, so marking back to normal priority.

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
   @@ -65,7 +65,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
              // We allow access, but only if the link is accessible as well.
   

   I just noticed that we also need to update this comment to reflect the change. Sorry for not catching this on the first review! However, I think we can make it even clearer. Comparing this to the the alternate fix proposed in #34, in both cases, it is actually a little confusing. Instead of either approach, I think we should add an explicit case to return earlier if the account has the permission, because if we do have the permission, we don't need to do any of this either:

   if (($url_object = $entity->getUrlObject()) && $url_object->isRouted()) {
               $link_access = $this->accessManager->checkNamedRoute($url_object->getRouteName(), $url_object->getRouteParameters(), $account, TRUE);

2. We always need to be careful when expanding the scope of permissions. The permission is defined by the System module and already flagged as a dangerous/access bypass permission, so that is good:
   

   link to any page:
     title: 'Link to any page'
     restrict access: true
     description: 'This allows to bypass access checking when linking to internal paths.'

   It's interesting that the check for this permission was overlooked, especially since numerous other places in core do check this permission:

   grep -r "link to any page" * | grep -v "Test" 
   core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php:    if ($this->account->hasPermission('link to any page')) {
   core/lib/Drupal/Core/Path/PathValidator.php:    if (!$access_check || $this->account->hasPermission('link to any page')) {
   core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php:      '#default_value' => (!$item->isEmpty() && (\Drupal::currentUser()->hasPermission('link to any page') || $item->getUrl()->access())) ? static::getUriAsDisplayableString($item->uri) : NULL,
   core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php:      // Disallow URLs if the current user doesn't have the 'link to any page'
   core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php:      $allowed = $this->current_user->hasPermission('link to any page') || $url->access();
   core/modules/system/system.permissions.yml:link to any page:
   

   I used git log -L and the two issues (other than for cacheability improvements) are #2340507: Make the new AccessResult API and implementation even better and the original #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage. There are no other references to this permission in that issue either. Since there are three different ways to define menu links, we should probably make sure that all three respect this permission. The fact that we need to check it so many times in so many different places also makes me wonder if the access check should be provided for menu links at a higher level.

   Based on all that, I think it would also be good to get a menu subsystem maintainer review.

I tagged the issue for subsystem maintainer review, but in the meantime, we can still update the patch for point 1. Thanks!

Ok if I change the comment in #38.1 to

We allow access when the link is accessible or the user has the 'link to any page' permission.?

Marking as RTBC since there are only comment changes.

1. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
   @@ -62,10 +62,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
              // If there is a URL, this is an external link so always accessible.
              $access = AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);
              /** @var \Drupal\menu_link_content\MenuLinkContentInterface $entity */
   -          // We allow access, but only if the link is accessible as well.
   +          // We allow access when the link is accessible or the user has the
   +          // 'link to any page' permission.
              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);
   +            if (!$account->hasPermission('link to any page')) {
   +              $access = $access->andIf($link_access);
   +            }
              }
   

   There's no need to check link access if the account has the 'link to any page' permission so this can be rewritten as:

             // If there is a URL, this is an external link so always accessible.
             $access = AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);
             /** @var \Drupal\menu_link_content\MenuLinkContentInterface $entity */
             // We allow access when the link is accessible or the user has the
             // 'link to any page' permission.
             if (!$account->hasPermission('link to any page') && ($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);
             }
             return $access;
   
   

   Plus lets fix the comment on the first line since this part of the if is not dealing with external links only. Assume that access is allowed. is an acceptable replacement. And the comment that starts "We allow..." should better explain the if. Ie something like If the link is routed determine whether the user has access unless they have the 'link to any page' permission

2. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
   @@ -50,6 +51,23 @@ public function testMenuLinkContentForm() {
      /**
   +   * Tests the 'link to any page' permission for a restricted page.
   +   */
   +  public function testMenuLinkContentFormLinkToAnyPage() {
   +    $menu_link = MenuLinkContent::create([
   +      'title' => 'Menu link test',
   +      'provider' => 'menu_link_content',
   +      'menu_name' => 'admin',
   +      'link' => ['uri' => 'internal:/user/login']
   +    ]);
   +    $menu_link->save();
   +    // The user should be able to edit a menu link to the page, even though
   +    // the user cannot access the page itself.
   +    $this->drupalGet('/admin/structure/menu/item/' . $menu_link->id() . '/edit');
   +    $this->assertResponse(200);
   +  }
   

   Can we also look for or provide a test the other way around ie a test where the user can't add a link because they don;t have access and don't have the permission.

Created a new patch that addresses the suggestions given in #43.

.

- Can reproduce initial problem
- Bug fixed by patch (8.2.x)
- Passes existing tests
- Has a new test
- No UI changes
- Coding standards fine

(NB: not knowledgeable enough to say if fully in scope)

Patch applied, it has solved the problem. Initially looking at the code it seems like all #43 requests have been addressed.

Thank you everyone for the work so far.

1. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
   @@ -21,12 +22,27 @@ class MenuLinkContentFormTest extends WebTestBase {
   +    $this->adminUser = $this->drupalCreateUser(array('administer menu', 'link to any page'));
   +    $this->basicUser = $this->drupalCreateUser(array('administer menu'));
   +    $this->drupalLogin($this->adminUser);
   

   ::testMenuLinkContentForm and ::testMenuLinkContentFormValidation test conditions are modified by this. I am not saying that's necessarily something to prevent this being committed, but it's something to watch out for.

2. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
   @@ -50,6 +66,31 @@ public function testMenuLinkContentForm() {
   +      'link' => ['uri' => 'internal:/user/login']
   

   Nitpick on coding standards: there is not a comma after this array element.

3. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
   @@ -50,6 +66,31 @@ public function testMenuLinkContentForm() {
   +
   +    $this->drupalLogin($this->adminUser);
   

   What is this for? ::setUp runs every time.

The last suggestion in the #43 review, "Can we also look for or provide a test the other way around ie a test where the user can't add a link because they don;t have access and don't have the permission." doesn't seem to be in the current patch.

(edited)

#50:

2. sorry, apparently had wrong coding style selected, should have spotted in earlier review.

3. removed.

Verified the patch cannot_edit_the-2492513-51.patch on 8.3.x and 8.4.x version. Its working fine, here are the test observations:
1. Patch applied successfully without errors.
2. Tested on Firefox & chrome browser (Ubuntu 14.04)
2. Added a link for 'Login' menu title http://localhost/drupal/admin/structure/menu/manage/account/add
3. Before applying patch: On click to edit link, giving an error "You are not authorized to access this page" for an admin user.
4. After applying patch: An admin user is able to access and change a menu link with value "/user/login" to another one.
Result: Issue is resolved after applying the patch. Admin user can able to access and change the menu link.

Screenshots attached

Needs reroll for short array syntax.

Re-roll.New short array syntax used.

Fixed a very minor coding standard issue.
And removed the Needs Reroll tag.

Verified and tested with the latest patch on 8.3.x-dev version, working fine screenshots attached in #53

Saving issue credit. Adding @Shabbir because they provided test evidence with a screenshot, @xjm, @cilefen and myself because we all provided feedback that resulted in changes to the patch.

Committed 6c37e3e and pushed to 8.4.x. Thanks!

I think we should consider cherry-picking this to 8.3.x before 8.3.0. Will ask another committer for an opinion.

Both @catch and @cilefen are +1 for 8.3.x