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

Editable encrypted fields config #116

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jenyatra
Copy link

This PR is a fork of #110

There is 2 more commits added:

The problem with #110 happens for a fields becoming unencrypted ("extracted"):

  1. It reads such fields from ciphertext just ok
  2. After filed is read, it still not exists in cleartext in Mongo, because nothing was updated yet (so far so good...)
  3. Upon save() the field will be eliminated from ciphertext (ok, we need that...)
  4. When user calls save(), this field is not marked as modified. Because of that, mongoose doesn't bother to set this field (which is still not exists in cleartext in Mongo). This result in a data loss.

This PR ensures that all "extracted" fields are marked as modified upon encryption, so Mongoose will sure set it in cleartext.

@jenyatra
Copy link
Author

jenyatra commented Jun 25, 2022

Hi, me again!
I've got the situation again, when encrypted fields have to be changed, and it required a workaround. So maybe we can consider this feature again?


A thought about nested fields support:
The source of the problem, in my opinion, is the complexity of the support for nested fields config. Sure, it's convenient, but it's sooo complicated when you think about reliable algorithms to support that, given that there is a second way to configure nested encryption: the nested Schema definitions. It seems pretty complicated to get these two features work without conflicts.

I think these PRs solve it in coerce way, but it always be a source of ambiguity for users.


A thought about this update:
This PR seem to be working fine, but I have no time to do rigorous testing. Maybe I'm chicken, but it's kinda scary to push it silently too users - what if there is a problem?
On the other hand, right now the library will just drop data when fields reconfigured, so it's not like current state is safe too.

So, I see two options:

  1. Fix documentation, explicitly warn about the problem with reconfiguring. Then, from this PR, create a feature branch or alpha version of some kind for testing.
  2. Merge this PR and hope it's fine.

Seem to me as a hard choice, but again, maybe I'm too alert or something. Seem to be working fine...

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

Successfully merging this pull request may close these issues.

3 participants