Open wout opened 3 years ago
I'm all for it. I think the reason @paulcsmith said it would be easier to make shard than adding it when I was asked for some guidance when making it was that some classes are in Lucky only and would need to be moved to Avram, or a tiny version of it that is used especially for this only. Then it was not sure how they wanted to do it, with the encryption key and all that. So it was easier to have a shard and wait.
So maybe the wait is done? :)
If the Lucky team thinks this needs to wait a bit more, I can build support for more types in Encrypted. Just haven't needed it so much but can add it so more people can use it.
I love the idea. I'm all for it being native in Avram. I think the tricky part here is, what does this look like?
# this email method would require some type casting to Encrypted::String
user = UserQuery.new.email("....").first
# this would be type String?
user.email
If we can find a nice balance to work through the Crystal compiler so the user doesn't always have to do something like email.as(String)
, then we can look at getting this in before 1.0.
Great to hear you're on board. I think this would be a valuable addition to the core.
Good point about the return value. Regardless of the type, we'd need to use a string column type and convert the decrypted value to the desired type:
def from_db!(value : String)
encryptor = ::Lucky::MessageEncryptor.new(Avram.settings.secret)
Avram::Encrypted::String.new(encryptor.verify_and_decrypt(value)).value
end
Or
def from_db!(value : String)
encryptor = ::Lucky::MessageEncryptor.new(Avram.settings.secret)
Avram::Encrypted::Int32.new(encryptor.verify_and_decrypt(value)).value
end
https://github.com/microgit-com/lucky_encrypted/blob/master/src/lucky_encrypted.cr#L47-L50
We'd need to clean this up of course, and probably write a macro. But you get the gist.
@confact How shall we go about this? Do you want to submit a PR? Do you want to work on it together? Shall I submit one?
For me, it's fine either way, but I will need something like Encrypted::Time
, Encrypted::JSON::Any
and Encrypted::Int32
quite soon.
@wout I am fine with either way. I am a bit busy right now so if you want this out soon you could do it. I will just be happy to have it part of Avram :)
The lucky_encrypted shard depends heavily on Lucky and Avram. I'm wondering if it wouldn't have a better place in Avram. It's tiny, literally just one file, and I strongly believe this type of functionality belongs there. Rails 7 will have it (https://github.com/rails/rails/pull/41659), so why not Lucky?
I'd propose to keep it simple and extend functionality when and if required. So starting with something like:
Then usage would be:
That would leave the door open to add more encrypted types later on. Just like Andrew Kane's Lockbox gem allows:
I'd also use a dedicated encryption key for easier key rotation. And key rotation itself is something that we can implement later.