Exporting endpoints with Features exposes password on file system

Please find the updated patch

rectifying errors

updated with few more fixes

Tested this patch. The patch seems to work fine with both the current dev version and current release.
Please rollout this patch in the next release.

Thanks for the patch! - I am not entirely sure this is the best solution for this problem. We could look into using key module, but I doubt we've got the momentum on the 7.x branch to do such a thing. So for now here's a CS review:

1. +++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_endpoint.class.php
   @@ -145,7 +145,12 @@ class deploy_ui_endpoint extends ctools_export_ui {
   +        $item->authenticator_config = $form_state['values']['authenticator_config'];
   

   Indentation is off.

2. +++ b/plugins/DeployAuthenticatorSession.inc
   @@ -26,6 +26,13 @@ class DeployAuthenticatorSession implements DeployAuthenticator {
   +    ¶
   

   Added spaces on empty line.

3. +++ b/deploy.module
   @@ -792,3 +792,31 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
   + *  @return boolean
   

   @return should leave an empty line before it.

+++ b/deploy.install
@@ -400,3 +400,13 @@ function deploy_update_7003(&$sandbox) {
+  if(_deploy_encrypt_endpoints()) {

+++ b/deploy.module
@@ -792,3 +792,31 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
+  if(module_exists('encrypt')) {
...
+      if(!empty($endpoint->authenticator_config['username']) && !empty($endpoint->authenticator_config['password'])) {

+++ b/plugins/DeployAuthenticatorSession.inc
@@ -26,6 +26,13 @@ class DeployAuthenticatorSession implements DeployAuthenticator {
+      if(!empty($config['username']) && !empty($config['password'])) {

Missing space after if statement

@manuel - Fixed the CS issues. Thanks! :)

Thanks @prashantgajare for that, patch is looking good, however I'd like more reviews, and other maintainers' signoff on this before we commit it.

@rogerpfaff The password will remain the same , but in database the encrypted password will get store

1. +++ b/deploy.install
   @@ -400,3 +400,13 @@ function deploy_update_7003(&$sandbox) {
   +  return t("Nothing to update as encrypt module is not there.");
   

   I don't think this message is clear enough. Encrypt module is not where? Also encrypt should have a capital E as it's the name of a module.

   Maybe something like "Updated skipped: This update only applies when using the Encrypt module."?

2. +++ b/deploy.module
   @@ -792,3 +792,33 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
   +
   

   We don't need this whitespace before the if.

+++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_endpoint.class.php
@@ -146,6 +146,11 @@ class deploy_ui_endpoint extends ctools_export_ui {
+        $item->authenticator_config =  $encrypt;

Too many spaces after =.

Fixing interdiff comments

Looks like the test bot blew up, lets try again...

Fixing the Interdiff #23

1. +++ b/deploy.install
   @@ -400,3 +401,13 @@ function deploy_update_7003(&$sandbox) {
   +function deploy_update_7004() {
   
   +++ b/deploy.module
   @@ -792,3 +793,33 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
   + * Implements hook_modules_enabled().
   ...
   +function deploy_modules_enabled($modules) {
   

   Probably should update hook_install for the new sites as hook_modules_enabled() might not cover few scenarios like:

   1. encrypt module is already enabled and installing deploy for the first time: Since the encrypt already enabled, deploy_modules_enabled() wouldn't do anything and hook_update_N wouldn't run on install.

2. +++ b/modules/deploy_ui/deploy_ui.module
   @@ -1,5 +1,9 @@
   +  * @file
   

   @file description is missing.

3. +++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_plan.class.php
   @@ -396,6 +398,7 @@ class deploy_ui_plan extends ctools_export_ui {
   +  ¶
   

   unnecessary empty spaces.

4. One final concern is, it is worth to maintain the state of the encryption somewhere in a variable for example. This we don't just check if the encrypt module enabled or not. This would help to assure that the multiple installs/uninstalls of the encrypt module wouldn't cause any problem.

@Vijay, Thanks for feedback I have implemented 1st 3 concerns

Thanks, @harshil.maradiya. Please make sure to provide interdiff next time which would helps a lot to review. I still see the empty line with space (see attached dreditor UI). It's trivial though. So if you reroll the patch for any other fix make sure to include it.

I still think #27.4 is a valid item to fix. Here is a sample scenario:
1. Install & enable encryption module -> Would trigger deploy_modules_enabled() and does the encription of username (e.g. foo to bar) and password.
2. Disable & uninstall encryption module
3. Enable encryption module again -> Now the deploy_modules_enabled() would double encrypt the username (i.e. bar to baz)

Dear Vijay,
Thanks for detailed explanation i have made required changes

Dear Vijay,

Please find updated patch and interdiff

It looks good to me. Setting to RTBC to see if we can get a maintainer review.

+++ b/deploy.module
@@ -792,3 +793,35 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
+/**
+ * Implements hook_modules_enabled().
+ */
+function deploy_modules_enabled($modules) {
+  if (in_array('encrypt', $modules)) {
+    _deploy_encrypt_endpoints();
+  }
+}

We should also decrypt the endpoints' username and password in the case that encrypt module is being disabled, otherwise sites will end up with unusable endpoints credentials.

1. +    if (module_exists('encrypt')) {
   +      // Remove defaulr php encryption state.
   +      if (_get_deploy_php_encryption_state()) {
   +        variable_del('deploy_php_encryption_state');

   not sure if it's a good idea to delete the variable every time. why can't we set it to false?

2. if (!variable_get('deploy_php_encryption_state')) {
   Use FALSE as default value here?

Looks good to me. Though I feel the introduction of new functions making the code bit complicated, I don't have any strong objection. +1 to RTBC.

Thanks for following up on this. I believe at least #34 needs to be addressed still.

#40: Its going to be very tricky because hook_modules_disabled and hook_modules_uninstalled are executed aftera module being disabled/unistalled. So by that time when we disable/uninstall encrypt, our data is already in a deadlock as the function decrypt doesn't exist anymore.

Removing enable hook so it could not end up in double encryption

Thanks @harshil.maradiya - looks like the last interdiff didnt come out correct, would you mind providing a valid one to facilitate reviews please?

Thanks, uploading correct inter diff

looks good to go.

The patch would now prevent double encryption, thanks for that.

However the problem described on #34 is still valid:
1. You enable deploy while having encrypt enabled, or you run this update hook while having encrypt enabled, so the endpoints get encrypted.
2. Later you disable encrypt module, and since its not a dependency of deploy, we cannot prevent this as far as I know.
3. Result: your endpoints are still encrypted, you have no way of decrypting them, and your enpdoints are unusable.

I have not tested this scenario manually but by reading the code I believe this would be the case.

#47: True, but I don't see any way we could prevent this as explained in #41. Even though it is possible in Drupal 8, seems it causes more issues. see #2899478: Uninstall validator makes uninstall impossible in some circumstances.

@Manuel Garcia, I am open for any alternative solution here.

Patch looks good generally speaking. I'm ok with being pragmatic and ignoring the issue raised in #34 if no one can think of a solution :)

If some does want to fix it, here's one approach:

if (module_enabled('encrypt') {
  variable_set('encrypt_module_was_enabled', TRUE);
}
[...]
if (variable_get('encrypt_module_was_enabled') && !module_enabled('encrypt')) {
  drupal_set_message('Your endpoints are broken');
}

Keeping this issue as "Needs work" because this namespacing issue should be fixed:

+/**
+ * Set default php encryption state.
+ */
+function _set_deploy_php_encryption_state() {
+  if (!variable_get('deploy_php_encryption_state', FALSE)) {
+    variable_set('deploy_php_encryption_state', TRUE);
+  }
+}
+
+/**
+ * Get default php encryption state.
+ */
+function _get_deploy_php_encryption_state() {
+  if (variable_get('deploy_php_encryption_state')) {
+    return TRUE;
+  }
+  return FALSE;
+}

Thanks, @dixon_. Here is an update:

If some does want to fix it, here's one approach:

FIXED: thanks, added. now both _deploy_encrypt and _deploy_decrypt would check for this variable before fallback to base64.

this namespacing issue should be fixed

FIXED.

committed

@vijaycs85 Can you verify all looks good, then we can do a release next week?

Crediting all the people.