BlockContentAccessControlHandler requires access block library permission for create

Created on 4 January 2024, 7 months ago
Updated 12 July 2024, 7 days ago

Problem/Motivation

🐛 BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations Fixed solved block access for update and delete but not create. The title left out "create" but several comments say that it works. Why as create left out? Hopefully this was just an oversight :)

Steps to reproduce

  1. Install Drupal with the standard profile
  2. Add a role that has create any basic block content permission
  3. Login as a user with the above role
  4. Visit /block/add/basic
  5. Get a 403
  6. Logout
  7. Log back in as an admin
  8. Grant the above role the "access block library" permission
  9. Logout
  10. Log back in as the user with the above role
  11. Visit /block/add/basic
  12. Get a 200, i.e the add form for a block

It should not be necessary to grant access to the block library in order to create a block content entity.

Proposed resolution

Change checkCreateAccess() to only check for 'create' and not 'access block library'.

Remaining tasks

Commit

Release note snippet

The "Access the Content blocks overview page" permission is no longer required to create blocks . This means that roles that have "Create new content block" permission for a given block type will be able to create these blocks when they could not in previous releases. Site owners should audit their Block Content permissions to ensure that block creation access is granted intentionally in all cases, and can also now consider revoking access to the content block overview listing for roles that do not need it.

🐛 Bug report
Status

Fixed

Version

11.0 🔥

Component
Block content 

Last updated 1 day ago

Created by

🇺🇸United States douggreen Winchester, VA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @douggreen
  • 🇺🇸United States douggreen Winchester, VA
  • 🇺🇸United States douggreen Winchester, VA

    Patch attached

  • Status changed to Needs review 7 months ago
  • 🇺🇸United States douggreen Winchester, VA
  • 🇺🇸United States douggreen Winchester, VA
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    Recommend turning to an MR.

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

  • Status changed to Needs review 7 months ago
  • 🇺🇸United States douggreen Winchester, VA

    MR created.

    In reply to the comment by @smustgrave that this was possibly done on purpose because of a layout builder scenario, that isn't mentioned at all in the original ticket. And this very problem here was mentioned there.

  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    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.

  • 🇺🇸United States douggreen Winchester, VA

    Odd, I don't see the "create Merge Request" button. I created a branch and pushed to it but the "create merge request" button is missing and the link that git gives me gives a 404.

    https://git.drupalcode.org/issue/drupal-3412420/-/compare/11.x...3412420...

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 7 months ago
    25,803 pass, 1,821 fail
  • Pipeline finished with Failed
    7 months ago
    Total: 529s
    #72389
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    25,781 pass, 1,840 fail
  • 🇦🇺Australia acbramley

    Tests need fixing.

    Also correcting permission in IS and hiding the patch.

  • 🇮🇳India Prashant.c Dharamshala

    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

  • First commit to issue fork.
  • Pipeline finished with Failed
    6 months ago
    Total: 196s
    #78612
  • Pipeline finished with Failed
    6 months ago
    Total: 621s
    #78949
  • Pipeline finished with Failed
    6 months ago
    Total: 652s
    #78969
  • Pipeline finished with Failed
    6 months ago
    Total: 204s
    #78975
  • 🇦🇺Australia acbramley

    acbramley changed the visibility of the branch 3424720-languagenegotiationurl-unnecessarily-adds to hidden.

  • Pipeline finished with Failed
    5 months ago
    Total: 564s
    #109957
  • 🇦🇺Australia acbramley

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 181s
    #192353
  • Pipeline finished with Failed
    about 1 month ago
    Total: 190s
    #192354
  • Pipeline finished with Success
    about 1 month ago
    Total: 671s
    #192355
  • Status changed to Needs review about 1 month ago
  • 🇦🇺Australia acbramley

    Pushing this one along.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 510s
    #192366
  • Status changed to RTBC about 1 month ago
  • 🇺🇸United States smustgrave

    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'

  • Status changed to Needs work about 1 month ago
  • 🇺🇸United States xjm

    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 do two things:

    1. git log -L the code to find the issue that added the bit about the block library permission, and see if the intent in doing so is discussed there.
    2. Once we've checked that, get subsystem maintainer feedback on whether the intent of the original issue (if documented) made sense, or if it's just wrong and there's usecases for accessing the block library without being able to create 'em. (Because like... I don't think there is...?)

    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!

  • 🇺🇸United States xjm

    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.

  • Status changed to Needs review about 1 month ago
  • 🇦🇺Australia acbramley

    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 🐛 BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations Fixed - 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.

  • 🇺🇸United States xjm

    Still need that feedback from an LB maintainer. Thanks!

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Not a layout builder subsystem maintainer, but framework manager opinion - this was an oversight from 🐛 BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations Fixed , which itself was an oversight from Add Block Content revision UI Fixed 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.

  • Status changed to RTBC about 1 month ago
  • 🇦🇺Australia acbramley

    Back to RTBC then. Thanks @larowlan.

  • 🇺🇸United States xjm

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

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    @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.

  • Status changed to Needs review 29 days ago
  • 🇺🇸United States xjm

    @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. Maybe manual testing steps for using the block library before-and-after would help.

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

  • Assigned to xjm
  • 🇺🇸United States xjm

    Working on the manual testing now.

  • Issue was unassigned.
  • 🇺🇸United States xjm

    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.

    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).

  • 🇺🇸United States xjm

    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.

  • 🇺🇸United States xjm
  • 🇺🇸United States xjm

    Also expanded the CR to be more clear about the actions a site owner should take.

  • 🇺🇸United States xjm
  • 🇦🇺Australia acbramley

    Updating steps to reproduce.

  • Status changed to RTBC 8 days ago
  • 🇺🇸United States smustgrave

    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.

    • alexpott committed 6ef6a009 on 11.x
      Issue #3412420 by acbramley, douggreen, Hardik_Patel_12, xjm, smustgrave...
  • Status changed to Fixed 8 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    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.

  • 🇦🇺Australia acbramley

    but the CR stops me from doing that

    What do you mean by this?

  • 🇺🇸United States xjm

    @acbramley I assume it was a typo. 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.

    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. :)

Production build 0.69.0 2024