Add user ID validation and custom error handling in ModelWithPermissions #7605
Conversation
… appending permissions
…nvalid user ID checks
| @@ -13,6 +13,7 @@ import { | |||
| UwaziQueryOptions, | |||
| EnforcedWithId, | |||
| } from './model'; | |||
| import { ObjectId } from 'mongodb'; | |||
There was a problem hiding this comment.
Let's try to adhere to the hierarchy of imports:
- external modules
- absolute paths
- relative paths
| @@ -127,20 +128,44 @@ const controlPermissionsData = <T>( | |||
| return { ...data, permissions: undefined }; | |||
| }; | |||
|
|
|||
| export class InvalidUserIdError extends Error { | |||
There was a problem hiding this comment.
I'm unsure about the Error practices we defined. Are we extending native JS Error or are we using our Uwazi Error? I thought we agreed the latter, but maybe I misunderstood something?
| private static validateUser(user: DataType<UserSchema> | undefined) { | ||
| try { | ||
| if (typeof user === 'undefined') return; | ||
| ObjectId.createFromHexString(user?._id?.toString()!); |
There was a problem hiding this comment.
Is it your IDE Typescript demanding all these type conditionals? If you are already returning for user === undefined, what is the possibility of here user needing the ? ? And toString() with a !. If you have _id and it is of the right class, how can toString() still require the !?
| 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' }, |
There was a problem hiding this comment.
Under what condition can the _id of mockedUser._id can be undefined? I'm concerned about the IDE configuration and typing throughout these commit.
| @@ -18,6 +18,7 @@ describe('ModelWithPermissions', () => { | |||
| permissions: { type: mongoose.Schema.Types.Mixed, select: false }, | |||
| fixed: Boolean, | |||
| }); | |||
| const userId = testingDB.id(); | |||
There was a problem hiding this comment.
This file used user1 and user2 (in line 67 for example).
Probably these are not being used in the test, but maybe it's worth it to create a:
const user1Id = testingDB.id();
const user2Id = testingDB.id();
and replicate the original structure but removing the deprecated practice of assigning strings to the user?
fixes #7389