[PP-1] Add entity access checks to routes that deal with entities

Created on 19 December 2024, 3 days ago

Overview

In πŸ“Œ Save metadata(page data) field with the entity revision Active there are some oddities with field access that only occur because there is no entity access checks on routes.
At present we're just using 'access administration pages' but need to properly check entity access.

Proposed resolution

Either add an access checker that can dynamically detect the entity-type OR split routes up like layout builder (And the Page entity) so that not everything runs off /xb/{entity_type}/{entity_id} and instead derive routes where the entity-type is fixed which will allow use of the existing _entity_access check.

User interface changes

πŸ“Œ Task
Status

Postponed

Version

0.0

Component

Page builder

Created by

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • 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 permissions

    The 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 the EntityConstraintViolationListInterface it returns mixes non-access violations and access violations.

    So either we could either

    1. extract out something ClientDataToEntityConverter::ensureAcess()` that would be called on autosave, but then access would still be checking in ClientDataToEntityConverter::convert()` during "Publish all"
    2. Introduce a new AccessAwareEntityConstraintViolationList extends EntityConstraintViolationList with additional addAccessViolation(ConstraintViolation $v), getAccessViolationsList() and getNonAccessViolationsList() and have `ClientDataToEntityConverter::convert()` return this.

      So then on autosave we could ensure getAccessViolationsList()->count() === 0
      and then on Publish all we could ensure getNonAccessViolationsList()->count() === 0

  • πŸ‡§πŸ‡ͺ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.

Production build 0.71.5 2024