_User ACL not working correctly

[awgeorge]

Issue Description

Updates to _User record do not adhere to the ACL correctly. If a moderator is editing another _User record (i.e not their own data). With the correct ACL's in place, the server always denies permission due to this line

if (this.className === '_User' &&
      this.query &&
      !this.auth.couldUpdateUserId(this.query.objectId)) {
    throw new Parse.Error(Parse.Error.SESSION_MISSING, `Cannot modify user ${this.query.objectId}.`);
  }

As you can see the function will only return true if we're using the master key or the object is the currently logged in user.

// Whether this auth could possibly modify the given user id.
// It still could be forbidden via ACLs even if this returns true.
Auth.prototype.couldUpdateUserId = function(userId) {
  if (this.isMaster) {
    return true;
  }
  if (this.user && this.user.id === userId) {
    return true;
  }
  return false;
};

The comment implies that the ACL can still override, but this is not the case.

Steps to reproduce

1. Correctly assign ACLs to the user record so that the currently authenticated user should be able to edit the record.

2. Edit the _User record

Expected Results

A successful outcome.

Actual Outcome

{"code":206,"message":"Cannot modify user IyYUu90HdL.","level":"error","timestamp":"2017-03-01T17:41:56.373Z"}

Environment Setup

• Server

  • parse-server version (Be specific! Don't say 'latest'.) : 2.3.6
  • Operating System: OSX 10.12.1
  • Hardware: MBP-2015
  • Localhost or remote server? (AWS, Heroku, Azure, Digital Ocean, etc): localhost

• Database

  • MongoDB version: 3.2.11
  • Storage engine: MMAPv1
  • Hardware: ??
  • Localhost or remote server? (AWS, mLab, ObjectRocket, Digital Ocean, etc): mLab

Logs/Trace

Include all relevant logs. You can turn on additional logging by configuring VERBOSE=1 in your environment.

[flovilmart]

We don't allow users to modify other user's ACL for security reasons.

[awgeorge]

Then what's the point of ACL's? Without editing users, you're limiting Parse's functionality as it can't be used for anything user management related, you can't design a system that has moderators or administrators without splitting all user content into another table. Which for already established applications is upsetting. We've unfortunately had to use the master key which arguable is now worse for security reasons as anyone with access to the page can now update the user - instead of the ACLs which was the purpose of them to begin with.

[flovilmart]

I believe this restriction was set in parse-server as it was there on parse.com

[awgeorge]

Are you sure? - I continued to use the parse hosted solution up until January (as I couldn't find the time to transfer it sooner). And my Administrator ACL was able to update users.

[flovilmart]

I believe

it doesn't mean I'm sure.

[johanarnor]

@awsGeorge you could use a cloud function Parse.Cloud.define('someFunction',... to achieve this. In that function you can first check for role and then use the master key.

Also are you now using the master key in a publicly facing webpage? That would be a huge security issue.

[awgeorge]

@johanarnor Yes - it was a quick work around, naturally the page in question requires elevated privileges, but I would prefer not to use the master key, as it seems like a hack to reinstate it's original functionality.

[johanarnor]

Ok, but how is a user authenticated to reach that page? Is the page a part of a single page application that anyone can access from public internet? If so, your master key is publicly available.

[awgeorge]

No - It's using the PHP SDK.

[dkarviga]

The same issue. I have to perform access control checkings myself using Parse Cloud functions.
It'd be great if ACL works for _User objects as for other objects

[coryshaw]

+1

I would expect the User class to adhere to the same ACLs as everything else. I can understand setting the default ACLs for the User class differently for security reasons, but if I explicitly set the ACL of a user to a role ("Admin" read/write), I would expect that a logged in Admin can read and write that User.

The work arounds are either to POST to an endpoint and useMasterKey, or create a separate "User Meta" class and have to deal with the complexities of the extra join query.

[sdonaway]

+1 The User class should adhere to the same ACLs as any other class. Otherwise, user management at an elevated role is impossible without using a hack/workaround.

[flovilmart]

I'll work on the enhancement while maintaining a good amount of security:

• calls will folllow the ACL
• public write will always be rejected
• self locking out will be prevented
• locking out from other source will be approved if following ACL

[oallouch]

Any news on this ?

[flovilmart]

Not yet, haven't had a chance to make a dent. But in the meantime, anyone can take it on, what's expected is detailed enough

[oallouch]

Looks easy...for you :)

[oallouch]

Anyway, I'll use a Cloud function.

[awgeorge]

Right - taking a stab. Can you point me to the ACL checking code? I removed the basic checks thinking it would fall back to normal ACL checking, but it didn't, contrary to this comment: // It still could be forbidden via ACLs even if this returns true.

[awgeorge]

It looks like the the ACL is translated to a native database ACL, so presumably there is no checking, and we let the database handle the permissions? I need a simple way to compare the current user ACL with the object ACL for the sensitive fields - IE. the database returns the data, but we're deleting email, in this scenario, the code must check and validate the ACL, so we can keep these fields in tact - am I right in thinking the only way to do this is to perform another query? I can see that we can get the roles of the current user auth.getUserRoles(), but there is no clear way to check whether these roles coincide with the objects ACL.

[opit7]

@flovilmart Is this already implemented? Or do i need to use Cloud functions?

[flovilmart]

@opit7 on the latest parse-server users can be locked out when using a masterKey but the rest of the implementation hasn't been implemented yet.

[sambing]

@flovilmart may you by chance still have the rest of the implementation on the road map for the nearer future?

[flovilmart]

I haven't had the chance to implement it, and at the time being, there is debate as for the security implication of changing this model. While I hear the ones from the camp saying Users should follow ACL's, I'm not sure changing it now is a great idea. Anyhow, this is still pending a good solution, that takes into account the maintainer concerns and the need of the community.

In the meantime, there are plenty of possible alternatives, including using a cloud function to lock out users with the masterKey.

[sambing]

@flovilmart The main use was for a moderation helper app. I did implement alternatives now, however it added more complexity which i am not sure that was needed, but i can be totally wrong here as i do not have much information about the security implications it may have.

[georgesjamous]

Related issue #4789

[flovilmart]

@awgeorge finally it's in! Users will follow ACL's while being extra secure!