Users with "configure any layout" can see entities they don't have "view" access to

Tested and the passing patch worked for me. Note that the steps to reproduce did not include enabling Layout builder for your type of content as well as per-content-item layouts. These 2 checkboxes are found on the "Manage Display" tab of your content type configuration.

/admin/structure/types/manage/content_type_name/display

Ah yes, these STR predate that opt-in checkbox.

Just documenting here that @tedbow and @tim.plunkett worked on the patch for this issue in the original private issue, so it has had peer code review there as well. Updating issue credit.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -253,20 +241,6 @@ public function buildLocalTasks($base_plugin_definition) {
-  /**
-   * Determines if this entity type's ID is stored as an integer.
-   *
-   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
-   *   An entity type.
-   *
-   * @return bool
-   *   TRUE if this entity type's ID key is always an integer, FALSE otherwise.
-   */
-  protected function hasIntegerId(EntityTypeInterface $entity_type) {
-    $field_storage_definitions = $this->entityFieldManager->getFieldStorageDefinitions($entity_type->id());
-    return $field_storage_definitions[$entity_type->getKey('id')]->getType() === 'integer';

At a minimum, the removal of this method should have a CR. Ideally, it would be deprecated instead of removed. https://www.drupal.org/core/deprecation#internal

When we are considering making internal BC breaks, the question shouldn't be "Is there any reason not to get rid of this?" There are two cases where we should break BC instead of deprecating:

1. The BC break is required to solve the issue (e.g., when a form controller needs to be rewritten for a UI change, or for the delegation issue).
2. Leaving the undesired code around is actually dangerous.

I don't think this method meets either of those criteria. It seems fairly harmless to me, so I'd propose just deprecating it and the EFM service injection. Another advantage of that is that the patch gets a lot smaller.

While mulling #9, I thought of something that actually sort of trumps the discussion: This is a security issue. It's a public one because it's an administrative access bypass, but still a security issue. For that reason, we should backport this to 8.6.x; otherwise, 8.6.x will have a publicly disclosed administrative access bypass until its EOL in December. And in order to backport it to a patch release, we should make the patch as non-disruptive as possible (meaning avoiding even internal breaks like constructor service fixes).

Also needs a release note snippet (basically a sentence linking the CR).

The current patch does not apply to 8.6.x, but maybe a minimal-er version will. Otherwise we can create the 8.6.x patch after we've finalized the 8.7.x one.

This is the smallest possible change.
Still needs CR.

Should apply to both branches

s/discard_changes/cancel for the 8.6 patch, see #3025870: Cancelling changes in the Layout Builder UI should use a confirmation form.

8.6.x is failing though:

1) Drupal\Tests\layout_builder\Unit\OverridesSectionStorageTest::testBuildRoutes
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'entity.from_canonical.canonical' => Symfony\Component\Routing\Route Object (...)
     'layout_builder.overrides.from_canonical.view' => Symfony\Component\Routing\Route Object (...)
     'layout_builder.overrides.from_canonical.save' => Symfony\Component\Routing\Route Object (...)
-    'layout_builder.overrides.from_canonical.discard_changes' => Symfony\Component\Routing\Route Object (...)
@@ @@
+    'layout_builder.overrides.from_canonical.cancel' => Symfony\Component\Routing\Route Object (...)

/var/www/html/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php:488

FAILURES!
Tests: 8, Assertions: 11, Failures: 1.

Oh, I see this is what is mentioned in #17. So NR for he 8.7.x patch and then a backport.

Since this would be a critical issue for a stable module, marking as a stable blocker.

Thanks for the patch. One thing I noticed that maybe should be added?

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -283,6 +283,37 @@ public function testLayoutBuilderUi() {
+    $assert_session->pageTextNotContains('The first node body');
+    $assert_session->pageTextContains('Access denied');
+
+    $this->drupalGet('node/1/layout');
+    $assert_session->pageTextNotContains('The first node body');

Add another 'Access denied' assertion for layout page?

==

I tested this and it is showing access denied for both node/1 and node/1/layout now as expected.

Marking back to "Needs work" in case the test should be updated.

Good point @Kristen Pol, idk why I didn't add that second check in the first place.

Here's an update 8.7 and 8.6 patch

Added a CR https://www.drupal.org/node/3032823 and wrote a release notes snipppet

The 8.7 patch needed a reroll, the 8.6 patch did not but reuploading anyway just to be safe.

Discussed with @xjm and @tedbow, specifically about why we need the else statement.

Turns out, we don't.
$entity_type->hasLinkTemplate('canonical') is checked before entering into this loop.
And the canonical link is used to build the path for this route below.

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -209,10 +209,8 @@ public function buildRoutes(RouteCollection $collection) {
   -      $requirements = [];
   -      if ($this->hasIntegerId($entity_type)) {
   -        $requirements[$entity_type_id] = '\d+';
   -      }
   +      // Retrieve the requirements from the canonical route.
   +      $requirements = $collection->get("entity.$entity_type_id.canonical")->getRequirements();
   

   Nice change. This seems a lot less brittle!

2. +++ b/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
   @@ -294,6 +294,14 @@ public function testBuildRoutes() {
   +    $from_canonical = $this->prophesize(EntityTypeInterface::class);
   +    $from_canonical->entityClassImplements(FieldableEntityInterface::class)->willReturn(TRUE);
   +    $from_canonical->hasViewBuilderClass()->willReturn(TRUE);
   +    $from_canonical->hasLinkTemplate('canonical')->willReturn(TRUE);
   +    $from_canonical->getLinkTemplate('canonical')->willReturn('/entity/{entity}');
   +    $from_canonical->hasHandlerClass('form', 'layout_builder')->willReturn(TRUE);
   

   Nit: I think it's possible to just do new ContentEntityType([...]), and not mock anything here. Might be a bit easier to read...but this will work too. That's why it's a nitpick. :)

Ughh posted the patch for the wrong version.

#28.2 Yes I considered that, but I decided mocking the specific methods instead of messing with annotation-based array structure was cleaner.

OK, well, those look like real fails... apparently we do need those if checks for uninstallation an may need the fallback for whatever entities have it as NULL. :) And an inline comment explaining about those entities that don't have routes.

I was slightly wrong in #27. Turns out that saying you have a canonical route and having a canonical route are different things.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,6 +230,10 @@ public function buildRoutes(RouteCollection $collection) {
+      // Ensure the canonical route exists.
+      if (!$collection->get("entity.$entity_type_id.canonical")) {
+        continue;
+      }

So what are the cases where it doesn't exist, and what are the consequences of this for those cases? Is access granted or denied?

In order to have a Layout Builder UI, we require that the entity type specify a canonical route.
When we check that, we don't have the ability to ensure that they actually provided one until we're in this loop.

So, this is the same behavior as if they never claimed to provide one: there will not be a Layout Builder UI provided.

Meaning they will get a 404.

This is reflected in the test coverage I added in the interdiff.

The interdiff in #36 illustrates how unhelpful and brittle the current tests are for Layout Builder routing.
I opened #3033439: Refactor OverridesSectionStorageTest::testBuildRoutes() and DefaultsSectionStorageTest::testBuildRoutes() to be functional tests to fix that.

Additionally, the placement of that continue statement was misleading. I moved the check to the top of the loop and gave it a better comment.

So @tim.plunkett assures me that this test code:

+++ b/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
@@ -294,6 +294,15 @@ public function testBuildRoutes() {
+    $canonical_link_no_route = $this->prophesize(EntityTypeInterface::class);
+    $canonical_link_no_route->entityClassImplements(FieldableEntityInterface::class)->willReturn(TRUE);
+    $canonical_link_no_route->hasViewBuilderClass()->willReturn(TRUE);
+    $canonical_link_no_route->hasLinkTemplate('canonical')->willReturn(TRUE);
+    $canonical_link_no_route->getLinkTemplate('canonical')->willReturn('/entity/{entity}');
+    $canonical_link_no_route->hasHandlerClass('form', 'layout_builder')->willReturn(TRUE);
+    $entity_types['canonical_link_no_route'] = $canonical_link_no_route->reveal();
+    $this->entityFieldManager->getFieldStorageDefinitions('canonical_link_no_route')->shouldNotBeCalled();

...somehow adds test coverage that ensures that the user gets a 404 when the conditions for the continue are met. #3033439: Refactor OverridesSectionStorageTest::testBuildRoutes() and DefaultsSectionStorageTest::testBuildRoutes() to be functional tests will definitely help make this clearer.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -227,13 +227,15 @@ private function extractEntityFromRoute($value, array $defaults) {
+      // If the canonical route does not exist, do not provide any Layout
+      // Builder UI routes for this entity type.

+1 for this comment; it is much clearer and with the new code location, makes it clear we're not adding a different kind of access bypass for entities that don't have a canonical route.

This looks good to me and I plan to commit it once we are out of code freeze. Thanks!

Committed to 8.7.x ad 8.6.x, thanks!

Does the bug exist in 8.5.x? (Since this is a security issue there too probably.)

Here's a backport, the bug does indeed exist in 8.5

Committed to 8.5.x. Thanks!