CLP objectId size validation fix (#6332)

* Relax regex for customId ; allow varying id length * test * remove trycatch, fix typo * de-duplicate test names; test pointer targetclass * fixed early return; detailed errors for protected
2020-01-14 12:01:14 +03:00
parent 9842c6ee42
commit 2d257e20a0
4 changed files with 225 additions and 88 deletions
--- a/spec/PointerPermissions.spec.js
+++ b/spec/PointerPermissions.spec.js
@@ -399,7 +399,7 @@ describe('Pointer Permissions', () => {
        });
    });

-    it('should prevent creating pointer permission on bad field', done => {
+    it('should prevent creating pointer permission on bad field (of wrong type)', done => {
      const config = Config.get(Parse.applicationId);
      config.database
        .loadSchema()
@@ -426,7 +426,34 @@ describe('Pointer Permissions', () => {
        });
    });

-    it('should prevent creating pointer permission on bad field', done => {
+    it('should prevent creating pointer permission on bad field (non-user pointer)', done => {
+      const config = Config.get(Parse.applicationId);
+      config.database
+        .loadSchema()
+        .then(schema => {
+          return schema.addClassIfNotExists(
+            'AnObject',
+            { owner: { type: 'Pointer', targetClass: '_Session' } },
+            {
+              create: {},
+              writeUserFields: ['owner'],
+              readUserFields: ['owner'],
+            }
+          );
+        })
+        .then(() => {
+          fail('should not succeed');
+        })
+        .catch(err => {
+          expect(err.code).toBe(107);
+          expect(err.message).toBe(
+            "'owner' is not a valid column for class level pointer permissions writeUserFields"
+          );
+          done();
+        });
+    });
+
+    it('should prevent creating pointer permission on bad field (non-existing)', done => {
      const config = Config.get(Parse.applicationId);
      const object = new Parse.Object('AnObject');
      object.set('owner', 'value');
@@ -984,7 +1011,7 @@ describe('Pointer Permissions', () => {
        );
    });

-    it('should fail with invalid pointer perms', done => {
+    it('should fail with invalid pointer perms (not array)', done => {
      const config = Config.get(Parse.applicationId);
      config.database
        .loadSchema()
@@ -1002,7 +1029,7 @@ describe('Pointer Permissions', () => {
        });
    });

-    it('should fail with invalid pointer perms', done => {
+    it('should fail with invalid pointer perms (non-existing field)', done => {
      const config = Config.get(Parse.applicationId);
      config.database
        .loadSchema()
@@ -1398,7 +1425,7 @@ describe('Pointer Permissions', () => {
      }
    });

-    it('should prevent creating pointer permission on bad field', async done => {
+    it('should prevent creating pointer permission on bad field (of wrong type)', async done => {
      const config = Config.get(Parse.applicationId);
      const schema = await config.database.loadSchema();
      try {
@@ -1421,7 +1448,7 @@ describe('Pointer Permissions', () => {
      }
    });

-    it('should prevent creating pointer permission on bad field', async done => {
+    it('should prevent creating pointer permission on bad field (non-existing)', async done => {
      const config = Config.get(Parse.applicationId);
      const object = new Parse.Object('AnObject');
      object.set('owners', 'value');
@@ -1955,7 +1982,7 @@ describe('Pointer Permissions', () => {
      }
    });

-    it('should fail with invalid pointer perms', async done => {
+    it('should fail with invalid pointer perms (not array)', async done => {
      const config = Config.get(Parse.applicationId);
      const schema = await config.database.loadSchema();
      try {
@@ -1971,7 +1998,7 @@ describe('Pointer Permissions', () => {
      }
    });

-    it('should fail with invalid pointer perms', async done => {
+    it('should fail with invalid pointer perms (non-existing field)', async done => {
      const config = Config.get(Parse.applicationId);
      const schema = await config.database.loadSchema();
      try {
--- a/spec/Schema.spec.js
+++ b/spec/Schema.spec.js
@@ -1665,7 +1665,7 @@ describe('Class Level Permissions for requiredAuth', () => {
      );
  });

-  it('required auth test create/get/update/delete not authenitcated', done => {
+  it('required auth test get not authenitcated', done => {
    config.database
      .loadSchema()
      .then(schema => {
@@ -1677,12 +1677,6 @@ describe('Class Level Permissions for requiredAuth', () => {
          get: {
            requiresAuthentication: true,
          },
-          delete: {
-            requiresAuthentication: true,
-          },
-          update: {
-            requiresAuthentication: true,
-          },
          create: {
            '*': true,
          },
@@ -1710,7 +1704,7 @@ describe('Class Level Permissions for requiredAuth', () => {
      );
  });

-  it('required auth test create/get/update/delete not authenitcated', done => {
+  it('required auth test find not authenitcated', done => {
    config.database
      .loadSchema()
      .then(schema => {
@@ -1722,12 +1716,6 @@ describe('Class Level Permissions for requiredAuth', () => {
          find: {
            requiresAuthentication: true,
          },
-          delete: {
-            requiresAuthentication: true,
-          },
-          update: {
-            requiresAuthentication: true,
-          },
          create: {
            '*': true,
          },
--- a/spec/schemas.spec.js
+++ b/spec/schemas.spec.js
@@ -1835,8 +1835,14 @@ describe('schemas', () => {
    });
  });

-  it('should throw with invalid userId (>10 chars)', done => {
-    request({
+  it('should aceept class-level permission with userid of any length', async done => {
+    await global.reconfigureServer({
+      customIdSize: 11,
+    });
+
+    const id = 'e1evenChars';
+
+    const { data } = await request({
      method: 'POST',
      url: 'http://localhost:8378/1/schemas/AClass',
      headers: masterKeyHeaders,
@@ -1844,20 +1850,25 @@ describe('schemas', () => {
      body: {
        classLevelPermissions: {
          find: {
-            '1234567890A': true,
+            [id]: true,
          },
        },
      },
-    }).then(fail, response => {
-      expect(response.data.error).toEqual(
-        "'1234567890A' is not a valid key for class level permissions"
-      );
-      done();
    });
+
+    expect(data.classLevelPermissions.find[id]).toBe(true);
+
+    done();
  });

-  it('should throw with invalid userId (<10 chars)', done => {
-    request({
+  it('should allow set class-level permission for custom userid of any length and chars', async done => {
+    await global.reconfigureServer({
+      allowCustomObjectId: true,
+    });
+
+    const symbolsId = 'set:ID+symbol$=@llowed';
+    const shortId = '1';
+    const { data } = await request({
      method: 'POST',
      url: 'http://localhost:8378/1/schemas/AClass',
      headers: masterKeyHeaders,
@@ -1865,16 +1876,53 @@ describe('schemas', () => {
      body: {
        classLevelPermissions: {
          find: {
-            a12345678: true,
+            [symbolsId]: true,
+            [shortId]: true,
          },
        },
      },
-    }).then(fail, response => {
-      expect(response.data.error).toEqual(
-        "'a12345678' is not a valid key for class level permissions"
-      );
-      done();
    });
+
+    expect(data.classLevelPermissions.find[symbolsId]).toBe(true);
+    expect(data.classLevelPermissions.find[shortId]).toBe(true);
+
+    done();
+  });
+
+  it('should allow set ACL for custom userid', async done => {
+    await global.reconfigureServer({
+      allowCustomObjectId: true,
+    });
+
+    const symbolsId = 'symbols:id@allowed=';
+    const shortId = '1';
+    const normalId = 'tensymbols';
+
+    const { data } = await request({
+      method: 'POST',
+      url: 'http://localhost:8378/1/classes/AClass',
+      headers: masterKeyHeaders,
+      json: true,
+      body: {
+        ACL: {
+          [symbolsId]: { read: true, write: true },
+          [shortId]: { read: true, write: true },
+          [normalId]: { read: true, write: true },
+        },
+      },
+    });
+
+    const { data: created } = await request({
+      method: 'GET',
+      url: `http://localhost:8378/1/classes/AClass/${data.objectId}`,
+      headers: masterKeyHeaders,
+      json: true,
+    });
+
+    expect(created.ACL[normalId].write).toBe(true);
+    expect(created.ACL[symbolsId].write).toBe(true);
+    expect(created.ACL[shortId].write).toBe(true);
+    done();
  });

  it('should throw with invalid userId (invalid char)', done => {
--- a/src/Controllers/SchemaController.js
+++ b/src/Controllers/SchemaController.js
@@ -173,8 +173,6 @@ const volatileClasses = Object.freeze([
  '_Audience',
 ]);

-// 10 alpha numberic chars + uppercase
-const userIdRegex = /^[a-zA-Z0-9]{10}$/;
 // Anything that start with role
 const roleRegex = /^role:.*/;
 // Anything that starts with userField
@@ -185,19 +183,23 @@ const publicRegex = /^\*$/;
 const requireAuthenticationRegex = /^requiresAuthentication$/;

 const permissionKeyRegex = Object.freeze([
-  userIdRegex,
  roleRegex,
  pointerPermissionRegex,
  publicRegex,
  requireAuthenticationRegex,
 ]);

-function verifyPermissionKey(key) {
-  const result = permissionKeyRegex.reduce((isGood, regEx) => {
-    isGood = isGood || key.match(regEx) != null;
-    return isGood;
-  }, false);
-  if (!result) {
+function validatePermissionKey(key, userIdRegExp) {
+  let matchesSome = false;
+  for (const regEx of permissionKeyRegex) {
+    if (key.match(regEx) !== null) {
+      matchesSome = true;
+      break;
+    }
+  }
+
+  const valid = matchesSome || key.match(userIdRegExp) !== null;
+  if (!valid) {
    throw new Parse.Error(
      Parse.Error.INVALID_JSON,
      `'${key}' is not a valid key for class level permissions`
@@ -217,66 +219,130 @@ const CLPValidKeys = Object.freeze([
  'writeUserFields',
  'protectedFields',
 ]);
-function validateCLP(perms: ClassLevelPermissions, fields: SchemaFields) {
+
+// validation before setting class-level permissions on collection
+function validateCLP(
+  perms: ClassLevelPermissions,
+  fields: SchemaFields,
+  userIdRegExp: RegExp
+) {
  if (!perms) {
    return;
  }
-  Object.keys(perms).forEach(operation => {
-    if (CLPValidKeys.indexOf(operation) == -1) {
+  for (const operationKey in perms) {
+    if (CLPValidKeys.indexOf(operationKey) == -1) {
      throw new Parse.Error(
        Parse.Error.INVALID_JSON,
-        `${operation} is not a valid operation for class level permissions`
+        `${operationKey} is not a valid operation for class level permissions`
      );
    }
-    if (!perms[operation]) {
-      return;
+
+    const operation = perms[operationKey];
+    if (!operation) {
+      // proceed with next operationKey
+      continue;
    }

-    if (operation === 'readUserFields' || operation === 'writeUserFields') {
-      if (!Array.isArray(perms[operation])) {
-        // @flow-disable-next
+    // validate grouped pointer permissions
+    if (
+      operationKey === 'readUserFields' ||
+      operationKey === 'writeUserFields'
+    ) {
+      // must be an array with field names
+      if (!Array.isArray(operation)) {
        throw new Parse.Error(
          Parse.Error.INVALID_JSON,
-          `'${perms[operation]}' is not a valid value for class level permissions ${operation}`
+          `'${operation}' is not a valid value for class level permissions ${operationKey}`
        );
      } else {
-        perms[operation].forEach(key => {
-          if (
-            !(
-              fields[key] &&
-              ((fields[key].type == 'Pointer' &&
-                fields[key].targetClass == '_User') ||
-                fields[key].type == 'Array')
-            )
-          ) {
-            throw new Parse.Error(
-              Parse.Error.INVALID_JSON,
-              `'${key}' is not a valid column for class level pointer permissions ${operation}`
-            );
-          }
-        });
+        for (const fieldName of operation) {
+          validatePointerPermission(fieldName, fields, operationKey);
+        }
      }
-      return;
+      // readUserFields and writerUserFields do not have nesdted fields
+      // proceed with next operationKey
+      continue;
    }

-    // @flow-disable-next
-    Object.keys(perms[operation]).forEach(key => {
-      verifyPermissionKey(key);
-      // @flow-disable-next
-      const perm = perms[operation][key];
-      if (
-        perm !== true &&
-        (operation !== 'protectedFields' || !Array.isArray(perm))
-      ) {
-        // @flow-disable-next
+    // validate protected fields
+    if (operationKey === 'protectedFields') {
+      for (const entity in operation) {
+        // throws on unexpected key
+        validatePermissionKey(entity, userIdRegExp);
+
+        const protectedFields = operation[entity];
+
+        if (!Array.isArray(protectedFields)) {
+          throw new Parse.Error(
+            Parse.Error.INVALID_JSON,
+            `'${protectedFields}' is not a valid value for protectedFields[${entity}] - expected an array.`
+          );
+        }
+
+        // if the field is in form of array
+        for (const field of protectedFields) {
+          // field should exist on collection
+          if (!Object.prototype.hasOwnProperty.call(fields, field)) {
+            throw new Parse.Error(
+              Parse.Error.INVALID_JSON,
+              `Field '${field}' in protectedFields:${entity} does not exist`
+            );
+          }
+        }
+      }
+      // proceed with next operationKey
+      continue;
+    }
+
+    // validate other fields
+    // Entity can be:
+    // "*" - Public,
+    // "requiresAuthentication" - authenticated users,
+    // "objectId" - _User id,
+    // "role:objectId",
+    for (const entity in operation) {
+      // throws on unexpected key
+      validatePermissionKey(entity, userIdRegExp);
+
+      const permit = operation[entity];
+
+      if (permit !== true) {
        throw new Parse.Error(
          Parse.Error.INVALID_JSON,
-          `'${perm}' is not a valid value for class level permissions ${operation}:${key}:${perm}`
+          `'${permit}' is not a valid value for class level permissions ${operationKey}:${entity}:${permit}`
        );
      }
-    });
-  });
+    }
+  }
 }
+
+function validatePointerPermission(
+  fieldName: string,
+  fields: Object,
+  operation: string
+) {
+  // Uses collection schema to ensure the field is of type:
+  // - Pointer<_User> (pointers/relations)
+  // - Array
+  //
+  //    It's not possible to enforce type on Array's items in schema
+  //  so we accept any Array field, and later when applying permissions
+  //  only items that are pointers to _User are considered.
+  if (
+    !(
+      fields[fieldName] &&
+      ((fields[fieldName].type == 'Pointer' &&
+        fields[fieldName].targetClass == '_User') ||
+        fields[fieldName].type == 'Array')
+    )
+  ) {
+    throw new Parse.Error(
+      Parse.Error.INVALID_JSON,
+      `'${fieldName}' is not a valid column for class level pointer permissions ${operation}`
+    );
+  }
+}
+
 const joinClassRegex = /^_Join:[A-Za-z0-9_]+:[A-Za-z0-9_]+/;
 const classAndFieldRegex = /^[A-Za-z][A-Za-z0-9_]*$/;
 function classNameIsValid(className: string): boolean {
@@ -558,12 +624,20 @@ export default class SchemaController {
  _cache: any;
  reloadDataPromise: ?Promise<any>;
  protectedFields: any;
+  userIdRegEx: RegExp;

  constructor(databaseAdapter: StorageAdapter, schemaCache: any) {
    this._dbAdapter = databaseAdapter;
    this._cache = schemaCache;
    this.schemaData = new SchemaData();
    this.protectedFields = Config.get(Parse.applicationId).protectedFields;
+
+    const customIds = Config.get(Parse.applicationId).allowCustomObjectId;
+
+    const customIdRegEx = /^.{1,}$/u; // 1+ chars
+    const autoIdRegEx = /^[a-zA-Z0-9]{1,}$/;
+
+    this.userIdRegEx = customIds ? customIdRegEx : autoIdRegEx;
  }

  reloadData(options: LoadSchemaOptions = { clearCache: false }): Promise<any> {
@@ -959,7 +1033,7 @@ export default class SchemaController {
          ' already exists.',
      };
    }
-    validateCLP(classLevelPermissions, fields);
+    validateCLP(classLevelPermissions, fields, this.userIdRegEx);
  }

  // Sets the Class-level permissions for a given className, which must exist.
@@ -967,7 +1041,7 @@ export default class SchemaController {
    if (typeof perms === 'undefined') {
      return Promise.resolve();
    }
-    validateCLP(perms, newSchema);
+    validateCLP(perms, newSchema, this.userIdRegEx);
    return this._dbAdapter.setClassLevelPermissions(className, perms);
  }