PCI Compliance : Database storage

Note that this module should *never* store the credit card number (PAN).

It is designed to work together with credit card *aliases*, which are only valid in combination with the credentials for your payment provider and only allow transactions for your own bank account.

According to the linked document, expiration dates, names and so on must only be encrypted if stored together with the PAN.

If you have suggestions on how to implement (possibly optional?) encryption, that is welcome but I think it is a) not critical and b) always poses the problem of where the encryption key should be stored. It would have to be not persisted at all or at least persisted somewhere outside of the database to be useful. Suggestions?

We could consider an optional dependency on one of the available encryption modules and say, if module X is installed and correctly configured, we will automatically encrypt the data.

About the data structure, we could have an additional "encrypted" column and put things either there (serialized then encrypted) if we can encrypt and NULL the other columns. Or just use an additional column to flag if the data is encrypted and keep storing it in the same columns.

This would need to happen transparently in the save/load wrappers, would imply that direct queries are no longer supported and is probably another reason to go with entities instead of the current arbitrary array data structure.

There are issues about making it entities already. One that does it and another that adds custom hooks.

However, it would make more sense for this to live in the default entity storage controller because otherwise a module would have to extend/alter the database table and I'm not a fan of such things :)

If possible, feel free to ignore the entity topic. If you want to push that or if you think it's necessary to come up with an API that makes sense, feel free to work on that first.

Thanks for working on this.

