- Issue created by @larowlan
- πΊπΈUnited States tedbow Ithaca, NY, USA
@larowlan thanks for making this issue.
I think we also might need a temporary generic
publish XB changes
permissionsThe account that submits "Publish All" might not have done the changes to all of the entities(or any?) that would be published. Are we going to require that account to have the permissions to make all the changes across all the entities.
I think having a simple
publish XB changes
should be good enough for 0.2.0 because once we use Workspaces we want use Workspaces related permissions to determine access to publishing changes to the live site.Related to π Save metadata(page data) field with the entity revision Active If we went with
publish XB changes
we would probably want to check access to field changes at the time the changes went into the auto-save. The other option I guess would be to check access as the person who the auto-save considers it's "owner" when submitting "Publish all". The problem with that is that multiple accounts may have edited the same entity. Also the account could have different permissions at that time or been deactivated, hopefully for pre-workspaces, 0.2.0 we won't to worry about these edge case.But again this may change with Workspace integration so I don't think we should spend that development time on it since 0.2.0 is not for production sites. For example we don't know that once we use Workspaces the final "Publish all" step will still be the point at which we convert auto-saves to real entity saves. We might just use auto-saves at that point to handle: 1) invalid entities, 2) speed or DB bloat concerns from many revisions. We could still require real entity revisions(whether user is aware of them or not tbd) before entities could be move to the live Workspace.
- πΊπΈUnited States tedbow Ithaca, NY, USA
Another complication with access is right now as π Save metadata(page data) field with the entity revision Active is done field access is checked in the process of
`\Drupal\experience_builder\ClientDataToEntityConverter::convert()`
. This make sense as this will be use to set entity fields we shouldn't allow setting field the user does not have access to. But we don't run`ClientDataToEntityConverter::convert()`
on auto-save. We couldn't really call`convert()`
to determine access though because theEntityConstraintViolationListInterface
it returns mixes non-access violations and access violations.So either we could either
- extract out something
ClientDataToEntityConverter::ensureAcess()`
that would be called on autosave, but then access would still be checking inClientDataToEntityConverter::convert()`
during "Publish all" - Introduce a new
AccessAwareEntityConstraintViolationList extends EntityConstraintViolationList
with additionaladdAccessViolation(ConstraintViolation $v)
,getAccessViolationsList()
andgetNonAccessViolationsList()
and have`ClientDataToEntityConverter::convert()`
return this.So then on autosave we could ensure
getAccessViolationsList()->count() === 0
and then on Publish all we could ensuregetNonAccessViolationsList()->count() === 0
- extract out something
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Indeed, route-level access checking only checks entity-level access.
But β¦ we could change that in principle, if that simplifies things: we could build a field-level equivalent of
\Drupal\Core\Entity\EntityAccessCheck
, which would then need to inspect all fields received in the request body.That'd move the moment of field-level access checking to before controller execution instead of during.
Would that simplify things?
P.S.: I recall having to deal with this while working on Drupal core's REST + JSON:API modules. I remember investigating field-level access checking. I do not recall whether there is a blocker to checking field access earlier (they both checking it during controller execution). I suspect it's mostly related to the need to deserialize the received data first. But if XB's request bodies follow a firm structure where the list of modified fields is easily accessible, I think it would be possible to do field-level access checking earlier.