Restrict access to empty top level administration pages for overview controller

Interesting that node has \Drupal\node\Routing\RouteSubscriber, which swaps out the controller and requirements.

\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage() was added in Drupal 5(!) as system_admin_menu_block_page()

\Drupal\system\Controller\SystemController::overview() was added in Drupal 7 as system_admin_config_page(), and was originally added only to represent /admin/config. It was reused for /admin/content in #2085571: admin/content should not depend on node.module

Both controllers use \Drupal\system\SystemManager::getAdminBlock() to display links, but differently:

SystemController::overview() iterates over the links 1 level down, and uses getAdminBlock() to retrieve the links 2 levels down. It displays all of the blocks in a grid.

SystemController::systemAdminMenuBlockPage() uses getAdminBlock() to retrieve the links 1 level down. Since there is only one "block" retrieved, it displays those directly, no grid.

I took screenshots of /admin/config and /admin/content with both of the possible controllers.
Note that I had to uninstall the node module first, thanks to the route subscriber mentioned in #3. This makes all of this a lot more theoretical, since keeping /admin/content in the admin menu while the node module is uninstalled seems like a dramatic edgecase.

/admin/config with SystemController::overview()

/admin/config with SystemController::systemAdminMenuBlockPage()

/admin/content with SystemController::overview()

/admin/content with SystemController::systemAdminMenuBlockPage()

MR looks good

Moving to NW so \Drupal\system\Controller\SystemController::overview can be removed or deprecated.

Assuming a CR will also be needed for the change to the controller and removal of \Drupal\system\Controller\SystemController::overview

No, there's nothing wrong with overview. It's just confusing. Hence the added docs in the MR. We're just switching it from 2 usages to 1 usage.

But this might not even be needed, postponing on #296693: Restrict access to empty top level administration pages to see how that shakes out

#296693: Restrict access to empty top level administration pages is in.

So noticed that /admin/config uses \Drupal\system\Controller\SystemController::overview you would still see "you have no administrative items." So this still has the problem that #296693 was solving. We just didn't notice because

// As menu_test adds a menu link under config.
    $this->assertMenuItemRoutesAccess(200, 'admin/config');

The Steps to reproduce in the IS look weird and unrelated to the issue?

I can see why it seems that way, but we found these discrepancies when dealing with *empty* admin pages, and to do so you'd need to remove a bunch of the admin items

With the fix ^ we can now debug the code in \Drupal\system\Access\SystemAdminMenuBlockAccessCheck. Can someone tell the next steps as it is not clear from the current IS or any comment. Thanks in advance.

See my comment in the MR. I did some work on the tests. I think the tests pass correctly now but somehow should double check. Also we should think if we are missing any test cases.

The test coverage looks adequate and assertUserRoutesAccess() is giving quite easy to understand error messages.

Commented on the MR. Here is patch that proves our tests pass with no concern for level checking which should be needed for the differences in expected behavior between overview and system block.

Basically proves our tests are wrong somewhere.

Hi, after spending some time on this, I think tests on #19 are passing because of https://www.drupal.org/project/drupal/issues/296693#comment-15328018, and I think we should have not included that in the test only patch.

Running the 'tests-only' pipeline shows a fail as expected. Keeping NW for the IS update and to resolve the remaining @todo's added in the MR.

Posted some small nits related to comments.

Looks good to me.So marking it RTBC.

FYI: This introduced a bug. See #3413508: Admin page access denied even when access is given to child items

Edit: should've commented this in the parent issue.

The patch seem to fix the regression introduced with the Drupal core update with #296693: Restrict access to empty top level administration pages

I'm triaging RTBC issues. I read the IS, the comments and the MR. I don't see any unanswered questions.

This is tagged for an issue summary update which was completed in #22, so removing tag.

Adding tag for a follow up for this comment, https://git.drupalcode.org/project/drupal/-/merge_requests/4608#note_242911

When reading the MR (not a review) I made suggestions for several comments. Since they are minor I decided to go back and apply them, tests are running now.

The tests have passed and they were minor changes so I am leaving this at RTBC.

Following this in case there's a backport to D10

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks! Enough change here not to try to backport to 10.2.x but sounds like this doesn't come up often in practice. I made and accepted a couple of comment suggestions on the MR prior to commit.

Created the follow-up at #3423822: Use 'level' instead of 'child' 'grandchild' 'great grandchild' in menu_test and associated tests