Add "edit block $type" permissions

The "Custom Block Library" page, currently in structure / block layout / custom block library, should be exposed for content editors to gain access to these entities. The following routes in core/block_content should be aware of new permissions for editing custom blocks:

• entity.block_content.collection
• entity.block_content_type.edit_form
• entity.block_content.edit_form
• block_content.type_add

Additionally, for blocks created and not placed, it would be nice to have access to:

• entity.block_content.delete_form

https://www.drupal.org/node/2644872#comment-11878253

looks like a good start (although he does full CRUD).

Personally, until this gets part of core (if ever), that solution is a great workaround, if not the solution, but the dev seems hesitant to move forward precisely because of the disucssions in these core-issues. So either this solution here should get adopted as a base, or we encourage the dev to move forward with his own release.

Would love to see this happen for 8.4

There's https://www.drupal.org/project/block_permissions as well.

But to be clear: the minimum viable change would be to only "Add an edit block permission per block type" so that we can give content editors the option to change the contents of that block with the current promotion or the special opening hours etc. without having to give them acces to the whole of the blocks admin page.

Anyone up for a patch at DevDays? :)

I'll work on this issue

I have created a patch for adding edit permissions for each custom block type. I'll continue writing a test.

I thought that i have to assign the issue to avoid several people working on the same issue. Sorry for that.

This options appears in custom blocks section but you need to create it first. After that, you can see in the list of permissions. I upload an screenshot.

I have created a new patch for this issue. I think it's fine now. When you create a new custom block type a new permission appears in /admin/people/permisions inside "Custom Block" section.

The users with this permission can access and update the block but after saving it, the user is redirecting to the URL '/admin/structure/block/block-content' which is accesible only by users with permission 'Administer blocks'.

I don't know what to do now. The view should be granted with more permissions or the redirection should be changed.

I think this is the beginning of a long issue... :)

Using this patch in production successfully for over 6 months now.

I've applied the patch to a Drupal 8 site but see no change in the permissions listed on "admin/people/permissions". I looked through the patch for the new permission that was supposed to be added, and it looks like it should be %type_name: Edit any content but the only place "Edit any content" appears on the permissions page is for Node types. There are no block types in this list at all.

Ah, never mind that comment. My single block type was not showing on my local site for some reason. Tested on the dev site and I can see the new permission! Turned over to the editors for testing.

Plan to attempt re-rolling this patch as part of #GlobalSprintWeekend

@leslieg That's great! FYI when you do start work, you should change the issue "assigned" option to your username (and then change it back when you're done) so everyone knows you're currently working on it.

Re-rolled add_edit_blocks-2809291-19.patch to an earlier commit. Patch applied cleanly. New patch file is attached - add_edit_blocks-2809291-31.patch

I have not tested yet, but I have a few minor complaints:

1. +++ b/core/modules/block_content/block_content.permissions.yml
   @@ -0,0 +1,2 @@
   +permission_callbacks:
   +  - \Drupal\block_content\BlockContentTypePermissions::blockTypePermissions
   \ No newline at end of file
   

   The Drupal coding standards require a single \n at the end of the file. If you figure out how to configure your editor/IDE correctly, then you will never have to worry about it again.

   Also, there does not seem to be a requirement, and core modules are inconsistent, but there seems to be near consensus on the pattern from the Node module. To be consistent with that, we would use BlockContentPermissions::blockTypePermissions instead of BlockContentTypePermissions::blockTypePermissions. In case the Block Content module ever needs to add other permissions, we might be glad to have followed this pattern.

