BlockContentAccessControlHandler requires access block library permission for create

Recommend turning to an MR.

Will say believe this was done on purpose because of a layout builder scenario.

Don't see an MR

Changing version to current development branch.

Will ask one of the other block_content maintainers.

If something that is changed will require a test assertion somewhere.

Tests need fixing.

Also correcting permission in IS and hiding the patch.

I was able to reproduce the issue with the steps mentioned in the issue summary and the code changes fixes the issue and it working as expected for the users with permission to Create new content block.

Can we add tests for this in this file /core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php itself?

Thanks

I think tests in BlockContentAccessHandlerTest should be sufficient. I've rebased the MR against 11.x.

I think this makes sense from an API perspective we don't need to restrict the creation of entities based on a permission that is UI based.

Pushing this one along.

Ran test only feature here https://git.drupalcode.org/issue/drupal-3412420/-/jobs/1790698 to show test coverage.

Trying to think of a scenario where this could be a BC concern but imagine if a site gave an editor this permission they also gave them 'access block library'

I'm not sure if this is the correct fix. It might actually have been intended that you have access with any of the three permissions, i.e.:

1. You can create a block if you have the permission to create a block.
2. You can create a block if you have access to the block library.
3. You can create a block if you have permission to administer block content.

So the question is, does having access to the block library mean that you should also be able to create blocks, or is there a usecase for being able to access the block library but not being able to create blocks? We should get subsystem maintainer feedback on whether there's usecases for accessing the block library without being able to create 'em. (Because like... I don't think there is...?)

EDIT: Removed the bit about the previous issue; I failed to read the explanation in #7 properly.

If what I suspect is true and "Access block library" should also grant permission to create them, then the correct fix is instead to OR together all three permissions equally, and correct the test accordingly.

Regardless of which fix we end up with, this is a disruptive change, because it changes which permissions people have to configure and what access site users get as a result. So, it will need a CR and a release note.

Thanks everyone!

Saving credits.

Oh one other thing; we should also get Layout Builder subsystem maintainer review on this to make sure we're not breaking some sort of access assumptions there.

So the question is, does having access to the block library mean that you should also be able to create blocks

No, it should not. Changing this would grant access to users that previously could not create blocks. I do not see any reason to open these permissions up.

This issue is about removing the need for accessing the block library in order to create blocks for all the same reasons already discussed in #3394741: BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations - there are use cases where roles should be able to be setup where they cannot access the block library, but should be able to create blocks.

Oh one other thing; we should also get Layout Builder subsystem maintainer review on this to make sure we're not breaking some sort of access assumptions there.

This has minimal interaction with Layout builder apart from allowing users to create reusable blocks without having access to the block library. layout_builder_block_content_access already grants access to inline blocks irrespective of any of these permissions (it uses its own create and edit custom blocks permission).

Added CR and release note.

Still need that feedback specifically confirmed by an LB maintainer though. Thanks!

Not a layout builder subsystem maintainer, but framework manager opinion - this was an oversight from #3394741: BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations, which itself was an oversight from #1984588: Add Block Content revision UI and filed almost straight after that went in by @jenlampton.

Confirming that \Drupal\layout_builder\Controller\ChooseBlockController::inlineBlockList does not consider create access when dealing with inline blocks.

Back to RTBC then. Thanks @larowlan.

So... what is the point of accessing the block library if you can't create block instances? What would you even use it for? Sorry, I'm still not understanding something or confused about something here.

@xjm - you might be able to edit blocks, but not create them. The block library has operations links for each block. But regardless, this isn't changing that - this is removing the need to access the block library in order to create inline blocks.

i.e this change allows a site to provide a link to create a new block, but without needing to give the user access to the block library.

In a node analogy, allowing the user to create a basic page without giving them access to admin/content.

@larowlan I agree with fixing it so that the "create" permission alone is sufficient to create. What I'm still stuck on is how the block library is useful without the permission to create, because as far as I know, every block instance placed from the library is a new one. Maybe I'm conflating two different things. Maybe manual testing steps for using the block library before-and-after would help. I'll try to get to this later.

Actually, that made me realize that the change hasn't had any documented manual testing of the main issue with block creation either (oops!). #12 is not sufficient evidence of manual testing, unfortunately. Documenting specific STR with manual testing will make it easer for confused reviewers like myself to get to the same page. :)

Working on the manual testing now.

So turns out I was conflating block instances with content blocks, and the block plugin modal that you get when you click the "Place block" link at /admin/structure/block with the block library at /admin/content/block. Both places have the "Add content block" button, but they are completely different UIs. My mistake!

It turns out there are issues with the "access block library" permission, but they exist without this patch as well. Manual testing steps.

1. Install Standard with HEAD.
2. At /admin/content/block, click "Add content block".
3. Create a test block.
4. At /admin/people/create, create a test user that only has the "authenticated user" role.
5. At /admin/people/permissions/authenticated, grant authenticated users only the "Access the Content blocks overview page" permission.
6. Log in as the test user.
7. Go to /admin/content/block.
8. Click on "Test block 1".
9. Observe the "Access denied" message, despite the content block being published? In fact, there doesn't even seem to be publishing status for the block.
10. Log back in as the administrative user.
11. On admin/structure/block, click the "Place block" button in the Header region, and place an instance of the test block from step 3.
12. Switch back to the test user.
13. Go back to /admin/content/block.
14. Clicking on the test block link still gives "access denied", despite the content block itself being clearly visible in the header.
15. Switch back to the admin user.
16. Go back to /admin/content/block and click the link for the test block. The link actually takes you to the edit page, not the view page (there is no standalone "view" page for a content block apparently), which is why it 403s for the test user.
17. At /admin/people/permissions/authenticated, grant authenticated users only the "Basic block: Edit content block" permission.
18. Log back in as the test user and go back to /admin/content/block. Clicking on the link for the block now works and takes you to the edit form as expected.

Repeating the above steps with the patch, there is no change. Edit: However, if the steps are repeated with Views and Views UI uninstalled, the block title is correctly displayed with no link. So it's actually an issue with the block library view, not the entity collection itself.

So if you don't have permission to edit a block, the link in the block library should be rendered as plain text instead. But that's out of scope here since it's an existing problem without the patch.

In BlockContentAccessControlHandler::checkAccess(), it looks like the intention was for "access block library" to give you permission to view the blocks even if they were unpublished, but there's actually no such thing as an unpublished content block, so that is also weird. You'd have to alter the entity type's base fields for that code to do anything.

Still need that manual testing for the actual case of creating the content blocks (with exact steps to reproduce like I've given above for testing the access content library permission).

Fleshed out the release note a bit to describe the action site owners should take, including the UI label for the permission since it's very different from the machine name.

The fact that this grants access that previously was (incorrectly) denied means it's a minor-only change, so should only be committed to 11.x at this point. One of the first issues for the 11.1.0 release notes, but not the first.

Also expanded the CR to be more clear about the actions a site owner should take, and corrected the target branch to be 11.1.x. (10.4.x is a maintenance minor, so would not generally receive disruptive bugfixes like this one.)

Filed the followup: #3455803: Block library view links to the edit page for each content block even when the user does not have edit access

Updating steps to reproduce.

With the MR applied I followed the steps in the issue summary having an editor role with only create any basic block content permission and I was able to create a block now.

Since no code change the test-only run is still there https://git.drupalcode.org/issue/drupal-3412420/-/jobs/1790698

Believe this is good.

Committed 6ef6a00 and pushed to 11.x. Thanks!

Only committing to 11.x as per the CR. I'd be in main to commit this to 11.0.x and 10.4.x but the CR stops me from doing that. And I don;t feel this bug fix is urgent.

but the CR stops me from doing that

What do you mean by this?

@acbramley I assume it was a typo or just an incomplete thought about decision factors. In this case, a release manager's decision in #30 and #32 that this is too disruptive for a maintenance minor is what stops it from being backported to 10.4. Edit: To clarify, the release manager (me) altered the change record (CR) with an update about its allowed versions as well as a tag indicating which minor release should mention the change, and I believe @alexpott referenced those changes in his decision. I think that's what the comment means.

11.1 and 10.4 ship at the same time, but 10.4 is supposed to have very limited changes per the maintenance minor docs. But this is the first time we've ever had a maintenance minor and so the committers haven't established a detailed policy yet; we'll learn as we go, just like we did each previous time we improved the release cycle. :)

Edit: To frame it a little better, I'm advising the absolute minimum of backports to 10.4 that are strictly necessary to meet the stated maintenance requirements, and relying on contrib to let us know if/when/how badly we break their compatibility between 11.1 and 10.4. We can always backport stuff later if we identify the need, but reverting loads of stuff is much more difficult.