Menu system unexpectedly uses different access control for node links

I have a feeling that the response is going to be "by design", but this did surprise me too.

My particular use case is a custom permission on the book navigation block (a custom block running the same code as core). I'm using the Chain Menu Access API module to append an additional check. This pipes all of the various menu access callbacks into a list and executes each callback in turn using _chain_menu_access_callback().

The menu tree is generated with:

$tree = menu_tree_all_data($node->book['menu_name'], $node->book);

And the entire tree is returned irrespective of the menu access callback. This is one link as an example, a child item to the currently active page.

49988 Database 433 (Array, 2 elements)
  link (Array, 45 elements)
    menu_name (String, 10 characters ) book-toc-1
    mlid (String, 3 characters ) 433
    plid (String, 3 characters ) 417
    link_path (String, 7 characters ) node/12
    router_path (String, 6 characters ) node/%
    link_title (String, 8 characters ) Database
    options (Array, 0 elements)
    module (String, 4 characters ) book
    hidden (String, 1 characters ) 0
    external (String, 1 characters ) 0
    has_children (String, 1 characters ) 1
    expanded (String, 1 characters ) 0
    depth (String, 1 characters ) 3
    customized (String, 1 characters ) 0
    p1 (String, 3 characters ) 415
    p2 (String, 3 characters ) 417
    p3 (String, 3 characters ) 433
    p4 (String, 1 characters ) 0
...
    updated (String, 1 characters ) 0
    load_functions (String, 26 characters ) a:1:{i:1;s:9:"node_load";}
    to_arg_functions (String, 0 characters )
    access_callback (String, 27 characters ) _chain_menu_access_callback | (Callback) _chain_menu_access_callback();
    access_arguments (String, 147 characters ) a:4:{i:0;a:5:{i:0;s:11:"node_access";i:1;s:41:"handbook_page_has_version_or_admin_access";i:2;i:1;i:3;b:0;i:4;b:0;}i:1;i:1;i:2;s:4:"view";i:3;i:1;}
...
    type (String, 1 characters ) 6
    description (String, 0 characters )
    in_active_trail (Boolean) FALSE
    access (Boolean) TRUE
    href (String, 7 characters ) node/12
    localized_options (Array, 0 elements)

The parent items in the active trail are tested against _chain_menu_access_callback() once and the current page is checked 3 or 4 times too, but all other items are never checked. This leads to 403's when trying to navigate to the child pages that show in the menu but do not have access.

My solution to fix the menu tree to follow the access was with:

function _handbook_filter_menu_items_by_path(&$data) {
  foreach ($data as $id => &$menu_item) {
    $item = menu_get_item($menu_item['link']['link_path']);
    $menu_item['link']['access'] = $item['access'];
    if ($item['access']) {
      _handbook_filter_menu_items_by_path($data[$id]['below']);
      // Test children to see if there is at least one with access.
      $child_access = FALSE;
      foreach ($data[$id]['below'] as $child_menu_item) {
        if ($child_menu_item['link']['access']) {
          $child_access = TRUE;
          break;
        }
      }
      $menu_item['link']['has_children'] = $child_access ? 1 : 0;
    }
    else {
      $menu_item['below'] = array();
      $menu_item['link']['has_children'] = 0;
    }
  }
}

$tree = menu_tree_all_data($node->book['menu_name'], $node->book);
_handbook_filter_menu_items_by_path($tree);

This still leaves the book navigation to fix and this solution is not scalable, but at least the navigation blocks respects the menu access hooks.

So maybe this issue should be titled something like "Menu item 'access callback' is bypassed unless the menu item is in the active path" and someone with intimate knowledge of the menu system can decide if it should be fixed (i.e. pwolanin or chx). And considering that this is a significant API change, it strongly doubt that it would ever see the light of day in Drupal 7 or below.

This obviously also goes the other way and if you grant access in hook_node_access the user can view the page but the menu items don't appear.

I don't see why this should be by design.

node_menu() defines 'node_access' as the access control function for node/%node.

This means that whatever node_access returns should control access to that item.

I haven't yet dug far into this but I don't even see a reason why this doesn't work as expected if node_access() is being called for access control.

In _menu_link_translate there is this code:

<?php
    if (!isset($item['access'])) {
      if (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
        // An error occurred loading an object.
        $item['access'] = FALSE;
        return FALSE;
      }
      _menu_check_access($item, $map);
    }
?>

If I remove the outer if then it works as expected, so the problem seems to be that something is already setting the access flag before now and then it doesn't check access properly.

Following this further, the menu_tree_collect_node_links() module sets access = FALSE for node links, which means that later in the rendering process _menu_link_translate() skips its access check.

Then there is menu_tree_check_access(), which does a node_access query on a $node_links array, which appears to never be used anywhere but is confusingly actually a reference to the menu $tree variable that was set up in menu_tree_collect_node_links().

This means that the way it works is normal manu access control is skipped for node links and instead access is granted only by node_access (grants) check in menu_tree_check_access().

I think this is bad because it means that anyone doing things like overriding access control for node menu items will also run into problems as the system is doing dodgy things in this case.

If we want to have a custom access check for node items then menu_tree_check_access() should call node_access() instead of doing the db query.

But if we do that then why have this custom setup for node links, why not make them work the same as all other links.

Any chance someone knows why the system works in this way for node links?

[EDIT] in case anything is different in d8 for this, not that all my code/function references are based on looking at D7.

Here is my workaround for the problem for d7

I don't know whether this is an acceptable fix or not but someone who knows more about the origins of the menu system will know.

There were a couple of places where menu_tree_collect_node_links() was still being used. This patch removes those occurrences and updates the calls to menu_tree_check_access() so they act in the same way as the one in the previous patch.

Otherwise, the patch in #7 seems to have solved the issues on our site.

Attaching interdiff (helpfully provided by Mark Pavlitski).

Setting this back to D7. This is the version where the patches refer to.
Also the D8 menu system is very, very different, and the problem might be solved in D8.

Attached patch is identical to #8, only against latest D7-version.

IMO, to resolve the problem "node menu items follow different access rules then node pages", only the first chunk (function _menu_link_translate) is necessary.

P.S. The link to #1978176: Build menu_tree without loading so many objects was added, because it contains a more rigourous chagne to menu_build_tree, also affecting the call to _menu_check_access() in _menu_link_translate().