2. +++ b/core/modules/block_content/src/BlockContentTypePermissions.php
   @@ -0,0 +1,52 @@
   +  /**
   +   * Build the all permissions for all the block types.
   ...
   +  public function blockTypePermissions() {
   

   At least strike "the all". Maybe "each block type" instead of "all the block types"?

3. +  /**
   +   * Return all the permissions available for all the custom block types.
   +   *
   +   * @param \Drupal\block_content\Entity\BlockContentType $type
   +   *   The block entity.
   +   *
   +   * @return array
   +   *   Permissions available for the given block.
   +   */
   +  public function buildPermission(BlockContentType $type) {
   

   I think "all the custom block types" should be replaced with "a custom block type". In the @param comment, "The block entity" usually refers to a content entity. I think "The block type" would be more accurate. Similarly, in the @return comment, I would say "the given block type".

I think @luismagr's comment in #19 shows that we need to do some more serious work here. If my "Builder" role has permission to edit all "Lego block" content, then a user with that role need a list of all the Lego blocks on the site, and should be redirected there (by default) after editing a block.

My first thought was to change the permissions on /admin/structure/block/block-content. Why not extend the existing list of blocks to show just the ones that the user has permission to edit?

The problem with that approach is that the parent pages (/admin/structure/block and /admin/structure) require more permissions ("administer blocks" and "access administration pages"). We might give "access administration pages" permission to the Builder role, but maybe not, and we probably do not want to give that role the "administer blocks" permission.

So I think that a better approach is to create a new page: /admin/content/block. This page will require a custom access function: any user with "administer blocks" or any of the "%type_name: Edit any content" permissions should be able to access the page. Then it should list the blocks that the user has permission to edit.

(The inconsistency is annoying. The existing sub-pages of admin/content are admin/content/comment and admin/content/files, one singular and one plural.)

It looks as though /admin/structure/block/block-content is already implemented as a View, so creating a separate page display should be easy. We can also make the new display disabled by default if we are afraid of cluttering up the admin UI, or we can enable it by default and let site builders decide if they want to disable it. The hard parts will be access control to the page and restricting the list to the appropriate types.

Or maybe we will get lucky. If each row of the view includes an edit link, the maybe the Views module will automatically filter out any blocks that the user does not have permission to edit. If so, then maybe we can get away with a suitable "No results" message and not actually worry about restricting access to the page.

Tested in UI.

Gave permission MyBlock: Edit any content" to authenticated role. By going to /block/block_id the edit custom block form was displayed. When selecting save the block was save the as noted above the user was taken to access denied doe to the redirect noted above by @benjifisher

Updated remaining tasks to reflect what's need to complete this issue.

Thanks for the testing and the IS update, @schiavone. For the record, the redirect problem was first noted by @luismagr in #19.

In #32, I mentioned the view that creates /admin/structure/block/block-content. I think that view is defined in core/modules/block_content/config/optional/views.view.block_content.yml.

A few of the comments, and the IS, mention tests. I am adding an issue tag for that.

Another thing we discussed off-line is that there has to be a better way for a user with permission to edit a block type to find the edit page. I have added an item to the IS for this. Probably there is already a way, and I have not looked hard enough to find it. Maybe it needs additional permissions (although we tried the Quickedit permission and that did not help).

I think at least one of the remaining tasks is suitable for a novice, so I am adding that issue tag.

We discussed this at the UX meeting today.

A simpler approach than the one I suggested earlier is to make sure that a user with permission to edit a block can see the "pencil" icon and use the edit link when viewing the block in the front end. Currently, a user with the "Administer blocks" permission can use that link. The link already has the destination parameter set, so that the user is returned to the front-end page where the block was shown as soon as the block is saved.

Or maybe I am missing something? I already asked how users with permission to edit a block can get to the edit page. If there is already a good answer to that, then perhaps we can just set the destination parameter when providing the edit link.

I am removing the Novice task for now since the next step requires some PHP coding.

It is out of scope for this issue, but there is already an issue to move the list of custom blocks from a child of /admin/structure/block to /admin/content, as I suggested in #32. I am adding #2862564: Move Custom block library to Content as a related issue.

(accidentally posted twice)

Patch applies to 8.4.5 with some fuzz.

Patch in #31 still applies cleanly to 8.5.3.

Patch in #31 still applies cleanly to 8.5.4.

I am keen to use this patch but find that it undoes the work of another core patch that I am already using

Field rendering should respect configurable field display

Patch in #31 still applies cleanly to 8.5.5.

Patch in #31 no longer applies.

Okay, I've taken a stab at addressing most of the feedback from @benjifisher in #32 as well as doing the reroll.

If my "Builder" role has permission to edit all "Lego block" content, then a user with that role need a list of all the Lego blocks on the site

I'm not finding this to be the case. My editor roles use the contextual links to edit blocks, and having another content listing that's not the admin content page would be confusing for someone who's not an admin (and doesn't have a deeper understanding of how Drupal works).

Still using this patch on Drupal 8.6.3.

Still using this patch on Drupal 8.6.4.

The patch from #50 apply properly on 8.7.x ! Nice work on it.

Also, the tests fails on many Class because having the permission administer blocks seems not to be enough for the code from #50. But it should be as described by both #32 & #38.

I will work on the following things - right now:

• Alter the code from #50 to allow access of Edit when having administer blocks or the corresponding edit any [entity-bundle] block.
• Adding a test to asserts having the new edit any [entity-bundle] block.

So here the first attempts to fix all tests fails

> Alter the code from #50 to allow access of Edit when having administer blocks or the corresponding edit any [entity-bundle] block.

Let's tests them on testbot.

