userSensitiveFields should adhere to ACLs

Is your feature request related to a problem? Please describe.
For a long time, the _Users table didn't adhere to ACLs - this was changed in #3588. However, the userSensitiveFields are still only readable by the master key or the user. This creates a problem as the admin/moderator cannot view the information, but they can change it.

Describe the solution you'd like
I would like the userSensitiveFields to adhere to ACL rules. I think it's around here:

parse-server/src/RestQuery.js

Line 571 in 46ac7e7

if (auth.isMaster || (auth.user && auth.user.id === result.objectId)) {

Describe alternatives you've considered
Alternatively, we should allow the server owner to override the default email being a sensitive field, however, I like the idea of sensitive fields in case public read is ever activated on the user object.

Additional context
Parse.com allowed reading and writing of _user information as long as the ACLs were correct. Because of this, I think many of us used the _Users table for personal info/meta, along with the authorisation details, you can argue either way about the architecture of that plan, but we are where we are.

Alternatively, we should allow the server owner to override the default email being a sensitive field, however, I like the idea of sensitive fields in case public read is ever activated on the user object.

This leaves open all your user’s emails to any attacker. Did you consider this?

Many discussions occurs around this defaults, and it is very likely we will not budge.

@awgeorge

What shouldn't be allowed is for sensitive fields to be public. Ever.

Email is the only sensitive field out of the box. Adding additional sensitive fields (ssn, cc#, dob, etc.) is fine, but email should always be recognized as personal.

Allowing an admin role to see and update sensitive fields is reasonable. I do think that parse.com allowed authenticated roles to see, and update email, without it being public.

My sledgehammer to solve the problem of leaking sensitive fields through the public api is here:

parse-server/src/RestQuery.js

Line 575 in 46ac7e7

for (const field of config.userSensitiveFields) {

@awgeorge, I'd be glad to help you craft a more nuanced solution that we could cover with sufficient tests to make even @flovilmart happy, if you're up for it. In which case, I think we could reopen this.

:).

@flovilmart

@flovilmart That was an alternative, as I was trying to think of something... The main idea which we should consider is changing the above code to auth.isMaster || (auth.user && auth.user.id === result.objectId) || auth.user.hasCorrectACLS(result.objectId) (something like that) - does that make sense?

@acinader - My original thought around sensitive fields is that it should be a whitelist, as opposed to a blacklist. But that would be a breaking API change. Either way, an authenticated user that has matching ACLs to read the data would be allowed to view the sensitive fields, at present nothing.

Added negative scenarios for parse-community#5301.

Added some tests to spec out what I'm after.

Allow MasterKey, Object Owner and Object with an Explicit ACL role access to PII
Never allow non-authenticated users to access PII
Never allow PublicRead ACL on Object to expose PII

Hopefully, the tests make this clear, and you would consider the scenario.

Thanks.

What you are describing is column based ACL’s which are not only for users but any object. PII, by default would generate those ACL’s for the _User table.

I reckon this can be useful.

What I wish to avoid is over engineering a solution that may cause security issues and is hard to maintain.

do think that parse.com allowed authenticated roles to see, and update email, without it being public

I do not recall that, if it was, that would have been quite bad.

@flovilmart

@flovilmart - As long as the correct column based ACL was in place, you could read the users table on parse.com, hence why all my user meta info is in the user table. Which has led us down this long path. You had to be authenticated with a Role that allowed access to another user.

Agree that security is the upmost importance, but thanks for understanding the use case.

If you set me in a direction, I can try to open a PR. Is there a function for comparing ACLs alread?

As long as the correct column based ACL was in place, you could read the users table on parse.com

Which is arguably a bad idea as it may expose private information publicly.

ACL are checked against the database objects directly. For all queries we build a list of possible value that describe the user making the call and ensure either the permissions match the provided value.
Those ACL checks are not made in memory.

What I would recommend is for you to look at class level permissions and add a new CLP type for those field based access control. We can define a new member alongside existing permissions. In this member we can keep the same object keys based permissions as we have the engine to compute what permission applies to who. Each of those values instead of ‘true’ / ‘false’ could be the PII keys for each Class

See the type described here

export type ClassLevelPermissions = {
  find?: { [string]: boolean },
  count?: { [string]: boolean },
  get?: { [string]: boolean },
  create?: { [string]: boolean },
  update?: { [string]: boolean },
  delete?: { [string]: boolean },
  addField?: { [string]: boolean },
  readUserFields?: string[],
  writeUserFields?: string[],
  
  // new feature
  protectedFields?: { [string]: boolean }
};

For example, with the _User class, if the server was initialized with userSensitiveFields: ['email', 'sin', 'phone'], this would be the equivalent of:

{
  // CLP for the class ... other 
  protectedFields: { "*": ["email", "sin"] }
};

Now if you wanted an moderator role to be able to see the user's email but not the sin and an admin which can read it all

{
  protectedFields: { 
     "*": ["email", "sin"],
    "role:moderator": ["sin"],
    "role:admin": []
  }
};

We could have as a first restriction that the _User always have access to it's own object.
For other objects, one could set restrictions by user_id.

Another neat feature, is that from there we can probably leverage de-selecting the fields, at the database adapter level. This way, the values are never in transit in the runtime.

@acinader

ACL are checked against the database objects directly. For all queries we build a list of possible value that describe the user making the call and ensure either the permissions match the provided value.
Those ACL checks are not made in memory.

Arh, yes, I think this is why I got stuck implementing #3588 as I'm not verse on Mongo or Postgres. That would be tricky to implement as we'd basically have to remove the userSensitveFields to allow the database to do the matching, right?

But wait - if the database does the check to make sure it can send the data, does that mean we're needlessly deleting the keys with @acinader sledgehammer? Or is that just a failsafe? Would it be as easy as checking whether result has a column based ACL, and if so, return early, here?

parse-server/src/RestQuery.js

Line 571 in 46ac7e7

if (auth.isMaster || (auth.user && auth.user.id === result.objectId)) {

I'll read up on CLPs, I personally didn't install any as each record had an ACL.

How do I get rid of this warning ??

DEPRECATED: userSensitiveFields has been replaced by protectedFields allowing the ability to protect fields in all classes with CLP.

@awgeorge

@awgeorge Thoughts?

Originally this warning would disappear after you migrate to the new protectedFields - however, after #5463, I believe this message will always show as options.userSensitiveFields has a default value of email still.

Options:

Remove warning message completely.
Revert some small changes in Protected fields fix #5463 to silence the warning after upgrading
Remove the default value from the deprecated options.userSensitiveFields as it should now have default protectedFields.

Thoughts?

2 or 3 works for me. sorry i busted it. :(.

and i'm glad to fix since i busted it, but i wont get to it for a few days...

@acinader

3 seems simple enough - just tested a fix and it does indeed stop the message appearing. I'll push the changes now...

@acinader, no worries, I should have picked it up when I reviewed your changes.

#5588

I'd like to implement protectedFields for a few classes.
Where do I implement CLP code for my app? main.js, index.js, or...?

Thanks.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Added negative scenarios for parse-community#5301.

Navigation Menu

Repository navigation

userSensitiveFields should adhere to ACLs #5301

18 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

userSensitiveFields should adhere to ACLs #5301

Description

Activity

flovilmart commented on Jan 18, 2019

acinader commented on Jan 18, 2019

awgeorge commented on Jan 18, 2019

awgeorge commented on Jan 18, 2019

flovilmart commented on Jan 18, 2019

flovilmart commented on Jan 18, 2019

awgeorge commented on Jan 18, 2019

flovilmart commented on Jan 18, 2019

flovilmart commented on Jan 18, 2019

awgeorge commented on Jan 18, 2019

18 remaining items

yingmu52 commented on May 13, 2019

dplewis commented on May 13, 2019

awgeorge commented on May 13, 2019

acinader commented on May 13, 2019

acinader commented on May 13, 2019

awgeorge commented on May 13, 2019

awgeorge commented on May 13, 2019

grantespo commented on May 27, 2019

stale commented on Jul 11, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions