Closed (fixed)
Project:
Experience Builder
Version:
0.x-dev
Component:
Auto-save
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
10 Jun 2025 at 16:03 UTC
Updated:
10 Jul 2025 at 07:14 UTC
Jump to comment: Most recent
Show commands
changes
Comments
Comment #6
penyaskitoComment #8
larowlanAre we sure this is the correct approach?
Shouldn't we be checking access in
ApiLayoutController::(get|patch|post)so that we can be sure anything that is in the autosave store has already been access checked?Comment #9
penyaskito@larowlan This complements #3494915: Support entity-level + field-level access checking in auto-save — i.e. in `experience_builder.api.api.(layout.post|auto-save.post)`, #3527244: Add test coverage that ensure fields values the user has view but not edit access to can NOT be saved to AutoSave and #3516432: Update all XB routes to respect content entity update/field edit access of edited XB field, that cover what you mention. But I could still make the edits, have them stored in auto-save, and you could be the one publishing them. And we could have separate permissions!
Comment #10
wim leers… this is why the permission mentioned in the meta is still needed 🙏
Comment #11
penyaskitoCreated #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes for that.
Comment #12
penyaskitoAssigning to @tedbow for reviews.
Comment #13
larowlanIn light of #10 and #11 I still agree with myself in #8 that this approach feels wrong.
We should validate the user has access at the time the autosave store is written to, and then check that new permission (only) at publish time.
Additionally #3529622: Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates is moving to remove
AutosaveManager::save(EntityInterface $entity, array $arbitrary_data)and replace it withAutosaveManager::saveEntity(EntityInterface $entity)such that you have to convert the entity before you can store it in the autosave and you can only store the entity and nothing else. That will mean theupdated_entity_form_fieldswe're using here will be removed. It will also bring us closer to a workspaces backed auto-save because internally toAutosaveManager::saveEntity(EntityInterface $entity)we could almost immediately switch the internals of that method to write a revision to a workspace if$entitywas a revisionable content entity.I think on #8 I could live with this checking entity update access, but the field level access should only be checked at the time of autosave write, not at publish time.
Thinking about an analogy to how Drupal works now.
If I have field level permissions for the node edit form and have two roles as follows:
* Super secret field editor, can edit the values in a secret field
* Publisher, can move any content from needs review to published
If a user of role Super secret field editor makes an edit to a secret field and sets it to needs review, the Publisher can still move it from needs review to published. In that operation all they are changing is the moderation state and that is covered by permissions in content moderation module around transitions they have access to (provided they have edit access to the node). At the time of publishing, each of the individual fields aren't checked, only the transition permission. The individual field permissions are checked at the time the draft revision is created.
I think we should be doing the same thing here.
Comment #14
penyaskitoGiven #3525829-8: If user with less/different permissions edits a page after a user with more/different permissions data loss could occur, confirmed today by Lauri, I feel like we need a sync on this.
Comment #15
penyaskitoDiscussed this with Ted + Alex. Even if we accomplish #3529622: Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates, we shouldn't postpone/won't fix this because regardless this is an entity or form data, we want to be able to prevent you to publish something you don't have access to.
Even if we think we might be able to relax this checks to make possible to publish things people might have no access to edit, we don't want to support this at this time because it's hard to reason about because the difference with workspaces/content_moderation is that they perform an actual entity save, while we are creating + serializing the entity and not completing the entire entity-save-flow.
E.g. This won't trigger hook_entity_insert/update or whatever other custom code might do whatever they need. E.g. maybe they are throwing an exception on preSave (yes, people might do non-standard things)
So, IOW:
If this is too restrictive, we actually will be able to alleviate this once selective publishing is in place.
TODO: Create a follow-up to explore if we can relax these requirements.
Comment #16
tedbow@penyaskito see my MR comments. What do you think about removing the field level access checks and tests and making follow-up for that after #3529622: Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates.
We would have your test coverage changes from here that we could start that issue with
Comment #17
penyaskitoRepurpose for only entity access check. Field access checks moved to #3532454: Add field access check on `ApiAutoSaveController::post()`
Comment #18
penyaskitoComment #19
tedbowIf tests pass I think this looks good!
If they fail I am ok with reverting this https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
and RTBC'ing again
Comment #21
tedbowIf tests pass I think this looks good!
If they fail I am ok with reverting this https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
and RTBC'ing again
Comment #22
tedbowComment #23
wim leersLooks very nice! 👏
Comment #24
wim leers