Decrypted data not available after saving

I'm afraid this may not work as expected.

It doesn't take in consideration Asymmetrical Cryptography, where the user can encrypt but not decrypt the data.
This is a upstream issue with Encrypt module not declaring enough if it supports asymmetrical encryption as well, when user only has public key.
I've opened an issue about this #2943231: Support for Public-key cryptography.

Actually scrap that. The email message (or any handler) can still use custom text and not adding any of the fields, so there is no reason why we should worry about asymmetrical encryption.

Additionally this patch is using decryptElements(), which may return "[Value Encrypted]" when not able to decrypt if #2943231: Support for Public-key cryptography will be accepted.

However tests are failing so this issue still needs work.

Now looks like all issues is covered.

+++ b/tests/src/Kernel/WebformEncryptSubmissionStorageTest.php
@@ -90,7 +90,9 @@ class WebformEncryptSubmissionStorageTest extends KernelTestBase {
-    $webform_submission->save();
+    $webform_submission->setData(\Drupal::entityTypeManager()
+      ->getStorage("webform_submission")
+      ->encryptElements($webform_submission->getData(), $webform));

Why are we switching from save() to setData()?
How is this testing our changes?

One option to test this issue changes would be to create a custom handler on postSave() and see if it runs and if it can access unencrypted data.
see #2943469: "preSave" elements and handlers called twice for an example.

Can we add test coverage for this so it does not crop up again please.

1. +++ b/src/WebformEncryptSubmissionStorage.php
   @@ -93,8 +93,10 @@ class WebformEncryptSubmissionStorage extends WebformSubmissionStorage {
   +   *   Are permissions should be checked before decryption.
   
   @@ -124,14 +126,16 @@ class WebformEncryptSubmissionStorage extends WebformSubmissionStorage {
   +   *   Are permissions should be checked before decryption.
   

   Grammar

2. +++ b/src/WebformEncryptSubmissionStorage.php
   @@ -156,6 +160,17 @@ class WebformEncryptSubmissionStorage extends WebformSubmissionStorage {
   +    $data = $this->decryptElements($entity, FALSE);
   +    $entity->setData($data);
   

   Can we add a comment here explaining why we want to do this please.

Also, +1 to #12

#2931711: Tracking an elements encryption status got in today, #10 will need to be rerolled.

Great finding from @undertext! Thanks for the patch, it worked for me.

I've now re-rolled the patch (as per #14) and well as I've address the feedback from #12 and #13.
It should be ready for a review and maybe getting pushed.

I've also updated the issue title and description a little bit. :)

Last patch failed because I've undone the @undertext change on the testEncryptDecrypt, pointed on #12, believing it wasn't necessary.
But he was right on changing that because if we use save(), it will go through the postSave() implementation we are adding back and the test will fail as the data in the webform submission object would be decrypted.

Using the setData() allows the test to check the data while it is still encrypted.

New patch. :)

Changing the status and fixing a code standard message from the previous patch.

1. Last patch failed because I've undone the @undertext change on the testEncryptDecrypt, pointed on #12, believing it wasn't necessary.
   But he was right on changing that because if we use save(), it will go through the postSave() implementation we are adding back and the test will fail as the data in the webform submission object would be decrypted.

   I see, however...

   Using the setData() allows the test to check the data while it is still encrypted.

   .
   But if we use setData() instead of save() aren't we kind-of invalidating the meaning of the test?

   Originally the test was checking data integrity/encryption on saving. If we manually set the data instead of following the normal procedure we intentionally skip checking the data is actually encrypted on saving!
   In fact commenting $entity->setData($encrypted_data); on WebformEncryptSubmissionStorage::doPreSave() gives WebformEncryptSubmissionStorageTest::testEncryptDecrypt() green (version from #18). The overall WebformEncryptSubmissionStorageTest kernel test still fails due some serialisation errors, but nothing related to encrypted/unencrypted data.

   I understand there is currently no easy way to retrieve the encrypted data, but maybe we should find one?
   Again: it's probably me being too peaky, but I'm worried if we remove the save() from that test then we still need a way to validate the whole workflow create->preSave->encrypt->store is covered.
   It could be as simple as testDataIsEncryptedOnSaving and we assert data in the db is encrypted...

2. +++ b/src/WebformEncryptSubmissionStorage.php
   @@ -213,10 +215,12 @@ class WebformEncryptSubmissionStorage extends WebformSubmissionStorage {
   +  public function decryptChildren(array &$data, $check_permissions = TRUE) {
   

   $check_permission argument is never used within the function, but it should be used on both decryptChilder() and decrypt() internal calls?

Setting Needs Work due - at least - point 2. I would love your thoughts on 1 though.

Thanks a lot for making progress! It looks really good.

Awesome to see progress on this, thank you @dscl & @gambry

+++ b/tests/src/Kernel/WebformEncryptSubmissionStorageTest.php
@@ -90,7 +90,9 @@ class WebformEncryptSubmissionStorageTest extends KernelTestBase {
-    $webform_submission->save();
+    $webform_submission->setData(\Drupal::entityTypeManager()
+      ->getStorage("webform_submission")
+      ->encryptElements($webform_submission->getData(), $webform));

I agree with #19.1 - we are loosing very critical test coverage here for this module, and I wonder if this is actually highlighting that we should find a different way to do this...

@Manuel Garcia I was thinking to swap WebformSubmission entity with our custom alteration implementing a private $unencryptedData and getUnencryptedData(); setUnencryptedData();, so we still can easily access the unencrypted data when we need... Thoughts?

@gambry I'm not sure we want to take on the burden of maintaining our own entity to be honest...

Would $webform_sumission->getOriginalData(); be of any use to us here?

Hey guys,

@gambry, I understand your concern about checking on data that isn't actually saved to the database. I agree it ins't testing if the encrypted data got saved to the database in the end, but given that the data gets encrypted on the $webform_sumission->setData() and it will get decrypted by the $webform_sumission->postSave() we are adding now, if we continue to use the $webform_sumission->save() we won't ever be sure if the data got really encrypted in the first place...

I can work on getting the (2) of #19 done.
For (1), if the Webform Encrypt is not decrypting data that comes through $webform_sumission->getOriginalData(), mentioned by @manuel-garcia on #22, then this should be our best option to gain access to the encrypted data.

So we could go back to $webform_subsmission->save() and use $webform_sumission->getOriginalData() to get the data to be used in the test.

Let me know your thoughts...
I can try it tomorrow. :)

Crossing my fingers for $webform_sumission->getOriginalData()...

If that doesn't work, we could either:

1. Swap in our entity class extending WebformSubmission as @gambry suggested on #21
2. Check the data from the DB directly on the test

I feel like 1. would be cleaner, though I'd like to see how it would look like, hopefully it won't add too much more complexity to maintain.

For now, if we could check if $webform_sumission->getOriginalData() returns encrypted or unencrypted data that would be great :)

I'm happy with whatever suggested on #23 and #25. I thought myself about getOriginalData(), but then I stopped mainly because I don't know what the meaning of that would be.
If - for example - the aim for original data is for contrib modules to compare and react when $data !== $original_data - then we can't use it because they will always be different.

If however the aim is to have in hands the raw data coming from the database "just in case we need it" then maybe we can use it.

Hey All,
There is progress, but not really a solution, and maybe a red flag.

The thing is that using $webform_sumission->getOriginalData() didn't return the encrypted data.
In fact, I noticed the $webform_sumission->originalData is actually a different attribute that never gets encrypted.

Given the effort we do on checking permissions before populating the Webform Submission form with decrypted data, I would say we actually *need* to encrypt the $webform_sumission->originalData too. Otherwise, the decrypted data would still be available through $webform_sumission->getOriginalData().
But then we will need to treat it the same way we treat $webform_sumission->data.

In the end, we would need to consider the last suggestions on #25, but I would vote for the (2) as I would prefer to simply query the database instead of creating the technical debt of maintaining a custom WebformSubmission that wouldn't be used anywhere else in the near future.

We can still leave the $webform_sumission->setData() as the test case is meant to test the Encryption and Decryption of the data, and not if the data gets saved.

/**
  * Test encryption and decryption.
  */
public function testEncryptDecrypt() {

Please share your thoughts.

Sorry @gambry, I left it open for too long and didn't see your comment before.

One thing that I noticed is that Webform uses the $webform_sumission->getOriginalData() internally only for deleting old files for the manage_file field (WebformManagedFileBase.php - postSave() implementation).
What I understood from this was that $webform_sumission->originalData holds the data that was saved in the submission before the last save...
So yeah, the data there would be an old version of the submission anyway.

Hey guys,

Following up on this issue, I'm submitting a new patch that fixes the point 2 on #19, but I'm leaving the $webform_submission->setData() call on the testEncryptDecrypt() test.

My reason for this is that I believe the test is supposed to test the ability to encrypt WebformSubmission data, which in my understanding is clearly tested in the moment that we are able to successfully set the WebformSubmission data with the encrypted values and get them afterwards without getting corrupted data.
I don't believe we need to save it in as part of the test, even because the $webform_submission->save() is getting tested as part of the test that I'm introducing which tests the access to decrypted data post save.

Testing the $webform_submission->save() on the testEncryptDecrypt() test would actually actual required the assertion to use the plain data since the postSave() would decrypt it, and it wouldn't ensure the data was even encrypted in the first place since both encryption and decryption would happen in the same flow without any test if they were successfully executed.

Hopefully you guys will agree with me on this one. :)

Cheers!

Just an interdiff so its easier to review.

I have had a bit of a thought here, and although we do loose a bit of coverage here, we are testing the actual saving process on WebformEncryptFunctionalTest so it would not be the end of the world...

+++ b/src/WebformEncryptSubmissionStorage.php
@@ -248,6 +252,19 @@ class WebformEncryptSubmissionStorage extends WebformSubmissionStorage {
+  protected function doPostSave(EntityInterface $entity, $update) {
...
+    /** @var \Drupal\webform\WebformSubmissionInterface $entity */
+    parent::doPostSave($entity, $update);

Would type hinting here to WebformSubmissionInterface $webform_submission be a better option?

Sure thing!

But then I would suggest to change the doPreSave() too for the sake of consistency.
I'm adding it to the patch too.

Ops! Read interdiff-29-34 in the previous comment... :)

Well, I didn't think through before saying yes and implementing it, but we need the doPreSave() and doPostSave() to be compatible with the EntityInterface . And that is why we have /** @var \Drupal\webform\WebformSubmissionInterface $entity */.

Moved it to the top of doPostSave(), and reverted the WebformSubmissionInterface $webform_submission changes.

Ignore #34. Adding the interdiff between #29 and #37.

Right of course @dscl -- should have checked myself... sorry!

Personally I think there's a trade off to be made here, but I'd like to get sign off from the other maintainers etc

This seems to be blocking #2983521: Add test coverage for multistep forms

Had a look through this and hopefully I haven't missed anything glaringly obvious - seems all sensible!

Appears to apply fine against the latest patch in #2983521: Add test coverage for multistep forms and looks to fix the problem we're having over there.
If we can get this in we'll have unblocked that and be a good step closer to 1.0 I think.

I am happy with this to getting committed as it allows multistep forms to function correctly without loss of data.

Pushed to 8.x-1.x

Hi,

I was testing a data encryption issue and got the opportunity to test the below given patch.

webform_encrypt-access_data_post_save-2934368-37-8.patch

But it seems this patch duplicates (doPostSave) function which already exists in WebformEncryptSubmissionStorage.php. Does anyone succeeded with the provided patch?

Appreciate your help.

Thanks,
R. Janakiraman.

@rjraman there is no need to apply this patch, it is already included in 8.x-1.0-alpha3.

Check which version of the module you are using. If you are in an earlier version, then please upgrade to the latest version.

https://www.drupal.org/project/webform_encrypt/releases/8.x-1.0-alpha3