parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.94k stars 4.78k forks source link

_User class email field protection should be modifiable #6949

Open 185driver opened 4 years ago

185driver commented 4 years ago

Is your feature request related to a problem? Please describe. As discussed in a Parse Community forum thread, the default behavior of the _User class email field is to protect it and only expose it via useMasterKey. This is a reasonable default setting.

However, developers who understand the risks involved may have a reasonable need to access the email field in a find query, but are currently unable to do so. The email field does not respond to a removal from protectedFields. A developer can get around this limitation via adding their own email-related field, but doing so feels hacky. Parse simply does not allow a developer to change the protected behavior of the default email field even though a method to do so is built into Parse.

Parse's default setting for the email field is good. Unless the risks are understood and the default behavior is intentionally overridden, it's reasonable to disallow exposing the email field.

Describe the solution you'd like If, however, a developer understands the risks and wishes to expose the email field in queries, s/he should be allowed to do so. The email field should respond to protectedFields changes in the same way that other fields do.

Describe alternatives you've considered Create a clone of the email field and expose its contents in User class queries. This is not an elegant solution.

Additional context Though I believe allowing developers to change the email field's behavior would be good, it should be accompanied by the addition of wording in the docs that both describes how to use the protected fields feature, and a caution against making a change to the email field's behavior unless the risks are understood. I believe that there is currently no verbiage in the docs about these things.

davimacedo commented 4 years ago

@mtrezza @dplewis thoughts on this one?

mtrezza commented 4 years ago

Thank you for suggesting.

I see this as a general Parse Server product strategy question in terms of security restrictions, there is a related discussion.

A thought experiment:

Would we eventually allow any Parse Server security restrictions to become optional, or are there some restrictions that we would not remove and if so, why?

Frankly, just the idea of allowing a session user query a sensitive field like email of all users gives me goosebumps. Not denying that there may be a serious real world application for this, but I can't think of any.

However, for a more procedural approach, here are some arguments for optional security restrictions:

Pro:

Contra:

An alternative solution could be creating a Cloud Code function that queries the email field using the masterKey, making Cloud Code an intermediary between a session user and sensitive user data, which gives the developer more control over sensitive data.

185driver commented 4 years ago

Some additional thoughts....

If Parse were to treat authentication in a similar way to Firebase (with which I have more experience), it would take master key type use to access both username and email. Parse takes a different tack. It allows session token access to username and also allows username to be modified via protected fields, if desired.

What strikes me is not that Parse chooses to lock down the emailfield, but that:

  1. The username field,(which is at least an equally significant member of auth) is not handled the same exact way as the email field
  2. The email field is listed in the schema's protected fields array, giving the impression its behavior can be modified, when in fact it cannot.

Some apps may be written for internal company use and that company might assign roles to users who have permission to iterate over the Users class and interact with email addresses for some business need. This roles-based approach seems like a reasonable use case for making the email field unprotected. I don't feel the same can be said for the username field, yet Parse allows session token access to it.

mtrezza commented 4 years ago

Some apps may be written for internal company use and that company might assign roles to users who have permission to iterate over the Users class and interact with email addresses for some business need.

This is possible with a Cloud Code function and masterKey. I still don't see a valid use case, but that doesn't matter because I think it's rather a matter of principle, how much security we should allow the developer to disable, and how much guidance Parse Server should give in that regard.

However, I agree that the email and username field should be considered equally sensitive, because we can assume that in many instances, the username field contains the email address.

mtrezza commented 4 years ago

I think the conclusion we can draw from the separate security discussion is:

Therefore I would suggest before we merge a PR for this, we should wait for #6973 to be merged so a security warning can be displayed.

@185driver, would you be willing to prepare a PR for this and add the warning message after #6973 is merged?

185driver commented 4 years ago

This sounds good.

We should handle the email field with the same caution as the username field

The intended meaning above is not quite clear to me given that less caution is currently given to the username field than the email field. That is, the username field is not currently a protected field by default, and it is available in query find ops via session token access by default.

If the intent is to treat them both the same by doing these things:

  1. Add username as a protected field by default as is currently done with the email field
  2. Enable both the username and email fields to be accessible in query find operations via session token access if they have been removed from the protectedFields array

Then, that would be great. If I'm misunderstanding, my apologies.

I am willing to help where I can, but are you asking me to work on a documents PR or a code PR? I can submit something for consideration for the Parse docs related to #6973 once it is merged and I can understand the changes, if that would be helpful.

mtrezza commented 4 years ago

@dplewis, @davimacedo since this would be a sensitive security change, can I get your quick opinions on this suggestion?

davimacedo commented 4 years ago

I agree with the suggestion.

mtrezza commented 4 years ago

The intended meaning above is not quite clear to me given that less caution is currently given to the username field than the email field.

It just meant that both fields should be handled equally, without drawing a conclusion about the level of protection.

Since @davimacedo confirmed we can go ahead, the next steps being:

@185driver Would you be willing to open a PR for the first point? We're here to help with guidance if you have any questions.

185driver commented 4 years ago

Unfortunately, I'm working with a project right now that doesn't use parse server so can't participate for the time being. I'll check back when I can.

Params10 commented 3 years ago

I would like to contribute to resolving this issue. I just started exploring the parse server. PR #6973 is merged so I think I can proceed with this.

  • [ ] Allow changing protected field setting for email and username.

Just wanted to confirm as to how can this be achieved?