+++ b/commerce_cardonfile.installundefined
@@ -81,6 +81,11 @@ function commerce_cardonfile_schema() {
+        'description' => 'An encrypted copy of potentially senstive card information.',

Not sure about using "copy" here, I'd just say something like "The encrypted card data if encryption is enabled."

+++ b/commerce_cardonfile.moduleundefined
@@ -388,9 +398,25 @@ function commerce_cardonfile_data_load_multiple($uid, $instance_id = NULL, $acti
+    $new_set = array();
+    foreach ($data_set as $index => $card_data) {
+      // If the encryption feature fails or was turned on after other cards were processed
+      // we may get a non-encrypted card. If so, detect it and avoid overwriting data.
+      if (!empty($card_data['encrypted'])) {
+        $decrypted_data = unserialize(decrypt($card_data['encrypted']));
+        $card_data = array_merge($card_data, $decrypted_data);
+        unset($card_data['encrypted']);
+        $new_set[ $card_data['card_id'] ] = $card_data;
+      }
+    }
+    $data_set = $new_set;

Do we really need the $new_set variable here? Should be possible to just replace the rows in the existing array?

Also, there shouldn't be spaces in "$new_set[ $card_data['card_id'] ]".

Also, while it might might be a bit more code, I would suggest to explicitly reference the encrypted properties just like you do when encrypting.

You could add a helper function do that, to avoid having to duplicate the code.

+++ b/includes/commerce_cardonfile.admin.incundefined
@@ -27,8 +27,8 @@ function commerce_cardonfile_settings_form($form, &$form_state) {
-      'radios' => t('Radio buttons (e.g. Visa belonging to John Doe: Ends in 1111, Expires 05/2015)'),
-      'select' => t('Select list (e.g. Visa ending in 1111, Exp. 05/2015)'),
+      'radios' => t('Radio buttons (e.g., Visa belonging to John Doe: Ends in 1111, Expires 05/2015)'),
+      'select' => t('Select list (e.g., Visa ending in 1111, Exp. 05/2015)'),

This change looks unrelated. Try to avoid unecessary changes in patches, as this increases the chance of conflicts and makes it harder to review them.

+++ b/includes/commerce_cardonfile.admin.incundefined
@@ -45,5 +45,13 @@ function commerce_cardonfile_settings_form($form, &$form_state) {
+  if (module_exists('encrypt')) {
+    $form['commerce_cardonfile_enable_encryption'] = array(
+      '#type' => 'checkbox',
+      '#title' => t('Enable additional encryption to encrypt data stored in the database. This feature depends on the Encryption module'),
+      '#default_value' => variable_get('commerce_cardonfile_enable_encryption', FALSE),
+    );

- Looks like this is missing a . at the end of the second description sentence.
- I'd also add a note that the encryption module must not be removed once enabled or it will not be possible to use the card data anymore. Maybe even throw a warning/error on load if the data is encrypted but the feature is disabled.

- Which means that to be able to actually allow to disable it, we need decrypt all data when this is done. And when we do that, we could also do the same when enabling it.

- Last (small) point, we could add a message if that module is not available and say that the data can be encrypted if you install that module, possibly with a link to the d.o page.

+++ b/includes/commerce_cardonfile.pages.incundefined
@@ -90,7 +90,7 @@ function commerce_cardonfile_update_form($form, &$form_state, $card_data) {
-  module_load_include ('inc', 'commerce_payment', 'includes/commerce_payment.credit_card');
+  module_load_include('inc', 'commerce_payment', 'includes/commerce_payment.credit_card');
   // Ensure non-default values are valid numbers.

Another unrelated change. I suggest you open a separate issue to fix coding style issues.

Good work, some feedback, mostly coding style stuff.

+++ b/commerce_cardonfile.installundefined
@@ -116,3 +121,15 @@ function commerce_cardonfile_schema() {
+ * Adds a new field to the schema, 'encrypted'.

Typically, the table name is used with {} instead of "schema", something like "Add the encrypted field to {commerce_card_data}."

+++ b/commerce_cardonfile.installundefined
@@ -116,3 +121,15 @@ function commerce_cardonfile_schema() {
+  // Add a new field, define in the hook to avoid potential issues in the future.

I don't understand what you are saying with that comment. Might not be necessary at all.

+++ b/commerce_cardonfile.installundefined
@@ -116,3 +121,15 @@ function commerce_cardonfile_schema() {
+  $field['encrypted'] = array(
+    'description' => 'The encrypted card data if encryption is enabled.',
+    'type' => 'blob',
+  );
+  db_add_field('commerce_card_data', 'encrypted', $field['encrypted']);

The default value seems to be missing here.

+++ b/commerce_cardonfile.moduleundefined
@@ -351,11 +351,26 @@ function commerce_cardonfile_options_list($stored_cards, $element = 'radios', $d
+  if (module_exists('encrypt')) {
+    // If the encryption feature fails or was turned on after other cards were processed
+    // we may get a non-encrypted card. If so, detect it and avoid overwriting data.
+    if (!empty($card_data['encrypted'])) {

Comment is > 80 characters.

I think you lost the check for the variable now.

+++ b/commerce_cardonfile.moduleundefined
@@ -445,3 +495,17 @@ function commerce_cardonfile_data_delete($card_id) {
+ * Helper function, encrypt or decrypt all card data in the database according to settings.

The "Helper function," part is not necessary. And without that, it's also not too long anymore :)

+++ b/commerce_cardonfile.moduleundefined
@@ -445,3 +495,17 @@ function commerce_cardonfile_data_delete($card_id) {
+function commerce_cardonfile_crypt_all_cards() {
+  $cards = db_query('SELECT card_id FROM {commerce_card_data}');
+  foreach ($cards as $card) {
+    // Loading the card at all while the Encrypt module exists gets the card data
+    // if any form of encrypted data is in place. Once saved, the card will be in the right format
+    // for the current configuration.
+    $this_card = commerce_cardonfile_data_load($card->card_id);
+    commerce_cardonfile_data_save($this_card);

Not sure to how many credit cards this scales. Might need a batch function but people will usually not enable it when there are already hundreds/thousands of records. So I think this is fine for now.

Ah right, that comment would then IMHO not really be necessary as this is the default behavior, it should always be done like that.

About the setting, I'm quite sure that it currently does not work correctly anymore. The data will always be encrypted now if the module is enabled, the setting doesn't matter. I'm quite sure that disabling it in the UI will not decrypt your data, why would it?

The setting check is necessary at least for the encryption. The decryption should possibly not rely on it and use the encrypted data if it's available but not when encrypting.

Sorry, missed that. Yes, looks good.

Want to do some manual tests but this looks good to me from looking at the code.

Thanks for testing, yes, sounds like the schema definition needs to be updated.

I have confirmed that the encryption is not required in our case (storing only 4 numbers of the card + other details).
Therefore I don't see a point in introducing complexity for this feature, either we do it everyone, or we don't do it at all, I don't see a point in
having it optional.

Marking as won't fix, and people who want this can swap out the entity controller (or even implement the load / presave hooks) and encrypt / decrypt there.