Add new type encrypted_string for preferences #3676
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@stefano-sarioli thanks for this PR. I left a few comments, let me know what you think about them. |
|
@spaghetticode Hi, I've pushed my fix to your comment, thanks again for the feedback. |
jarednorman
requested changes
Aug 10, 2020
backend/app/views/spree/admin/shared/preference_fields/_encrypted_string.html.erb
Outdated
Show resolved
Hide resolved
added 4 commits
August 10, 2020 21:54
Encryptor is a thin wrapper around ActiveSupport::MessageEncryptor, it can be used when a simple encryption solution is needed. Encryptor does not have key management features, you have to generate, store, and rotate key by hand, if needed.
The new type encrypted_string encrypt the content of the preferences, the original value can be read-only using the preferences accessor method. It's still recommended to use env variables to manage secrets, but if it's needed to save the preference on the database, now it's possible to use encrypted_string to add a security layer. The encryption key can be passed using the encryption_key option of the preference, if the option is missing the system fallback to the env variable SOLIDUS_PREFERENCES_MASTER_KEY, it even this env variable is missing the system will use the Rails master key. If a default value it's assigned to an encrypted_string preference, this will not be encrypted.
On the backend encrypted_string preferences are treated like normal string, the are no differences for the end-user.
Update the documentation section about Preferences, now it explains all the setter/getter methods created for the preferences. The update is inspired by the Spree documentation about Preferences.
spaghetticode
approved these changes
Aug 13, 2020
There was a problem hiding this comment.
@stefano-sarioli thank you 👍
And thank you for updating the doc as well ❤️
|
Hi @jarednorman, does the change I made thanks to your feedbacks looks good to you? |
jarednorman
approved these changes
Aug 17, 2020
stefano-sarioli
deleted the
stefano-sarioli/3652-encrypted-preferences
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request wants to resolve the issue Ref #3652 adding a new type encrypted_string as a possible type when declaring preferences.
The new type encrypted_string would encrypt the value assigned to the preference so that when saved to a Database the value would be safe if a malicious actor gets access to the DB or a dump.
The value to be encrypted need to be set/get using the preferences setter and getter method, the encryption would be skipped if the preferences hash is accessed directly.
The idea behind this is that the value would always be encrypted inside the preferences' hash so that the end-user doesn't need to change the way it interacts with the preferences.
This implementation would not handle the encryption key, it's up to the end-user to safely generate, store, and rotate the key.
The key can be passed as an option to the preference (encryption_key), it the option is missing the system would look for an environment variable called SOLIDUS_PREFERENCES_MASTER_KEY, if even this variable is missing the system will use the Rails secret key.
I'm not sure that this solution would cover all the use cases that the author of the original issue had in mind, so I open the pull request as a draft to ask for feedback about possible missing use cases.
Checklist: