-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat/HIT26_GDPR-anonymity-of-beneficiaries #23
base: hit-oort
Are you sure you want to change the base?
feat/HIT26_GDPR-anonymity-of-beneficiaries #23
Conversation
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.
@GuilhermeGabriel I refactored a bit the code, and addressed most of the comments I left already. Just review the logic for the beneficiary case please
import { User } from '@models'; | ||
import { startDatabaseForMigration } from '../src/utils/migrations/database.helper'; | ||
|
||
/** | ||
* Update lastLogin field. | ||
*/ | ||
export const up = async () => { | ||
await startDatabaseForMigration(); | ||
|
||
const users = await User.find({}); | ||
for (const user of users) { | ||
// Create field lastLogin with value of modifiedAt | ||
user.lastLogin = user.modifiedAt; | ||
|
||
// Save user | ||
await user.save(); | ||
} | ||
}; | ||
|
||
/** | ||
* Sample function of down migration | ||
* | ||
* @returns just migrate data. | ||
*/ | ||
export const down = async () => { | ||
/* | ||
Code you downgrade script here! | ||
*/ | ||
}; |
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.
I changed my mind, since we're only using this on the cron job, we can just do this same logic there and skip the migration entirely
src/index.ts
Outdated
// Find the resource with name staff | ||
const staff = await Resource.findOne({ name: 'Staff' }); | ||
|
||
// Find all the records of staff with the users found above | ||
const usersStaffRecords = await Record.find({ | ||
resource: staff._id, |
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.
We can just pass the id directly and skip one DB call
src/index.ts
Outdated
user.username = `${Math.random() | ||
.toString(36) | ||
.substring(2, 15)}@anonymus-oort.com`; |
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.
Changed my mind about this too, instead of a random string, we can just use the mongo object id
src/index.ts
Outdated
|
||
await user.save(); |
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.
Instead of doing operations inside loops, use Users.bulkSave(usersToDelete)
to save a bunch of DB calls
src/index.ts
Outdated
staffRecord._createdBy = new User({ | ||
name: 'ANONYMOUS', | ||
username: `${Math.random() | ||
.toString(36) | ||
.substring(2, 15)}@anonymus-oort.com`, | ||
}); |
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.
We don't need to do this, we are already hiding the information of the original user
src/index.ts
Outdated
// Find all the records of Ais was given more than 18 months ago | ||
const AidRecords = await Record.find({ | ||
resource: Aid._id, | ||
createdAt: { | ||
$lt: new Date(Date.now() - 18 * 30 * 24 * 60 * 60 * 1000), | ||
}, // 18 months ago | ||
}); | ||
|
||
// Anonymize all members of that family |
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.
This logic is incorrect, like this it would erase all beneficiaries that got an aid longer than 18 months ago, when in fact we want to erase those who have NOT gotten an aid in the last 18 months.
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.
I added a new commit to fix that!
if (!aidGivenToFamily) { | ||
// Find all members of the family | ||
const members = await Record.find({ | ||
_id: { $in: family?.data?.members }, |
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.
This would throw an error if family?.data?.members
resolves to undefined, as the $in
operator requires an array as value
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.
Would also be nice to check if the record is in the Person form, but no big deal
// Anonymize the member | ||
member._createdBy = new User({ | ||
name: 'ANONYMOUS', | ||
username: `${member._id.toString()}@oort-anonymous.com`, | ||
}); |
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.
I don't think we need this, the createdBy would be a staff member, and their anonymization is already handled in the other function
@@ -3,63 +3,72 @@ import { Types } from 'mongoose'; | |||
|
|||
/** Staff resource ID */ | |||
const AID_RESOURCE_ID = new Types.ObjectId('64e6e0933c7bf3962bf4f04c'); | |||
const FAMILY_RESOURCE_ID = new Types.ObjectId('64de75fd3fb2a11c988dddb2'); |
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.
missing jsdoc, don't forget to always run npm run lint before marking the PR as ready for review
member.data = { | ||
...member.data, | ||
location: 'ANONYMOUS', | ||
surname: 'ANONYMOUS', | ||
firstname: 'ANONYMOUS', | ||
phone: 'ANONYMOUS', | ||
nom_employes: 'ANONYMOUS', | ||
gender: 'ANONYMOUS', | ||
birthdate: 'ANONYMOUS', | ||
prenom_employes: 'ANONYMOUS', | ||
nom_prenom_employes: 'ANONYMOUS', | ||
tel_staff: 'ANONYMOUS', | ||
email_staff: 'ANONYMOUS', | ||
birthdate_employes: 'ANONYMOUS', | ||
file_gdpr_staff: [], | ||
}; |
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.
When anonymizing the data, we don't need to remove fields that are not traceable back to the beneficiary. e.g: their location and gender.
Also, make sure to only save valid data. For the birthdate for example, it should always be a valid date, check the other anonymization function to see how it should be dealt with. The email should also be a valid email, you can use the same logic of [id]@oort-anonymous.com
. To make sure that all fields have valid data, you can create a record and see how the data is stored in the db.
And finally, it looks like there are fields here that are not part of the form (and it's possibly missing some that should be there, but I didn't check)
member._lastUpdatedBy = new User({ | ||
name: 'ANONYMOUS', | ||
username: `${member._id.toString()}@oort-anonymous.com`, | ||
}); |
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.
Same here, can be removed. And actually, another change you should make is doing this but in the staff anonymization, so when anonymizing a staff, you have to check all records created or last updated by them and updated the _lastUpdatedby and the _createdBy. However, do not create a new user, as that creates a new id, and we do not want that.
Description
To stay in conformity of GDPR, we need to deleted some data after some long time that users don't use it:
Add field (lastLogin) to user model that would be updated with the date on every new login.
Deleting a user (using cron):
Finding the staff record related to the user (users question with their userID selected).
Hiding the info both on the user and their staff record
email: [RANDOM STRING]@anonymus-oort.com
nome: [ANONYMOUS]
...
Trigger when: The last aid to the family was given more than 18 months ago
Than: Anonymize all members of that family
Useful links
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Changing the records in database to past, running the script and check if it anonymize it.
Screenshots
before
after
Checklist:
( * == Mandatory )
More explanation
https://www.loom.com/share/05a716d61b9744faaf51fb304c21d1e5?sid=f87cf896-582a-4f76-93ae-8ceed801b145