If user with less/different permissions edits a page after a user with more/different permissions data lost could occur

I need to update the summary more and figure out the relationship to 📌 [PP-1] Add entity access checks to routes that deal with entities Postponed which I also need to review

I tagged this "Needs tests" because someone could actually write a test that confirms the problem even if don't know that actually solution yet.

This feels like it should be a beta blocker?

+1 for this being a beta blocker because of data loss.

wim leers → credited effulgentsia → .

wim leers → credited jessebaker → .

Just discussed this issue for ~20 mins with @jessebaker and @effulgentsia.

We propose that:

1. if an auto-save entry exists for an entity with a field that has granular access control (which is not the case by default) and that field has been changed in the auto-save entry compared to the saved entity (this should NOT be blocked on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) → because \Drupal\experience_builder\ClientDataToEntityConverter::convert() takes everything in the entity form and upcasts it to the full entity, but … inaccessible fields would not be shown in the entity form, so shouldn't be in the auto-save entry!)
2. then the entity_form_fields blob returned by the experience_builder.api.layout.get route from auto-saveshould be able to check whether the auto-save entries are modifying fields the current user cannot access.
3. Then we tweak the response returned by that controller (kinda similar to how we did in 📌 Omit `PageRegion` representation from `ApiLayoutController::get()`, 403 if sending it in `::patch()` or `::post()` Active ), but since the content entity is the primary purpose of this very route, we can't omit entity_form_fields — but what we can do is something like not returning entity_form_fields.
   Note: the XB component tree can still be edited, so this user can still make meaningful changes! They just won't be able to edit anything under the tab.
   (They are guaranteed to have field-level edit access to the component tree thanks to \Drupal\experience_builder\Access\ComponentTreeEditAccessCheck aka the _xb_component_tree_edit_access: TRUE route requirement added last week in 📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active having verified that first.)

(Which AFAICT means 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active had some oversights 😅)

Implementation outline of this proposal attached as a patch.

I haven't looked at the XB codebase related to this deep enough to comment on the code-level implementation suggestions of #8, but I'm +1 to the high level premise if I understand it correctly, which is:

If an auto-saved entry exists, and there is 1 or more fields for which:

• The value is different than what the published entity has, and
• The current user doesn't have edit permission for it

Then the user should not be allowed to make any changes at all on the Page Data tab. I think ideally the Page Data tab would still be visible, but be read-only, with a message at the top saying that it's read-only because there exist unpublished changes to it that were made by someone with different permissions and must be published or reverted by someone with those permissions before it can be editable by others.

Then we can punt any further scope (i.e., allowing edits of the permissible fields and merging those into the auto-save) to a post-beta, and maybe even post-stable, follow-up.

I agree that #8 🐛 If user with less/different permissions edits a page after a user with more/different permissions data lost could occur Active would be an acceptable way to get us to stable. This could be then improved post-1.0.0.

Thanks!

I think 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active will resolve this, postponing

🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active is in!

This turned out to be pretty straight-forward after 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active

MR with test-coverage

The cypress errors look legit.

Wondering why they happened here and not in #3529622 tho.

Remaining fail is coming from \Drupal\datetime\DateTimeComputed::getValue

It's calling DrupalDateTime::createFromFormat with an array for $value

Which yields DateTime::createFromFormat(): Argument #2 ($datetime) must be of type string, array given

Ran out of time to work out what was causing this.

#19: that sounds a lot like 📌 Provide visibility into field types with different matching SDC prop shapes depending on field storage settings Active ? 🤔

I wanted to make sure I understood why this works and I think it is bit difficult to understand.

1. \Drupal\experience_builder\Controller\ApiLayoutController::get() calls $entity_form_fields = $this->getEntityData($entity);
2. then ApiLayoutController::getEntityData has
   

   // Filter out form values that are not accessible to the client.
       $values = self::filterFormValues($form_state->getValues(), $form);
   

So to understand this you actually read the code line by line. There are no function comments for ApiLayoutController::get(), ApiLayoutController::getEntityData or EntityFormTrait::filterFormValues. Also the name getEntityData to doesn't imply that we would be doing any filtering

Seems like at the very least <code>$entity_form_fields = $this->getEntityData($entity); should have comment explaining it is getting just the data accessible to the user. otherwise you really have to dig down.

Also in our openapi.yml file we have paths./xb/api/v0/layout/{entityTypeId}/{entityId}.get.responses.200.content.application/json.schema.properties.entity_form_fields

 entity_form_fields:
    type: object
    description: The full entity data.

which is no longer true, if it ever was

I think if I having trouble keeping track of how this works I think others will be too.

#21 excellent points - I've:

• Renamed getEntityFields to getFilteredEntityFields as that's a private method so it should convey what it is doing
• Updated openapi.yml
• Added a docblock to ApiLayoutController::get

Found the cause of #19 - and I think it exists in HEAD, and did even before 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active because its this in inputBehavioursEntityForm in inputBehaviors.tsx

const validateNewValue = (e: React.ChangeEvent, newValue: any) => {
    // @todo Implement this.
    return { valid: true, errors: null };
};

i.e. we don't do any client-side validation for entity-forms.

In HEAD we don't see this because each POST is loading the default entity.
Before 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active we didn't see it because we didn't convert until we hit publish.

We see it now because we load the last submitted auto-save entity during POST

The simplest way to trigger this is to add a date timestamp field to the entity form, delete the time and note you get a client-side (browser) error for the required field. Then edit the date field and this triggers a POST to the server, even though other fields on the form are invalid.

