Skip to content

Allowing userSensitiveFields to be overriden in constructor #3738

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

Conversation

mross22
Copy link

@mross22 mross22 commented Apr 20, 2017

I ran into the same issue outlined here (#3301) where I was unable to access user.get('email') in cloud code because it because a sensitive field in version 2.3.0.

@acinader, in the thread for the issue above, you mentioned you would be open to a change that allows unsetting 'email' as a sensitive field, so this is my attempt to do so.

Also tagging @Amex22 so you see this since you filed the initial issue

// allow 'userSensitiveFields' from constructor to override default list
if (typeof userSensitiveFields === 'undefined'){
userSensitiveFields = defaults.userSensitiveFields
}
Copy link
Author

Choose a reason for hiding this comment

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

I think this new behavior makes sense, if you specify 'userSensitiveFields' then that takes precedence over the default list of fields instead of merging the two.

Technically this could be considered a breaking change because users that are currently specifying 'userSensitiveFields' are currently getting 'email' appended to that list so this change would alter that behavior. That said, the new behavior would just expose an extra field, which shouldn't break anything. If we really want to avoid making this in a breaking fashion, I can add a new field to the constructor, something like 'includeDefaultUserSensitiveFields' to trigger the new behavior but that seemed a but overkill.

@flovilmart
Copy link
Contributor

Discussed here: #3155 (comment)

@acinader
Copy link
Contributor

@mross22 if its in cloud code, why not just use the master key?

@mross22
Copy link
Author

mross22 commented Apr 23, 2017

@acinader you're right I actually wasn't specifying the master key correctly in my query in cloud code so when i didnt get user.email back I attributed it to this issue, but really it was because the query wasn't being executed with master permissions. My issue is solved and I understand your comments in the other thread about not wanting to override userSensitiveFields completely and instead just allowing some form of role based access. I'll go ahead and close this PR.

@mross22 mross22 closed this Apr 23, 2017
@oallouch
Copy link
Contributor

@acinader in my case, roles already secure the whole thing, so totally overriding userSensitiveFields is very fine with me. Anyway, what's the simplest thing to do to get the email ? Redo the search in an afterFind ?

Thx in advance,
Olivier

@oallouch
Copy link
Contributor

Btw, maybe you can help me. I can't find any doc for afterFind (only beforeFind)

@flovilmart
Copy link
Contributor

There is no after find

@acinader
Copy link
Contributor

acinader commented May 30, 2017

@oallouch a work around if you want to send email address of users to a client is to create a cloud function that can do the look up on the user table using the master key.

@oallouch
Copy link
Contributor

oallouch commented Jun 2, 2017

@acinader thx. I used to master key in an afterFind. Work good (but not very efficient).
@flovilmart ok... ?!? (I guess there must be a deep debate here) :)

@flovilmart
Copy link
Contributor

Ah there is sorry, my bad :)

@oallouch
Copy link
Contributor

Just a note to say that I wish you could reopen this issue.
I try not to use the masterKey on the server and rely completely on ACL/CLP .
Not having the email field makes me check role manually or do an "outside patch" using afterFind.

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.

4 participants