Add granular permissions to the Layout Builder

added changes to permissions

Changing permission in test.

Fixing permission access to layout section

+1 for RTBC

+++ b/core/modules/layout_builder/layout_builder.permissions.yml
@@ -1,5 +1,14 @@
+configure blocks:

This one could probably be split into multiple permissions?

+++ b/core/modules/layout_builder/layout_builder.permissions.yml
@@ -1,5 +1,14 @@
+configure entity layout:
+  title: 'Configure entity layout'
+  restrict access: true
+configure sections:
+  title: 'Can add and configure sections'
+  restrict access: true
+remove sections:
+  title: 'Can remove sections'
+  restrict access: true
+configure blocks:
+  title: 'Can add, update, remove and move blocks into sections'

Thanks for the patch. Here is some feedback on naming conventions.

1) The first permission has the "active verb" prominent but all the rest are "Can xyz". I took a quick look at other core permissions and it looks like changing these to be something like the following would be good:

* Add and configure sections (or just Configure sections)
* Remove sections
* Add, update, remove, and move blocks in sections

(note on the last one that a) it should have a serial/Oxford comma in the list, and b) I changed "into" to "in".

2) In my quick search, didn't see any existing permissions that use "configure". Instead, "administer" is used heavily. In light of that, for consistency, perhaps these should instead be something like:

* administer entity layout
* administer sections
* remove sections
* administer section blocks

3) Should "remove sections" be there or just handled with the configure/administer permission? Configure/administer might imply that removal is part of the permission.

4) The list of permissions in the patch doesn't line up one-for-one with the issue summary so it would be good to discuss the reasons for collapsing these rather than having more fine-grained permissions

Added changes to permissions and code to hiding links if the user does not have access to remove.

Permission IDs update were missing so I updated them according to point 2 in #15
I agree with point 3 in #15 as well so I updated "remove sections" usage by "administer sections"
We still need further discussion as point on point 4 in #15

Thanks for the updates.

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -247,20 +253,7 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
+      'remove' => $this->getRemoveLinkData($storage_type, $storage_id, $delta),

Was there a compelling reason to move out this code to its own function? Was it to simplify the access check?

I'm also now wondering if there is any value for someone to be able to only "administer sections" or only "administer section blocks". I can't think of a use case of the top of my head. The issue summary has a bunch of granular permissions. I understand that for something like adding, editing, deleting, and viewing content but for working with the layout, if they can use the builder, it seems like they should be able to do anything. I see why someone would argue that deleting things might be more restrictive but I think it would be weird UX if someone can add, move, and update sections and blocks but not be able to delete blocks.

+++ b/core/modules/layout_builder/layout_builder.permissions.yml
@@ -1,5 +1,8 @@
+administer sections:
+  title: 'Administer sections'
+  restrict access: true
+administer section blocks:
+  title: 'Administer section blocks'

These titles are a bit vague. Can we say "Administer layout sections" and "Administer layout section blocks" instead? Ideally we would also do this for the actual permission names too.

Re-rolled the patch in 22 against latest 8.6.x, additionally updated permission label + name per phenaproxima's suggestion.

Updated tests w/ new permissions

1. Re #23
   

   if they can use the builder, it seems like they should be able to do anything.

   +1 for this.

2. There are purposed permissions around "Landing Pages". What are these? I imagine if wanted to create landing pages I would just create a content type with no other fields. This will be much more powerful after #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder.

   Are we going to something different directly in Layout Builder for adding Landing pages? Is there an existing issue?

Testing the patch

If give a user "administer layout sections" but not "administer layout section blocks" it is very weird experience.

The can add new sections but they can't do anything with them.

They also now can see the "Add block" link but nothing happens b/c they get an access denied.

If the user has "administer layout section blocks" but not "administer layout sections" they can see the "Layout" tab but then get an "access denied" if they click on it.

From the purposed Must Have permissions

Can reorder sections
Can add sections
Can delete sections

How were these chosen? Is this something similar to the Panels contrib modules?

These seem like these permissions could lead to very weird UX for the builder.

If you have only had "add sections" here. What if you only had "add sections"? If added a section but put in the wrong place you couldn't move the section or delete it.

Would we allow moving or deleting of sections that you just added before you clicked "Save Layout"?

Otherwise it seems the common workflow if you only had "add section" permission would be

1. Add section
2. Oh no I put in the wrong place(maybe I already added fields)
3. Realize I can move it.
4. Click "Cancel Layout"
5. Add section again
6. Readd fields

Of course if you realized that you put the section in the wrong place after you save there would be nothing you could do about it.

Bundle based

Adding a permission per bundle would be allow fine grained control but it would get overwhelming on permissions page.

But what if under "LAYOUT OPTIONS" when "Allow each content item to have its layout customized." was checked there was an additional option "Allow users to configure the layout for any content items they can edit"

I think for many entity types for many sites this would make sense. Of course the global "Configure any layout" would still apply.

I agree with #30 - The proposed permissions are too granular IMO and I can't think of a use case.

There's something that's not touched on here that I think is important to address as well: The "administer display" permission (each entity type has one) is used for controlling access to modifying the default layout for each view mode. I can see why this chosen (since LB essentially replaces Field UI), but I think LB introduces a new paradigm for site building that Field UI did not have.

I'd like to give access for site builders to modify the default layouts for a view mode, but I don't want them to have access to turn LB on/off, toggle overrides, or control what view modes an entity has access to.

Here is try at the "Bundle based" idea in #30

I added testing the permission to the existing test method \Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testLayoutBuilderUi()

So that override logic is done with a user with the existing permission and then with permission to edit the node.

1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -3,7 +3,6 @@ layout_builder.choose_section:
   -    _permission: 'configure any layout'
   

   I removed the permission on the route and now '_layout_builder_access' will check the permission when needed.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -237,6 +237,17 @@ public function save() {
   +    // Create an additional access result that will allow access to the layout
   +    // if the user has access to update the entity, if configured for entity
   +    // edit access or default access.
   +    $additional_access_result = AccessResult::neutral();
   +    if ($default_section_storage->isEditAccessControlled()) {
   +      $entity = $this->getEntity();
   +      $additional_access_result = $additional_access_result->orIf($entity->access('update', $account, TRUE));
   +      $result->addCacheableDependency($entity);
   +    }
   +    $additional_access_result = $additional_access_result->orIf(parent::access('view', $account, TRUE));
   +    $result = $result->andIf($additional_access_result);
   

   Here is the main part where the user would have to have either entity update access or access via the parent, which will check 'configure any layout'

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -103,4 +105,12 @@ public function removeSection($delta) {
   +  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
   +    $result = AccessResult::allowedIfHasPermission($account, 'configure any layout');
   +    return $return_as_object ? $result : $result->isAllowed();
   +  }
   

   This is the only place now directly checks the permission

Re #32 being able to control default without "administer display" permission for the entity type:

While I could see use for that I think more commonly if you have you "administer display" permission for an entity type you should automatically should be able control the layout defaults for that entity type.

Right now we add the "Manage layout" to button to the "Manage display" and don't even check if the user "configure any layout" permission. So if you don't have permission when click the button you get "access denied". The fact that as far as I can tell nobody has failed as an issue points to the probability people are probably not assigning "administer display" without also having "configure any layout".

Also you are still "administering the display", you are organizing field order and configuring widgets.

Right now we add the "Manage layout" to button to the "Manage display" and don't even check if the user "configure any layout" permission. So if you don't have permission when click the button you get "access denied". The fact that as far as I can tell nobody has failed as an issue points to the probability people are probably not assigning "administer display" without also having "configure any layout".

Right, the thing is, I don't want to hand out the "administer display" permission. I just want "configure any layout" to be sufficient for controlling the default layout per view mode, but as I recall that's not currently the case.

I'll have to spend some time on this soon to figure out if I can easily modify these perms with a route modifier, and if so, it's not that big of a deal to me.

@bkosborne yeah I understand your use case. I was talking about different case. The case where user already does the "administer display" permission.

I think that permission should sufficient enough to access to the Layout builder because they can already turn on the layout builder for the bundle and they have power to allow overrides. These are power users.

I just want "configure any layout" to be sufficient for controlling the default layout per view mode, but as I recall that's not currently the case.

If we allow this do we also have the solve the problem in core about how the users would access the default layouts because I think right now if they have the permission then they won't have a way to get to the pages.

but that is true also if you have "administer display" permission and not "administer fields" permission.

I'll have to spend some time on this soon to figure out if I can easily modify these perms with a route modifier, and if so, it's not that big of a deal to me.

That should be pretty easy. Just create a routing subscriber that runs after \Drupal\layout_builder\Routing\LayoutBuilderRoutes which has

public static function getSubscribedEvents() {
    // Run after \Drupal\field_ui\Routing\RouteSubscriber.
    $events[RoutingEvents::ALTER] = ['onAlterRoutes', -110];
    return $events;
  }

So would have to be below -110. And then if you look at \Drupal\layout_builder\Plugin\SectionStorage\DefaultsSectionStorage::buildRoutes()

and then unset $requirements['_field_ui_view_mode_access']

another proposal

My current thinking is we should do:

1. "administer display" permission should allow also configuring defaults for the corresponding entity type
2. Bundles have "Allow users to configure the layout of any @entity they can edit" I added in #34
3. Add a new permission "Configure Layout Overrides"
4. Maybe: "Configure Layout defaults"
5. for existing "configure any layout" permission Deprecate it meaning don't create this permission new sites 🔥

Having 3 & 4 permissions would be much more powerful than the single permissions now. To give a role the power of the current permission just give the role 3 & 4.

4) would allow the case @bkosborne was #32 and #37

1) would allow power users to continue have same kind of power they currently have just with a new and better tool. It also allows that permission to be given on entity type basis. It also would limit them in the same way in that are not give power to create or update "content".

We have to consider that when we have #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder(🤞) giving users the permission to configure layout overrides in a lot of sites is effectively going to give the those users the ability to edit content even if they can't actually edit those content entities. Because sites are going to using inline blocks to create content in a paragraph style way.

Right now our permissions make it very easy give permission edit defaults without editing content and I think we should maintain that.

2) allows for sites set up where configuring layout overrides is effectively editing content to maintain whatever fine grained control they have set up around editing content whether it is setup throw custom code or contrib. The point they won't have to create new logic and code around the layout builder.

It also allows for other sites to have users who can configure the layout override to be separate from content editors. They just don't turn on this permission.

reroll

Fixing the tests. If \Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase::access() is not give a user it should used the currently logged in user.

This is the behavior that is expected with AccessibleInterface

This removes the options on Manage display when Layout builder is enabled for overrides "Allow users to configure the layout of any @entity they can edit"

This was in the previous patches. It is different than how other access are done in core because you don't control access by giving a permission by giving permission to a role.

Instead this patches makes 2 new permissions for each bundle where overrides are enabled.

1. Configure any override for [ENTITY_TYPE] of [BUNDLE]
2. Configure overrides for editable [ENTITY_TYPE] of [BUNDLE]

The second one basically means if you can edit the entity you can edit the override. I am not sure if that would be hard to explain the permission page.

These 2 permissions would let you set up different kinds of users

1. Some "design" users that can configure Layouts for articles but don't have any edit the access to other fields.
2. when using Landing pages(maybe with no other fields), users who can only configure the layout for their own landing pages(others they are somehow granted access to edit.)

Updated the summary for this new approach.

I talked to @tim.plunkett and @DyanneNova about this this week and I think they were onboard with this.

1 problem with more complicated locating and section level permission is that we hope someday to be able to offer decoupled API access to updating layouts. However complicated we get in the UI we would have to support over the API which could get very complicated with validation.

@tedbow What's the best way to test this?

@Kristen Pol thanks for asking.

1. Install via standard profile
2. Enable layout builder
3. enable layout builder on Articles and allow overrides
4. create 3 roles
   layout_admin has "configure any layout"
   article layouter has "Configure any layout overrides for Content items of Content type Article" + "Article: Edit own content"
   article editor has "Configure layout overrides for editable Content items of Content type Article"
5. assigner a user each role
6. create 2 nodes, 1 authored by each user with the article specific permissions
7. user with layout_admin permission should be able to access
   /admin/structure/types/manage/article/display-layout/default
   and node/*/layout for both nodes
8. user with article layouter permission should be able to access
   node/*/layout for both nodes

   Not able to access: /admin/structure/types/manage/article/display-layout/default

9. user with article editor permission should be able to access
   node/*/layout for only the node they authored. Not for the other node

   Not able to access: /admin/structure/types/manage/article/display-layout/default

Needed a reroll

Rerolled patch #49.

@bendeguz.csirmaz thanks for the reroll.

Probably all the fails were from what @tim.plunkett mentioned in #52.

All the kernel and unit tests passed for me locally plus \Drupal\Tests\layout_builder\Functional\LayoutBuilderTest which is the only test that changed.

So hopefully this is good.

Added "how to test" to issue summary from #49.

Reroll

The test steps in #49 will not work - some of the test users require additional permissions and some of the paths used have been changed. I am in the process of rewriting these steps and will add them here once completed.

I updated the test instructions to reflect what was necessary to test locally. My changes are in italics, and comments in parentheses. Before moving these to the IS, I'd like @tedbow to review and make sure that's how the permissions should behave, or if this exposes additional work that needs to be done.

1. Install via standard profile
2. Enable layout builder
3. enable layout builder on Articles and allow overrides
4. create 3 roles
   layout_admin has "configure any layout" + Content: Administer display

   article layouter has "Configure any layout overrides for Content items of Content type Article" + "Article: Edit own content" +Article: Create new content +

   article editor has "Configure layout overrides for editable Content items of Content type Article" + Article: Edit own content (node/*/layout returns Access Denied without this permission, even for nodes created by user)

