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

@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.

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

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.

Updated this per the plan at 📌 SdcController cleanup tasks Active .

Blocked on 📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active . If you follow the steps to reproduce 📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active there, you'll see

This is because for the request that the XB UI makes for /xb/api/layout/node/2, the server responds with:

{
    "errors": [
        {
            "detail": "The current user is not allowed to update the field \u0027path\u0027.",
            "source": {
                "pointer": "entity_form_fields.path"
            }
        }
    ]
}

While this is true, the real problem here is that XB shouldn't try to auto-save fields that the current user cannot access (and FWIW, the tab aka the content entity form does not show the path field's widget!).

@lauriii to confirm

1. AFAICT the internal discussion between @tedbow and @lauriii leading to 📌 SdcController cleanup tasks Active failed to discuss field-level access control. 😅 And that's what this issue was originally opened for.

   But it does specify the following functional requirements:

   1. Allow users with “Publish Experience Builder Content” to access the review changes panel, and to view all content that is about to be published.
   2. User with permission “Publish Experience Builder Content” must have access to edit all underlying entities at the time of publishing, in order to be able to publish changes.
   3. User with “Edit pages” without the “Publish Experience Builder Content” permission must be able to view changes to all content that they have edit access to.

   which seems to imply that all necessary field-level access checks MUST be performed for the current user.

   This is the fundamental question @tedbow raised → in #2.

2. The list of unpublished changes in the popover does NOT need to indicate which ones the current user does not have sufficient permissions for to publish (1️⃣ in the screenshot):
   
   The current user CANNOT know which parts they would be unable to publish, they would get an error message upon clicking .
3. The button in the popover does NOT need to be disabled if the current user does not have sufficient permissions to publish everything in the auto-save store (2️⃣ in the screenshot):
   
   The current user can click , even if XB can technically already know they're guaranteed to get a 403 response (because >=1 entity cannot be updated by the current user, or >=1 field cannot be edited by the current user).
4. AFAIK "selective publishing" is a thing we intend to work on soon, and for that, we'd AFAIK need to pretty much solve both points 2 and 3. Do you want me to go ahead and create an issue for those?

@tedbow FYI: the simpler, non-auto-save parts of this issue's scope have been extracted into 📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active .

wim leers → credited effulgentsia → .

@effulgentsia indicated that he expects at least #7.3 would be disabled when >=1 entity or >=1 content entity field cannot be saved by the current user.

re #11 so are talking about disabling "Publish all changes"? If so are we planning on working on that before 🐛 [Meta] Selective Publishing and Reverting Active .

It seems like we shouldn't do the work of disabling "Publish all changes" before this because at that point the user may some changes they can't publish but if they don't select them then they would not get an error. We could choose to disable the ability to select entities they won't have permission to publish or just let them select whatever and then get an error(less ideal UX probably less work)

Anyways it would be not have to do this work over again

@tedbow I agree we should avoid duplicate efforts, or even temporary efforts. Can you propose the (issue) implementation order? 🙏

I think 5 maybe a different issue.

In \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields() we try to filter out fields and only update the ones that were shown on the form.
In \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess, which `setEntityFields` calls don't throw the AccessException if the user has view access and the field has not changed.

so from what you describe something in there is not working.

From the summary

Update ApiLayoutController::post() to perform field-level access checking; but ONLY check field edit access for fields that are actually being modified (this ensures that 4xx responses continue to be thrown when trying to bypass access, but would not trigger a 4xx for the inaccessible path field in the example in

So if understand this we are basically already trying to do this. It may be some problem specific to the path field. Wed had to put in special logic already for changed

I will debug and try to see what is happening with the path field

I tried the steps to reproduce. #5 seemed to imply you could get the path access problem described by just

Grant the access administration pages permission to the anonymous user.

(from the steps to reproduce)

For me /xb/node/1/editor was accessible but UI didn't actually work because the client requests got 403s

Here is what I did

1. installed the standard profile
2. added all XB permissions to "content editor" role and 'access administration pages'
3. Removed the 2 "Path" permission from the "content editor" role
4. created node
5. went /xb/node/1/editor
6. the UI load without the "path" field
7. changed the title
8. click "publish all"

This works fine. I debugged \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields during this process and path is NOT sent back in $entity_form_fields.

I find way to get a similar problem.
You have to have 2 users. 1 user with field edit access on path and 1 without.

If the user with field edit access opens the XB editor and makes and changes a value for path will get in the auto-save.
Then if the user without field edit access opens the same node in XB then they will get an error.

but they get the on error on \Drupal\experience_builder\Controller\ApiLayoutController::get not post. This is because
\Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable is called and that calls \Drupal\experience_builder\ClientDataToEntityConverter::convert which checks the field access to path because it is auto-save not because it was rendered in the field.

It would probably have the same problem on \Drupal\experience_builder\Controller\ApiLayoutController::post but I can't get it not error before that in this situation

We could try to get around this by passing a flag \Drupal\experience_builder\ClientDataToEntityConverter::convert like $no_field_edit_check. Because in the case of buildPreviewRenderable should we be checking edit access? At that point they are not editing they just need to see the preview. If another user has edited a field they don't have access then they should still effects in the preview(if any).

I think another problem this points out though is even if we figured a way around this. What happens if a user with field edit access first uses XB on node/1 and edits the path and then a user without field edit access edits the node in XB.

Right now calling \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable() with $updateAutoSave = true just calls sets entity_form_fields in the auto-save to what was on the form for the user. therefore it would wipe out any field values the user doesn't have access to edit

Ideally we would somehow merge the fields the user doesn't have access to
So if

1. user 1, has path edit access, user 2 does NOT have path edit access
2. user 1 uses XB with node 1 and sets path to /my-page
3. user 2 uses XB with node 1 and edits a different field
4. user 1 uses XB with node 1, should still see there /my-page change and user 2 field change
5. user 2 uses XB with node 1, should see the field 2 change
6. user 1 uses XB with node 1 clicks publish all, all changes should be saved(because they have complete access)

Right now set 3 would fail, user 2 could not use XB on node 1 after user 1 edited path.
If we got around that 4 would fail because user 2's use of XB would remove path from entity_form_fields

I think we could probably work on this now regardless of 🐛 [Meta] Selective Publishing and Reverting Active

I created 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active to solve the smaller problem