Regression: Bring back node access optimization to menu trees

The decision to remove the optmization is referred to here https://drupal.org/node/2207893#comment-8579829

I did a quick test as follows: make admin users other than user 1. Make 5 nodes. Log in as second admin user and copy cookie ID & value. Run an apache bench test as that logged-in user by passing the user's cookie ID and value to ab as follows
ab -n 400 -c 10 -C 'SESS<cookie id>=<cookie value>' http://example8.dev/admin/content.

WITHOUT PATCH
Concurrency Level: 10
Time taken for tests: 227.651 seconds
Complete requests: 400
Failed requests: 0

WITH PATCH
Concurrency Level: 10
Time taken for tests: 184.360 seconds
Complete requests: 400
Failed requests: 0

The patch looks OK to me, and the site works with the patch. I am hesitating to mark as RBTC without someone more experienced looking at it. If no else one does look at it, I may return to it.

I wasn't sure best way to test, and am I not even sure performance testing is really needed. I am willing to retest if someone advises on design of test. Otherwise maybe this is best left to someone else to review.

Adding a reference to the place where this was decided (thanks, John_B in #10!) because the issue summary is... lacking in details. ;P

I don't quite follow the logic of the referenced comment myself. Almost every site is going to have custom nodes for page like About Us, ToS, etc. and almost every site is going to put those custom nodes (and others) into Primary/Secondary Links menus, meaning most sites will incur the performance penalty. Book module is a pretty minor use case of this in the grand scheme of things, so not sure why the optimization is reserved for that module only, which I can only ever recall seen used on Drupal.org (which doesn't even use node access) on all the many dozens of Drupal sites I've ever had backend access to.

#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees and/or #1869476: Convert global menus (primary links, secondary links) into blocks would mean that the access checks and in fact the rendered HTML for those menus will be cached. This also has the advantage that it will cover access checks for things like Views. I haven't profiled views router access in 8.x but it's the worst offender in 7.x for menu access checks.

For primary/secondary menus, you have a finite menu size linking to maybe 5-10 nodes at most - at least if it's to FAQ/About pages.

With block/render caching implemented, the node_access() checks would happen only on a cache miss. Then this issue would mean that instead of loading 5-10 nodes on a cache miss, we do a node access query on them, which is probably still an optimization but not nearly the same as it is in Drupal 7 where there is no render caching of menus so those checks have to happen every request.

Book module needs this more because it has menus with dozens if not hundreds of nodes in them. That's 1. Why I agreed to remove this in the first place 2. why I don't think this issue is critical (given the other two are).

That's a big improvement for Views access.

How is the 10k entries in menus different from something like https://drupal.org/project/taxonomy_menu which wouldn't be affected either way by this?

In D7, the problem is that every object in the menu is loaded to check the access, resulting in increased memory usages and lower page loading times.
This has no impact if you are only showing nodes, since there is a built-in exception for nodes, but it has problems with Views displays or large TaxonomyMenu trees.

See below issue for D7 use cases (in the summary) and a patch-in-progress:
#1978176: Build menu_tree without loading so many objects
Just warning you to not trap into this pitfall again for D8.

9: menu_tree_access-2250315-9.patch queued for re-testing.

@dawehner. Pre-filtering seems reasonable yes.

Can we postpone this issue until: #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module

getUrlObject() may load the entity to get the description, so we should both optimize that code a bit to be like:

 if ($title_attribute && ($description = $this->getDescription()))

and add back dedicated methods to get the route name and parameters - this will also be important for our fix for the Account menu link

