userSensitiveFields should adhere to ACLs

[awgeorge]

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.

[flovilmart]

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.

[acinader]

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.

:).

[awgeorge]

@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.

[awgeorge]

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.

[flovilmart]

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.

[flovilmart]

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.

[awgeorge]

@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?

[flovilmart]

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

[flovilmart]

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.

[awgeorge]

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.