search

trustedlogin / trustedlogin-vendor

Plugin to interact with TrustedLogin's encrypted storage infrastructure to redirect support staff into an authenticated session on client installations.
GNU General Public License v3.0
0 stars 1 forks source link

Development Branch to Master #11

Closed inztinkt closed 4 years ago

inztinkt commented 5 years ago

Also added:

inztinkt commented 5 years ago

I don't like how the Keys and the Encryption class are all interconnected. There should be a separation of concerns.

There should be a Keys class that handles the logic of the keys and an encryption class that handles the encrypt/decrypt.

The keys class can handle CRUD of the keys, and the keys class can be passed the Encryption class when instantiating it.

@zackkatz My thinking here is that having a singular class for handling anything to do with encryption (including handling the keys) would be the right level of abstraction, as there's no other classes that would need these key management CRUD/getter functions.

By keeping them in one class, that TrustedLogin_Encryption object is what handles the whole encryption flow and can be maintained, swapped out and overridden pretty easily by ourselves or 3rd party developers in the future.

Happy to abstract it out if you disagree.

zackkatz commented 5 years ago

Happy to abstract it out if you disagree.

Abstracting into two classes might help clarify what needs to be passed to which methods. I find it abstraction useful when a class gets more complicated than it should be.

I'm okay to stick with a single class, but the class needs to get cleaner. @inztinkt

inztinkt commented 5 years ago

I'm okay to stick with a single class, but the class needs to get cleaner. @inztinkt

I've tidied up the TrustedLogin_Encryption class and removed extra steps with setting up keys.

Right now no keys are created until the first site clicks the TrustedLogin button. @zackkatz should we move this to an installation step on vendor side? This will also allow us to test and warn site admins about not having openssl enabled.