search

genecommerce / module-encryption-key-manager

Tooling to help generate and invalidate magento encryption keys
GNU Lesser General Public License v3.0
54 stars 14 forks source link

Added re-encryption functionality for env.php #30

Closed davekleijn closed 1 month ago

davekleijn commented 1 month ago

Description

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

Issue

Fixes #12

davekleijn commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

Hello @davekleijn

I've been testing your code in here https://github.com/genecommerce/module-encryption-key-manager/pull/31

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

Thanks

davekleijn commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Hi @convenient

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

convenient commented 1 month ago

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

convenient commented 1 month ago

Thank you for your contribution