5. create 2 nodes, 1 authored by each user with the article specific permissions. Then, as admin user, publish these two new nodes.
6. user with layout_admin permission should be able to access
   /admin/structure/types/manage/article/display/default/layout
   and node/*/layout for both nodes
7. user with article layouter permission should be able to access
   node/*/layout for both nodes
   Not able to access: /admin/structure/types/manage/article/display/default/layout
8. user with article editor permission should be able to access
   node/*/layout for only the node they authored. Not for the other node
   Not able to access: /admin/structure/types/manage/article/display/default/layout

@bnjmnm re #58 thanks for laying this out in detail!

article layouter should not need ""Article: Edit own content" but does need "View published content". that user can create an article not edit it but still configure the layout. Any for any published article

(node/*/layout returns Access Denied without this permission, even for nodes created by user)

Yes this is correct for article editor because their access is dependent on edit access(and view but so is everyone's)

@tim.plunkett re #59
Thanks for the review!

1. Ok then removing override
2. yeah I just figure 1 less place we have to copy the string
3. Tried to clear up permissions
   Changed for first 1 to

   Configure all layout overrides

   instead of "any"
   Change the second to "editable by the user"

   Agree they could use more work.

   Also, when comparing the "per-bundle bypass edit-access" one, do we still need the "configure any layout" permission?

   For BC wise I am sure we can get rid of it.
   Also these permission only deal with overrides not defaults.

   Finally, right now Field UI permissions are needed to enable LB itself (defaults and overrides). I think we can punt any perms related to that to

   Not sure how that would change anything here.

4. fixed
5. Fixed to use entity_type.bundle.info service.
   Add to also making different permission titles and descriptions for entity types that don't have bundles. So it doesn't say "User items of user".
   Also had to change the titles because permission are sorted by title and the current ones would not group entity types together.
6. fixed
7. Fixed
8. fixed
9. reworded
10. added "s"
11. fixed
12. I reorder things a bit here. Now there are only 2 variables $access_result and $edit_only_result. I would like to keep the edit result separate because it is an AND condition nested in an OR.

    I move checking the layout builder to the end which eliminates the need for another variable.

I talked this over in the UX meeting. Thanks everyone!(will credit attendees in subsequent comments).

It was decided that we needed to shorten up the descriptions and make them more clear.

Here is what we came up with:

For entity types with bundles, for example articles bundle for nodes
Content - Article: Configure all layout overrides
Content - Article: Configure layout overrides for content items that the user can edit.

The pattern is:
[entity type collection label] - [bundle]: Configure all layout overrides
[entity type collection label] - [bundle]: Configure layout overrides for [entity type plural] that the user can edit.

For entity types without bundles, for example user:
Users: Configure all layout overrides
Users: Configure layout overrides for users that the user can edit.

The pattern is:
[entity type collection label]: Configure all layout overrides
[entity type collection label]: Configure layout overrides for [entity type plural] that the user can edit.

I really like the permissions! They are clear now.

We need a test for testing an entity without bundles.
And there are todos in the patch.

Setting to NW for this.

Reroll of patch #61.

I have some questions.

We need a test for testing an entity without bundles.
And there are todos in the patch.

- #68

Could you elaborate on what you mean by testing an entity without bundles please? - ah, I realized #62 already described it. Sorry. What should be tested though? The permission labels?

diff --git a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php 
...
+    // Enable overrides without a form to ensure permission cache gets cleared.
+    // @todo Test access changes when overridable changes.

I'm not sure what this todo means.
Do we need a test for when an entity display's overridable property changes from true to false, users without the configure any layout permission are no longer able to edit layouts?

#71 There are no scenarios in the test where the entity has no bundles, which has separate permissions.

1. Retesting with out a bundle you can just test the same scenario with

   +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -154,17 +157,40 @@ public function testPreserverEntityValues() {
   +  public function providerLayoutBuilderUi() {
   +    return [
   +      'configure any layout' => [
   +        ['configure any layout'],
   +      ],
   

   This dataProvider could be changed to return entity types and bundle(null for user). So basically you double the test cases.

   but you probably would also have to return something like $manage_display_path because the patter would be different for nodes and users.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -226,8 +252,16 @@ public function testLayoutBuilderUi() {
   +    // @todo Test access changes when overridable changes.
   

   I think the thought here is to check access to /node/1/layout when it is not actually overridable. That might out of scope of this issue.

   Also before in this patch there was a setting on manager display for "Allow users with edit access to entity to configure layout for entity". Since this is not here any more and just standard permission I think we can just remove todo.

The OverridesSectionStorageTest::testDeriveContextsFromRoute (#70) fails because the current_user service is set to a prophesized account with the id 999.
When CurrentUserContext::getRuntimeContexts tries to load the current user (with the id 999), it obviously returns null.
I wonder why this hasn't failed before.

Fixing the test and changing a type hint.

#73.1
I think adding more complexity to the LayoutBuilderTest is not a good idea.
This seems like a good opportunity to refactor the testLayoutBuilderUi method into a new class.

re #74 I think this new fail is because changes committed in #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

re #76
@bendeguz.csirmaz I think you are correct about not adding more complexity. But I don't refactoring is needed in this issue(though we could in another issue)

Here is my idea
I initially updated `\Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testLayoutBuilderUi()` with data provider to test with different permissions because I wanted a quick way to test that permissions worked.

But that method is very complicated and tests a lot more that we need to prove the permissions themselves work.

we should probably instead simply create a new test method and/or class that simply tests the permissions .

basically that you get access denied on overrides when you should and that none of the new permissions give you access to defaults.

this test method could use the existing dataProvider but with added cases for user entity which has no bundle.

then we can revert all changes to testLayoutBuilderUi in this issue.

does #78

i really like seeing this being put in its own file!

1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,245 @@
   +    $this->drupalGet('admin/config/people/accounts/display/default');
   +    $this->drupalPostForm(NULL, ['layout[enabled]' => TRUE], 'Save');
   +    $this->drupalPostForm(NULL, ['layout[allow_custom]' => TRUE], 'Save');
   +    $this->drupalLogout();
   

   we should considering making a base class for functional tests, we could definitely re-use some code. (not in this issue though)

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,245 @@
   +  public function testUserAccess($permissions, $default_access, $non_editable_access, $editable_access) {
   

   Nit: Maybe we can call this testUserLayoutAccess (only User threw me off while reading)

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,245 @@
   +  private function assertAccessCorrect($expected_access) {
   

   instead of checking for text, we could test the http status code returned 200 vs 403, saves us a variable to manage

1. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderAccessCheck.php
   @@ -31,6 +32,13 @@ class LayoutBuilderAccessCheck implements AccessInterface {
   +    // Check for the global permission unless the section storage checks
   +    // permissions itself.
   +    if (!$section_storage->getPluginDefinition()->get('handles_permission_check')) {
   +      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'configure any layout'));
   +    }
   

   It took me a long time to understand this, but @tim.plunkett was kind and patient enough to explain it to me in a call.

   Although this seems pretty awkward (and it kind of is), it preserves the behavior we already have in core. Previously, any Layout Builder route required the 'configure any layout' permission in order to work. That's no longer the case -- we're delegating access to the section storage plugin. And that is fine, except that we cannot assume that the section storage is checking for the 'configure any layout' permission, and that discrepancy constitutes a BC break. This stems from the fact that the access checking for the routes is not necessarily going to produce the same result as the access checking for the plugin, and ever thus has it been.

   So effectively, this is preserving the behavior in HEAD, whilst also giving section storage plugins a way to take a different path during routing-level access checks. I think this should be made very clear in the change record.

2. +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
   @@ -46,6 +46,20 @@ class SectionStorage extends Plugin {
   +  /**
   +   * Indicates that this section storage handles its own permission checking.
   +   *
   +   * If FALSE, the 'configure any layout' permission will be required. If TRUE,
   +   * no additional permission check will be added and the
   +   * \Drupal\layout_builder\SectionStorageInterface::access() implementation
   +   * will be fully responsible for all access checking. Defaults to FALSE.
   +   *
   +   * @var bool
   +   *
   +   * @see \Drupal\layout_builder\Access\LayoutBuilderAccessCheck
   +   */
   +  public $handles_permission_check = FALSE;
   

   Apropos, I think we might want to mention that this is only applicable during routing-level access checks, and that the 'configure any layout' permission is only required under those circumstances.

3. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,127 @@
   +  public static function getBundleEditOverridePermissionKey($entity_type_id, $bundle) {
   +    return "configure layout overrides $entity_type_id $bundle editable";
   +  }
   

   I know these are machine-readable permission names, but the phrasing seems odd to me. How about "configure editable $bundle $entity_type_id layout overrides"? That will result in things like "configure editable remote_video media layout overrides", which reads almost like a sentence to me.

4. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,127 @@
   +  public static function getBundleOverridePermissionKey($entity_type_id, $bundle) {
   +    return "configure layout overrides $entity_type_id $bundle all";
   +  }
   

   Similarly, I wonder if "configure all $bundle $entity_type_id layout overrides" would read better.

5. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -423,6 +423,7 @@ public function getThirdPartyProviders() {
        $result = AccessResult::allowedIf($this->isLayoutBuilderEnabled());
   +    $result->addCacheableDependency($this);
   

   Nit: These can be chained.

6. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,222 @@
   +   * Test access to Layout Builder for user entities which do not have bundles.
   

   This needs a comma. "...for user entities, which do not..."

7. +++ b/core/modules/layout_builder/tests/src/Unit/LayoutTestUserTrait.php
   @@ -0,0 +1,27 @@
   +trait LayoutTestUserTrait {
   +
   +  /**
   +   * Gets a user with only the Layout Builder permission.
   +   *
   +   * @return \Drupal\Core\Session\AccountInterface
   +   *   The mocked user.
   +   */
   +  protected function getProphesziedLayoutUser() {
   +    $account = $this->prophesize(AccountInterface::class);
   +    $account->hasPermission('configure any layout')->willReturn(TRUE);
   +    $account->hasPermission(Argument::type('string'))->willReturn(FALSE);
   +    $account->id()->willReturn(999);
   +    return $account->reveal();
   +  }
   +
   +}
   

   I may be missing something, but this method appears to be used in only one place. Does it really need to be a trait?

Two nits, then RTBC.

1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,221 @@
   +  /**
   +   * Asserts expect access.
   +   *
   +   * @param bool $expected_access
   +   *   The expected access.
   +   */
   +  private function assertAccessCorrect($expected_access) {
   

   "Asserts expect access" is quasi-broken English. :)

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,221 @@
   +    $expected_status_code = $expected_access ? 200 : 403;
   +    $this->assertSession()->statusCodeEquals($expected_status_code);
   

   Nit: This can be all one line if we put the ternary inside statusCodeEquals().

Thanks! We're done here.

1. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,97 @@
   +        $permissions["configure all $bundle $entity_type_id layout overrides"] = [
   +          'title' => $this->t('%entity_type - %bundle: Configure all layout overrides.', $args),
   ...
   +        $permissions["configure all $bundle $entity_type_id layout overrides"] = [
   +          'title' => $this->t('%entity_type: Configure all layout overrides.', $args),
   

   should we mark these two as restrict access given they bypass entity access?

2. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,97 @@
   +      }
   +      else {
   

   minor nit, we could continue here and avoid the else

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -360,8 +370,28 @@ public function save() {
   +    $edit_only_bundle_access = $edit_only_bundle_access->andIf($entity->access('update', $account, TRUE));
   

   should we ->addCacheableDepelency($entity) too?

Changes look okay to me.

I think it's not "regardless of permission restrictions". It is still restricted based on view access, right? So maybe "even if they cannot edit the content itself" or such?

We could prefix it with "warning" to raise the visibility without lumping it into the same category as "bypass node access" and "administer views" and such.

BTW, the reasoning for #98 is that IMO we should not introduce a pattern where site owners are encouraged to hand out restricted access permissions to a content reviewer role. If so, any security issues for that permission would be handled in public by default, and I don't think that's what we want in this case. (Also, other, more dangerous permissions than this one still don't have the flag.)

Tagging for usability review of whatever we finalize on as the alternate proposal.

Maybe "Manage inline blocks" should be "Create and manage inline blocks" or something to clarify its purpose?

re #107 I think the problem is that since #2968500: Change inline blocks workflow in Layout Builder to match mocks we don't actually have the phrase "Inline Blocks" anywhere in the UI. We just have at the top of the block listing "Create custom block" the name we get from getSingularLabel().

So maybe "Create and edit custom blocks" because I don't think we want "manage" because you can put reusable/global blocks that in your block library regardless of this permission.

Adding a couple wording changes and additions after UX meeting today and removing "Needs usability review"
See interdiff for actual wording changes. New "description" was also for inline blocks.

Crediting @rainbreaw and @dyannenova from the meeting. I think everyone else was at the previous meeting and has been credit already.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -497,4 +504,88 @@ public function testAddWorkFlow() {
   +    $page = $this->getSession()->getPage();
   

   unused var

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -497,4 +504,88 @@ public function testAddWorkFlow() {
   +    $this->addInlineBlockToLayout('The block label', 'The body value');
   +    $assert_session->assertWaitOnAjaxRequest();
   

   We don't actually need to wait for the ajax request because this is already called inside addInlineBlockToLayout()

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -497,4 +504,88 @@ public function testAddWorkFlow() {
   +    $this->clickContextualLink(static::INLINE_BLOCK_LOCATOR, 'Configure');
   +    $assert_session->assertWaitOnAjaxRequest();
   

   This a second use of a contextual link on the same which can cause the random failure. see #2918718: Using ContextualLinkClickTrait::clickContextualLink() multiple times per page can cause random failures

   This preventable by just reloading page. Adding with a todo to other issue.

   UPDATE: looking down at the rest of the test, I am not why we are clicking the "Configure" contextual link at all here.

   We are explicitly testing the permissions directly below this. Removing the click.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -497,4 +504,88 @@ public function testAddWorkFlow() {
   +      $page = $this->getSession()->getPage();
   

   unused var

1. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,100 @@
   +    $entity_displays = $this->entityTypeManager->getStorage('entity_view_display')->loadByProperties(['third_party_settings.layout_builder.allow_custom' => TRUE]);
   

   When an entity view display's support for overrides is changed, and then the display is saved, will the permissions cache (and/or other relevant caches) be automatically invalidated? If not, should they be?

2. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,100 @@
   +          'warning' => $this->t('Warning: Allows configuring the layout even if the user cannot edit the @entity_type_singular itself.', $args),
   

   I would slightly change the phrasing of this: "Allows the user to configure the layout even if they cannot..." Not a big deal, though, since labels can be changed up until RC.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -121,6 +132,7 @@ public function blockForm($form, FormStateInterface $form_state) {
   +      '#access' => $this->currentUser->hasPermission('create and edit custom blocks'),
   

   Shouldn't this maybe go in the blockAccess() method? If not, can there be a comment explaining why not?

All good answers. "There is no permissions cache!" sounds like it should be a sticker they give out at DrupalCon.

I haven't reviewed the code here, but FWIW, I was curious to test out how use case 1 worked with content moderation. AFAICT, it worked great. I created 3 roles with one user each: "editor", "designer", "publisher". I gave editor and designer permission to create drafts, and publisher permission to publish. I gave editor permission to create and edit articles, and designer permission to configure all article layout overrides.

As expected, I was able to login as an editor and create and edit articles. For each save, I could only choose "Draft" as the state. I did not have access to the Layout tab.

As expected, I was able to login as a designer, and so long as there already existed a published revision of the article, I was able to access it, and via it, the Layout tab. This worked, even if there was also a forward draft. As expected, when making Layout changes, I could only choose "Draft" as the state, and the newly created revision incorporated the changes from the latest draft created by the editor.

As expected, I was able to login as a publisher, view the revisions created by both the editor and the designer, and publish them.

This issue isn't directly about CM integration and didn't make any changes in regards to that, so I'm just leaving this comment here to confirm that it looks like that all works properly, so that the use case identified in the issue summary is in fact viable.

Mix of notes for myself, questions, and feedback. I haven't read the tests yet or drilled down on the existing access checking API in HEAD.

Non-actionable notes

• +++ b/core/modules/layout_builder/layout_builder.routing.yml
  @@ -134,7 +126,6 @@ layout_builder.move_block:
  -    _permission: 'configure any layout'
       _layout_builder_access: 'view'
  
  +++ b/core/modules/layout_builder/src/Access/LayoutBuilderAccessCheck.php
  @@ -31,6 +32,13 @@ class LayoutBuilderAccessCheck implements AccessInterface {
  +      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'configure any layout'));
  

  This line is the equivalent of the previous combination of the two route requirements for the configure any layout permission and the view access (by making view access andIf() the permission).

• +++ b/core/modules/layout_builder/src/Access/LayoutBuilderAccessCheck.php
  @@ -31,6 +32,13 @@ class LayoutBuilderAccessCheck implements AccessInterface {
  +    if (!$section_storage->getPluginDefinition()->get('handles_permission_check')) {
  
  +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
  @@ -46,6 +46,20 @@ class SectionStorage extends Plugin {
  +  /**
  +   * Indicates that this section storage handles its own permission checking.
  +   *
  +   * If FALSE, the 'configure any layout' permission will be required during
  +   * routing access. If TRUE, the section storage's implementation of
  +   * \Drupal\Core\Access\AccessibleInterface::access() will be fully responsible
  +   * for the routing access checking. Defaults to FALSE.
  +   *
  +   * @var bool
  +   *
  +   * @see \Drupal\layout_builder\Access\LayoutBuilderAccessCheck
  +   */
  +  public $handles_permission_check = FALSE;
  

  This allows individual storages (in the patch, overrides) to take control of access checking and override the default access checking behavior.

• +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutesTrait.php
  @@ -45,7 +45,6 @@ protected function buildLayoutRoutes(RouteCollection $collection, SectionStorage
       // Trigger the layout builder access check.
  -    $requirements['_has_layout_section'] = 'true';
  

  This is the removed usage of the deprecated access check.

Actionable feedback and questions

1. +++ b/core/modules/layout_builder/src/Access/LayoutSectionAccessCheck.php
   @@ -27,6 +27,7 @@ class LayoutSectionAccessCheck implements AccessInterface {
   +    @trigger_error(__NAMESPACE__ . '\LayoutSectionAccessCheck is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use \Drupal\layout_builder\Access\LayoutBuilderAccessCheck instead. See https://www.drupal.org/node/3039551.', E_USER_DEPRECATED);
   

   This should say "deprecated in 8.7.0" (not 8.7.x).

   @todo Followup issue to talk about deprecations of access checks (and as compared to tagged services generally).

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -44,10 +52,13 @@ class ChooseBlockController implements ContainerInjectionInterface {
   -  public function __construct(BlockManagerInterface $block_manager, EntityTypeManagerInterface $entity_type_manager) {
   +  public function __construct(BlockManagerInterface $block_manager, EntityTypeManagerInterface $entity_type_manager, AccountInterface $current_user) {
   ...
   +    $this->currentUser = $current_user;
   
   +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -74,8 +81,10 @@ class InlineBlock extends BlockBase implements ContainerFactoryPluginInterface,
   +   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   *   The current user.
       */
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository, AccountInterface $current_user) {
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -84,16 +85,24 @@ class OverridesSectionStorage extends SectionStorageBase implements ContainerFac
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, SectionStorageManagerInterface $section_storage_manager, EntityRepositoryInterface $entity_repository) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, SectionStorageManagerInterface $section_storage_manager, EntityRepositoryInterface $entity_repository, AccountInterface $current_user) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
    
        $this->entityTypeManager = $entity_type_manager;
        $this->entityFieldManager = $entity_field_manager;
        $this->sectionStorageManager = $section_storage_manager;
        $this->entityRepository = $entity_repository;
   +    $this->currentUser = $current_user;
   

   New services. Can we use the null/static fallback for internal BC?

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -74,7 +86,7 @@ public static function create(ContainerInterface $container) {
   +    if ($this->currentUser->hasPermission('create and edit custom blocks') && $this->entityTypeManager->hasDefinition('block_content_type') && $types = $this->entityTypeManager->getStorage('block_content_type')->loadMultiple()) {
   

   This might merit an inline comment describing the restrictions.

4. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,100 @@
   + * Provides dynamic permissions for Layout Builder.
   + */
   +class LayoutPermissions implements ContainerInjectionInterface {
   ...
   +   * Returns an array of layout override permissions.
   +   *
   +   * @return array
   +   *   The layout override permissions.
   +   *
   +   * @see \Drupal\user\PermissionHandlerInterface::getPermissions()
   +   */
   +  public function layoutOverridePermissions() {
   +    $permissions = [];
   

   This class has a very generic name and description,but then the method has knowledge of a particular storage. Should the class be specific to overrides (and named as such)?

   Also -- I haven't read the code here yet, but just want to confirm we're not accidentally bypassing delegation again.

   Finally, maybe this method should be something like getLayoutOverridePermissionsMap() or something?

5. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,100 @@
   +          'title' => $this->t('%entity_type - %bundle: Configure layout overrides for @entity_type_plural that the user can edit', $args),
   ...
   +          'title' => $this->t('%entity_type: Configure layout overrides for @entity_type_plural that the user can edit', $args),
   

   I'm wondering if these should be formatPlural()... I guess not because they don't include a count. However, this might be difficult to translate correctly in some languages where singular/plural is more than just a binary concept. I raised this concern when entity plural labels were introduced originally, so it may be out of scope for this issue. However, it'd be good to check for existing examples in core to ensure we're just following the existing pattern. (Also I should check and see if I can find an existing issue about plural labels to @todo.)

6. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -121,6 +132,7 @@ public function blockForm($form, FormStateInterface $form_state) {
          '#type' => 'container',
          '#process' => [[static::class, 'processBlockForm']],
          '#block' => $block,
   +      '#access' => $this->currentUser->hasPermission('create and edit custom blocks'),
   

   So this restricts access to the form. How is access restricted on the backend? Does it currently just fall back to the Custom Block module's permissions? (Which I think we discussed is straight up administer blocks or something.)

   Also, for my reference and maybe for reference in the patch, what mechanism do we use to restrict the creation/editing of inline blocks outside LB?

7. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -422,7 +422,7 @@ public function getThirdPartyProviders() {
   -    $result = AccessResult::allowedIf($this->isLayoutBuilderEnabled());
   +    $result = AccessResult::allowedIf($this->isLayoutBuilderEnabled())->addCacheableDependency($this);
   

   Seems like an existing bug WRT missing the cacheable dependency; does it have tests?

8. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -360,8 +370,28 @@ public function save() {
   +    // Create an access result that will allow access to the layout
   +    // if one of these conditions applies:
   

   Nit: I think this comment is wrapping too early.

9. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -360,8 +370,28 @@ public function save() {
   +    // 1. The user can configure any layouts.
   +    // 2. The user can configure layouts on all items of the bundle type.
   +    // 3. The user can configure layouts items of this bundle type they can edit
   +    //    AND the user has access to edit this entity.
   

   We've referred back to this inline comment a lot. I wonder if it'd be helpful to list examples of the actual permissions under each numbered item in the comment. Not a big deal but I have to read through and line up 1-2-3 each time.

   Also, is there a place at a higher level of documentation we could document the access/permission scheme? Maybe even an API docs section. Maybe docs gate, sorta.

10. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
    @@ -184,4 +184,12 @@ public function save();
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @return \Drupal\layout_builder\SectionStorage\SectionStorageDefinition
    +   *   The section storage definition.
    +   */
    +  public function getPluginDefinition();
    

    I think {@inheritdoc} here is a lie. This is an API addition ne?

    Is there a base implementation of this for BC? I don't see anything else being added in the patch.

1. +++ b/core/modules/layout_builder/src/LayoutPermissions.php
   @@ -0,0 +1,100 @@
   +   * @return array
   

   What kind of array? array[]? string[][]? Etc.

2. +++ b/core/modules/layout_builder/tests/fixtures/update/layout-builder-permissions.php
   @@ -0,0 +1,30 @@
   + * @file
   + * Test fixture.
   

   We should maybe say what the test fixture is for?

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,221 @@
   +   * Tests Layout Builder access for an entity type that has bundles.
   +   *
   +   * @dataProvider providerTestAccessWithBundles
   +   */
   +  public function testAccessWithBundles($permissions, $default_access, $non_editable_access, $editable_access) {
   ...
   +    return [
   +      'configure any layout' => [
   +        ['configure any layout', 'administer node display'],
   +        TRUE,
   +        TRUE,
   +        TRUE,
   +      ],
   +      'override permissions' => [
   +        ['configure all bundle_with_section_field node layout overrides'],
   +        FALSE,
   +        TRUE,
   +        TRUE,
   +      ],
   +      'editable override permissions' => [
   +        ['configure editable bundle_with_section_field node layout overrides'],
   +        FALSE,
   +        FALSE,
   +        TRUE,
   +      ],
   +    ];
   ...
   +  public function testAccessWithoutBundles($permissions, $default_access, $non_editable_access, $editable_access) {
   ...
   +    return [
   +      'configure any layout' => [
   +        ['configure any layout', 'administer user display'],
   +        TRUE,
   +        TRUE,
   +        TRUE,
   +      ],
   +      'override permissions' => [
   +        ['configure all user user layout overrides'],
   +        FALSE,
   +        TRUE,
   +        TRUE,
   +      ],
   +      'editable override permissions' => [
   +        ['configure editable user user layout overrides'],
   +        FALSE,
   +        FALSE,
   +        TRUE,
   +      ],
   

   Usually we document data provider parameters as well as adding inline comments for each scenario.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,221 @@
   +   * Dataprovider for testAccessWithBundles.
   

   "Provides test data for..." ?

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderAccessTest.php
   @@ -0,0 +1,221 @@
   +  /**
   +   * Asserts expected access.
   

   I think this should be:

   Asserts the correct response code (200 or 403) is returned based on expected access.

6. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderMultilingualTest.php
   @@ -72,6 +72,7 @@ protected function setUp() {
          'configure any layout',
          'translate interface',
   +      'create and edit custom blocks',
        ]));
   

   Is this permission addition needed?

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -497,4 +504,83 @@ public function testAddWorkFlow() {
   +    $assert = function ($permissions, $expected) {
   +      $assert_session = $this->assertSession();
   +      $page = $this->getSession()->getPage();
   +
   +      $this->drupalLogin($this->drupalCreateUser($permissions));
   +      $this->drupalGet(static::FIELD_UI_PREFIX . '/display/default/layout');
   +      $page->clickLink('Add Block');
   +      $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#drupal-off-canvas .block-categories'));
   +      if ($expected) {
   +        $assert_session->linkExists('Create custom block');
   +      }
   +      else {
   +        $assert_session->linkNotExists('Create custom block');
   +      }
   +    };
   +
   +    $permissions = [
   +      'configure any layout',
   +      'administer node display',
   +    ];
   +    $assert($permissions, FALSE);
   +    $permissions[] = 'create and edit custom blocks';
   +    $assert($permissions, TRUE);
   ...
   +    $assert = function ($permissions, $expected) {
   +      $assert_session = $this->assertSession();
   +
   +      $this->drupalLogin($this->drupalCreateUser($permissions));
   +      $this->drupalGet(static::FIELD_UI_PREFIX . '/display/default/layout');
   +      $this->clickContextualLink(static::INLINE_BLOCK_LOCATOR, 'Configure');
   +      $assert_session->assertWaitOnAjaxRequest();
   +      if ($expected) {
   +        $assert_session->fieldExists('settings[block_form][body][0][value]');
   +      }
   +      else {
   +        $assert_session->fieldNotExists('settings[block_form][body][0][value]');
   +      }
   +    };
   ...
   +    $assert($permissions, FALSE);
   

   Uhhh why are we doing this with an anonymous function? Hard to read. :)

8. +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
   @@ -63,14 +63,14 @@ protected function setUp() {
   +   * @param array $permissions
   

   Is this a string[] rather than just array?

9. +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
   @@ -105,14 +111,46 @@ public function providerTestAccess() {
        // Data provider values are:
        // - the expected outcome of the call to ::access()
   -    // - the operation
        // - whether Layout Builder has been enabled for this display
   -    // - whether this display has any section data.
   +    // - whether this display has any section data
   +    // - any permissions to grant to the user.
   

   Yah, like this. :)

   Except the third arg seems to be an array, not a "whether" (which would imply bool). "Any section data"?

@tim.plunkett, @tedbow, and I discussed that there's still a place where we're not updating the custom block permission correctly for layout_builder_block_content_access(). At a minimum, we need to use the new permission there instead of "configure any layout".

However, having those accessible from the normal custom block form could cause other problems as it's another entry point for things like revisions, maybe translations, etc. We might want to look at why we allowed access to them via that form and see if it's still needed (maybe in a separate issue).

@tim.plunkett and I discussed that a release note about the new permissions structure is a good idea. There's an upgrade path, but also security hardening opportunity and possible change to anything extending the functionality.

Attempting to write a release notes snippet.

For #119.1: #3042376: [Policy, no patch] How to deprecate access checks?.

AFAICT all other feedback has been addressed. Back to RTBC.

Thanks @tim.plunkett and @webchick.

The release note snippet is missing an action for the site owner or developer (helpful for sites to determine if they're affected). Does this work?

In earlier versions of Drupal 8, Layout Builder only provided a single permission to access all aspects of Layout Builder (configuring/deleting layouts, adding/removing inline blocks, etc.). In 8.7, more granular permissions have been added, to restrict access further (for example, to only allow manipulating the layout on content to which the user has edit access). Review Layout Builder's permissions after updating and change to more granular permissions where appropriate for better access control.

Or something along those lines?

I would strike ", to restrict access further" from the release note snippet. Adding new permissions does not restrict access; it is only when the site admin removes one permission (and perhaps grants a more restrictive one) that access is restricted. Besides, less is more.

I will not edit the IS myself: just make the suggestion.

@benjifisher I changed it to:

In earlier versions of Drupal 8, Layout Builder only provided a single permission to access all aspects of Layout Builder (configuring/deleting layouts, adding/removing inline blocks, etc.). In 8.7, more granular permissions have been added (for example, to only allow manipulating the layout on content to which the user has edit access). Review Layout Builder's permissions after updating and change to more granular permissions where appropriate for better access control.

Is there a reason that this patch isn't removing the 'configure any layout' permission requirement from layout_builder.move_block_form?

+++ b/core/modules/layout_builder/layout_builder.permissions.yml
@@ -1,5 +1,9 @@
+create and edit custom blocks:
+  title: 'Create and edit custom blocks'
+  description: 'Manage the single-use blocks within the Layout Builder'
...
+++ b/core/modules/layout_builder/layout_builder.module
@@ -251,7 +256,7 @@ function layout_builder_block_content_access(EntityInterface $entity, $operation
-  if ($account->hasPermission('configure any layout')) {
+  if ($account->hasPermission('create and edit custom blocks')) {

The CR doesn't mention this.

+++ b/core/modules/layout_builder/layout_builder.module
@@ -55,6 +55,11 @@ function layout_builder_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dt>' . t('User permissions') . '</dt>';
+      $output .= '<dd>' . t('The Layout Builder module makes a number of permissions available, which can be set by role on the <a href=":permissions">permissions page</a>. For more information, see the <a href=":layout-builder-security-documentation">security documentation</a>', [
+        ':permissions' => Url::fromRoute('user.admin_permissions', [], ['fragment' => 'module-layout_builder'])->toString(),
+        ':layout-builder-security-documentation' => 'https://www.drupal.org/docs/8/core/modules/layout-builder/security-considerations',
+      ]) . '</dd>';

https://www.drupal.org/docs/8/core/modules/layout-builder/security-consi... currently only has a copy/paste of #118. What information do we still plan on adding there?

+++ b/core/modules/layout_builder/src/Access/LayoutBuilderAccessCheck.php
@@ -31,6 +34,13 @@ class LayoutBuilderAccessCheck implements AccessInterface {
+    // Check for the global permission unless the section storage checks
+    // permissions itself.
+    if (!$section_storage->getPluginDefinition()->get('handles_permission_check')) {
+      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'configure any layout'));
+    }

Are we worried at all about the possibility that a section storage plugin has the handles_permission_check annotation, but in fact doesn't check any permissions? For example, suppose some alters the 'overrides' plugin with a different class, with an access() method that does not check permissions (because in 8.6, it didn't have to).

What if in the above code we add an else that then checks to see if $access has a 'user.permissions' cache context, and if not, then throw an exception? Since if you declare that you handle permissions, but return an access result that does not vary by permissions, then you did something wrong, right?

I'm willing to be convinced that it's not worth adding such an exception, if it doesn't feel right to do so, but then should we add something to the CR and/or to https://www.drupal.org/docs/8/core/modules/layout-builder/security-consi... that specifically calls out the need for anyone overriding section storage plugins to be aware of this annotation and to ensure that the overridden class complies with it? If we decide that that info belongs only in that handbook page, then per #147, that's not a commit blocker.

Ok to #149.

The rest all looks good to me, so removing the "Needs framework manager review" tag.

+++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
@@ -76,7 +92,9 @@ public static function create(ContainerInterface $container) {
-    if ($this->entityTypeManager->hasDefinition('block_content_type') && $types = $this->entityTypeManager->getStorage('block_content_type')->loadMultiple()) {
+    // Add a link to create a custom block if the user has the correct
+    // permission and there is at least one custom block type available.
+    if ($this->currentUser->hasPermission('create and edit custom blocks') && $this->entityTypeManager->hasDefinition('block_content_type') && $types = $this->entityTypeManager->getStorage('block_content_type')->loadMultiple()) {

I think it would be better to add the permission as a #access on the element rather than not create the element at all. That's not a commit blocker though and can be punted to a followup if need be.

+++ b/core/modules/layout_builder/src/Access/LayoutSectionAccessCheck.php
@@ -27,6 +27,7 @@ class LayoutSectionAccessCheck implements AccessInterface {
+    @trigger_error(__NAMESPACE__ . '\LayoutSectionAccessCheck is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\layout_builder\Access\LayoutBuilderAccessCheck instead. See https://www.drupal.org/node/3039551.', E_USER_DEPRECATED);

Let's add an @deprecated to the class, and mention in the CR that _has_layout_section is now deprecated. Should we also add a deprecated: line to the access_check.entity.layout service?

Nice catch on #143. I had made a note about needing to check this issue once that one landed but forgot. :)

I'll see if I can work on the docs some today.

Few notes while I work through the doc:

1. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   + * Layout Builder access check. Any section storage plugin may use the
   + * 'handles_permission_check' in order to not require that permission, and will
   

   Missing noun here -- the "handles permission check" what? The annotation key, right? What's the right word to use here?

2. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   + * In determining access rights for a the Layout Builder UI,
   + * \Drupal\layout_builder\Access\LayoutBuilderAccessCheck checks if the
   + * specified section storage plugin (an implementation of
   + * \Drupal\layout_builder\SectionStorageInterface) grants access.
   

   Typo "a the".

3. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   + * By default, the 'configure any layout' permission will be required by the
   + * Layout Builder access check. Any section storage plugin may use the
   + * 'handles_permission_check' in order to not require that permission, and will
   + * be fully responsible for the routing access checking.
   

   By default, the Layout Builder access check requires the 'configure any layout' permission. Individual section storage plugins may override this by setting the 'handles_permission_check' annotation key to TRUE. Any section storage plugin that uses 'handles_permission_check' must provide its own complete routing access checking to avoid any access bypasses.

4. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   + * This access checking is only enforced on the routing level. All API access to
   + * Layout Builder data is currently forbidden.
   
   +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   --- a/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   
   +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -77,7 +77,9 @@ public function equals(FieldItemListInterface $list_to_compare) {
   +   * Overrides \Drupal\Core\Field\FieldItemListInterface::defaultAccess().
   

   [...] on the routing level with additional form access restrictions.

   (Is the above true?)

   And then since "API"'s meaning is contextual, maybe "client API access"? or "API access (e.g. via REST, JSON:API, etc.)".

   Then, I think we should add (at a minimum:

   
   @see \Drupal\layout_builder\Field\Field\LayoutSectionItemList::defaultAccess()
   @see \Drupal\layout_builder\Normalizer\LayoutEntityDisplayNormalizer
   

   (The former is already in the ingroup but it's specifically relevant to clarify the bit about API access restrictions.)

maybe "client API access"?

How about "HTTP API"? We use that elsewhere: e.g., in the docs for EntityTypeInterface::isInternal().

+++ b/core/modules/layout_builder/layout_builder.module
@@ -55,6 +55,11 @@ function layout_builder_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dt>' . t('User permissions') . '</dt>';
+      $output .= '<dd>' . t('The Layout Builder module makes a number of permissions available, which can be set by role on the <a href=":permissions">permissions page</a>. For more information, see the <a href=":layout-builder-security-documentation">security documentation</a>', [
+        ':permissions' => Url::fromRoute('user.admin_permissions', [], ['fragment' => 'module-layout_builder'])->toString(),
+        ':layout-builder-security-documentation' => 'https://www.drupal.org/docs/8/core/modules/layout-builder/security-considerations',
+      ]) . '</dd>';

Instead of "security documentation" (which it isn't really; bit different from the JSON:API case), the link text should be something like "documentation on configuring Layout Builder permissions".

I also renamed and retitled the handbook page. The new URL is:

https://www.drupal.org/docs/8/core/modules/layout-builder/configuring-la...

So let's update to use that. Edit: Probably rename the URL placeholder as well for consistency.

Or, better,

For more information, see the documentation on <a href=":layout-builder-permissions-docs">Configuring Layout Builder permissions</a>.

+++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
@@ -46,6 +46,20 @@ class SectionStorage extends Plugin {
+   * routing access. If TRUE, the section storage's implementation of
+   * \Drupal\Core\Access\AccessibleInterface::access() will be fully responsible
+   * for the routing access checking. Defaults to FALSE.

In the CR or above (I forget which), I switched away from the wording "will be fully responsible" because that could be read a couple different ways. I think we need a stronger statement, something like If TRUE, Layout Builder will not enforce any access restrictions for the storage, so the section storage's implementation of access() must perform the routing access checking itself."

+++ b/core/modules/layout_builder/layout_builder.api.php
@@ -0,0 +1,29 @@
+ * This access checking is only enforced on the routing level. All API access to

In addition to what I said above about this sentence, I wonder if we should explicitly say "enforced on the routing level (not on the entity or field level)". Maybe?

NW for the above several comments. Meanwhile I'm working on https://www.drupal.org/docs/8/core/modules/layout-builder/configuring-la... a bit.

All of the changes in #160 look great!

+++ b/core/modules/layout_builder/layout_builder.module
@@ -56,6 +56,11 @@ function layout_builder_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('The Layout Builder module makes a number of permissions available, which can be set by role on the <a href=":permissions">permissions page</a>. For more information, see the <a href=":layout-builder-permissions">Configuring Layout Builder permissions</a>', [

The last sentence just sort of leaves me hanging. First of all, we need a "." at the end of the sentence. Secondly, do we also want some other word like "page" or "documentation" either inside or after that last anchor?

The added docs and misc. other improvements addressed all the specifically release-management-related questions I had, so untagging. (Note that I've not done a full code review of the final patch yet.) Thanks!

#162 addresses all of my feedback as well, so RTBC. The test failure is likely a CI problem, so we'll need a new test run once that's resolved.

@tim.plunkett walked me through this patch. The upgrade path (and accompanying text) was surprisingly straight-forward! The "core" part of the patch is well-documented, and that ->andIf()/->orIf() stuff is pretty sweet! :D Also ran through a manual test earlier with the team and confirmed the permissions are operating as they should.

Confirmed with @effulgentsia that he has performed a code review of the patch, and that his feedback has been addressed. Also seems we have addressed all release management feedback, per #163.

Committed and pushed to 8.8.x and cherry-picked to 8.7.x since this is an experimental module. WOOHOO!! :D