From 3bcb5a09979e0f8161de30dc90c7e44d41c35bec Mon Sep 17 00:00:00 2001 From: Florent Vilmart <364568+flovilmart@users.noreply.github.com> Date: Thu, 28 Jun 2018 16:31:22 -0400 Subject: [PATCH] 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 | 19 +++++++ spec/ParseUser.spec.js | 77 ++++++++++++++++++++++++--- spec/helper.js | 1 - src/Auth.js | 12 ++--- src/Controllers/DatabaseController.js | 9 +++- src/RestQuery.js | 33 ++++++++---- src/RestWrite.js | 2 +- src/Routers/UsersRouter.js | 4 +- src/rest.js | 40 +++++++++----- 9 files changed, 158 insertions(+), 39 deletions(-) diff --git a/spec/ParseAPI.spec.js b/spec/ParseAPI.spec.js index 8f1ed0b5..a9919483 100644 --- a/spec/ParseAPI.spec.js +++ b/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); diff --git a/spec/ParseUser.spec.js b/spec/ParseUser.spec.js index a91ac5f8..c453ee97 100644 --- a/spec/ParseUser.spec.js +++ b/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(); }) } diff --git a/spec/helper.js b/spec/helper.js index b7dd8138..d2914afd 100644 --- a/spec/helper.js +++ b/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; diff --git a/src/Auth.js b/src/Auth.js index 1cb2e965..8658f130 100644 --- a/src/Auth.js +++ b/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) { - return true; + if (this.user) { + 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(), diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 0b3b1ef5..f28d51d1 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -869,7 +869,8 @@ class DatabaseController { op, distinct, pipeline, - readPreference + readPreference, + isWrite, }: any = {}): Promise { 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) { diff --git a/src/RestQuery.js b/src/RestQuery.js index 51341028..95e2cb16 100644 --- a/src/RestQuery.js +++ b/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 - const aclSet = new Set([].concat(this.findOptions.acl, roles)); - this.findOptions.acl = Array.from(aclSet); + + this.findOptions.acl = ['*']; + + 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') { diff --git a/src/RestWrite.js b/src/RestWrite.js index fae37a1f..0e246c3f 100644 --- a/src/RestWrite.js +++ b/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}.`); } diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 2e8af004..3c8690ea 100644 --- a/src/Routers/UsersRouter.js +++ b/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. diff --git a/src/rest.js b/src/rest.js index b428f43c..7ff4c852 100644 --- a/src/rest.js +++ b/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) { - originalRestObject = response.results[0]; + if (results && results.length) { + originalRestObject = results[0]; } - - var write = new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK); - return write.execute(); + return new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK) + .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) {