- Issue created by @larowlan
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π Once previewed in XB an entity with no changes will still show up in "Review x changes" Active already added the essential missing piece: auto-save hashes. See @larowlan's comment at https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....
- π§πͺ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
Right now in the MR I have only worked on getting the 3
\Drupal\Tests\experience_builder\Kernel\ApiLayoutController*Test
classes to passa 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
- the only field the user does not have access to is a boolean field.
- the user knows the name of the field
- 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 valuesMy 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
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 isentity-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
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