-
Notifications
You must be signed in to change notification settings - Fork 11
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
[BACK-43] jellyfish updates based on reuse of legacy _id for migration #203
base: master
Are you sure you want to change the base?
Conversation
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
/deploy qa3 |
jh-bate updated values.yaml file in qa3 |
jh-bate updated flux policies file in qa3 |
jh-bate deployed jellyfish add-dedup-hash branch to qa3 namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a few changes. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, one possible change. Let me know what you think and I can approve without changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One comment on a bit of simplification, if you so choose.
Also, there are a number of outstanding vulnerabilities (npm audit, snyk, dependabot). I ran my usual update process and created a new PR on top of this one. You can either merge into this PR or cherry pick it. The PR is #205.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A couple of comments/questions, but nothing blocking approval.
@@ -0,0 +1 @@ | |||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might as well go ahead and check in similar files for all the environments. That way if this is deployed anywhere it will still function, but effectively act the same and use Jellyfish.
var iv = `${env}-environment`.substring(0, 16); | ||
var key = serverSecret.substring(0, 32); | ||
var algorithm = 'aes-256-cbc'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Consider pulling these out into a separate function so we guarantee that encryption and decryption use the same values.
var key = serverSecret.substring(0, 32); | ||
var algorithm = 'aes-256-cbc'; | ||
|
||
var encryptedArray = dataArray.map((item) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you are encrypting each user id separately, not just the entire file?
@@ -65,3 +65,35 @@ exports.generateId = function(fields) { | |||
return base32hex.encodeBuffer(hasher.digest(), { paddingChar: '-' }); | |||
}; | |||
|
|||
exports.encryptArrayToFile = function (dataArray, filePath, env, serverSecret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we use this when creating/updating the unencrypted file?
see tidepool-org/development#297