mmhh ... still 9 fails (10 less than before). Seems it fails because of the message thrown on permission denied from Rest/Hal. Let's inverse the new added condition to prevent breaking the whole Drupal MessageResourceTests.

Here is the patch with the new coverage.

Also, by creating this coverage I discover this edge case:

1. view operation is not accessible for user with the new per-block permission (edit any block-bundle block) when the block is not published.

   Should we change this ? By altering this code:

      if ($operation === 'view') {
         $access = AccessResult::allowedIf($entity->isPublished())
           ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));
       }
   

Thanks to @jenlampton for addressing my review in #32 and to @wengerk for adding test coverage!

1. From #58:

   [The] view operation is not accessible ... Should we change this ?

   Yes, good idea!

2. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,52 @@
   ...
   +   * @param \Drupal\block_content\Entity\BlockContentType $type
   +   *   The block entity.
   

   I still think this should be "The block type". This was part of #32.3.

3. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -58,9 +58,17 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
          $access = AccessResult::allowedIf($entity->isPublished())
            ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'));
        }
   +    elseif ($operation === 'update') {
   +      // Allow access to user with permission 'administer blocks' or 'any
   +      // per-block' permission.
   

   I find it confusing to break "any per-block" across two lines. Can we make an exception here to the "wrap at 80 characters" guideline? Come to think of it, "any per-block" is a poor choice for a literal string. Why not make is "Allow access to user with the 'edit any (type) block' permission or the 'administer blocks' permission"? That will fix the line-break problem.

4. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,52 @@
   +   * @see \Drupal\user\PermissionHandlerInterface::getPermissions()
   +   */
   +  public function blockTypePermissions() {
   

   Why the @see comment? This code seems to be modeled on NodePermissions::getPermissions().

5. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentAccessHandlerTest.php
   @@ -292,8 +324,79 @@ public function providerTestAccess() {
              'forbidden',
            ],
          ];
   -      return $cases;
        }
   +
   +    $cases += [
   

   Hold on! If I am reading this right, then you are fixing a bug in the test function. That return used to be inside the foreach loop, right? If so, then fixing that bug is out of scope for this issue. Let's create a separate issue to fix it, and hope that we can get it committed quickly, and then return to this issue. If anyone is trying to figure out the git history of how that mistake was corrected, I do not want to confuse them by mixing up that fix with this issue.

6. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentAccessHandlerTest.php
   @@ -184,6 +184,22 @@ public function providerTestAccess() {
            NULL,
            'allowed',
          ],
   +      'view:unpublished:reusable:per-block-editor:basic' => [
   ...
   +      ],
   +      'view:unpublished:reusable:per-block-editor:square' => [
   ...
   +      ],
          'view:published:reusable:admin' => [
            'view',
            TRUE,
   @@ -192,6 +208,22 @@ public function providerTestAccess() {
            NULL,
            'allowed',
          ],
   +      'view:published:reusable:per-block-editor:square' => [
   ...
   +      ],
   +      'view:published:reusable:per-block-editor:basic' => [
   ...
   +      ],
   

   Please be consistent in which goes first, basic or square.

7. @@ -292,8 +324,79 @@ public function providerTestAccess() {
              'forbidden',
            ],
          ];
   -      return $cases;
        }
   +
   +    $cases += [
   +      'update:unpublished:reusable:per-block-editor:square' => [
   ...
   +      ],
   

   It looks to me as though half of these cases could be added inside the foreach loop. More important than saving a few lines of code, that tells anyone looking at the test that there are some tests where you get the same result for the "delete" and "update" actions and some cases where you get different results.

8. Why do we get the same permissions for both block types when the operation is "view" or "delete", but different results when the operation is "update"?

I created the children issue to fix BlockContentAccessHandlerTest::providerTestAccess wrong coverage by early return.

See #3022183.

Once fixed/merged, I will be happy to continue this issue by applying suggestion from #61.
If someone wants to work on this issue, in the meantime, feel free.

@wengerk:

Thanks for creating #3022183: Fix BlockContentAccessHandlerTest::providerTestAccess wrong coverage by early return. I just reviewed it.

Tip: create links to other issues with the syntax [# 3022183] (without the space). It is easier, and it generates a link with the issue title and a color to indicate the issue status. (Light green = RTBC.)

I encourage you to work on this issue while waiting for that to be committed. It should be easy enough to reroll the patch for this issue when the time comes.

Update: The child issue is now Fixed (dark green), so there is no reason to put off further work here.

Thanks for all the continued progress on this issue @wengerk!

I've done a reroll of the patch since [# 3022183] got in, and attempted to address the feedback in #61. The current patch leaves off refactoring of tests as suggested in point 7, but I believe everything else has been addressed.

> Why do we get the same permissions for both block types when the operation is "view" or "delete", but different results when the operation is "update"?

Because this patch only adds the permission for editing/updating a block. Every other operation should remain unchanged.

Thanks all for all your help & works on this issue !

From #65, I just fix the tests, because of the change I suggested on #58.1.

      'view:unpublished:reusable:per-block-editor:square' => [
        'view',
        FALSE,
        TRUE,
        ['edit any square block'],
        NULL,
-        'neutral',
+        'allowed',
      ],

Latest patch works for me!
It applies cleanly to 8.6.7, for those who need it.

@jenlampton, @wengerk, thanks for the continued work.

I looked at my review from #61 and checked the various points there. I still think there is room for improvement in (1) and (7), so I will not say +1 for RTBC, but these are minor points so I will not move this issue back to NW.

Because of the reroll after #3022183: Fix BlockContentAccessHandlerTest::providerTestAccess wrong coverage by early return, I cannot make a true interdiff, but I made a fake one to help with my review, and it is attached.

One thing confuses me. There is only one real difference between the patches in #65 and #67, but the interdiff makes it look as if the patch in #65 left out an entire file. I get the same result using the interdiff utility. I thought I understood interdiffs. :-(

1. I agreed with the suggestion from #58.1 to grant view access to anyone with the new permission. This is now done. My only complaint is that the code comment is repeated:

     protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
       $permission = 'edit any ' . $entity->bundle() . ' block';
       if ($operation === 'view') {
         // Allow access to user with the 'edit any (type) block' permission or the
         // 'administer blocks' permission.
         $access = AccessResult::allowedIf($entity->isPublished())
           ->orIf(AccessResult::allowedIfHasPermission($account, 'administer blocks'))
           ->orIf(AccessResult::allowedIfHasPermission($account, $permission));
       }
       elseif ($operation === 'update') {
         // Allow access to user with the 'edit any (type) block' permission or the
         // 'administer blocks' permission.
         $access = AccessResult::allowedIfHasPermission($account, 'administer blocks')
           ->orIf(AccessResult::allowedIfHasPermission($account, $permission));
       }
       else {
         $access = parent::checkAccess($entity, $operation, $account);
       }
       

   I would rather combine the two comments at the start of the function:

       // Allow view and update access to user with the 'edit any (type) block'
       // permission or the 'administer blocks' permission.
       $permission = 'edit any ' . $entity->bundle() . ' block';
       

   This would save a few lines. More important, this would make it easier to see the difference between the "view" and "update" cases.

2. Change the @param comment from "The block entity." to "The block type." Done.
3. The code comment was changed as I suggested. See (1) above.
4. The @see comment was removed.
5. This out-of-scope bug fix was handled in #3022183: Fix BlockContentAccessHandlerTest::providerTestAccess wrong coverage by early return.
6. The order of the cases in providerTestAccess() is now consistent: always "basic" first, then "square". It looks as though you removed a couple of blank lines that I liked.
7. This recommendation was not addressed. I still think it is a good idea, for the reasons I gave. The four cases (update/delete, published/unpublished) involving "basic" blocks could be made into two arrays inside the foreach (['update', 'delete'] as $operation) block.
8. @jenlampton, thanks for answering my question in #65.

Here I address #61.1 & #61.7

Let's tests in with testbot !

I have updated the patch provided in comment #72 to include the recommended code comment changes from comment #71 at the beginning of the 'checkAccess' method in BlockContentAccessControlHandler.php.

My previous patch failed to stage the new files created by the previous patches.

New patch and interdiff provided, for the same update mentioned in my previous comment #73

@wengerk:

Thanks for updating the test! I think the update makes it a little more maintainable.

The one thing that was bothering me is that, when you replaced two comments with one, you did not change "Allow access" to "Allow view and update access" as I suggested in #71. This is such a minor point that I did not want to insist on another round of revisions. Luckily, I am working with @mikemiles86 and others at the Boston meeting of the Global Sprint weekend, and he agreed to make this change.

@mikemiles86:

Thanks for updating the comment!

I have two tips for avoiding the mistake of leaving out the newly added files (as opposed to modified files) from a previous patch:

1. Apply the earlier patch with git apply -v --index /path/to/previous.patch
2. Instead of using git diff to generate the interdiff, use the interdiff utility.

See Creating an interdiff for instructions on installing the utility.

I like this patch! RTBC

1. This is looking really close

2. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,50 @@
   +  public function buildPermission(BlockContentType $type) {
   

   Is there a reason for this to be public API? I think it should be protected unless we have a use-case. Or even inlined into the blockTypePermissions method..

         $perms["edit any $type_id block"] = [
           'title' => $this->t('%type_name: Edit any content', $type_params),
         ];
   

   in the loop is not bad.

3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentPermissionsTest.php
   @@ -0,0 +1,87 @@
   +    $this->assertTrue(isset($permissions['edit any square block']), 'The per-block dynamic permission exists.');
   

   We can use assertArrayHasKey here.

   Also the test could make an assertion about more of the expected return for example if we set more than one block type.

4. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentPermissionsTest.php
   @@ -0,0 +1,87 @@
   +  /**
   +   * @covers ::buildPermission
   +   */
   +  public function testBuildPermission() {
   

   If this is protected we don't need this test and also asserting a count of 1 is odd because this returns an array with 1 element so it can never not be the case.

@alexpott: Regarding #76.2: in #61.4, I mentioned

This code seems to be modeled on NodePermissions::getPermissions().

I missed that buildPermission() is protected in the Node module, public here. OTOH, staying consistent with that model is a good thing, so I would not be too quick to inline the function. Also, even though this version is much simpler than the one in the node module, you never know what feature requests will come in the future, so it might not stay so simple.

Overall, I am quite happy with that review. I was afraid you would say something like "Nice code, why not put it in a contrib module?" ;)

okay, giving this a try:

Is there a reason for this to be public API? I think it should be protected unless we have a use-case.

Changed to protected.

We can use assertArrayHasKey here.

Changed.

we don't need this test

Removed.

@jenlampton:

Thanks for moving this forward! I think the new patch addresses most of the points @alexpott raised in #76.

1. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentPermissionsTest.php
   @@ -0,0 +1,68 @@
   +    $this->assertFalse(isset($permissions['edit any square block']), 'The per-block dynamic permission does not exists.');
   

   Since we are using assertArrayHasKey() for the other assertion, let's use assertArrayNotHasKey() here, if only for consistency.

   While you are at it, please change "does not exists" to "does not exist".

2. From #76.3:

   Also the test could make an assertion about more of the expected return for example if we set more than one block type.

   I am not sure what the suggestion here is. If the intention is to make sure that different block types get different permissions, then it should be enough to add an assertion that $permissions['edit any square block'] is 'A square block type: Edit any content'.

P.S. Here is an interdiff for the two most recent patches.

Here a patch that address the #79.1
Also, I'm not sure how I can address #79.2 (#76.3) - any help or suggestion ?

In #79.3 I was suggesting something like

    $this->assertEquals(
      'A square block type: Edit any content',
      $permissions['edit any square block']
    );

As I said there, I am not sure that this is what @alexpott meant in #76.3:

Also the test could make an assertion about more of the expected return for example if we set more than one block type.

Maybe, as in the other tests, we should have a "basic" block type as well as the "square" block type. Then test the expected values of $permissions['edit any basic block'] as well as $permissions['edit any square block'].

The test failure was random. I re-tested and it came up green.

I will leave the status at NW for the sake of #76.3.

There is something wrong with the interdiff. I think that the interdiff CLI tool gets confused because Line 205 of core-add_edit_blocks-2809291-78.patch is actually empty. If I add a space character to that line, then I get the expected +1/-1 interdiff. Notice the first two lines of the interdiff:

only in patch2:
unchanged:

I am not sure what that means.

Here an attempts to resolve both #79.3 & #76.3

What do you think ? I also think about adding everything using a dataProvider instead, but it would add a lot of "buisness logic" into the tests (foreach to tests X numbers of dynamic created blocks) - which is in my point of view a bad idea.

PS: We could also just keep the testDynamicPermissions untouched & add a new one testMultipleDynamicPermissions with only an assertion on assertArrayHasKey & assertArrayNotHasKey.

I think the test looks good, now. Having two block types in the test gives us some flexibility for adding further tests in the future.

I am bothered by the grammar in the code comments:

+++ b/core/modules/block_content/tests/src/Kernel/BlockContentPermissionsTest.php
@@ -0,0 +1,88 @@
...
+    // Assert the basic permissions has been created.
+    $this->assertArrayHasKey('edit any basic block', $permissions, 'The per-block dynamic permission exists.');
...
+    // Assert the square permissions has been created.
+    $this->assertArrayHasKey('edit any square block', $permissions, 'The per-block dynamic permission exists.');

It should be either "the basic permissions have been" or "the basic permission has been". Same point for "square".

I also notice that the assertion messages refer to "per-block dynamic permission". I think that should be "per-block-type dynamic permission". Or maybe follow the "less is more" principle: "per-block-type permission".

+++ b/core/modules/block_content/tests/src/Kernel/BlockContentPermissionsTest.php
@@ -0,0 +1,88 @@
...
+    $this->assertArrayNotHasKey('edit any basic block', $permissions, 'The per-block dynamic permission does not exist.');
+    $this->assertArrayNotHasKey('edit any square block', $permissions, 'The per-block dynamic permission does not exist.');
...
+    $this->assertArrayHasKey('edit any basic block', $permissions, 'The per-block dynamic permission exists.');
...
+    $this->assertArrayHasKey('edit any square block', $permissions, 'The per-block dynamic permission exists.');

Thanks for your review !

Here a patch which address grammar issues.

@wengerk:

Sorry to nit-pick, but there are two other places where "per-block permission" should be changed to "per-block-type permission". Have another look at the second snippet in #86.

My bad .... here a patch which fix that.

@wengerk:

Thanks for all the updates! This version looks good to me. I have marked it RTBC.

I have attached an interdiff comparing the patches attached to #74 (the last one that was RTBC) and #90. One reason is that the patch file attached to #78 is problematic, as I pointed out in #84.

We need a change record here, thanks

1. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,50 @@
   +      "edit any $type_id block" => [
   +        'title' => $this->t('%type_name: Edit any content', $type_params),
   +      ],
   

   To me this further complicates the incredibly confusing naming around the multiple meanings of "block" in D8.

   since this is the block_content module and the description itself uses the word "content", shouldn't this be "edit any $type_id block content"?

2. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,50 @@
   +   * Build permissions for each block type.
   ...
   +   *   The block type permissions.
   ...
   +   *   Permissions available for the given block type.
   

   Less important than the machine names, but this is still confusing with "block type" without mention of it being block_content.module

@tim.plunkett:

1. I always look to the Node module for patterns to use, and I see permissions like "Basic page: Edit any content". That is the pattern the current patch follows. I think we should keep the word order and use the label rather than the machine name, unless there is some larger initiative in progress to change that. Would it help if we changed "Lego: Edit any content" to "Lego: Edit any block content"?
2. Could you give a suggestion of how to change it? Since this file is inside the block_content module, I am not sure what the confusion would be. Looking at block_content_help(), I see "custom block type" all over the place, so maybe we should just add "custom" in these code comments?

I don't recall why the module is called "Custom Block" in the info file and it's block_content as a machine name.

Foo: Edit any block content would indeed be much clearer than Foo: Edit any content
And custom block type would be clearer than block type

But aren't those backwards from the UI vs machine name split?

NW for this change as well as for the CR.

Discussed this with Tim Plunkett and found that it would be useful for layout builder, but not without a create equivalent, so added that too.

Also addressed #96 and added a CR at [#3041203]

@larowlan: Thanks for these updates! They mostly look good, but I have some nits to pick.

I just arrived in Chicago for MidCamp 2019, so I will add an issue tag for that.

1. I notice that the filename of the patch does not match the issue number (98).

2. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -54,13 +54,22 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +    // Allow view and update access to user with the 'edit any (type) block'
   +    // permission or the 'administer blocks' permission.
   +    $permission = 'edit any ' . $entity->bundle() . ' block content';
       

   Please update the comment to match the code.
   Notice that if a user has edit access, then we are granting view access, but a user might have create access but not be able to view.

3. @@ -85,4 +94,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
   +    return parent::checkCreateAccess($account, $context, $entity_bundle)->orIf(AccessResult::allowedIfHasPermission($account,  'create ' . $entity_bundle . ' block content'));
       

   Can we break the line at ->orIf()?

4. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentAccessHandlerTest.php
   @@ -2,14 +2,12 @@
   -use Drupal\user\Entity\Role;
   -use Drupal\user\Entity\User;
   +use Drupal\Tests\user\Traits\UserCreationTrait;
       

   This simplifies the tests, but it is out of scope for this issue. I do not want to insist on staying in scope, but there is some danger that this will create merge conflicts if someone else is working on making similar changes on a large scale.

5. @@ -39,11 +39,4 @@
    
      /**
   -   * The BlockContent entity used for testing.
   -   *
   -   * @var \Drupal\block_content\Entity\BlockContent
   -   */
   -  protected $blockEntity;
       

   You are right to replace the class property with a local variable in the test where it is used, BUT please rename that local variable to $block_entity. The coding standards allow either underscores or camelCase, but "Be consistent; do not mix camelCase and snake_case variable naming inside a file." I see $parent_access, $expected_access, $block_type_id, and more in this file. I did not notice any camelCase variables except for class properties.

6. @@ -62,30 +55,24 @@
        $this->installEntitySchema('block_content');
    
        // Create a block content type.
   -    $block_content_type = BlockContentType::create([
   +    $blockType = BlockContentType::create([
       

   Same thing here.

7. @@ -384,2 +362,45 @@
    
   +  /**
   +   * @covers ::checkCreateAccess
   +   *
   +   * @dataProvider providerTestCreateAccess
   +   */
   +  public function testCreateAccess(array $permissions, $block_type_id, $expected_access) {
   +
   +    $user = $this->createUser($permissions);
   +
   +    $this->assertEquals($expected_access, $this->accessControlHandler->createAccess($block_type_id, $user, []));
   +  }
       

   Some people do not like empty lines in such a short function. I do not care strongly.

8. +++ b/core/modules/block_content/src/BlockContentPermissions.php
   @@ -0,0 +1,53 @@
   ...
   +   * Build permissions for each block type.
   ...
   +   * Build permissions for each block type.
   ...
   +   *   The block type.
   ...
   +   *   Permissions available for the given block type.
       

   These should say "custom block type" according to #94.2 and the follow-up discussion in #95 and #96.

I will edit the draft CR.

Thanks for the review @benjifisher

1) That is only convention, there is no rule saying it has to match 🤷‍♂️
2) done

Notice that if a user has edit access, then we are granting view access, but a user might have create access but not be able to view.

right only if the block isn't published, I think that gets into a complex area - should they be able to see unpublished blocks they didn't create? I don't think so. So I think we should err on the side of caution here. In most cases people would be given update access before they were given create access, so I think that works.
3) done
4) I had to add the trait to add a new user in the new test, so I think it is in scope to use that trait instead of the custom code
5) 6) done
7) done
8) done

