This is a subset of the original scope of #3494915: Support entity-level + field-level access checking in auto-save — i.e. in `experience_builder.api.api.(layout.post|auto-save.post)`.
Currently, the access administration pages permission is hardcoded in a few places critical to loading the XB UI:
experience_builder.experience_builder:
path: '/xb/{entity_type}/{entity}'
…
requirements:
_permission: 'access administration pages'
+
experience_builder.api.layout.get:
path: '/xb/api/layout/{entity_type}/{entity}'
defaults:
_controller: 'Drupal\experience_builder\Controller\ApiLayoutController::get'
requirements:
_permission: 'access administration pages'
…
experience_builder.experience_builder to use _entity_accessexperience_builder.api.layout.get to respect entity update/field edit access of edited XB field:
_entity_access route requirement, which supports dynamic entity types
::get() method, call ::fieldAccess(operation: 'edit') — this is not available as a route requirement (plus the XB field name must first be resolved, which can kinda only happen in the controller)
access administration pages permission to the anonymous user./xb/node/1/editor, which is an article node that the anonymous user cannot access.| Comment | File | Size | Author |
|---|---|---|---|
| #25 | Screenshot 2025-06-04 at 4.05.19 PM.png | 356.66 KB | deepakkm |
Show commandsStart 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:
changes
Comments
Comment #2
wim leersComment #4
wim leersActually, this is AFAICT not blocked.
Crediting @tedbow because his comments on #3494915: Support entity-level + field-level access checking in auto-save — i.e. in `experience_builder.api.api.(layout.post|auto-save.post)` have helped me craft this. 😊
Comment #5
wim leersComment #8
deepakkm commentedComment #9
deepakkm commentedThe failing pipeline is due to the rebase done with 0.x otherwise all tests are passing.
Comment #10
wim leersComment #11
deepakkm commentedComment #14
deepakkm commentedMR !957 was messed and hence created a new MR with similar changes
Comment #17
wim leersBased on meeting just now, can we get:
.gitlabci.ymlto use MariaDB by default instead of SQLite? (Or MySQLite, whichever is fastest.)Comment #18
deepakkm commentedThe pipeline passed - https://git.drupalcode.org/issue/experience_builder-3516432/-/pipelines/..., when the work was done. but failed after a rebase done from another person and i may know why because of the major changes introduced in ApiLayoutController.
I'll update the test case for this once.
but the actual update in gitlab file goes to this issue - https://www.drupal.org/project/experience_builder/issues/3518292, where pipeline failing for 1 and passes for another , i'll update the gitlab file for that. Thanks
Comment #19
mglamanIt failed due to type -> component_id in the tree
Comment #20
mglamanComment #21
deepakkm commentedThis is now good for review. The failing pipeline in cypress test is not part of the changes done in this MR.
Comment #22
wim leersand
But … this one surely isn't introducing transliteration, so why is this one failing? 🤔 Anyway, because you really want this merge order, I tried to land #3518292: Allow searching for content in the navigator, via `/xb/api/v0/content/{entity_type}`, but couldn't: #3518292-28: Allow searching for content in the navigator, via `/xb/api/v0/content/{entity_type}`.
Comment #23
deepakkm commentedSo right now there are 3 cypress tests failing and i have no idea why these tests are failing. No idea how i can move forward in fixing those. The major problem i have in setting up my cypress test is the XbSetup is throwing error.
Though looking for a way forward on this.
Comment #24
mglamanAssigning to deepakkm, had some MR review feedback
Comment #25
deepakkm commentedI have no idea now as to how to fix this random cypress failure [component-operation.cy.js] though i reran this test but it still fails and passes locally as shown in the screenshot.
Comment #26
deepakkm commentedComment #27
mglamanGiving it a quick look over
Comment #28
mglamanLooks good to me!
Comment #29
penyaskitoLGTM too
Comment #30
penyaskitoThe playwright failure looks legit though?
Comment #31
mglamanI see this as a a not-us-failure due to an experimental job. I'll move back to RTBC and someone else can decide different. Re-running the job.
Comment #32
wim leersI spotted several reductions in test coverage without an MR comment that provides guidance why. Looks like @larowlan spotted it too.
Needs follow-up for at least the component instance form route, because the meta doesn't have an issue for it yet: #3452581: [META] XB Permissions. This (IMHO) confirms the need for a route requirement access check, although that too could be deferred to that follow-up if you prefer.
Same thing for the content entity form route, but AFAICT that could trivially be added here — follow-up is fine too.
Comment #33
deepakkm commentedComment #34
larowlanI think this is ready, assigning to Wim for a final review and possible merge.
Comment #35
wim leersThe
hook_entity_field_access()implementation unfortunately contained multiple bugs … but only due to terrible DX of core's Entity + Field access! 😭 Sorry you had to wrestle through that, @deepakkm.Per the issue summary, this was extracted from #3494915: Support entity-level + field-level access checking in auto-save — i.e. in `experience_builder.api.api.(layout.post|auto-save.post)`, ~2.5 months ago. A lot has happened since then. Including that issue having landed. The original was that this would land first, which would then allow #3494915: Support entity-level + field-level access checking in auto-save — i.e. in `experience_builder.api.api.(layout.post|auto-save.post)` to take advantage of the infra this added. I disagree respectfully with all comments #41 through #51 on that issue, because they overlooked something crucial: it would've been up to that issue to ensure the "patch" and "post" routes for editing layout (aka the component tree of an entity) respected field access. #3527156: Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess() exists, but that's about generic intra-controller field access checking across all edited fields. When possible, the controller should never be executed; access should be denied at the route access level.
So, retitling to reflect that.
And fixing that was trivial, but the fallout of test breakages was not … turns out this ended up crashing the "breadcrumb block" (because it checks the current URL, strips it path-by-path and then eventually starts hitting the new access control added here 🤪) — all because #3509500: In XB's preview canvas, the Breadcrumb block does not show the edited entity's path breadcrumbs is not yet fixed. So, worked around that by forcing the preview to always use "front page" as the breadcrumb. 🫣
Comment #36
wim leersMarking this to signal the incompleteness. Because:
The follow-up needed (as described in #32) still needs creating.→ done in #35 :)— @mglaman at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
👉
multiple follow-ups stillsingle follow-up needed 🙏😊Feels so good to finally have this done! 😃 Thanks, @deepakkm!
Comment #38
mglamanOpened #3529836: Enable starting with an empty XB UI (so without first having to create an entity with a component tree) as follow up
Comment #39
wim leersComment #40
wim leersShoot — this caused a regression 😭 I've requeued e2e tests dozens of times today, which is how I failed to spot that these failures were legitimate 😞
The
0.xCI job for this MR landing is consistently failing on:contain-css.cy.jsglobal-regions.cy.js… which I only noticed after re-queuing those CI jobs multiple times too.
The latter explicitly uses the breadcrumb block (which this MR made better (out of sheer necessity to be able to add the necessary access protection to routes, so reverting is not really an option), the former probably is implicitly relying on it? 🤔 I don't see how yet though.
Comment #41
isholgueras commentedI'll take a look
Comment #47
wim leersThanks, @isholgueras, for saving the day! ❤️
Comment #48
wim leers