Problem/Motivation
Custom access checks can be added for operations in the layout builder either via custom code or via contrib modules like https://www.drupal.org/project/layout_builder_perms. In this way it is also possible to disallow moving blocks. In an advance use case it can be desirable to disallow blocks of a certain type into sections with a certain layout. This works fine but the layout builder UI does not respect the access on routes involved. This can lead to a situation in which the layout builder UI shows a move operation as successful (i.e. the block is move to another region in another section) but the ajax call to actually register the change resulted in a 403. The user will not notice this until reloading the page or saving the layout. Only then they will discover that the block was not moved.
Steps to reproduce
* Install Drupal, enable layout builder and create a layout builder enabled content type.
* Install Layout Builder Advanced Permissions (https://www.drupal.org/project/layout_builder_perms) and enable the Layout Builder Advanced Permissions Layout sub module.
* Create a custom role and configure permissions to allow the role to place blocks in sections with a layout of a certain type but not in sections with a layout of another type.
* Create a node of a layout builder enabled content type.
* Add a section (A) in the layout builder and configure it with a layout in which the custom role is allowed to place blocks.
* Add a section (B) in the layout builder and configure it with a layout in which the custom role is not allowed to place blocks.
* Add a block in section A.
* Save the layout.
* Go back to the layout builder and move the block from section A and section B. The layout builder will visually allow this and move the block to section B.
* Save the layout.
* See that the block is still in section A.
Proposed resolution
In core/modules/layout_builder/js/layout-builder.es6.js there is a behavior layoutBuilderBlockDrag. The Sortable that is created in this behavior can be extended with an onMove() callback. This callback is executed every time a block is dragged over a region. Returning false from this callback will disallow drop in that region. It will also prevent the region from being highlighted.
For this to work we will also need to add a URL that can be called via ajax and that only executes the access check for the layout_builder.move_block route and return the result (without actually executing the logic in the controller linked to the route).
Remaining tasks
- Create patch
- Review patch
- Commit patch
User interface changes
Block operations in layout builder that are not allowed will not be possible anymore in the layout builder UI.
API changes
N/A
Data model changes
N/A
Release notes snippet
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3213792-17-10.1.patch | 8.81 KB | seanb |
| #15 | 3213792-15.patch | 10.85 KB | seanb |
| #13 | 3213792-13.patch | 10.69 KB | _utsavsharma |
| #13 | interdiff_10-13.txt | 4.42 KB | _utsavsharma |
| #11 | interdiff_10-11.txt | 5.1 KB | pooja saraah |
Issue fork drupal-3213792
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3213792-custom-access-checks
changes, plain diff MR !3296
Comments
Comment #2
ricovandevin commentedHere is a patch. It seems to work fine but code might need grooming.
Comment #3
ricovandevin commentedComment #4
gauravvvv commentedRe-rolled patch #3, added interdiff for same. Please review.
Comment #7
pooja saraah commentedFixed failed commands on #4
Attached patch against Drupal 9.4.x
Comment #10
ricovandevin commentedRerolled against 9.5.2.
Comment #11
pooja saraah commentedFixed failed commands on #10
Attached patch against Drupal 9.5.x
Comment #12
smustgrave commented#11 is failing CI builds.
make sure to run
./core/scripts/dev/commit-code-check.shbefore uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...Tagging for tests to show this issue.
Did not attempt to test myself.
Comment #13
_utsavsharma commentedFixed CCF for #10.
Comment #14
gauravvvv commentedUpdating attributions
Comment #15
seanbAdded missing use statement, which probably confirms we need tests.
Comment #16
smustgrave commented@seanB 100% correct. It's good to see that this change didn't break anything but could use a test case to make sure the issue being fixed here doesn't pop back up.
Also can we typehint public function allowed().
Not sure if you want to make it a D10 patch or both?
Comment #17
seanbReroll for 10.1
Comment #18
heykarthikwithuMoving to Needs review, based on the #17 reroll.
(Pl move back if not correct)
Comment #19
smustgrave commentedWas previously tagged for tests which still appear needed.
Thanks.