@larowlan:

Thanks for the updates! Back to RTBC.

2) I agree. I just wanted to call it out, not suggest a change.
4) That is a good enough reason for me. I guess the alternatives would be to follow the same pattern in testCreateAccess() or else to follow different patterns in the two tests. Neither of those options would be improving the code overall.

+1 on RTBC, patch in #101 still applies to 8.6.15, and 8.7.1, and 8.7.2, and 8.7.3.

Adding a "create" and "edit" block per type permission is already working in patch #145 from the parent issue #1975064: Add more granular block content permissions. It also adds a "Access the Custom block library page" permission and some other permissions.

Patch #145 from parent issue #1975064: Add more granular block content permissions applies to 8.7.3 and makes the patch from #2809291: Add "edit block $type" permissions obsolete.

What does it mean when a user has permission to 'create $entity_bundle block content'? Should they be able to create instances of that block type? Right now, it looks like users need to have the 'administer blocks' permission to be able to add any instances of a custom block type.

My use case:
I'm asking about this because layout builder creates instances of custom blocks. If a user doesn't have access to create an instance of a custom block in layout builder and but they have the 'create and edit custom blocks' permission, they can still create the block but they can't access the text formatter for any fields with formatted text. The 'edit $entity_bundle block content' permission works as expected, so once the custom block is created they are able to format their text as they like.

