Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key_id now part of the encrypted binary to make full use of Ecto.Type behaviour #34

Merged
merged 4 commits into from
May 14, 2020

Conversation

dovadi
Copy link
Collaborator

@dovadi dovadi commented May 12, 2020

I implemented the idea of @moritzploss as he suggested in #33

This solved several issues:

  • double (implicit and explicit) encryption and decryption is unnecessary
  • now we make full use of the Ecto.Type behaviour and we are able to remove the complicated prepare_fields/1 function
  • In the previous implementation key rotation would simple not work. After adding a new key the initial decryption through the load function (as defined as Ecto.Type behaviour) of an already encrypted field would always be done with the latest encryption key, which would cause an error because it was encrypted with the second last encryption key.
  • The original prepare_fields/1 function could not handle parameters with keys as string because of extra merging of the attrs, which is a common practice done in controllers.

… in issue dwyl#33) in order to make full use of the Ecto-Type behaviour and prevent unnecessary double load and dump and to make key rotation really work
@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #34   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines           64        36   -28     
=========================================
- Hits            64        36   -28     
Impacted Files Coverage Δ
lib/encryption/aes.ex 100.00% <100.00%> (ø)
lib/encryption/encrypted_field.ex 100.00% <100.00%> (ø)
lib/encryption/user.ex 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c89ea7...ceec148. Read the comment docs.

@nelsonic nelsonic self-assigned this May 12, 2020
@moritzploss
Copy link
Contributor

moritzploss commented May 14, 2020

Hi @dovadi, great that you implemented the changes! I think this is a much needed improvement. My only concern is that these changes would also need to be reflected in the README. Looking at the issue a couple of weeks ago, I felt that changing the README is a much bigger task than changing the code since it basically requires a complete re-write. But without updating the README it's probably quite confusing. What do others think?

@nelsonic
Copy link
Member

I agree that it's great that @dovadi has made the time to update the code in this tutorial to reflect the suggestion. Ideally the README.md instructions would match the code to avoid confusing people who are following the instructions in the README.md.
But given how much effort/time it will take to update the README.md I would not expect anyone to do it. Also, I would sooner completely re-write the README.md to use Fields rather than instructing people to hand-code Ecto Types.

So with that in mind I'm going to approve/merge the PR and if anyone in the community wants to do "Part Two" of this task (Updating the README.md to match the code) the PR will be very welcome.

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dovadi thanks very much for making the time to make these updates/simplifications to the code. 🎉

@nelsonic nelsonic merged commit 43b5883 into dwyl:master May 14, 2020
@dovadi
Copy link
Collaborator Author

dovadi commented May 15, 2020

Ah I was so focused on the code itself that I totally overlooked the readme.

I feel my job is not finished yet, I will have a go at it. Expect a new PR the coming days ......

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants