MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields

Our module blocked by this issue #2925283: Additional fields in menu items not visible for anonymous users
It disallows view menu_link_content fields for the anonymous user.

Patch from #3 helps to solve issue #2925283: New fields visibility and access

Patch #3 worked for me to enable the menu links to be available for anonymous user when using JSON API.

Just need to update the tests for this :).

Oops)
Late night patches) I`ll upload correct patches with some changes ASAP.

Added creation access check. Since we do not use the "administer menu" permission for view menu item, we will need rewrite Drupal\Tests\rest\Functional\EntityResource\MenuLinkContent\MenuLinkContentResourceTestBase test.

I asked similar questions about block_content entities in core. How would you even access a menu link exactly? IMHO, what would be more useful is a REST representation of all the links in a menu or maybe a menu block.

in menu_items_extras module used entity view builder to render fields of menu_link_contents.
#2925283: Additional fields in menu items not visible for anonymous users
https://www.drupal.org/project/menu_item_extras/issues/2925283#comment-1...

This patch allows access for all menu items without access checks.

It's usefull when jsonapi is used in more complex setups. Domain access in our case & cross domain links don't work with the access restrictions.

Edit - removing.

EDIT: Please ignore the patch attached.

The same bug happens when we use JSONAPI and include menu fields. The menu fields which are included start giving out 403 error.
The patch in #3 does not help in such a scenario.

The problem is this code in MenuAccessControlHandler

protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    // 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') {
      if ($entity->isLocked()) {
        return AccessResult::forbidden('The Menu config entity is locked.')->addCacheableDependency($entity);
      }
      else {
        return parent::checkAccess($entity, $operation, $account)->addCacheableDependency($entity);
      }
    }

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

#18 works without menu name filter. Once &filter[menu_name]=main is added, menu disappears for anonymous user.

Got it working. some background can be found in #3025372

the actual fix is to create hook_jsonapi_entity_filter_access that allows access. Otherwise by default it is checking wether user has admin permission.

function hook_jsonapi_entity_filter_access(EntityTypeInterface $entity_type, AccountInterface $account) {
  if ('menu_link_content' == $entity_type->id()) {
    return ([
      JSONAPI_FILTER_AMONG_ALL => AccessResult::allowed()
    ]);
  }
}

This just bit another JSON:API user: #3030056: [upstream] Menu link content entities are not accessible after RC4.

I still think that exposing this API directly for read access like that is the wrong approach if what you want to see/display is a menu tree.

IMHO, there should be a dedicated API that exposes the menu tree API, similar to how SystemMenuBlock::build() does it. While it might not be that common in decoupled scenarios, only getting menu_link_content entities means you don't get views and module provided menu links, and you also don't support additional types of menu links in contrib, like https://www.drupal.org/project/menu_link and https://www.drupal.org/project/menu_link_config.

Then we could also properly process trees and expose the same options as the block has to get the full hierarchy or just a certain level/depth.. I assume right now people are re-implementing that in their clients?

Still get the problem when use into graphQL

The patch #12 work but not compatible with 8.7

This was reported again and appears to be a JS Admin UI Initiative blocker: #3067627: API users cannot access menu links.

Bumping priority.

I don't think anyone replied about my concerns yet? :)

What about editing non-menu_link_content entities? That needs to be possible too. And view is tied to edit, so if you can edit, you can also view them, what exactly is missing for the Admin UI use case?

#33: sorry, I didn't re-read the issue!

#25: You're absolutely right. I share this concern. I said exactly that in Drupal Slack when @justafish raised her original bug report (#3066768: `parent` field on MenuLinkContent entities uses `string` field type, which is semantically wrong and burdens HTTP API clients — I included it in the issue summary there too). That being said, I do think it's valid to be able to list and query and update just menu_link_content entities too. Some sites only have those. That'd at least solve part of the problem.

> I do think it's valid to be able to list and query and update just menu_link_content entities too. Some sites only have those. That'd at least solve part of the problem

This is exactly the use case I have currently, and being able to just list and query those entities totally solves the problem for us

Accessing the menu entity as an anonymous user seems to work fine already, so this was sufficient to allow me to access menu links.

#35: Thanks, that' super helpful to know 👍

#36: This patch looks very different than previous iterations. It sounds like you extracted just the parts that you need?

@Wim Leers - It's #18 with the code for viewing menu entities removed (as this already works), and a documentation update.

I was confused why you started from #18, but then I started looking at the patches and #31 surprisingly used #12 instead of #18. #18 in turn confusingly says to please ignore it. The patch in #16 is outright dangerous. So #18 does seem like the most logical choice now 😀

Let's continue iterating on #18 with the goal to land it in Drupal core 🙏

+++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
@@ -50,9 +50,21 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+        // If menu link is internal, and user has access, grant view access to
+        // the menu link.
...
+        // Grant view access to external links.

Is this truly acceptable? Seeing the target URI or target route is one thing, that is probably fine indeed, but menu links also contain other information such as description. I think pretty much always that information will not be sensitive. But I don't know that we can assume that that will always be true.

I think a safer option may be to add a permission?

ON FURTHER INSPECTION I noticed:

    $fields['description'] = BaseFieldDefinition::create('string')
      ->setLabel(t('Description'))
      ->setDescription(t('Shown when hovering over the menu link.'))

So never mind my concern above :)

That's why I started contributing to the patch to help fix the failing tests :)

Looks like #36 doesn't actually get the job done sadly - filters don't work for anonymous users

I wonder if there is any update on this issue related to the menu childs and graphql?

patch #40 is no longer valid for 9.x. Re-rolling #40

Ignore #46

Without using any of the patches that alter core files, I took the logic used in one of the patches to make this work using only drupal hooks. Assuming `yourmodule` is a custom module, place these codes inside `yourmodule.module` file:

use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\Core\Session\AccountInterface;

/**
 * Implements hook_ENTITY_TYPE_access().
 */
function yourmodule_menu_link_content_access(EntityInterface $entity, $operation, AccountInterface $account) {
  if ($operation == 'view') {
    // Check whether user has permission to access menu.
    if ($account->hasPermission('administer menu')) {
      return AccessResult::allowed()
        ->cachePerPermissions()
        ->addCacheableDependency($entity);
    }
    // If menu link is internal, and user has access, grant view access to the menu link.
    if (($url_object = $entity->getUrlObject()) && ($url_object->isRouted())) {
      $link_access = \Drupal::accessManager()->checkNamedRoute($url_object->getRouteName(), $url_object->getRouteParameters(), $account, TRUE);
      if ($link_access->isAllowed()) {
        return AccessResult::allowed()
          ->cachePerPermissions()
          ->addCacheableDependency($entity);
      }
    }
    // Grant view access to external links.
    elseif ($url_object->isExternal()) {
      return AccessResult::allowed()
        ->cachePerPermissions()
        ->addCacheableDependency($entity);
    }
  }

  return AccessResult::neutral();
}

/**
 * Implements hook_jsonapi_ENTITY_TYPE_filter_access() for 'menu_link_content'.
 */
function yourmodule_jsonapi_menu_link_content_filter_access(EntityTypeInterface $entity_type, AccountInterface $account) {
  return ([
    JSONAPI_FILTER_AMONG_ALL => AccessResult::allowed(),
  ]);
}

That worked for me. You have to override menu_link_content access and then allow user to access the menu_link_content json endpoint.

Can this be still fixed in core, as it seems to be a "new feature" which changes current behavior? Maybe a new permission like "view menu link content" could be introduced to ensure backward compatibility.

The workaround from oscarino in #49 works well for for me to access menu items with JSON:API without granting "administer menu" permission to the API consumer. So if this can't be fixed in core, it might be done by a tiny contrib module.

We've been testing #47 on a project and that fixes getting menu items.

+++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
@@ -50,9 +50,27 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+        if (($url_object = $entity->getUrlObject()) && ($url_object->isRouted())) {
+          $link_access = $this->accessManager->checkNamedRoute($url_object->getRouteName(), $url_object->getRouteParameters(), $account, TRUE);

This should probably be rewrapped to be < 80 cols wide?

It looks like the remaining test-failure is because of caching, so that still needs to be fixed,

I'm also adding a reference to the GraphQL Issue.

#47 works on core 8.9.13

#47 works for GraphQL and menu_item_extras in Drupal 8.9.13

Rerolled #47
Weird, but Drupal\Tests\jsonapi\Functional\MenuLinkContentTest::testRelationships test successfully passed locally.

Addressed #52 and Drupal\Tests\jsonapi\Functional\MenuLinkContentTest::testRelationships test is passing now.

Any suggestions on how to fix this for D9 and GraphQL?

#60 Hi @hhvardan!
You can use https://git.drupalcode.org/project/drupal/-/merge_requests/339.patch as the patch for your drupal/core project dependency.

Hi @voleger,
I've applied patch and I'm still not able to query menu links as an anonymous user without "Administer menus and menu links" permission (the result is NULL).

Regarding Graphql: like @hhvardan mentioned the patches in this issue won't help you because in that case the problem is not in the menu link content entity access checking, but in the menu entity access check. Because in graphql we typically do an entity load for the menu config entity, and then retrieve the items. etc.

But when loading the menu entity, MenuAccessControlHandler kicks in and will also deny access because of the admin permission needed.

I'm not aware of a contrib module which fixes this, but it is pretty easy to implement if you have for instance a set of defined menu's which are public. In that case you can use hook_entity_type_build to override the MenuAccessControlHandler to allow the view operation for a certain set of menu ID's.

Thanks @voleger et al for the work on this. The MR solved an issue for me that caused problems with the menu_item_extras module: #3178103: Custom block inside menu item not visible for anonymous

The patch from #61 fixed the permissions issue with menu_item_extras for me too. Thanks @voleger!

Using the issue fork MR as patch fixes missing paragraphs for anonymous users when applying them via menu_item_extras in menu items.

@voleger: The fix looks good, but I am missing the testing for all the added access possibilities. Could you add one or two @dataProvider methods, so that all access possibilities get testing.

I'm having the same issue as #42. I can get JSON data for an anonymous user, but when I try to filter the data it returns nothing. The filter does work when I'm logged in as an administrator. Here's the filter I used: /jsonapi/menu_link_content/main?filter[id]=f7d367ef-f0fb-4d8e-96e6-1fca17f20224

Was this issue addressed in the patch?

I havent been able to go deep into what is causing it but the patch in MR #339 is still giving me leaked metadata issues.
The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early.
Somewhere in this process, ->render() is being called without the cache metadata.

As a result, the jsonapi endpoint returns a 500.

I just ran into this issue while attempting to build a mega menu with Layout Builder following this guide with Menu Item Extras. The patch by @voleger seems to fix it for Menu Item Extras, not sure if it fixes it for all the other cases having problems.

I tried the patch from #61 and also encountered the 500 error when attempting to load the /jsonapi/menu/items/. The logged error was:
The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early.

Hello @voleger,

Thank you for preparing this patch.

This works perfectly but unfortunately, it doesn't cover all cases. At least I found the one that doesn't work.

The issue exists in the second condition:

// If menu link is internal, and user has access, grant view access to the menu link.
    if (($url_object = $entity->getUrlObject()) && ($url_object->isRouted())) {

How to reproduce:
1. Create the node and generate automatic alias for the node. (for example, "/test-node-alias")
2. Create the menu item and use the internal path (not autocomplete) - "/test-node-alias"
3. Update the node title or make another action that will update the alias automatically. After this change, the path alias should be changed automatically and an automatic redirect should be generated.

Result: Admins can view the menu link, but anonymous users cannot since the condition $url_object->isRouted() return false.

re-roll to work with 9.4.9

Fixed CCF for #78.
Please review.

Rebased against 10.1.x branch. Needs work for #77

Rerolled patch #80.

Modified #82 patch to allow unrouted links that starts with "/"

Closed #2607978: Custom menu item entity reference is not accessible to anonymous user as a duplicate of this. Discussed with @catch in #bugsmash and the possible problems with entity references should probably be handled here too.

Patch #85 is working on Drupal 10.0.9.

Attached file is broken one, please ignore it.

Attached file is broken one, please ignore it.

@Lendude regarding "Discussed with @catch in #bugsmash and the possible problems with entity references should probably be handled here too." ... Could you give some more info on what needs to be done to get this issue moving again?

@Lendude & @Voleger, we just came to this issue from here: #2925283: Additional fields in menu items not visible for anonymous users

TL;DR: Due to this, entity reference (revision) contents are not visible for guests within the Menu (example: Mega Menu implementation using Paragraphs). Workaround: Use paragraphs_type_permissions submodule, which seems to override this using hook_ENTITY_TYPE_permission()

Still we'd like to help fixing this in core, so we can remove the workaround.

You have both invested a lot of time here and I think you're aware of what has to be done to finish this. What ca we do?
We can reroll the MR against 11.x and 10.3.x but then we should try to finish it and hot have it around for years again... any feedback?

The MR still seems good, a clear improvement and has a lot of positive feedback.

Here's the current state of the MR339 which still works fine against 10.2.x, 10.3.x and eventually 11.x (haven't tried). Great work @voleger!

rest and json_api integration tests are failing

Test are failing because the new link to user/1 is missing in the menu tests. So that should be a very easy fix.

See this link for all tests that are failing.

Not as simple as it seems. The expected cache tags are invalid, since we are swapping the "https://nl.wikipedia.org/wiki/Llama" link with a cacheable internal link to user:1. We can implement a dirty workaround and manually prepend the new cache to "getExpectedCacheTags()", but then the test will just throw an error later on.

To be honest, the current test changes generally seem really rushed. Simply replacing a remote link string with an internal link inside already existing tests / test bases without changing anything else just doesn't feel right. It also looks like we removed testing for external links entirely?

I'd say we revert the current test changes and create dedicated tests for internal menu links (json and rest) additionally to the already existing external link tests. Only problem being, that the test structure seems quite complex, so this will probably need quite a bit of work...

Really unsure about this. Does anybody else have an idea? Maybe there is a super simple way to test this, and we only need to add a quick test without changing any existing tests / test bases?

I reverted the test changes and created a new test class inside the jsonapi space. Unsure, whether to create a REST test class as well. But I don't think we should adjust the rest test base, how we did it in "MenuLinkContentResourceTestBase.php" originally, which I reverted here: https://git.drupalcode.org/project/drupal/-/merge_requests/339/diffs?com....

Unfortunately, now the old "MenuLinkContentTest" fails. Here, it expects a 403 status code, but it actually receives an unexpected 200 status code. This might show, that the current MR needs some refactoring? I am not sure. These tests are highly complex with many dependencies.

Attached Drupal 11.1 compatible patch. Would still be great, if someone could finish the great work done here already to get this fixed. Any hero present to move this over the finish line?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Alright, that should be it. Test is still failing, so leaving this at "Needs work" for now.

Current MR as patch attached.

Maybe we could get a subsystem maintainer to have a look at the tests, since nobody dares to do the final refinement and this is open for a while now?

I'll try and dive into this sometime soon.

One thing that I did notice, there were some conceirns in #77, are those valid or handled?

Issue summary should be complete, helps reviewer and sub-maintainers quickly understand.

#102 works for our project.

I updated the issue summary. None of the tests fail, but the pipeline still says "failed" no idea why. Setting back to "Needs review".

It needs a rebase, it has conflicts, so cannot do it in the interface :)

One thing I'm wondering is whether this issue still has relevance given the decoupled menu support already in core - see #3227824: Move the linkset functionality from the decoupled menus contributed module to core's system module

Answering #112 for myself - in the case of my client project they are accessing media fields on the menu link content entity and that's not provided by the linkset functionality.

Re-rerolled patch to cleanly apply in newer core version

@alexpott RE #113: Yes it's still really relevant - our case are menu items with paragraphs (for mega menu) which can't be rendered for anonymous users without this patch. See #2925283: Additional fields in menu items not visible for anonymous users

We'll rebase this one, but would be really great to finish this then?

Alright, rebased should be good again.

Tests fail.

Hm unsure why the tests fail. The tests expect a 200er status code, but receive a 302 (redirect):

1) Drupal\Tests\jsonapi\Functional\MenuLinkContentTest::testGetIndividual
NULL
Failed asserting that 302 is identical to 200.

Weird, the test results are completely different online, my apologies.

Hm, no idea, how to properly fix the test issues. And I can't really wait for the remote test output after every single change. Maybe someone else has more luck fixing it.

That patch from this merge request is odd. It contains a ton of code that is jibberish
https://git.drupalcode.org/project/drupal/-/merge_requests/339.patch
It looks like it is coming from a gzip file that got added and then removed
/core/modules/system/tests/fixtures/update/drupal-9.3.0.bare.standard.php.gz
In this commit, which seems completely unrelated https://git.drupalcode.org/project/drupal/-/merge_requests/339/diffs?com...

If any of the maintainers have the ability to do an interactive rebase on this and remove the noise, it would be good.

@voleger can you adjust the MR target to target main?

@swirt never seen using ".patch" at the end of a merge_request link. The plain diff looks fine to me though: https://git.drupalcode.org/project/drupal/-/merge_requests/339.diff

#123: done