What I expected:
- A user I created with 'create test_block block content' should be able to create new instances of test_block custom blocks (hitting the block_content.add_form route).

What happened:
- The user was given Access Denied when they attempted to access the test_block creation page at /block/add/test_block

To replicate:
- Build a site with the patch from comment #101
- Create a custom block type. In this example, I called mine test_block.
- Create a user, give them the 'create test_block block content' permission.
- Log in as that user and try to create a test_block at /block/add/test_block
- Get an Access Denied

@angelamnr:

I do not have time to test right now, but can you figure out why your test user is getting Access Denied? For example, it might be because /block/add/test_block uses the admin theme and the test user does not have permission to use the admin theme.

@benjifisher

I checked the route with devel and it looks like the block_content.add_form route still uses the 'Administer Blocks' permission, not 'create test_block block content', which is what I would expect. I still got an access denied when trying to create new test_blocks while using Bartik as the admin theme.

Yep, the routing still uses the hard-coded permissions.

So a) we need tests for the UI and b) we need to add an access handler. Might just be simpler to swap to use EntityController for this.

crediting @angelamnr for that find, great sleuth work!

Through manually testing I've confirmed the contextual edit link works when the user has edit $type block content and access contextual links permissions. Updated IS based on #109

Here's tests for the UI, the create one will fail setting this back to NW.

