Skip to content
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

_User class email field protection should be modifiable #6949

Open
185driver opened this issue Oct 19, 2020 · 11 comments
Open

_User class email field protection should be modifiable #6949

185driver opened this issue Oct 19, 2020 · 11 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@185driver
Copy link

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
Copy link
Member

@mtrezza @dplewis thoughts on this one?

@mtrezza
Copy link
Member

mtrezza commented Oct 21, 2020

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:

  • Parse Server becomes more flexible and applies to more use cases.

Contra:

  • Optional security restrictions may tempt a developer to adapt Parse Server to suite a flawed app concept instead of rethinking the flawed concept and adapting it to security best practice.
  • Optional security restrictions encourage insecure deployments and disregard of security best practices.
  • Codebase and docs to maintain grow just to cover niche use cases that are not suitable for any serious production environment anyway.

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
Copy link
Author

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 email field, 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
Copy link
Member

mtrezza commented Oct 22, 2020

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
Copy link
Member

mtrezza commented Nov 3, 2020

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

  • We should handle the email field with the same caution as the username field (because we can assume most deployments store the email address or phone number in that field anyway)
  • In the spirit of Parse Server's multi-functionality, we could allow the user to change the protectedField setting of the email field, but:
    • it should be restrictive by default
    • when a sensitive field like email or username is set to be less restrictive (e.g. searchable with a session token), we should display a security warning

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
Copy link
Author

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
Copy link
Member

mtrezza commented Nov 7, 2020

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

@davimacedo
Copy link
Member

I agree with the suggestion.

@mtrezza
Copy link
Member

mtrezza commented Nov 10, 2020

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:

  • Change username to be a protected field by default. This also requires to verify that SDKs are not impaired, although I don't think any of the built-in methods execute a find operation on the field.

  • Allow changing protected field setting for email and username.

  • Add breaking change warning note to change log because making username a protected field by default could be a breaking change for some deployments.

  • Add warning message if email or username protection is less restrictive than the default. This requires waiting for Add Security Checks Log #6973 to be merged before merging this PR.

  • Search docs for any necessary changes and open a separate PR in the docs repo if necessary.

@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
Copy link
Author

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
Copy link

Params10 commented Sep 11, 2021

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?

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

4 participants