Add rudimentary conflict prevention to the preview end-point

Created on 29 November 2024, 6 months ago

Overview

Currently we post to /api/preview/{entity_type}/{entity} when the model changes and we need to udpate the preview.
This saves to the auto-save without discrimination.
It is possible that two people may be editing the same entity.
At present each user's changes clobber the other user.

Proposed resolution

Keep a hash or version ID for each auto-save record and fail if the incoming hash doesn't match the currently stored value? When it fails show the user a 'your changes cannot be saved, click to reload latest version' (for now).

In a future version we'll need some sort of conflict resolution

User interface changes

πŸ“Œ Task
Status

Active

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

Merge Requests

Comments & Activities

  • Issue created by @larowlan
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    … which AFAICT would not need everything written up in the issue summary β€” because an even more rudimentary implementation is now possible.

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

    The auto save hashes are at the whole entity level. But we need them at the component level

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    re #4 and #5
    I chatted with @lauriii, @effulgentsia and @balintbrews about this on yesterday about this.

    The decision was that as first pass we should only check that the user has the latest version of auto-save, probably by comparing that last known auto-save.

    so re #5

    The auto save hashes are at the whole entity level. But we need them at the component level

    I agree this is not an as ideal as what you have suggested, it still would allow us to add more advanced conflict detection later.

  • Assigned to larowlan
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    working on this

  • Merge request !1059#3490565 Basic conflict detection β†’ (Open) created by tedbow
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    Right now in the MR I have only worked on getting the 3 \Drupal\Tests\experience_builder\Kernel\ApiLayoutController*Test classes to pass

    a bunch of other things should break because they are not sending the new 'autoSaves' key back to the server. Probably all of the e2e tests should fail

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    This is still very rough. I will pick it back up tomorrow

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    I am wondering if there will be any security concerns with sending the actual hash of the auto-save to the client.

    Right now I don't think there are because we pretty much send the contents of the auto save to the client via \Drupal\experience_builder\Controller\ApiLayoutController::get anyways, so having the hash is not giving any more information.

    but after πŸ› If user with less/different permissions edits a page after a user with more/different permissions data lost could occur Active we may store fields in the auto-save that user does not view or edit access to. We will then probably fitler what's in the auto-save before we send it to the client.

    In that case imagine this scenario

    1. the only field the user does not have access to is a boolean field.
    2. the user knows the name of the field
    3. the knowledge from the source code the hash is actually of whole auto-save including the field they don't have access to.

    Would it be possible for the user to reconstruct the complete auto-save data from the information they have andgenerate the has themselves, 1 time with the boolean field FALSE and 1 time with the boolean field TRUE and then just check which one matches the hash they were sent?
    I know it might be an edge case but this could also work with any field that has limited number of possible values

    My other thought is to just send a auto-save version number.

    Initially in this issue @larowlan suggested

    For each item in the model, keep a version number, initialize this to 1 when we first write to the auto-save entry for that entity.

    I am not sure he was considering this or he just suggested using a version number because of security concerns or just he was referring to sub-items of the auto-save we don't have hashes already for.

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    To address #12 I am just going to use UUID that will get saved with auto-save each time.

    I didn't want to use an auto-incremented number because a malicious user could just guess the current ones to purposefully wipe of another users changes and save there's.

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    re #12 and #13 I actually realized that modules/contrib/experience_builder/src/Controller/ApiAutoSaveController.php:107 already returns hash of auto-save entries so this is not new problem.

    So I have reverted back to using the hash

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    I think this could probably use some other eyes on before I work more on it.

    So far it is the back-end implementation I mentioned in #6.

    I gave a first pass(with AI help at the front). The front-end changes work with the caveat I mention in the MR about rapid fire requests.
    The last test that is failing for me locally is entity-form-field-types-test.cy.js when the test loop through many different fields, so I am pretty sure this is because of the same problem. I could probably get the tests passing with the correct waiting on requests. I have fixed a few test this way in this issue already.

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    so I don't forget

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    To address @larowlan's feedback I have made this a backend only issue and follow-up handle the Frontend work. I create πŸ“Œ [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed as follow-up

    In that issue I pointed to the commits that remove the /ui if the person tackling that wants to start from my work

Production build 0.71.5 2024