Looking at swapping the routes to use EntityController unfortunately it doesn't look like we can do that (until maybe D9?) due to a couple of things:

EntityController::addPage expects the add bundle route to be entity.entity_type_id.add_form whereas block_content_type's route is block_content.type_add. Also the messaging will be slightly different when there are no block types, we'd want to get rid of the block_content_add_list themeing, etc...

This patch will fix the failure from #113 but introduces some new stuff that need additional testing. Since the BlockController::add route used to just list ever block type, we have to add access control based on the new permissions. This is similar to what NodeController does but now we need to test that. Adding the tag back for that.

Woops, fixed the delete link failure from #113

This expands the test coverage to cover what I discussed in #116 but I've discovered a new issue. The user is taken to http://127.0.0.1/admin/structure/block/add/block_content%3AUUID/theme after saving a block via the UI which they don't have access to. That is shown by the failure in this patch on the statusCodeEquals after submitting the form.

Patch #101 still works and applies cleanly to 8.7.4. Since it looks like the more recent patches don't work, I'll stick with the old one for now.

Would it be possible to bump the "create blocks" change to a new issue, and get the already-working feature in first? "Better is the enemy of done", and all that...

Patch #101 still works and applies cleanly to 8.7.5.

Since it looks like the more recent patches don't work, I'll stick with the old one for now.

