Ensure User ACL's are more flexible and secure #3588 (#4860)

* Fixes an issue that would let the beforeDelete be called when user has no access to the object

* Ensure we properly lock user

- Improves find method so we can attempt to read for a write poking the right ACL instead of using masterKey
- This ensure we do not run beforeDelete/beforeFind/beforeSave in the wrong scenarios

* nits

* Caps insufficient

spec/ParseAPI.spec.js

@@ -975,6 +975,25 @@ describe('miscellaneous', function() {
     });
   });
 
+  it('test beforeDelete with locked down ACL', async () => {
+    let called = false;
+    Parse.Cloud.beforeDelete('GameScore', (req, res) => {
+      called = true;
+      res.success();
+    });
+    const object = new Parse.Object('GameScore');
+    object.setACL(new Parse.ACL());
+    await object.save();
+    const objects = await new Parse.Query('GameScore').find();
+    expect(objects.length).toBe(0);
+    try {
+      await object.destroy();
+    } catch(e) {
+      expect(e.code).toBe(Parse.Error.OBJECT_NOT_FOUND);
+    }
+    expect(called).toBe(false);
+  });
+
   it('test cloud function query parameters', (done) => {
     Parse.Cloud.define('echoParams', (req, res) => {
       res.success(req.params);

spec/ParseUser.spec.js

@@ -525,6 +525,71 @@ describe('Parse.User testing', () => {
     });
   });
 
+  it('never locks himself up', async () => {
+    const user = new Parse.User();
+    await user.signUp({
+      username: 'username',
+      password: 'password'
+    });
+    user.setACL(new Parse.ACL());
+    await user.save();
+    await user.fetch();
+    expect(user.getACL().getReadAccess(user)).toBe(true);
+    expect(user.getACL().getWriteAccess(user)).toBe(true);
+    const publicReadACL = new Parse.ACL();
+    publicReadACL.setPublicReadAccess(true);
+
+    // Create an administrator role with a single admin user
+    const role = new Parse.Role('admin', publicReadACL);
+    const admin = new Parse.User();
+    await admin.signUp({
+      username: 'admin',
+      password: 'admin',
+    });
+    role.getUsers().add(admin);
+    await role.save(null, { useMasterKey: true });
+
+    // Grant the admins write rights on the user
+    const acl = user.getACL();
+    acl.setRoleWriteAccess(role, true);
+    acl.setRoleReadAccess(role, true);
+
+    // Update with the masterKey just to be sure
+    await user.save({ ACL: acl }, { useMasterKey: true });
+
+    // Try to update from admin... should all work fine
+    await user.save({ key: 'fromAdmin'}, { sessionToken: admin.getSessionToken() });
+    await user.fetch();
+    expect(user.toJSON().key).toEqual('fromAdmin');
+
+    // Try to save when logged out (public)
+    let failed = false;
+    try {
+      // Ensure no session token is sent
+      await Parse.User.logOut();
+      await user.save({ key: 'fromPublic'});
+    } catch(e) {
+      failed = true;
+      expect(e.code).toBe(Parse.Error.SESSION_MISSING);
+    }
+    expect({ failed }).toEqual({ failed: true });
+
+    // Try to save with a random user, should fail
+    failed = false;
+    const anyUser = new Parse.User();
+    await anyUser.signUp({
+      username: 'randomUser',
+      password: 'password'
+    });
+    try {
+      await user.save({ key: 'fromAnyUser'});
+    } catch(e) {
+      failed = true;
+      expect(e.code).toBe(Parse.Error.SESSION_MISSING);
+    }
+    expect({ failed }).toEqual({ failed: true });
+  });
+
   it("current user", (done) => {
     const user = new Parse.User();
     user.set("password", "asdf");
@@ -2379,7 +2444,7 @@ describe('Parse.User testing', () => {
         }, (error, response, body) => {
           expect(error).toBe(null);
           const b = JSON.parse(body);
-          expect(b.error).toBe('invalid session token');
+          expect(b.error).toBe('Invalid session token');
           request.put({
             headers: {
               'X-Parse-Application-Id': 'test',
@@ -2471,7 +2536,7 @@ describe('Parse.User testing', () => {
             expect(error).toBe(null);
             const b = JSON.parse(body);
             expect(b.code).toEqual(209);
-            expect(b.error).toBe('invalid session token');
+            expect(b.error).toBe('Invalid session token');
             done();
           });
         });
@@ -2513,7 +2578,7 @@ describe('Parse.User testing', () => {
         }, (error,response,body) => {
           const b = JSON.parse(body);
           expect(b.code).toEqual(209);
-          expect(b.error).toBe('invalid session token');
+          expect(b.error).toBe('Invalid session token');
           done();
         });
       });
@@ -2550,7 +2615,7 @@ describe('Parse.User testing', () => {
       done();
     }, function(err) {
       expect(err.code).toBe(Parse.Error.INVALID_SESSION_TOKEN);
-      expect(err.message).toBe('invalid session token');
+      expect(err.message).toBe('Invalid session token');
       done();
     });
   });
@@ -2626,7 +2691,7 @@ describe('Parse.User testing', () => {
     });
   });
 
-  it("invalid session tokens are rejected", (done) => {
+  it("Invalid session tokens are rejected", (done) => {
     Parse.User.signUp("asdf", "zxcv", null, {
       success: function() {
         request.get({
@@ -2639,7 +2704,7 @@ describe('Parse.User testing', () => {
           },
         }, (error, response, body) => {
           expect(body.code).toBe(209);
-          expect(body.error).toBe('invalid session token');
+          expect(body.error).toBe('Invalid session token');
           done();
         })
       }

spec/helper.js

@@ -114,7 +114,6 @@ if (process.env.PARSE_SERVER_TEST_CACHE === 'redis') {
 }
 
 const openConnections = {};
-
 // Set up a default API server for testing with default configuration.
 let server;
 

src/Auth.js

@@ -21,14 +21,14 @@ function Auth({ config, isMaster = false, isReadOnly = false, user, installation
 
 // 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) {
+Auth.prototype.isUnauthenticated = function() {
   if (this.isMaster) {
-    return true;
+    return false;
   }
-  if (this.user && this.user.id === userId) {
+  if (this.user) {
-    return true;
+    return false;
   }
-  return false;
+  return true;
 };
 
 // A helper to get a master-level Auth object
@@ -64,7 +64,7 @@ var getAuthForSessionToken = function({ config, sessionToken, installationId } =
     return query.execute().then((response) => {
       var results = response.results;
       if (results.length !== 1 || !results[0]['user']) {
-        throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
+        throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
       }
 
       var now = new Date(),

src/Controllers/DatabaseController.js

@@ -869,7 +869,8 @@ class DatabaseController {
     op,
     distinct,
     pipeline,
-    readPreference
+    readPreference,
+    isWrite,
   }: any = {}): Promise<any> {
     const isMaster = acl === undefined;
     const aclGroup = acl || [];
@@ -930,7 +931,11 @@ class DatabaseController {
                   }
                 }
                 if (!isMaster) {
-                  query = addReadACL(query, aclGroup);
+                  if (isWrite) {
+                    query = addWriteACL(query, aclGroup);
+                  } else {
+                    query = addReadACL(query, aclGroup);
+                  }
                 }
                 validateQuery(query);
                 if (count) {

src/RestQuery.js

@@ -24,12 +24,13 @@ function RestQuery(config, auth, className, restWhere = {}, restOptions = {}, cl
   this.clientSDK = clientSDK;
   this.response = null;
   this.findOptions = {};
+  this.isWrite = false;
+
   if (!this.auth.isMaster) {
-    this.findOptions.acl = this.auth.user ? [this.auth.user.id] : null;
     if (this.className == '_Session') {
-      if (!this.findOptions.acl) {
+      if (!this.auth.user) {
         throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN,
-          'This session token is invalid.');
+          'Invalid session token');
       }
       this.restWhere = {
         '$and': [this.restWhere, {
@@ -188,17 +189,28 @@ RestQuery.prototype.buildRestWhere = function() {
   });
 }
 
+// Marks the query for a write attempt, so we read the proper ACL (write instead of read)
+RestQuery.prototype.forWrite = function() {
+  this.isWrite = true;
+  return this;
+}
+
 // Uses the Auth object to get the list of roles, adds the user id
 RestQuery.prototype.getUserAndRoleACL = function() {
-  if (this.auth.isMaster || !this.auth.user) {
+  if (this.auth.isMaster) {
     return Promise.resolve();
   }
-  return this.auth.getUserRoles().then((roles) => {
+
-    // Concat with the roles to prevent duplications on multiple calls
+  this.findOptions.acl = ['*'];
-    const aclSet = new Set([].concat(this.findOptions.acl, roles));
+
-    this.findOptions.acl = Array.from(aclSet);
+  if (this.auth.user) {
+    return this.auth.getUserRoles().then((roles) => {
+      this.findOptions.acl = this.findOptions.acl.concat(roles, [this.auth.user.id]);
+      return;
+    });
+  } else {
     return Promise.resolve();
-  });
+  }
 };
 
 // Changes the className if redirectClassNameForKey is set.
@@ -523,6 +535,9 @@ RestQuery.prototype.runFind = function(options = {}) {
   if (options.op) {
     findOptions.op = options.op;
   }
+  if (this.isWrite) {
+    findOptions.isWrite = true;
+  }
   return this.config.database.find(this.className, this.restWhere, findOptions)
     .then((results) => {
       if (this.className === '_User') {

src/RestWrite.js

@@ -965,7 +965,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
 
   if (this.className === '_User' &&
       this.query &&
-      !this.auth.couldUpdateUserId(this.query.objectId)) {
+      this.auth.isUnauthenticated()) {
     throw new Parse.Error(Parse.Error.SESSION_MISSING, `Cannot modify user ${this.query.objectId}.`);
   }
 

src/Routers/UsersRouter.js

@@ -130,7 +130,7 @@ export class UsersRouter extends ClassesRouter {
 
   handleMe(req) {
     if (!req.info || !req.info.sessionToken) {
-      throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
+      throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
     }
     const sessionToken = req.info.sessionToken;
     return rest.find(req.config, Auth.master(req.config), '_Session',
@@ -140,7 +140,7 @@ export class UsersRouter extends ClassesRouter {
         if (!response.results ||
           response.results.length == 0 ||
           !response.results[0].user) {
-          throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
+          throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
         } else {
           const user = response.results[0].user;
           // Send token back on the login, because SDKs expect that.

src/rest.js

@@ -8,7 +8,6 @@
 // things.
 
 var Parse = require('parse/node').Parse;
-import Auth from './Auth';
 
 var RestQuery = require('./RestQuery');
 var RestWrite = require('./RestWrite');
@@ -54,9 +53,9 @@ function del(config, auth, className, objectId) {
       'bad objectId');
   }
 
-  if (className === '_User' && !auth.couldUpdateUserId(objectId)) {
+  if (className === '_User' && auth.isUnauthenticated()) {
     throw new Parse.Error(Parse.Error.SESSION_MISSING,
-      'insufficient auth to delete user');
+      'Insufficient auth to delete user');
   }
 
   enforceRoleSecurity('delete', className, auth);
@@ -67,14 +66,16 @@ function del(config, auth, className, objectId) {
     const hasTriggers = checkTriggers(className, config, ['beforeDelete', 'afterDelete']);
     const hasLiveQuery = checkLiveQuery(className, config);
     if (hasTriggers || hasLiveQuery || className == '_Session') {
-      return find(config, Auth.master(config), className, {objectId: objectId})
+      return new RestQuery(config, auth, className, { objectId })
+        .forWrite()
+        .execute()
         .then((response) => {
           if (response && response.results && response.results.length) {
             const firstResult = response.results[0];
             firstResult.className = className;
             if (className === '_Session' && !auth.isMaster) {
               if (!auth.user || firstResult.user.objectId !== auth.user.id) {
-                throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
+                throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
               }
             }
             var cacheAdapter = config.cacheController;
@@ -110,6 +111,8 @@ function del(config, auth, className, objectId) {
     }, options);
   }).then(() => {
     return triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config);
+  }).catch((error) => {
+    handleSessionMissingError(error, className, auth);
   });
 }
 
@@ -130,20 +133,33 @@ function update(config, auth, className, restWhere, restObject, clientSDK) {
     const hasTriggers = checkTriggers(className, config, ['beforeSave', 'afterSave']);
     const hasLiveQuery = checkLiveQuery(className, config);
     if (hasTriggers || hasLiveQuery) {
-      return find(config, Auth.master(config), className, restWhere);
+      // Do not use find, as it runs the before finds
+      return new RestQuery(config, auth, className, restWhere)
+        .forWrite()
+        .execute();
     }
     return Promise.resolve({});
-  }).then((response) => {
+  }).then(({ results }) => {
     var originalRestObject;
-    if (response && response.results && response.results.length) {
+    if (results && results.length) {
-      originalRestObject = response.results[0];
+      originalRestObject = results[0];
     }
-
+    return new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK)
-    var write = new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK);
+      .execute();
-    return write.execute();
+  }).catch((error) => {
+    handleSessionMissingError(error, className, auth);
   });
 }
 
+function handleSessionMissingError(error, className) {
+  // If we're trying to update a user without / with bad session token
+  if (className === '_User'
+      && error.code === Parse.Error.OBJECT_NOT_FOUND) {
+    throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth.');
+  }
+  throw error;
+}
+
 const classesWithMasterOnlyAccess = ['_JobStatus', '_PushStatus', '_Hooks', '_GlobalConfig', '_JobSchedule'];
 // Disallowing access to the _Role collection except by master key
 function enforceRoleSecurity(method, className, auth) {