Skip to content

Strip personally identifiable information from user table for unautho… #3158

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

Merged
merged 1 commit into from
Dec 3, 2016

Conversation

acinader
Copy link
Contributor

@acinader acinader commented Dec 2, 2016

…rized users.

  • add a config option to explicitly enumerate pii fields beyond email
  • in query controller, strip pii of user table results before sending out the door.

fixes: #3155

@facebook-github-bot
Copy link

@acinader updated the pull request - view changes

@acinader
Copy link
Contributor Author

acinader commented Dec 2, 2016

oops. forgot to commit the unit test...now added.

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Small nits otherwise it's great, we probably could handle authData the same way

@@ -407,7 +417,7 @@ RestQuery.prototype.runFind = function(options = {}) {
if (this.className === '_User') {
for (var result of results) {
delete result.password;

cleanResultOfSensitiveUserInfo(result, this.auth, this.config);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems that authData is pretty much the same :)

@@ -386,6 +386,16 @@ RestQuery.prototype.replaceDontSelect = function() {
})
};

const cleanResultOfSensitiveUserInfo = function (result, auth, config) {
if (auth.isMaster || ( auth.user && auth.user.id === result.objectId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit:   || ( auth.user 
-----------------^ remove space

@flovilmart
Copy link
Contributor

@acinader just a few small things that you could address. Otherwise, LGTM!

@facebook-github-bot
Copy link

@acinader updated the pull request - view changes

@acinader
Copy link
Contributor Author

acinader commented Dec 2, 2016

@flovilmart

I added a second commit with what I think you have in mind for authdata? I like having the logic pulled out of the loop into their own functions. I also like having them be two, small function instead of combining.

Also, if this looks good, let me know and I'll squash the commits. The squash and merge button is the devil.

@flovilmart
Copy link
Contributor

The squash and merge button is fine :)

@facebook-github-bot
Copy link

@acinader updated the pull request - view changes

@acinader
Copy link
Contributor Author

acinader commented Dec 2, 2016

@flovilmart

  1. squashed
  2. au contraire the squash and merge wreaks havoc on my local workflow. changes all the hashes so i can't easily tell which branches have been merged (need to google some more.)
  3. looking into why the build is busted (it wasn't me!) ;)

@flovilmart
Copy link
Contributor

  1. Right, it rewrites the history, but that's nice. As it also adds on the top of master.

Now that you're a contributor, you don't need your fork for your commits you can push branches directly

@acinader
Copy link
Contributor Author

acinader commented Dec 2, 2016

noted on the fork stuff. any reason for me to resubmit this not from my fork?

@flovilmart
Copy link
Contributor

No reason!

…rized users.

- add a config option to explicitly enumerate pii fields beyond email
- in query controller, strip pii of user table results before sending out the door.
@facebook-github-bot
Copy link

@acinader updated the pull request - view changes

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.

keeping Personal Info personal....
3 participants