@jenlampton which part isn't working for you? Everything from #101 is the same in #119 except with the addition of allowing users to create via the UI (with the obvious bugs around that). The other stuff should be working fine.

Since it looks like the more recent patches don't work...

Sorry @acbramley, what I meant was:

Since it looks like the more recent patches have bugs...

Patch #101 still applies cleanly to 8.7.6.

here is my re-roll for the #119

The second attempt

Excuse me :)
Next turn

Looks like I missed files in my previous patches.
The new one attached

I hope this one will be successful

@aleevas can you confirm that #129 is just a reroll of #119? It shouldn't be passing tests if it is.

@acbramley not just reroll, I also fixed failed test from #119 (\Drupal\Tests\block_content\Functional\BlockContentCreationTest::testBlockContentCreation)

+++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
@@ -77,6 +77,34 @@ public function testBlockContentCreation() {
+    $user = $this->drupalCreateUser(['create basic block content', 'administer blocks']);

This is not correct and completely defeats the purpose of the testing that follows. This issue is about removing the dependency on the administer blocks permission. Please see my explanation in #119

EDIT: To clarify - the fix lies somewhere in where the user is being directed to after saving a block via the UI. Currently users without the administer blocks permission are taken to a page they don't have access to (as demonstrated by the failure in 119). I'm not 100% sure where the fix should go here but it may be somewhere in the submit handlers for that add block form.

Confirming that #129 applies cleanly to 8.7.8. AND allows my Admins to edit blocks. My Admins do not have the administer blocks permission.

@acbramley is it just the test that's not correct, or do you suspect there are still bugs in the new access checks?

Patch #101 still applies cleanly, but now creates fatal errors.

Here i have updated the patch from #119, to add a check to see if a user should be redirected or not when a new block is created.

Please review and suggest

Thanks

I'd vote for option #4. If we have a working solution that makes sense, why not get that in first, then decide what to do about the create block permission.

It looks like we added the create option in #98, so #90 would be the most recent with only edit permissions. After that, we were working on names for the permissions, so I've rerolled #98 with the permission names from #136.

The patch above from @jenlamptom works, adds a permission that is used by many other entities, and has a passing test. I'm marking this as RTBC hoping to move it forward.

#140 is failing on 9.1.x due to a phpunit change.

Remove the 'needs tests' tag but also adding 'needs manual testing' since it'd be good to confirm that end users can actually make use of this as expected.

Fixing tests for 9.1.x

The changes made to make this pass tests, are test only changes, with no other changes to the code, thus I'm reinstating my previous RTBC.

Removing a couple of unused use statements

I've reviewed this (#148) and tested manually. It creates a "Edit any block content" permission for every block type in the custom block library, and it works as expected, enabling the permission allows the edition of the block in the contextual link and after changes are saved the block is updated on the page. No other action is presented, no other page related to blocks can be visited.

Great work!

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -54,13 +54,22 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    // Allow view and update access to user with the 'edit any (type) block content'
+    // permission or the 'administer blocks' permission.
+    $permission = 'edit any ' . $entity->bundle() . ' block content';
     if ($operation === 'view') {

Can this be $edit_any_permission or $any_permission - it's not the only permission checked in this method.

Or if we made 'administer blocks' $admin_permission it'd be $permission and $admin_permission - since that's also used twice in this hunk (didn't check the rest of the method).

updated patch changes $permission to $edit_any_permission per the review from @catch.

Please review my simple change and mark as RTBC again.

Applied patch #151 in 9.2.x and it works fine.The blocks can be edited now if the user have edit block permissions(using contextual links).Also the issue in #150 is fixed in this patch.Adding screenshots.

Permission:

Edit option: