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.

[awgeorge]

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

This looks like a neat solution, as you could use it on any object right? What do you think about marking all User records as sensitive, and have a whitelist instead? So we'd need to pass unprotectedFields.

[flovilmart]

The problem with this approach is that it breaks as soon as a new field is added as it becomes unprotected which is problematic. Would you be willing to attempt an implememtation on the class level permissions for this feature?

[awgeorge]

I think I'll be able to achieve a PR if it doesn't touch the Mongo adapters, as I got very lost last time. I'll dig around the CLP and checkback.

The problem with this approach is that it breaks as soon as a new field is added as it becomes unprotected which is problematic

I would have thought this to be the opposite, a new field added would be accessible in our current implementation, we'd have to re-release the server with a change to the userSensitiveFields, no?

[flovilmart]

Yes, you are right, with this new feature, you’ll be able to update it from the dashboard or REST through a call to the CLP API.

As for the PR, there is no need to touch the adapters, we can remove the user sensitive fields in the runtime, similarly to what we do now. Probably the stripping logic will need to take place somewhere closer to the CLP logic.

[awgeorge]

I'm trying to set the default CLP's I am attempting to use injectDefaultSchema to set the default class level permissions on the user object.

Something like:

if(className === "_User"){
    classLevelPermissions = {...classLevelPermissions, "protectedFields": {"*": ['email']}}
}

I'm struggling to find an easy way of accessing the parse server configuration, without needing to pass this.config down the call stack. Is this the only way / best way to add the defaults, here?

[flovilmart]

This is unlikely to be in injectDefaulrSchema, check into SchemaData and the SchemaController constructor.

Also

https://github.com/parse-community/parse-server/blob/master/src/Controllers/SchemaController.js#L518

As we want to keep the email a protected field, we could hardcode it in the default Schema.

[awgeorge]

I think I'm getting there. Using Mongo Projection, I am able to filter out the fields set in CLP, which is great - however, postgres doesn't support anything similar, so either we need to query the scheme and remove the protected fields, or we fetch them and delete them after. Schema wise, I'm not sure we have this data for postgres, otherwise, we wouldn't do SELECT * but I haven't got there as I have a MongoDB. Otherwise, I stumbled across this: https://dba.stackexchange.com/a/1958, but not sure a double query is a good idea.

Now I'm struggling with keeping backwards compatibility support. I need to find a way to merge the existing config.userSensitiveFields with the CLP stuff - I tried in SchemaData but this seemed to duplicate over time, meaning that my CLP ended up with ['user','user','user','user','user','user','user',] etc. - I could filter duplicates, but again, if SchemaData is behaving this way, it's clearly not the best place for it.

I can set the defaults in defaultCLPS but this will set email as a restricted field on all classes. Not sure if this is what you want. Regardless, if people have edited their config.userSensitiveFields then those fields would become unprotected unless we can find a way to inject it into the CLP.

It needs to be added to a place where the CLP can't be overridden by somebody using the Dashboard, as they could remove protection. I'll keep looking, but if you had any ideas, that'd be great. Thanks!

[flovilmart]

As I mentioned earlier, the field filtering, for the sake of simplicity, can happen after the data is fetched from the database. Not before.

[awgeorge]

@flovilmart - The mongo projection works great. Would you want a different approach depending on the DB used? Do you have any ideas about where to set the default CLP for the protectedFields using the original config setting?

[flovilmart]

The Mongo projection does not work on Postgres. As i understand, this just works for mongo now. Moving at the database adapter level is an optimization, that we should leave out for now.

The backwards compatibility can be ensured at runtime, as userSensitiveFields should always be added to the contents of the Schéma. This way, you can ‘warn’ that either the user should migrate to using the database implementation (if uSF’s are only passed as an option), or remove the flag if the database calls return the same (or more). If the information is conflicting, then il should also notify the user.

I would not mutate the schema and save it based on the original confit, unless a schema save call is made, with the changes

Thé SchemaController and DatabaseControllers seems to be a good start.

[awgeorge]

@flovilmart - Hope the changes are to your satisfaction, please give me guidance towards how they can be improved.

[stale]

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.

[awgeorge]

This has been fixed and merged, but awaiting supporting documentation and additional tests, which I'll get to soon...

[yingmu52]

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.

[dplewis]

@awgeorge Thoughts?

[awgeorge]

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:

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

Thoughts?

[acinader]

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

[acinader]

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

[awgeorge]

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.

[awgeorge]

#5588

[grantespo]

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.

[stale]

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.