Add user ID validation and custom error handling in ModelWithPermissions

app/api/entities/specs/entitySavingManager.spec.ts

@@ -11,6 +11,7 @@ import { writeFile } from 'fs/promises';
 import { ObjectId } from 'mongodb';
 import path from 'path';
 import { EntityWithFilesSchema } from 'shared/types/entityType';
+import { UserInContextMockFactory } from 'api/utils/testingUserInContext';
 import waitForExpect from 'wait-for-expect';
 import entities from '../entities';
 import {
@@ -39,6 +40,8 @@ trailer<</Root 1 0 R>>
 const tmpDir = (filename: string) => path.join(os.tmpdir(), filename);
 
 describe('entitySavingManager', () => {
+  const userInContextMock = new UserInContextMockFactory();
+
   const file = {
     originalname: 'sampleFile.txt',
     mimetype: 'text/plain',
@@ -81,11 +84,12 @@ describe('entitySavingManager', () => {
 
     describe('new entity', () => {
       it('should create an entity without attachments', async () => {
+        const mockedUser = userInContextMock.mockEditorUser();
         const entity = { title: 'newEntity', template: template1Id };
         const { entity: savedEntity } = await saveEntity(entity, { ...reqData });
 
         expect(savedEntity.permissions).toEqual([
-          { level: 'write', refId: 'userId', type: 'user' },
+          { level: 'write', refId: mockedUser._id.toString(), type: 'user' },
         ]);
       });
 

app/api/odm/ModelWithPermissions.ts

@@ -1,9 +1,12 @@
+import { ObjectId } from 'mongodb';
 import mongoose from 'mongoose';
+
 import { permissionsContext } from 'api/permissions/permissionsContext';
 import { AccessLevels, PermissionType } from 'shared/types/permissionSchema';
 import { UserSchema } from 'shared/types/userType';
 import { PermissionSchema } from 'shared/types/permissionType';
 import { ObjectIdSchema } from 'shared/types/commonTypes';
+
 import { createUpdateLogHelper } from './logHelper';
 import {
   DataType,
@@ -127,20 +130,44 @@ const controlPermissionsData = <T>(
   return { ...data, permissions: undefined };
 };
 
+export class InvalidUserIdError extends Error {
+  constructor() {
+    super('You are trying to persist a userId that is invalid');
+    this.name = 'InvalidUserIdError';
+  }
+}
+
 export class ModelWithPermissions<T> extends OdmModel<WithPermissions<T>> {
+  private static validateUser(user: DataType<UserSchema> | undefined) {
+    try {
+      if (typeof user?._id === 'undefined') return;
+      ObjectId.createFromHexString(user._id.toString());
+    } catch (e) {
+      throw new InvalidUserIdError();
+    }
+  }
+
   async save(data: WithPermissionsDataType<T>) {
     const user = permissionsContext.getUserInContext();
     const query = { _id: data._id };
-    return data._id || data.permissions
-      ? super.save(data, appendPermissionQuery(query, AccessLevels.WRITE, user))
-      : super.save(appendPermissionData(data, user));
+
+    if (data._id || data.permissions) {
+      return super.save(data, appendPermissionQuery(query, AccessLevels.WRITE, user));
+    }
+
+    ModelWithPermissions.validateUser(user);
+    return super.save(appendPermissionData(data, user));
   }
 
   async saveMultiple(dataArray: WithPermissionsDataType<T>[]) {
     const user = permissionsContext.getUserInContext();
-    const dataArrayWithPermissions = dataArray.map(data =>
-      data._id || data.permissions ? data : appendPermissionData(data, user)
-    );
+
+    const dataArrayWithPermissions = dataArray.map(data => {
+      if (data._id || data.permissions) return data;
+
+      ModelWithPermissions.validateUser(user);
+      return appendPermissionData(data, user);
+    });
     const query = appendPermissionQuery({}, AccessLevels.WRITE, user);
     return super.saveMultiple(dataArrayWithPermissions, query, !!user);
   }

app/api/odm/specs/ModelWithPermissions.spec.ts

@@ -19,6 +19,8 @@ describe('ModelWithPermissions', () => {
     permissions: { type: mongoose.Schema.Types.Mixed, select: false },
     fixed: Boolean,
   });
+  const userId1 = testingDB.id();
+  const userId2 = testingDB.id();
   const readDocId = testingDB.id();
   const writeDocId = testingDB.id();
   const writeDoc2Id = testingDB.id();
@@ -34,19 +36,25 @@ describe('ModelWithPermissions', () => {
       _id: readDocId,
       name: 'readDoc',
       published: false,
-      permissions: [{ refId: 'user1', type: PermissionType.USER, level: AccessLevels.READ }],
+      permissions: [
+        { refId: userId1.toString(), type: PermissionType.USER, level: AccessLevels.READ },
+      ],
       fixed: true,
     },
     {
       _id: writeDocId,
       name: 'writeDoc',
-      permissions: [{ refId: 'user1', type: PermissionType.USER, level: AccessLevels.WRITE }],
+      permissions: [
+        { refId: userId1.toString(), type: PermissionType.USER, level: AccessLevels.WRITE },
+      ],
       fixed: true,
     },
     {
       _id: writeDoc2Id,
       name: 'writeDoc2',
-      permissions: [{ refId: 'user1', type: PermissionType.USER, level: AccessLevels.WRITE }],
+      permissions: [
+        { refId: userId1.toString(), type: PermissionType.USER, level: AccessLevels.WRITE },
+      ],
       fixed: true,
     },
     {
@@ -58,7 +66,9 @@ describe('ModelWithPermissions', () => {
     {
       _id: otherOwnerId,
       name: 'no shared with user',
-      permissions: [{ refId: 'user2', type: PermissionType.USER, level: AccessLevels.WRITE }],
+      permissions: [
+        { refId: userId2.toString(), type: PermissionType.USER, level: AccessLevels.WRITE },
+      ],
       fixed: true,
     },
     {
@@ -81,7 +91,9 @@ describe('ModelWithPermissions', () => {
     {
       _id: deleteDocId,
       name: 'docToDelete',
-      permissions: [{ refId: 'user1', type: PermissionType.USER, level: AccessLevels.WRITE }],
+      permissions: [
+        { refId: userId1.toString(), type: PermissionType.USER, level: AccessLevels.WRITE },
+      ],
     },
     {
       _id: public2Id,
@@ -105,7 +117,7 @@ describe('ModelWithPermissions', () => {
     describe('collaborator user', () => {
       beforeAll(async () => {
         jest.spyOn(permissionsContext, 'getUserInContext').mockReturnValue({
-          _id: 'user1',
+          _id: userId1,
           username: 'User 1',
           email: '[email protected]',
           role: 'collaborator',
@@ -229,10 +241,11 @@ describe('ModelWithPermissions', () => {
 
         it('should add the user in the permissions property of the new doc', async () => {
           const saved = await model.save({ name: 'newDoc' });
+
           expect(saved).toEqual(
             expect.objectContaining({
               name: 'newDoc',
-              permissions: [{ refId: 'user1', type: 'user', level: 'write' }],
+              permissions: [{ refId: userId1.toString(), type: 'user', level: 'write' }],
             })
           );
         });
@@ -303,11 +316,11 @@ describe('ModelWithPermissions', () => {
           expect(saved).toMatchObject([
             {
               name: 'newDoc',
-              permissions: [{ refId: 'user1', type: 'user', level: 'write' }],
+              permissions: [{ refId: userId1.toString(), type: 'user', level: 'write' }],
             },
             {
               name: 'newDoc2',
-              permissions: [{ refId: 'user1', type: 'user', level: 'write' }],
+              permissions: [{ refId: userId1.toString(), type: 'user', level: 'write' }],
             },
           ]);
         });

app/api/utils/testingUserInContext.ts

@@ -2,6 +2,7 @@ import { permissionsContext } from 'api/permissions/permissionsContext';
 import { UserSchema } from 'shared/types/userType';
 import { UserRole } from 'shared/types/userSchema';
 import { DataType } from 'api/odm';
+import { ObjectId } from 'mongodb';
 
 export class UserInContextMockFactory {
   spy: jest.SpyInstance | undefined;
@@ -11,12 +12,14 @@ export class UserInContextMockFactory {
   }
 
   mockEditorUser() {
-    this.mock({
-      _id: 'userId',
+    const user = {
+      _id: new ObjectId(),
       role: UserRole.EDITOR,
       username: 'editorUser',
       email: '[email protected]',
-    });
+    };
+    this.mock(user);
+    return user;
   }
 
   restore() {