Update entities in batch when field encryption settings change #26

svendecabooter · 2016-03-10T16:23:24Z

This PR fixes the following:

Re-enable the functionality to update fields when the encryption settings get changed, that was disabled in the last PR (WIP - first attempt to store encrypted field values in a dedicated Co… #25)
Implement Batch API to update the entities in groups of 5 after field storage settings are changed.

This has been tested with the following scenario's:

Non-encrypted field gets encryption --> plain text data gets encrypted
Encrypted field (with method X) settings get changed to use method Y --> encrypted data gets re-encrypted with the new encryption method
Encrypted field made unencrypted (checkbox unchecked) --> encrypted data gets stored as plain text

…ncrypt field storage if it wasn't encrypted before.

…updating the field storage settings: - field without encryption becomes encrypted - field with encryption gets encrypted with another encryption profile - field with encryption becomes unencrypted

svendecabooter · 2016-03-10T16:27:52Z

After the update is performed, old values still seem to be in the cache.
Can't quite figure out how to remove them from the cache... normally the node:[NID] cache invalidation gets called on node save, so they should be removed...

nerdstein · 2016-03-10T16:32:50Z

@svendecabooter i am really curious if the Batch API is the way to go. Doesn't this signify a lapse of time between the encryption settings of a field (updated) and the potential data updates needed to process the change? Maybe this is resolved by revisions or something, but I need to understand this a lot more from your point of view

svendecabooter · 2016-03-10T16:54:09Z

I'm not sure I understand the concern exactly.
You mean there would be a problem if entities get created in the meantime?

Upon submitting the form to change the field settings, we query the database to get all entities with this field, and store the original (pre-update) encryption settings.

The batch is then run on each of the entities, (optionally) decrypting with the old settings, and (re)encrypting with the newly saved settings.

While the batch is running, the new settings are already saved, so new entities would be encrypted with the correct settings.

svendecabooter · 2016-03-10T16:59:37Z

I guess there would be a problem if entities get updated manually, while the batch is running...

I think realistically we only have 2 choices:

Work like core: once there is data in the field, the storage settings can no longer be updated. This is the easiest to preserve data integratity, but not very user friendly.
Allow the option to change the settings (and perform encryption / decryption even when data exists). Though this should probably be marked with a big warning about data integrity.
It could be very helpful if you want to encrypt data that was already stored in plaintext earlier, but doing it in a live and changing environment can indeed cause problems, so a decent backup and a controlled environment to perform the updates seem like necessities.

svendecabooter · 2016-03-10T17:06:10Z

Either way this feature is a pretty big game of whack-a-mole, with all the possible variations (multiple languages, multiple revisions, multiple bundles) and encryption / decryption actions that get performed.

Every time I fix a use case, another use case breaks again, so this will need thorough testing, if we can get it into a state where every use case seems to work.

nerdstein · 2016-03-10T18:16:49Z

@svendecabooter understood and I'm comfortable making incremental progress. Let's hop on a hangout and chat.

…translations

svendecabooter · 2016-03-11T10:34:13Z

The latest commits make the code work for 3 scenario's mentioned above (start encrypting existing field, change encryption method on existing field, stop encrypting existing field), tested with the following use cases:

regular entity
entity with revisions
entity with multiple translations

I can start writing some tests to confirm this, if we agree to add this feature.

…ngs for fields with existing data.

nerdstein · 2016-03-12T15:16:44Z

@svendecabooter I gave this a lot more thought and yes the Batch API is probably the most elegant way to get this done. I was confusing myself a bit thinking through all of this, but nothing that wasn't cured with a good night's sleep.

I do think, if possible, we could "lock" records when we process them inside of the batch API. Do you know of any features?

I am not talking about "code" level locking, as demonstrated here: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Lock%21LockBackendInterface.php/group/lock/8

I believe each entity storage has a "locked" attribute (as seen here: https://api.drupal.org/api/drupal/core!modules!field!src!Entity!FieldStorageConfig.php/class/FieldStorageConfig/8).

Maybe it's possible for us to set locked=true for the entire field before processing the batch API records. Or, maybe this is overkill altogether. I would love to hear your thoughts.

Regardless, I want to stress we're on the right track with this. I'm going to review the code in depth and provide any inline comments right now

nerdstein · 2016-03-12T15:17:54Z

field_encrypt.module

@@ -34,6 +33,11 @@ function field_encrypt_form_alter(&$form, FormStateInterface $form_state, $form_
        '#open' => TRUE,
      );

+      // Display a warning about changing field data.
+      if ($form_id == "field_storage_config_edit_form" && $field->hasData()) {
+        $form['field_encrypt']['#prefix'] = '<div class="messages messages--warning">' .t('Warning: changing field encryption settings may cause data corruption!<br />When changing these settings, existing fields will be (re)encrypted in batch according to the new settings. <br />Make sure you have a proper backup, and do not perform this action in an environment where the data will be changing during the batch operation, to avoid data loss.') . '</div>';


nerdstein · 2016-03-12T15:30:23Z

Code reviewed and it looks really good. Minor comments.

One other potential black hole I thought of:

Are we assuming field config changes happen only on production? These settings are in CMI …. would we possibly need the batch API to run if these config changes happen on another environment? E.g. someone updates a YML file w/ field encrypt settings that are then pushed up to another environment

nerdstein · 2016-03-12T15:35:31Z

Black hole comments:

https://pantheon.io/blog/hard-things-are-possible-configuration-management-drupal-8

Config API has some mediation events through event subscribers (it seems). An example: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!ConfigEvents.php/constant/ConfigEvents%3A%3ASAVE/8

Full set of events listed here: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!ConfigEvents.php/class/ConfigEvents/8

In short, it seems we need an EventSubscriber that monitors config changes to FieldSettings. If we see encrypted == true and a change between existing field encryption settings, we invoke our same batch API code (in theory and maybe not in practice).

svendecabooter · 2016-03-13T11:31:32Z

I think this is probably good to be merged.

Followup issues for the remaining outstanding tasks:

Let me know if I missed something

nerdstein · 2016-03-13T15:09:22Z

Merging

Update entities in batch when field encryption settings change

svendecabooter added 4 commits March 10, 2016 09:15

Batch update entities when field_encrypt storage settings are updated.

2cd88b3

Add encryption update support for revisions.

38187d8

Make sure field storage updates reflect language variants as well + e…

fd96bd1

…ncrypt field storage if it wasn't encrypted before.

Refactor processStoredField() to work in the following scenario's of …

9f94b04

…updating the field storage settings: - field without encryption becomes encrypted - field with encryption gets encrypted with another encryption profile - field with encryption becomes unencrypted

Don't save translated entities 2 times.

976ca1f

svendecabooter added 2 commits March 11, 2016 10:24

avoid errors when decrypting

3a867fa

Refactor batch update code to work correctly with entity revisions & …

e38099d

…translations

svendecabooter added 3 commits March 11, 2016 11:45

Add warning about the implications of changing field encryption setti…

a24c75b

…ngs for fields with existing data.

Add support for multi-value fields

a243c38

Update schema and add function tests for encrypting fields.

1abd4c1

nerdstein reviewed Mar 12, 2016
View reviewed changes

This was referenced Mar 12, 2016

Investigate field updates through CMI #27

Closed

Batch Process #11

Closed

Add batch_size settings + add extra comments + clean up code

cb82a55

nerdstein added a commit that referenced this pull request Mar 13, 2016

Merge pull request #26 from svendecabooter/issue-11

0943a38

Update entities in batch when field encryption settings change

nerdstein merged commit 0943a38 into d8-contrib-modules:master Mar 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update entities in batch when field encryption settings change #26

Update entities in batch when field encryption settings change #26

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

nerdstein commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

nerdstein commented Mar 10, 2016

svendecabooter commented Mar 11, 2016

nerdstein commented Mar 12, 2016

nerdstein Mar 12, 2016

nerdstein commented Mar 12, 2016

nerdstein commented Mar 12, 2016

svendecabooter commented Mar 13, 2016

nerdstein commented Mar 13, 2016

Update entities in batch when field encryption settings change #26

Update entities in batch when field encryption settings change #26

Conversation

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

nerdstein commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

svendecabooter commented Mar 10, 2016

nerdstein commented Mar 10, 2016

svendecabooter commented Mar 11, 2016

nerdstein commented Mar 12, 2016

nerdstein Mar 12, 2016

Choose a reason for hiding this comment

nerdstein commented Mar 12, 2016

nerdstein commented Mar 12, 2016

svendecabooter commented Mar 13, 2016

nerdstein commented Mar 13, 2016