I think the fix will be to prevent any submissions while there are HTML5 errors in the form, no different to what any other Drupal form submission does.

Ok was able to trace this down, it was this change in behaviour in 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active

This was another 'fun adventures in debugging submitting form api forms over JSON' 😭

tl;dr if there are form validation errors, we can't assume what's in the entity field value is anything near valid.

In the case of #23 and #19 it is indeed an array with keys 'date', 'time' and 'object' as seen in DateTime element value callback, and not a string as the entity-api expects.

So the correct behavior is to not only record those form violations, but also make sure the entity in the auto-save state isn't mangled beyond repair. And to only do this for invalid deltas as we don't want to wipe new deltas or valid updates to existing deltas from the auto-save entry.

I've added extra tests and handling for that.

Yes ideally we could do this client-side, and I went down that path but without much joy.

Reverting that behavior change feels like the more robust approach.

So the correct behavior is to not only record those form violations, but also make sure the entity in the auto-save state isn't mangled beyond repair. And to only do this for invalid deltas as we don't want to wipe new deltas or valid updates to existing deltas from the auto-save entry.

This seems an unavoidable consequence of 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active switching from "pure client-side representations stored in auto-save" to "validate le entities stored in auto-save".

But AFAICT #24 does mean that *certain* invalid values would not be recorded in auto-save, which in theory is equivalent to data loss. But … pragmatically speaking, it's A) a tiny bit of information (an invalid delta of a field), B) client-side validation on the entity form likely would've prevented this too. IOW: it affects only "very" invalid data. That seems acceptable 👍

The sole failure is:

    Client Data To Entity Converter (Drupal\Tests\experience_builder\Kernel\ClientDataToEntityConverter)
     ⚠ Convert with 0
     ✔ Convert with 1
    
    1 test triggered 1 PHP warning:
    
    1) /builds/project/experience_builder/web/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php:22
    Undefined array key "value"
    
    Triggered by:
    
    * Drupal\Tests\experience_builder\Kernel\ClientDataToEntityConverterTest::testConvert#0
      /builds/project/experience_builder/tests/src/Kernel/ClientDataToEntityConverterTest.php:94

But oddly, this is happening in this call chain:

1. buildPreviewRenderable(updateAutoSave: FALSE)
2. ClientDataToEntityConverter::convert([..., entity_form_fields: []], validate: FALSE)
3. … which then proceeds to do $original_entity_violations = $entity->validate();, when AFAICT there's absolutely no reason for it, because both entity_form_fields == [] and validate == FALSE 🤔

I think that we can add an early return in ClientDataToEntityConverter::convert() when $entity_form_fields is empty — because then all that was needed is converting the client-side representation of the XB component tree to the server-side representation. Nothing else.

(Until 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active , that method always returned $updated_entity_form_fields, which is why this is much easier to see now.)

Did that, and it seems to work.

P.S.: this seems tangentially related to 📌 [PP-1] Return validation errors from LayoutController::post and ::patch Active .

AFAICT my analysis in #27 was both right and wrong 😅

It was right in that I did spot a genuine bug or at least something that could/should be simplified. I was a bit overzealous with my early return, and in fixing that, the original symptom analyzed in #27 reappeared! 😬

WTF?!

Note that

    $test_node = $this->createTestNode([
      $date_field => [
        [
          'value' => $date->setTimezone($utc)->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT),
        ],
        [
          'value' => $date->setTimezone($utc)->modify('+2 days')->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT),
        ],
      ],
    ]);
    self::assertSame([], self::violationsToArray($test_node->validate()));

passes, but then the next line fails:

$result = \Drupal::classResolver(ApiLayoutController::class)->get($test_node);

Why?!

Because
ApiLayoutController::get() calls
\Drupal\experience_builder\Controller\ApiLayoutController::getFilteredEntityData()
which builds the entity form
and then filters it using \Drupal\experience_builder\Controller\ApiLayoutController::getFilteredEntityData()

The building of the entity form is what introduces "invalid" data, which really is just this: \Drupal\Core\Field\WidgetBase::formMultipleElements()'s

      // Add a new empty item if it doesn't exist yet at this delta.
      if (!isset($items[$delta])) {
        $items->appendItem();
      }

🤪

Yes, it's an empty field item that DateTimeFormatConstraintValidator is crashing on!

Arguably that validator is far too brittle. But also arguably, all this infrastructure should match how entity forms work, and entity forms do not validate empty field items — they use \Drupal\Core\Field\FieldItemListInterface::filterEmptyItems() to strip those away, see \Drupal\Core\Field\WidgetBase::extractFormValues():

      // Let the widget massage the submitted values.
      $values = $this->massageFormValues($values, $form, $form_state);

      // Assign the values and remove the empty ones.
      $items->setValue($values);
      $items->filterEmptyItems();

I hope I got it c🤞

The new failures are now about

-    15 => 'revision_log[0][value]',

… which seems fine, because the user didn't enter anything for those, it was just auto-generated 👍

tedbow → changed the visibility of the branch 3525829-user-without-access-no-clobber to hidden.

tedbow → changed the visibility of the branch 3525829-user-without-access-no-clobber to active.

🎉

Thanks for the updates here folks, the change to remove the extra field.

Cross-linking a core issue about that behavior

@larowlan here it was not unlimited cardinality though — just a specific cardinality (3) with only a subset (2) populated. If cardinality was 7, then not 1 but 5 empty items would've been added!