- Issue created by @penyaskito
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
penyaskito β credited wim leers β .
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
penyaskito β credited larowlan β .
- πΊπΈUnited States tedbow Ithaca, NY, USA
penyaskito β credited tedbow β .
- Merge request !1124#3529426: Add field and entity access check on `ApiAutoSaveController::post()` β (Open) created by penyaskito
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Are 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? - πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
@larowlan This complements π [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active and π [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active , 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!
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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!
β¦ this is why the permission mentioned in the meta β is still needed π
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
Created π Add a Publish Experience Builder Content permission Postponed for that.
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
Assigning to @tedbow for reviews.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
In 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 π Page status changes from "Published" to "Changed" even when no actual changes are made Active 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_fields
we'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$entity
was 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 publishedIf 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.
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
Given #3525829-8: If user with less/different permissions edits a page after a user with more/different permissions data lost could occur β , confirmed today by Lauri, I feel like we need a sync on this.
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
Discussed this with Ted + Alex. Even if we accomplish π Page status changes from "Published" to "Changed" even when no actual changes are made Active , 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:
* We don't think this is a requirement and could be relaxed in the future.
* But it's more costly to verify, validate and document why we don't do the access checks, than actually just do then even if they look paranoid.
* The safest and actually easiest to implement is just verify the publisher has access to everything and the entity (or form data) is valid.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.