From f3e8bb2ad2f7c62ab5e20ae8bdbcf214d092e27c Mon Sep 17 00:00:00 2001 From: Musa Yassin-Fort Date: Tue, 23 Jun 2020 15:25:12 -0400 Subject: [PATCH 1/7] Optimize CLP pointer query --- src/Controllers/DatabaseController.js | 42 +++++++++++++++++++-------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index b56a8d3efc..d1f5931176 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1570,23 +1570,41 @@ class DatabaseController { objectId: userId, }; - const ors = permFields.flatMap((key) => { - // constraint for single pointer setup - const q = { - [key]: userPointer, - }; - // constraint for users-array setup - const qa = { - [key]: { $all: [userPointer] }, - }; + const ors = permFields.map((key) => { + console.log( + 'Key details is:', + query, + key, + className, + schema.getExpectedType(className, key) + ); + + const fieldDescriptor = schema.getExpectedType(className, key); + const fieldType = fieldDescriptor.type; + let queryClause; + + if (fieldType === 'Pointer') { + // constraint for single pointer setup + queryClause = { [key]: userPointer }; + } else if (fieldType === 'Array') { + // constraint for users-array setup + queryClause = { [key]: { $all: [userPointer] } }; + } else { + // This means that there is a CLP field of an unexpected type. This condition should not happen, which is + // why is being treated as an error. + throw Error( + `Unexpected condition occurred when resolving pointer permissions: ${className} ${key} ${fieldType}` + ); + } // if we already have a constraint on the key, use the $and if (Object.prototype.hasOwnProperty.call(query, key)) { - return [{ $and: [q, query] }, { $and: [qa, query] }]; + return { $and: [queryClause, query] }; } // otherwise just add the constaint - return [Object.assign({}, query, q), Object.assign({}, query, qa)]; + return Object.assign({}, query, queryClause); }); - return { $or: ors }; + + return ors.length === 1 ? ors[0] : { $or: ors }; } else { return query; } From 649c37bef6d486a15c43ff622001d098d2ea98df Mon Sep 17 00:00:00 2001 From: Musa Yassin-Fort Date: Tue, 23 Jun 2020 16:00:21 -0400 Subject: [PATCH 2/7] remove console log --- src/Controllers/DatabaseController.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index d1f5931176..14fb7e9752 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1571,14 +1571,6 @@ class DatabaseController { }; const ors = permFields.map((key) => { - console.log( - 'Key details is:', - query, - key, - className, - schema.getExpectedType(className, key) - ); - const fieldDescriptor = schema.getExpectedType(className, key); const fieldType = fieldDescriptor.type; let queryClause; From bfad56a183bd382e8a2ba39fe079f942d542cc7c Mon Sep 17 00:00:00 2001 From: Musa Yassin-Fort Date: Tue, 23 Jun 2020 19:58:00 -0400 Subject: [PATCH 3/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d719f9fca2..868cbafc65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### master [Full Changelog](https://github.com/parse-community/parse-server/compare/4.2.0...master) +- FIX: Optimize query decoration based on pointer CLPs by looking at the class schema to determine the field's type. ### 4.2.0 [Full Changelog](https://github.com/parse-community/parse-server/compare/4.1.0...4.2.0) From 42c2dd843826e80bd5c4f103d64608cc882cafde Mon Sep 17 00:00:00 2001 From: Musa Yassin-Fort Date: Tue, 23 Jun 2020 20:49:30 -0400 Subject: [PATCH 4/7] Fix flow type checker issues --- src/Controllers/DatabaseController.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 14fb7e9752..a89ce8c91f 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1572,7 +1572,13 @@ class DatabaseController { const ors = permFields.map((key) => { const fieldDescriptor = schema.getExpectedType(className, key); - const fieldType = fieldDescriptor.type; + const fieldType = + fieldDescriptor && + typeof fieldDescriptor === 'object' && + Object.prototype.hasOwnProperty.call(fieldDescriptor, 'type') + ? fieldDescriptor.type + : null; + let queryClause; if (fieldType === 'Pointer') { @@ -1585,7 +1591,7 @@ class DatabaseController { // This means that there is a CLP field of an unexpected type. This condition should not happen, which is // why is being treated as an error. throw Error( - `Unexpected condition occurred when resolving pointer permissions: ${className} ${key} ${fieldType}` + `An unexpected condition ocurred when resolving pointer permissions: ${className} ${key}` ); } // if we already have a constraint on the key, use the $and From b86d7ebc1085e0ca7680b89e91a9097ee9d7a523 Mon Sep 17 00:00:00 2001 From: mess Date: Wed, 24 Jun 2020 15:42:55 -0400 Subject: [PATCH 5/7] Remove unused properties --- spec/DatabaseController.spec.js | 209 +++++++++++++++++++++++++- src/Controllers/DatabaseController.js | 4 +- 2 files changed, 204 insertions(+), 9 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index af373e64f2..51916186be 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -1,9 +1,9 @@ const DatabaseController = require('../lib/Controllers/DatabaseController.js'); const validateQuery = DatabaseController._validateQuery; -describe('DatabaseController', function() { - describe('validateQuery', function() { - it('should not restructure simple cases of SERVER-13732', done => { +describe('DatabaseController', function () { + describe('validateQuery', function () { + it('should not restructure simple cases of SERVER-13732', (done) => { const query = { $or: [{ a: 1 }, { a: 2 }], _rperm: { $in: ['a', 'b'] }, @@ -18,7 +18,7 @@ describe('DatabaseController', function() { done(); }); - it('should not restructure SERVER-13732 queries with $nears', done => { + it('should not restructure SERVER-13732 queries with $nears', (done) => { let query = { $or: [{ a: 1 }, { b: 1 }], c: { $nearSphere: {} } }; validateQuery(query); expect(query).toEqual({ @@ -31,7 +31,7 @@ describe('DatabaseController', function() { done(); }); - it('should not push refactored keys down a tree for SERVER-13732', done => { + it('should not push refactored keys down a tree for SERVER-13732', (done) => { const query = { a: 1, $or: [{ $or: [{ b: 1 }, { b: 2 }] }, { $or: [{ c: 1 }, { c: 2 }] }], @@ -45,14 +45,209 @@ describe('DatabaseController', function() { done(); }); - it('should reject invalid queries', done => { + it('should reject invalid queries', (done) => { expect(() => validateQuery({ $or: { a: 1 } })).toThrow(); done(); }); - it('should accept valid queries', done => { + it('should accept valid queries', (done) => { expect(() => validateQuery({ $or: [{ a: 1 }, { b: 2 }] })).not.toThrow(); done(); }); }); + + describe('addPointerPermissions', function () { + const CLASS_NAME = 'Foo'; + const USER_ID = 'userId'; + const ACL_GROUP = [USER_ID]; + const OPERATION = 'find'; + + const databaseController = new DatabaseController(); + const schemaController = jasmine.createSpyObj('SchemaController', [ + 'testPermissionsForClassName', + 'getClassLevelPermissions', + 'getExpectedType', + ]); + + it('should not decorate query if no pointer CLPs are present', (done) => { + const clp = buildCLP(); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(true); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ ...query }); + + done(); + }); + + it('should decorate query if a pointer CLP is present', (done) => { + const clp = buildCLP(['user']); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Pointer' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ ...query, user: createUserPointer(USER_ID) }); + + done(); + }); + + it('should decorate query if an array CLP is present', (done) => { + const clp = buildCLP(['users']); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'users') + .and.returnValue({ type: 'Array' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ + ...query, + users: { $all: [createUserPointer(USER_ID)] }, + }); + + done(); + }); + + it('should decorate query if a pointer CLP is present and the same field is part of the query', (done) => { + const clp = buildCLP(['user']); + const query = { a: 'b', user: 'a' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Pointer' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ + $and: [{ user: createUserPointer(USER_ID) }, { ...query }], + }); + + done(); + }); + + it('should transform the query to an $or query if multiple array/pointer CLPs are present', (done) => { + const clp = buildCLP(['user', 'users']); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Pointer' }); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'users') + .and.returnValue({ type: 'Array' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ + $or: [ + { ...query, user: createUserPointer(USER_ID) }, + { ...query, users: { $all: [createUserPointer(USER_ID)] } }, + ], + }); + + done(); + }); + }); }); + +function buildCLP(pointerNames) { + const OPERATIONS = [ + 'count', + 'find', + 'get', + 'create', + 'update', + 'delete', + 'addField', + ]; + + const clp = OPERATIONS.reduce((acc, op) => { + acc[op] = {}; + + if (pointerNames && pointerNames.length) { + acc[op].pointerFields = pointerNames; + } + + return acc; + }, {}); + + clp.protectedFields = {}; + clp.writeUserFields = []; + clp.readUserFields = []; + + return clp; +} + +function createUserPointer(userId) { + return { + __type: 'Pointer', + className: '_User', + objectId: userId, + }; +} diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index a89ce8c91f..b70e1e8960 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1570,7 +1570,7 @@ class DatabaseController { objectId: userId, }; - const ors = permFields.map((key) => { + const queries = permFields.map((key) => { const fieldDescriptor = schema.getExpectedType(className, key); const fieldType = fieldDescriptor && @@ -1602,7 +1602,7 @@ class DatabaseController { return Object.assign({}, query, queryClause); }); - return ors.length === 1 ? ors[0] : { $or: ors }; + return queries.length === 1 ? queries[0] : { $or: queries }; } else { return query; } From 241256abb1ce061d0ec9cea6b6953d8f997b3ed3 Mon Sep 17 00:00:00 2001 From: mess Date: Sun, 5 Jul 2020 10:25:04 -0400 Subject: [PATCH 6/7] Fix typo, add one more test case for coverage --- spec/DatabaseController.spec.js | 31 +++++++++++++++++++++++++++ src/Controllers/DatabaseController.js | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index 51916186be..5fd7bfec77 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -213,6 +213,37 @@ describe('DatabaseController', function () { done(); }); + + it('should throw an error if for some unexpected reason the property specified in the CLP is neither a pointer nor an array', (done) => { + const clp = buildCLP(['user']); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Number' }); + + expect(() => { + databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + }).toThrow( + Error( + `An unexpected condition occurred when resolving pointer permissions: ${CLASS_NAME} user` + ) + ); + + done(); + }); }); }); diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index b70e1e8960..06ef7f4411 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1591,7 +1591,7 @@ class DatabaseController { // This means that there is a CLP field of an unexpected type. This condition should not happen, which is // why is being treated as an error. throw Error( - `An unexpected condition ocurred when resolving pointer permissions: ${className} ${key}` + `An unexpected condition occurred when resolving pointer permissions: ${className} ${key}` ); } // if we already have a constraint on the key, use the $and From ea1a4785765e5c584c49bc3316e9681fa1d96e6a Mon Sep 17 00:00:00 2001 From: mess Date: Tue, 7 Jul 2020 23:19:25 -0400 Subject: [PATCH 7/7] Add support for CLP entry of type Object --- spec/DatabaseController.spec.js | 40 +++++++++++++++++++++++++-- src/Controllers/DatabaseController.js | 3 ++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/spec/DatabaseController.spec.js b/spec/DatabaseController.spec.js index 5fd7bfec77..e2df0f1e95 100644 --- a/spec/DatabaseController.spec.js +++ b/spec/DatabaseController.spec.js @@ -93,7 +93,7 @@ describe('DatabaseController', function () { done(); }); - it('should decorate query if a pointer CLP is present', (done) => { + it('should decorate query if a pointer CLP entry is present', (done) => { const clp = buildCLP(['user']); const query = { a: 'b' }; @@ -120,7 +120,7 @@ describe('DatabaseController', function () { done(); }); - it('should decorate query if an array CLP is present', (done) => { + it('should decorate query if an array CLP entry is present', (done) => { const clp = buildCLP(['users']); const query = { a: 'b' }; @@ -150,6 +150,36 @@ describe('DatabaseController', function () { done(); }); + it('should decorate query if an object CLP entry is present', (done) => { + const clp = buildCLP(['user']); + const query = { a: 'b' }; + + schemaController.testPermissionsForClassName + .withArgs(CLASS_NAME, ACL_GROUP, OPERATION) + .and.returnValue(false); + schemaController.getClassLevelPermissions + .withArgs(CLASS_NAME) + .and.returnValue(clp); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'user') + .and.returnValue({ type: 'Object' }); + + const output = databaseController.addPointerPermissions( + schemaController, + CLASS_NAME, + OPERATION, + query, + ACL_GROUP + ); + + expect(output).toEqual({ + ...query, + user: createUserPointer(USER_ID), + }); + + done(); + }); + it('should decorate query if a pointer CLP is present and the same field is part of the query', (done) => { const clp = buildCLP(['user']); const query = { a: 'b', user: 'a' }; @@ -180,7 +210,7 @@ describe('DatabaseController', function () { }); it('should transform the query to an $or query if multiple array/pointer CLPs are present', (done) => { - const clp = buildCLP(['user', 'users']); + const clp = buildCLP(['user', 'users', 'userObject']); const query = { a: 'b' }; schemaController.testPermissionsForClassName @@ -195,6 +225,9 @@ describe('DatabaseController', function () { schemaController.getExpectedType .withArgs(CLASS_NAME, 'users') .and.returnValue({ type: 'Array' }); + schemaController.getExpectedType + .withArgs(CLASS_NAME, 'userObject') + .and.returnValue({ type: 'Object' }); const output = databaseController.addPointerPermissions( schemaController, @@ -208,6 +241,7 @@ describe('DatabaseController', function () { $or: [ { ...query, user: createUserPointer(USER_ID) }, { ...query, users: { $all: [createUserPointer(USER_ID)] } }, + { ...query, userObject: createUserPointer(USER_ID) }, ], }); diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 06ef7f4411..6bd7790907 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -1587,6 +1587,9 @@ class DatabaseController { } else if (fieldType === 'Array') { // constraint for users-array setup queryClause = { [key]: { $all: [userPointer] } }; + } else if (fieldType === 'Object') { + // constraint for object setup + queryClause = { [key]: userPointer }; } else { // This means that there is a CLP field of an unexpected type. This condition should not happen, which is // why is being treated as an error.