d8-contrib-modules / field_encrypt

CODE HAS BEEN MOVED TO https://www.drupal.org/project/field_encrypt
0 stars 5 forks source link

WIP - first attempt to store encrypted field values in a dedicated Co… #25

Closed svendecabooter closed 8 years ago

svendecabooter commented 8 years ago

…ntentEntity

As mentioned on IRC, this is some WIP code that tries to implement the approach laid out in https://github.com/d8-contrib-modules/field_encrypt/issues/24.

Upon the first save of a node, the fields don't actually get encrypted and will return empty. But when you update the node, things work roughly: the real field data is set to [ENCRYPTED] currently (still need to figure out why NULL or '' doesn't work) - upon loading the entity, this is dynamically replaced with the decrypted value from the EncryptedFieldValue entity.

The batch encrypting / decypting of values when field storage settings are changed, is also pretty broken due to these updates. To be fixed once the general approach works well.

svendecabooter commented 8 years ago

Code is now fixed for inserts, so encryption / decryption of text field works.

Currently we still have to set a value for the storage of the unencrypted field (in current code this is set to "[ENCRYPTED]"). If we don't do this, the call to $field->getValue() in FieldEncryptProcessEntities processField() will return an empty array when decrypting, when it should be an array containing the properties. i.e. we get: $field_value = []; instead of: $field_value = [ 'value' => '', 'summary => '', 'format => 'basic_html', ]

We could try to recreate this array by getting a default value for the field or loading the correct properties, but there will still be a problem with properties that didn't get encrypted.

In the case above, when encrypting a text_with_summary field with format "full_html" and not setting any value for the unencrypted storage, we could retrieve the encypted text value, but the format setting would be gone, since no database entry was saved in the unencrypted field storage table.

So from what I can see so far, we'd have to set some placeholder value for things to keep working. Maybe an alternative approach could fix this, but I can't see how to do this right now.

Still TODO:

svendecabooter commented 8 years ago

The latest commits fix the following TODOs:

(You'll have to reinstall the module for this to work)

I'd like to get some feedback before I tackle the following TODO:

nerdstein commented 8 years ago

I have finished reviewing it thus far and have left comments. Overall, it's great.

Please review my comments. I can merge whenever you would like (if you think we need to get the code in and iterate), or I can merge after you update from my comments.

I will say there is a lot of code commented out. I would really like to see "refactor" lines done before we merge, if possible.

svendecabooter commented 8 years ago

Functionality added in the latest commit:

Better suggestions for naming then the ones above are always welcome :)

nerdstein commented 8 years ago

Looks good. I'm merging and hoping we can continue with the remaining effort.

I think we very quickly need to get into the caching systems