This looks great!

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -84,6 +96,63 @@ public function checkAccess(array $tree) {
   +   * Performs access checking for nodes in an optimized way.
   

   Below this, I'd add a line saying that this should run before the checkAccess() manipulator, because this takes over part of checkAccess()'s workload and does it much more efficiently.

2. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -250,4 +260,48 @@ public function testExtractSubtreeOfActiveTrail() {
   +  public function  testCheckNodeAccess() {
   +    $links = array(
   +      1 => MenuLinkMock::create(array('id' => 'node.1', 'route_name' => 'node.view', 'title' => 'foo', 'parent' => '', 'route_parameters' => array('node' => 1))),
   +      2 => MenuLinkMock::create(array('id' => 'node.2', 'route_name' => 'node.view', 'title' => 'bar', 'parent' => 'node.1', 'route_parameters' => array('node' => 2))),
   +      3 => MenuLinkMock::create(array('id' => 'node.3', 'route_name' => 'node.view', 'title' => 'baz', 'parent' => 'node.2', 'route_parameters' => array('node' => 3))),
   +      4 => MenuLinkMock::create(array('id' => 'node.4', 'route_name' => 'node.view', 'title' => 'qux', 'parent' => 'node.3', 'route_parameters' => array('node' => 4))),
   +    );
   

   This tests a tree with only node links. I think it should only be a subset (so we can verify non-node links are unmodified), and with at least one node link nested below a non-node link.

3. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -84,6 +96,63 @@ public function checkAccess(array $tree) {
   +          $node_links[$nid][$key]->access = TRUE;
   

   Why only set ->access = TRUE on nodes that are accessible to the current user?

   Why not also set ->access = FALSE on nodes that are inaccessible to the current user?

checkAccess() test coverage already specifically asserts that if a link already has ->access set, that no access checking performed anymore by it for that link. So test-coverage wise, I think we're complete, with the above modification.

Question: doesn't checkNodeAccess() blindly assume that any published link is accessible to any user? What if hook_entity_access() hooks impose further restrictions? And what if the node.view route is altered to have more requirements besides _entity_access: 'node.view'?

Why doesn't this optimization break those scenarios?

If this lands before #2301317: MenuLinkNG part4: Conversion, we'll also have to use this manipulator wherever else menu.default_tree_manipulators:checkAccess is used.

So, one reason why we took this out of 8.x is that users were denied seeing their own unpubished nodes in the tree - we need to decide what behavior is correct.

In 8.x there are tests that they can see them. In 7.x, they are not expected to be able to see them (and also in book module in 8.x they can't see them in the book block).

re-roll for HEAD changes

Add missing dependency in the service definition - may fix some test fails.

Re: earlier comments, I think we should mark these FALSE if they are not accessible? I'm worried we are going to be doubling the work for every inaccessible node.

As far as I understand the node access system it is based upon grants, so you can just allow someone to look at a noe, not deny it via query based access check.

AFAICT that's not entirely correct. The node access system consists of two access systems layered on top of each other. The first is the entity/node access system that allows hook_entity_access() and hook_node_access() hooks to returns TRUE/FALSE/NULL. If they all returned NULL (i.e. none of them had an opinion), and then it falls back to the node grant system.
(See NodeAccessController::access(), this may call EntityAccessController::access(), which may call NodeAccessController::checkAccess(), which in turn may call the node grants system.)

#37: Can you clarify? I don't understand why you refer to listings?

Oh wow, I did not know that!

I can imagine why they don't consider entity access when determining which entities should live on which page. But upon *rendering*, the entities are surely access checked, aren't they? It'd otherwise be easy to make a Drupal site where sensitive information was being leaked.

Do we have any documentation surrounding this? It feels like we're missing the necessary facts to have a proper discussion about this.

@Wim Leers - if I have a node listing page and I make a View of it, I don't expect that entity access checks run again after the SQL-based check before showing the title.

So, for book module we use a pure node access query. In D6 and D7 we did the same for menu links to nodes. If we don't think the node grant system is sufficient for this use case, then we we should just close this issue. The biggest reason we went away from it was to handle unpublished nodes. I can imagine special casing those and re-checking access as a compromise.

So, I don't think this is sufficient, but we can debate the point. Do we want people to see their own unpublished nodes? I think that's important for avoiding "lost content"

+      // Allow admins to also see unpublished nodes as menu links.
+      if (!$this->account->hasPermission('bypass node access')) {
+        $query->condition('status', NODE_PUBLISHED);
+      }

From node.api.php:

 * Also note that this function isn't called for node listings (e.g., RSS feeds,
 * the default home page at path 'node', a recent content block, etc.) See
 * @link node_access Node access rights @endlink for a full explanation.

That section can be found in node.module, and explains it in more detail. The most important part there is this:

 * In node listings (lists of nodes generated from a select query, such as the
 * default home page at path 'node', an RSS feed, a recent content block, etc.),
 * the process above is followed except that hook_node_access() is not called on
 * each node for performance reasons and for proper functioning of the pager
 * system. When adding a node listing to your module, be sure to use an entity
 * query, which will add a tag of "node_access". This will allow modules dealing
 * with node access to ensure only nodes to which the user has access are
 * retrieved, through the use of hook_query_TAG_alter(). See the
 * @link entity_api Entity API topic @endlink for more information on entity
 * queries.

In other words: I was able to confirm what dawehner said in #39.

But… doesn't this mean we're (still) treating all other entity types as second-class citizens? Doesn't this mean we simply cannot render access-restricted listings of Commerce Product entities, for example? Isn't this highly problematic? Shouldn't this be the same system for all entities?

Every entity type can use query altering, Entity Query automatically adds a tag. Commerce thingies can and do implement that.

The only thing that is node-specific is the grants system/node_access table, which is used instead of a random mix of query alters from different modules.

There is an issue to make that generic, but it did not happen and every entity type can decide to implement something like that themself if they want to.

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -84,6 +96,74 @@ public function checkAccess(array $tree) {
   +        $nid = $route_parameters = $element->link->getRouteParameters()['node'];
   

   I think it is like this
   $route_parameters = $element->link->getRouteParameters();
$nid = $route_parameters['node'];

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -117,10 +117,23 @@ public function isCacheable() {
   +    return isset($this->pluginDefinition['route_name']) ? $this->pluginDefinition['route_name'] : array();
   

   it should be ''; not array();

Thanks for the changes. I'll let @Berdir and @Wim Leers RTBC this.

Well, we require PHP 5.4, so we can use its capabilities, don't we?

And it'll save us tons of memory :P

So, I think the correct logic is a little more complicated here?

The code in NodeAccessController::checkAccess() seems to imply that the access check will depend on the current language in the case of unpublished nodes - that the author of a translation may be able to see that but not the original node if it's unpublished.

For a first pass we need to exclude uid 0

I'm curious how that language check works if the base table just has one langcode

Do you know which issue allowed seeing own unpublished nodes in menus? I personally am not convinced that's worth the complexity here.

@dawehner - not true. In Drual 7 you are *not* allowed to see them. In Drupal 8, you *are* allowed and there is a test for it.

The change happened someplace more recent than this commit:

commit 477c06c413fbb180165ffa5cdddb4b1d196cd56f
Author: Alex Pott
Date: Sun May 26 13:18:10 2013 -0700

Maybe I'l have to run a bisect to find it

@dawehner - so I guess we take the hit of an additional check for any unpublished nodes in the menu? Seems like something that could be an unexpected performance drag on a site (e.g. if it has a menu with dozens of unpublished nodes in it)

@dawehner - you are misinterpreting the Drupal 7 code - it set all node links to access FALSE by default:

function menu_tree_collect_node_links(&$tree, &$node_links) {
  foreach ($tree as $key => $v) {
    if ($tree[$key]['link']['router_path'] == 'node/%') {
      $nid = substr($tree[$key]['link']['link_path'], 5);
      if (is_numeric($nid)) {
        $node_links[$nid][$tree[$key]['link']['mlid']] = &$tree[$key]['link'];
        $tree[$key]['link']['access'] = FALSE;
      }
    }
    if ($tree[$key]['below']) {
      menu_tree_collect_node_links($tree[$key]['below'], $node_links);
    }
  }
}

minor typo:
readd -> re-add

Let's reverse this condition so the if handles the positive case (it's easier to understand):

+      // Allow admins to also see unpublished nodes as menu links.
+      if (!$this->account->hasPermission('bypass node access')) {
+        $query->condition('status', NODE_PUBLISHED);
+      }
+      else {
+        $query->accessCheck(FALSE);
+      }

Three nitpicks and one minor thing, after that I'll RTBC.

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -85,6 +96,71 @@ public function checkAccess(array $tree) {
   +   * because it provides an performance optimization for ::checkAccess().
   

   s/an/a/

2. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -85,6 +96,71 @@ public function checkAccess(array $tree) {
   +   *   Stores references to menu link elements to effective set access.
   

   s/effective/effectively/

3. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -275,4 +285,61 @@ public function testExtractSubtreeOfActiveTrail() {
   +      1 => MenuLinkMock::create(array('id' => 'node.1', 'route_name' => 'entity.node.canonical', 'title' => 'foo', 'parent' => '', 'route_parameters' => array('node' => 1))),
   +      2 => MenuLinkMock::create(array('id' => 'node.2', 'route_name' => 'entity.node.canonical', 'title' => 'bar', 'parent' => 'node.1', 'route_parameters' => array('node' => 2))),
   +      3 => MenuLinkMock::create(array('id' => 'node.3', 'route_name' => 'entity.node.canonical', 'title' => 'baz', 'parent' => 'node.2', 'route_parameters' => array('node' => 3))),
   +      4 => MenuLinkMock::create(array('id' => 'node.4', 'route_name' => 'entity.node.canonical', 'title' => 'qux', 'parent' => 'node.3', 'route_parameters' => array('node' => 4))),
   ...
   +    $tree = array();
   +    $tree[1] = new MenuLinkTreeElement($links[1], FALSE, 1, FALSE, array());
   +    $tree[2] = new MenuLinkTreeElement($links[2], TRUE, 1, FALSE, array(
   +      3 => new MenuLinkTreeElement($links[3], TRUE, 2, FALSE, array(
   +        4 => new MenuLinkTreeElement($links[4], FALSE, 3, FALSE, array()),
   +      )),
   +    ));
   +    $tree[5] = new MenuLinkTreeElement($links[5], TRUE, 1, FALSE, array(
   +      6 => new MenuLinkTreeElement($links[6], FALSE, 2, FALSE, array()),
   +    ));
   +
   

   The parent information does not match the tree, which makes this very confusing to understand.

   Please change either so they're consistent.

4. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -275,4 +285,61 @@ public function testExtractSubtreeOfActiveTrail() {
   +    // Ensure that other routes than entity.node.canonical gets set as well.
   

   s/gets/are/

Indeed it is.

Since catch committed the original patch that removed this optimization, and since it's about, well, optimization. :) Assigning to catch.

One nitpick, one question:

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -85,6 +96,71 @@ public function checkAccess(array $tree) {
   +      // Allow admins to also see unpublished nodes as menu links.
   

   The comment doesn't quite match the code.

   Admins get to both see unpublished nodes and ignore other access checks.

   Non-admins get both access checks and the published condition.

   With the comment just about the ->accessCheck(FALSE) it looked for a second like this was allowing the viewing of unpublished nodes, but it has no effect on that.

   Would also be good to add an explicit comment that we're ignoring the 'view own unpublished nodes' permission here to avoid a per-user caching requirement.

2. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -275,4 +285,61 @@ public function testExtractSubtreeOfActiveTrail() {
   +    $tree = $this->defaultMenuTreeManipulators->checkNodeAccess($tree);
   

   How difficult would it be to add to the test to confirm that the usual access check on the links isn't also performed here?

That's great. Committed/pushed to 8.0.x, thanks!