Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs

@Wim Leers thanks for finding this.

Perhaps the intent is "if you can edit the entity, you can also change its layout"?

No that is not the intent.

Here are is unit test for access.

Wow I forgot @group for the new test file. Totally never did that before 😉

Updated/repurposed the issue.

This patch implements \Drupal\layout_builder\Field\LayoutSectionItemList::defaultAccess()

See the summary for reasons

I removed the unit test because there is no logic in the method now.

Moving the tag here

The patch in #8 is clear but I'm not sure how to test this manually. Thoughts?

@Kristen Pol thanks for the offer to test. Basically this patch should make it so the field 'layout_builder__layout' is return on REST GET request.

You could test by:

1. Turn on REST and Layout Builder
2. Enable Layout overrides for articles
3. Enable ReST Get requests on Articles. Probably easiest with https://www.drupal.org/project/restui
4. Make a get request for an article node with an override https://www.drupal.org/docs/8/core/modules/rest/2-get-for-reading-conten...
5. Inspect the JSON response
6. Without the patch: There should be a key for layout_builder__layout
7. You probably need to save the node in between requests. No need to change the layout. Just change the title to make sure you aren't getting a cached response.
8. With the patch: There should NOT be a key for layout_builder__layout

I realized now after #2724823: EntityResource: read-only (GET) support for configuration entities you can actually make GET rest request for config entities. So we need to make sure that the sections are not return with that entity either. Since it is not a fieldable entity we will actually have to have our own normalizer.

This patch just adds the normalizer. We will need test for the normalizer and probably rest test to show it does not get returned.

Changing title to encompass more that just the field

The overall plan described in the issue summary sounds correct to me from a release management perspective. I checked with @tedbow and right now HEAD just gives you:
[]
So there's no real BC break for the beta by adding this restriction now.

+1 for forbidding access by default, but still making a workaround possible if needed. What's missing from the patch right now is:

1. Something warning the site owner that BC is not provided if they choose to alter the access and create a custom normalizer.
2. A @todo referencing a followup that will add read access to the intentionally designed API. (The warning above could also reference this issue, so that the deliberately living-on-the-edge developer knows when they will need to change their custom code.)
3. A change record.

Other than that, I'm comfortable signing off on the general approach here from a release management perspective, and we also have @Wim Leers' (implied) signoff as API-first maintainer.

Thanks!

To clarify #15 after discussing with Ted in Slack, we need to document in the patch why we're restricting access and why we're not exposing sections in the normalizer, with @todos. Let's see what that looks like firstt. And then if those docs do mention overriding the access as a workaround, then they might need the warning of that not having BC.

@xjm thanks for the comment

This patch adds REST coverage for the override field

Tests override field with rest:

1. Doesn't get returned in GET
2. Including in PATCH get 403
3. Including in POST gets 403

We still need GET test for defaults. Other methods are not supported on config entities.

This patch adds the rest test coverage for layout defaults.

No sure why this is failing. It is passing for me locally.

I am going to try to switch to json_decode directly to decode. Just running this 1 test.

1. I changed the title of the issue in #14 to reflect that it is get rid of all "sections". The sections exist in the override field but also in Entity View Displays third party settings. See testLayoutEntityDisplay().
2. I assume this is 👍 for overriding defaultAccess() and not access(). Just checking to be sure.
3. Changed to extend ResourceTestBase
4. Remove/not need anymore, copy/paste error.
5. Thanks I think that fixed. I still have 2 cases where we need the contents. But refactoring into method.

🤞 for tests passing on drupalci

1. Stilling getting the NotEncodableValueException exception above.

2. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/SectionsNotExposed.php
   @@ -0,0 +1,322 @@
   +    // @todo why does this fail? unrelated error?
   +    $this->assertCount(1, $this->node->get(OverridesSectionStorage::FIELD_NAME));
   +     */
   

   @tim.plunkett help me debug this and we think it might be related to #2985882: Workaround for "Call to a member function getLabel() after enabling layout_builder"

   For now I can't get this to work unless I clear the field cache. We can tell the field is actually there because the override gets stored in setup().

3. Because of how \Drupal\Tests\rest\Functional\ResourceTestBase::provisionResource works I have spilt this into 2 test classes with a base class.
   This allows remove the test module.

+++ b/core/modules/layout_builder/tests/src/Functional/Rest/LayoutRestTestBase.php
@@ -0,0 +1,154 @@
+    return $this->serializer->decode((string) $response->getBody()->getContents(), static::$format);

Whoops I don't need the ->getContents().

Adding "how to test" to the issue summary per #12.

+++ b/core/modules/layout_builder/layout_builder.services.yml
@@ -44,9 +44,8 @@ services:
+    parent: serializer.normalizer.config_entity
...
-    arguments: ['@entity_type.manager', '@entity_type.repository', '@entity_field.manager']

In order for this to work, we'd have to move this to LayoutBuilderServiceProvider and check for moduleExists('serialization')

1. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -74,4 +76,11 @@ public function equals(FieldItemListInterface $list_to_compare) {
   +  public function defaultAccess($operation = 'view', AccountInterface $account = NULL) {
   +    return AccessResult::forbidden();
   +  }
   
   +++ b/core/modules/layout_builder/src/Normalizer/LayoutEntityDisplayNormalizer.php
   @@ -0,0 +1,30 @@
   +  protected static function getDataWithoutInternals(array $data) {
   +    $data = parent::getDataWithoutInternals($data);
   +    // Do not expose the actual layout sections in normalization.
   +    unset($data['third_party_settings']['layout_builder']['sections']);
   +    return $data;
   

   We should link the issues where we're discussing whether to add back normalization and how with an inline comment and an @todo.

2. +++ b/core/modules/layout_builder/src/LayoutBuilderServiceProvider.php
   @@ -35,6 +37,13 @@ public function register(ContainerBuilder $container) {
   +    if (isset($modules['serialization'])) {
   +      $definition = (new ChildDefinition('serializer.normalizer.config_entity'))
   +        ->setClass(LayoutEntityDisplayNormalizer::class)
   +        // Priority must be higher than serializer.normalizer.config_entity.
   +        ->addTag('normalizer', ['priority' => 5]);
   +      $container->setDefinition('layout_builder.normalizer.layout_entity_display', $definition);
   

   Why must the priority be higher? Edit: My point, is the comment should say why.

3. +++ b/core/modules/layout_builder/src/Normalizer/LayoutEntityDisplayNormalizer.php
   @@ -0,0 +1,30 @@
   +/**
   + * Normalizes/denormalizes LayoutEntityDisplay objects into an array structure.
   + *
   + * @internal
   + */
   

   Let's document why it's internal.

4. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/EntityDisplaySections.php
   @@ -0,0 +1,58 @@
   +    // Ensure the sections have been removed from third party settings.
   

   Nit: "third-party settings".

   This comment also made it sound like section data was actually being deleted from a config entity. I think the comment should end with "...in the serialized data."

5. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/LayoutRestTestBase.php
   @@ -0,0 +1,156 @@
   +      'title' => 'A node at rest will stay at rest.',
   
   +++ b/core/modules/layout_builder/tests/src/Functional/Rest/OverrideSections.php
   @@ -0,0 +1,153 @@
   +    $this->assertSame('A node at rest will stay at rest.', $get_data['title'][0]['value']);
   

   lol puns!

6. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/LayoutRestTestBase.php
   @@ -0,0 +1,156 @@
   +    ]);
   +
   +
   +    $this->drupalGet('node/' . $this->node->id() . '/layout');
   ...
   +    $this->node = $this->nodeStorage->load($this->node->id());
   +
   +
   +    $this->drupalLogout();
   

   Double blank lines; these will cause a phpcs fail and make this patch uncommittable. :)

7. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/LayoutRestTestBase.php
   @@ -0,0 +1,156 @@
   +    // Give the test user all permissions on the site. There should be no
   +    // permission that gives the user access to layout sections.
   

   For clarity, we probably should say "...no permission that gives the user access to layout sections over REST."

8. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/OverrideSections.php
   @@ -0,0 +1,153 @@
   +    $this->assertEquals('On with the rest of the test.', $posted_node->getTitle());
   

   MORE puns.

Oh this also still needs its change record.

Actually this probably needs to be mentioned in the release notes as well.

Sorry, never mind about the release note, I forgot since the last time I reviewed this that currently we don't expose anything and this is just making it explicit. So just the CR. Thanks!

#36
1) Done
2) Done, but opened #3031698: Consider making the default normalizers take a lower priority by default as well because this seems unwieldy.
3) Done
4) Fixed and expanded the test slightly
5) 😂
6) Fixed
7) Fixed
8) I love it

Also I changed the test classes to have a Test suffix.

Opened a CR https://www.drupal.org/node/3031697

1. +++ b/core/modules/layout_builder/src/LayoutBuilderServiceProvider.php
   @@ -40,7 +40,8 @@ public function register(ContainerBuilder $container) {
   +        // Priority must be higher than serializer.normalizer.config_entity in
   +        // order to take precedence.
   
   diff --git a/core/modules/layout_builder/src/Normalizer/LayoutEntityDisplayNormalizer.php b/core/modules/layout_builder/src/Normalizer/LayoutEntityDisplayNormalizer.php
   index e7a83d0cbe..c4c7640471 100644
   

   Okay yes that's what a higher priority is by definition, but why must it take precedence?

2. +++ b/core/modules/layout_builder/tests/src/Functional/Rest/EntityDisplaySectionsTest.php
   @@ -51,7 +51,10 @@ public function testLayoutEntityDisplay() {
   +    // Ensure the sections not present in the serialized data, but other Layout
   +    // Builder data is.
   

   There's a verb missing now.

Fixed

Looks good!

Committed to 8.7.x and published the change record. Thanks!

@xjm thanks for committing.

@Wim Leers thanks for your help on this an suggesting not exposing the field right now.

@tim.plunkett @Kristen Pol thanks for the help too