Added re-encryption functionality for env.php

[davekleijn]

Description

This PR adds functionality to re-encrypt values in the env.php configuration file.

Issue

Fixes #12

[convenient]

Hello

Thank you for your PR.

So this implementation would attempt to fix up the env.php at this point in the process

https://github.com/magento/magento2/blob/11e7d89380c3ef9b9bd18c1d8b7815b0fad96e34/app/code/Magento/EncryptionKey/Model/ResourceModel/Key/Change.php#L136

I think the sequencing of this may need a bit of tweaking, possibly do this custom handling after the changeEncryptionKey function has successfully completed?

https://github.com/magento/magento2/blob/11e7d89380c3ef9b9bd18c1d8b7815b0fad96e34/app/code/Magento/EncryptionKey/Model/ResourceModel/Key/Change.php#L136-L139

It would not be ideal for an error in _reEncryptCreditCardNumbers to cause a database rollback, including preventing updating the crypt/key in env.php at this point. As with this change in place in its current form we would have moved along all the other encrypted data in env.php but they will no longer have a corresponding key.

Perhaps we can follow the same (ugly) pattern as the rest of the functions in this class and extend changeEncryptionKey and then inject our additional custom handling afterwards? Or alternatively make it part of the console command itself 🤔

[convenient]

Please see above comment, I just realised I left it as a comment and not a "request changes" sorry.

[davekleijn]

Hi @convenient,

Thank you for your feedback. I already tried this, but was getting some strange issue. Probably because I was changing the env.php multiple times and was manually changing the file during testing. Now it is working.

I am also not happy with adding a __construct function, because it requires a lot of extra dependencies. It was needed for DeploymentConfig. I see that the \Magento\EncryptionKey\Model\ResourceModel\Key\Change::changeEncryptionKey method is public. Is it an idea to create an after plugin for this?

[convenient]

Hey @davekleijn

If a plugin achieved this then that's sensible 👍 would it be possible to make the plugin scoped only to our custom class to leave the vanilla Magento controller action untouched?

[convenient]

@davekleijn Or alternatively create a new service class who's whole responsibility is this functionality. Then you can inject that into the console command and call it in the same way you have done.

That way we can avoid fiddling too much more with the class we're extending?

That would have a nicer and more predictable callstack than a plugin

[convenient]

Hello @davekleijn

I've been testing your code in here #31

Can you please have a look at that and let me know what you think?

Thanks

[davekleijn]

Hi @convenient,

Sorry, couldn't finish the code yesterday. The wrong version 0:3 was used for re-encryption, but I see that you have fixed it now.

Everything looks good I think.

[convenient]

Hey @davekleijn no worries. I spotted that issue pretty quick while scaffolding a test so just fixed it.

Have you had the chance to pull it down locally and verify it meets your expectations?

[davekleijn]

Hi @convenient

Yes, I also tested this with some real data from projects. It all looks good.

[convenient]

Thank you @davekleijn i'll get this merged and tagged soon

[convenient]

Thank you for your contribution