From 816db9817a2d60c9af6b6805cc11837e2b638e6e Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 08:26:29 +0100 Subject: [PATCH 1/7] fix: apply strict null checks updates to the User Module --- .../user/controllers/auth.controller.spec.ts | 4 +- api/src/user/controllers/auth.controller.ts | 2 +- .../user/controllers/model.controller.spec.ts | 27 ++++---- .../user/controllers/user.controller.spec.ts | 69 ++++++++++--------- api/src/user/controllers/user.controller.ts | 24 ++++--- .../invitation.repository.spec.ts | 12 ++-- .../repositories/model.repository.spec.ts | 18 ++--- .../user/repositories/user.repository.spec.ts | 15 ++-- api/src/user/schemas/user.schema.ts | 2 +- api/src/user/services/auth.service.spec.ts | 2 +- .../user/services/invitation.service.spec.ts | 2 +- api/src/user/services/model.service.spec.ts | 16 ++--- .../services/passwordReset.service.spec.ts | 11 ++- api/src/user/services/user.service.spec.ts | 28 ++++---- api/src/utils/types/filter.types.ts | 10 ++- 15 files changed, 132 insertions(+), 110 deletions(-) diff --git a/api/src/user/controllers/auth.controller.spec.ts b/api/src/user/controllers/auth.controller.spec.ts index 5b3ca462..b35a5db7 100644 --- a/api/src/user/controllers/auth.controller.spec.ts +++ b/api/src/user/controllers/auth.controller.spec.ts @@ -60,7 +60,7 @@ describe('AuthController', () => { let invitationService: InvitationService; let roleService: RoleService; let jwtService: JwtService; - let role: Role; + let role: Role | null; let baseUser: UserCreateDto; beforeAll(async () => { @@ -136,7 +136,7 @@ describe('AuthController', () => { username: 'test', first_name: 'test', last_name: 'test', - roles: [role.id], + roles: [role!.id], }; await invitationService.create(baseUser); }); diff --git a/api/src/user/controllers/auth.controller.ts b/api/src/user/controllers/auth.controller.ts index 8aa2a4aa..559bc2ee 100644 --- a/api/src/user/controllers/auth.controller.ts +++ b/api/src/user/controllers/auth.controller.ts @@ -87,7 +87,7 @@ export class LocalAuthController extends BaseAuthController { logger: LoggerService, private readonly userService: UserService, private readonly validateAccountService: ValidateAccountService, - private readonly invitationService?: InvitationService, + private readonly invitationService: InvitationService, ) { super(logger); } diff --git a/api/src/user/controllers/model.controller.spec.ts b/api/src/user/controllers/model.controller.spec.ts index 549a87fb..dc221350 100644 --- a/api/src/user/controllers/model.controller.spec.ts +++ b/api/src/user/controllers/model.controller.spec.ts @@ -28,7 +28,7 @@ import { ModelRepository } from '../repositories/model.repository'; import { PermissionRepository } from '../repositories/permission.repository'; import { RoleRepository } from '../repositories/role.repository'; import { UserRepository } from '../repositories/user.repository'; -import { ModelModel } from '../schemas/model.schema'; +import { ModelFull, ModelModel } from '../schemas/model.schema'; import { PermissionModel } from '../schemas/permission.schema'; import { RoleModel } from '../schemas/role.schema'; import { UserModel } from '../schemas/user.schema'; @@ -104,21 +104,24 @@ describe('ModelController', () => { it('should find models, and for each model populate the corresponding permissions', async () => { jest.spyOn(modelService, 'findAndPopulate'); - const allRoles = await modelService.findAll(); + const allModels = await modelService.findAll(); const allPermissions = await permissionService.findAll(); const result = await modelController.find(['permissions'], {}); - const modelsWithPermissionsAndUsers = allRoles.reduce((acc, currRole) => { - const modelWithPermissionsAndUsers = { - ...currRole, - permissions: allPermissions.filter((currPermission) => { - return currPermission.role === currRole.id; - }), - }; + const modelsWithPermissionsAndUsers = allModels.reduce( + (acc, currRole) => { + const modelWithPermissionsAndUsers = { + ...currRole, + permissions: allPermissions.filter((currPermission) => { + return currPermission.role === currRole.id; + }), + }; - acc.push(modelWithPermissionsAndUsers); - return acc; - }, []); + acc.push(modelWithPermissionsAndUsers); + return acc; + }, + [] as ModelFull[], + ); expect(modelService.findAndPopulate).toHaveBeenCalledWith({}); expect(result).toEqualPayload(modelsWithPermissionsAndUsers); diff --git a/api/src/user/controllers/user.controller.spec.ts b/api/src/user/controllers/user.controller.spec.ts index 041d8e27..b4c82fb3 100644 --- a/api/src/user/controllers/user.controller.spec.ts +++ b/api/src/user/controllers/user.controller.spec.ts @@ -47,7 +47,7 @@ import { UserRepository } from '../repositories/user.repository'; import { InvitationModel } from '../schemas/invitation.schema'; import { PermissionModel } from '../schemas/permission.schema'; import { Role, RoleModel } from '../schemas/role.schema'; -import { User, UserModel } from '../schemas/user.schema'; +import { User, UserFull, UserModel } from '../schemas/user.schema'; import { PasswordResetService } from '../services/passwordReset.service'; import { PermissionService } from '../services/permission.service'; import { RoleService } from '../services/role.service'; @@ -63,9 +63,9 @@ describe('UserController', () => { let roleService: RoleService; let invitationService: InvitationService; let notFoundId: string; - let role: Role; + let role: Role | null; let roles: Role[]; - let user: User; + let user: User | null; let passwordResetService: PasswordResetService; let jwtService: JwtService; beforeAll(async () => { @@ -157,12 +157,12 @@ describe('UserController', () => { describe('findOne', () => { it('should find one user and populate its roles', async () => { jest.spyOn(userService, 'findOneAndPopulate'); - const result = await userController.findOne(user.id, ['roles']); - expect(userService.findOneAndPopulate).toHaveBeenCalledWith(user.id); + const result = await userController.findOne(user!.id, ['roles']); + expect(userService.findOneAndPopulate).toHaveBeenCalledWith(user!.id); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), - roles: roles.filter(({ id }) => user.roles.includes(id)), + roles: roles.filter(({ id }) => user!.roles.includes(id)), }, [...IGNORED_FIELDS, 'password', 'provider'], ); @@ -176,13 +176,17 @@ describe('UserController', () => { jest.spyOn(userService, 'findPageAndPopulate'); const result = await userService.findPageAndPopulate({}, pageQuery); - const usersWithRoles = userFixtures.reduce((acc, currUser) => { - acc.push({ - ...currUser, - roles: roles.filter(({ id }) => user.roles.includes(id)), - }); - return acc; - }, []); + const usersWithRoles = userFixtures.reduce( + (acc, currUser) => { + acc.push({ + ...currUser, + roles: roles.filter(({ id }) => user?.roles?.includes(id)), + avatar: null, + }); + return acc; + }, + [] as Omit[], + ); expect(userService.findPageAndPopulate).toHaveBeenCalledWith( {}, @@ -205,7 +209,7 @@ describe('UserController', () => { last_name: 'testUser', email: 'test@test.test', password: 'test', - roles: [role.id], + roles: [role!.id], }; const result = await userController.create(userDto); expect(userService.create).toHaveBeenCalledWith(userDto); @@ -224,16 +228,16 @@ describe('UserController', () => { it('should return updated user', async () => { jest.spyOn(userService, 'updateOne'); const result = await userController.updateOne( - { user: { id: user.id } } as any, - user.id, + { user: { id: user!.id } } as any, + user!.id, updateDto, ); - expect(userService.updateOne).toHaveBeenCalledWith(user.id, updateDto); + expect(userService.updateOne).toHaveBeenCalledWith(user!.id, updateDto); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), ...updateDto, - roles: user.roles, + roles: user!.roles, }, [...IGNORED_FIELDS, 'password', 'provider'], ); @@ -243,44 +247,43 @@ describe('UserController', () => { describe('updateStateAndRoles', () => { it('should return updated user', async () => { const updateDto: UserUpdateStateAndRolesDto = { - roles: [role.id], + roles: [role!.id], }; jest.spyOn(userService, 'updateOne'); const result = await userController.updateStateAndRoles( - user.id, + user!.id, updateDto, { passport: { - user: { id: user.id }, + user: { id: user!.id }, }, } as ExpressSession, ); - expect(userService.updateOne).toHaveBeenCalledWith(user.id, updateDto); + expect(userService.updateOne).toHaveBeenCalledWith(user!.id, updateDto); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), ...updateDto, }, - [...IGNORED_FIELDS, 'first_name', 'password', 'provider'], ); }); it('should return updated user after adding an extra role', async () => { const updateDto: UserUpdateStateAndRolesDto = { - roles: [role.id, roles[1].id], + roles: [role!.id, roles[1].id], }; jest.spyOn(userService, 'updateOne'); const result = await userController.updateStateAndRoles( - user.id, + user!.id, updateDto, { passport: { - user: { id: user.id }, + user: { id: user!.id }, }, } as ExpressSession, ); - expect(userService.updateOne).toHaveBeenCalledWith(user.id, updateDto); + expect(userService.updateOne).toHaveBeenCalledWith(user!.id, updateDto); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), @@ -296,9 +299,9 @@ describe('UserController', () => { state: false, }; await expect( - userController.updateStateAndRoles(user.id, updateDto, { + userController.updateStateAndRoles(user!.id, updateDto, { passport: { - user: { id: user.id }, + user: { id: user!.id }, }, } as ExpressSession), ).rejects.toThrow(ForbiddenException); @@ -309,9 +312,9 @@ describe('UserController', () => { roles: [], }; await expect( - userController.updateStateAndRoles(user.id, updateDto, { + userController.updateStateAndRoles(user!.id, updateDto, { passport: { - user: { id: user.id }, + user: { id: user!.id }, }, } as ExpressSession), ).rejects.toThrow(ForbiddenException); @@ -343,8 +346,8 @@ describe('UserController', () => { describe('deleteOne', () => { it('should delete user by id', async () => { - const result = await userController.deleteOne(user.id); - notFoundId = user.id; + const result = await userController.deleteOne(user!.id); + notFoundId = user!.id; expect(result).toEqual({ acknowledged: true, deletedCount: 1, diff --git a/api/src/user/controllers/user.controller.ts b/api/src/user/controllers/user.controller.ts index adcc0b8a..f765b58e 100644 --- a/api/src/user/controllers/user.controller.ts +++ b/api/src/user/controllers/user.controller.ts @@ -142,12 +142,12 @@ export class ReadOnlyUserController extends BaseController< ); const currentPermissions = await this.permissionService.findAndPopulate({ role: { - $in: currentUser.roles.map(({ id }) => id), + $in: currentUser?.roles.map(({ id }) => id), }, }); return { - roles: currentUser.roles, + roles: currentUser?.roles, permissions: currentPermissions.map((permission) => { if (permission.model) { return { @@ -248,7 +248,9 @@ export class ReadWriteUserController extends ReadOnlyUserController { roles: (await this.roleService.findAll()) .filter((role) => user.roles.includes(role.id)) .map((role) => role.id), - avatar: (await this.attachmentService.findOne(user.avatar))?.id, + avatar: user.avatar + ? (await this.attachmentService.findOne(user.avatar))?.id + : undefined, }, }); return await this.userService.create(user); @@ -285,7 +287,7 @@ export class ReadWriteUserController extends ReadOnlyUserController { @Body() userUpdate: UserEditProfileDto, @UploadedFile() avatarFile?: Express.Multer.File, ) { - if (!('id' in req.user && req.user.id) || req.user.id !== id) { + if (!(req.user && 'id' in req.user && req.user.id) || req.user.id !== id) { throw new ForbiddenException(); } @@ -339,18 +341,20 @@ export class ReadWriteUserController extends ReadOnlyUserController { @Body() body: UserUpdateStateAndRolesDto, @Session() session: ExpressSession, ) { - const oldRoles = (await this.userService.findOne(id)).roles; + const oldRoles = (await this.userService.findOne(id))?.roles; const newRoles = body.roles; - const { id: adminRoleId } = await this.roleService.findOne({ - name: 'admin', - }); + const { id: adminRoleId } = + (await this.roleService.findOne({ + name: 'admin', + })) || {}; if (id === session.passport?.user?.id && body.state === false) { throw new ForbiddenException('Your account state is protected'); } if ( + adminRoleId && session?.passport?.user?.id === id && - oldRoles.includes(adminRoleId) && - !newRoles.includes(adminRoleId) + oldRoles?.includes(adminRoleId) && + !newRoles?.includes(adminRoleId) ) { throw new ForbiddenException('Admin privileges are protected'); } diff --git a/api/src/user/repositories/invitation.repository.spec.ts b/api/src/user/repositories/invitation.repository.spec.ts index 3b7e0ecd..0f61ef0b 100644 --- a/api/src/user/repositories/invitation.repository.spec.ts +++ b/api/src/user/repositories/invitation.repository.spec.ts @@ -23,7 +23,11 @@ import { rootMongooseTestModule, } from '@/utils/test/test'; -import { Invitation, InvitationModel } from '../schemas/invitation.schema'; +import { + Invitation, + InvitationFull, + InvitationModel, +} from '../schemas/invitation.schema'; import { PermissionModel } from '../schemas/permission.schema'; import { RoleModel } from '../schemas/role.schema'; @@ -79,10 +83,10 @@ describe('InvitationRepository', () => { ); const result = await invitationRepository.findOneAndPopulate( - invitation.id, + invitation!.id, ); expect(invitationModel.findById).toHaveBeenCalledWith( - invitation.id, + invitation!.id, undefined, ); expect(result).toEqualPayload({ @@ -111,7 +115,7 @@ describe('InvitationRepository', () => { roles: allRoles.filter((role) => currInv.roles.includes(role.id)), }); return acc; - }, []); + }, [] as InvitationFull[]); expect(invitationModel.find).toHaveBeenCalledWith({}, undefined); expect(result).toEqualPayload(invitationsWithRoles); diff --git a/api/src/user/repositories/model.repository.spec.ts b/api/src/user/repositories/model.repository.spec.ts index 867c648e..0679d34f 100644 --- a/api/src/user/repositories/model.repository.spec.ts +++ b/api/src/user/repositories/model.repository.spec.ts @@ -20,7 +20,7 @@ import { import { ModelRepository } from '../repositories/model.repository'; import { PermissionRepository } from '../repositories/permission.repository'; -import { ModelModel } from '../schemas/model.schema'; +import { ModelFull, ModelModel } from '../schemas/model.schema'; import { Permission, PermissionModel } from '../schemas/permission.schema'; import { Model as ModelType } from './../schemas/model.schema'; @@ -29,7 +29,7 @@ describe('ModelRepository', () => { let modelRepository: ModelRepository; let permissionRepository: PermissionRepository; let modelModel: Model; - let model: ModelType; + let model: ModelType | null; let permissions: Permission[]; beforeAll(async () => { @@ -45,7 +45,7 @@ describe('ModelRepository', () => { modelRepository = module.get(ModelRepository); modelModel = module.get>(getModelToken('Model')); model = await modelRepository.findOne({ name: 'ContentType' }); - permissions = await permissionRepository.find({ model: model.id }); + permissions = await permissionRepository.find({ model: model!.id }); }); afterAll(closeInMongodConnection); @@ -55,8 +55,8 @@ describe('ModelRepository', () => { describe('findOneAndPopulate', () => { it('should find a model and populate its permissions', async () => { jest.spyOn(modelModel, 'findById'); - const result = await modelRepository.findOneAndPopulate(model.id); - expect(modelModel.findById).toHaveBeenCalledWith(model.id, undefined); + const result = await modelRepository.findOneAndPopulate(model!.id); + expect(modelModel.findById).toHaveBeenCalledWith(model!.id, undefined); expect(result).toEqualPayload({ ...modelFixtures.find(({ name }) => name === 'ContentType'), permissions, @@ -73,12 +73,12 @@ describe('ModelRepository', () => { const modelsWithPermissions = allModels.reduce((acc, currModel) => { acc.push({ ...currModel, - permissions: allPermissions.filter((permission) => { - return permission.model === currModel.id; - }), + permissions: allPermissions.filter( + (permission) => permission.model === currModel.id, + ), }); return acc; - }, []); + }, [] as ModelFull[]); expect(modelModel.find).toHaveBeenCalledWith({}, undefined); expect(result).toEqualPayload(modelsWithPermissions); }); diff --git a/api/src/user/repositories/user.repository.spec.ts b/api/src/user/repositories/user.repository.spec.ts index e5f64029..80a42de5 100644 --- a/api/src/user/repositories/user.repository.spec.ts +++ b/api/src/user/repositories/user.repository.spec.ts @@ -28,13 +28,13 @@ import { RoleRepository } from '../repositories/role.repository'; import { UserRepository } from '../repositories/user.repository'; import { PermissionModel } from '../schemas/permission.schema'; import { Role, RoleModel } from '../schemas/role.schema'; -import { User, UserModel } from '../schemas/user.schema'; +import { User, UserFull, UserModel } from '../schemas/user.schema'; describe('UserRepository', () => { let roleRepository: RoleRepository; let userRepository: UserRepository; let userModel: Model; - let user: User; + let user: User | null; let allRoles: Role[]; const FIELDS_TO_IGNORE: string[] = [ @@ -90,12 +90,12 @@ describe('UserRepository', () => { describe('findOneAndPopulate', () => { it('should find one user and populate its role', async () => { jest.spyOn(userModel, 'findById'); - const result = await userRepository.findOneAndPopulate(user.id); - expect(userModel.findById).toHaveBeenCalledWith(user.id, undefined); + const result = await userRepository.findOneAndPopulate(user!.id); + expect(userModel.findById).toHaveBeenCalledWith(user!.id, undefined); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), - roles: allRoles.filter(({ id }) => user.roles.includes(id)), + roles: allRoles.filter(({ id }) => user!.roles.includes(id)), }, FIELDS_TO_IGNORE, ); @@ -113,10 +113,11 @@ describe('UserRepository', () => { const usersWithRoles = allUsers.reduce((acc, currUser) => { acc.push({ ...currUser, - roles: allRoles.filter(({ id }) => user.roles.includes(id)), + roles: allRoles.filter(({ id }) => user?.roles.includes(id)), + avatar: null, }); return acc; - }, []); + }, [] as UserFull[]); expect(userModel.find).toHaveBeenCalledWith({}, undefined); expect(result).toEqualPayload(usersWithRoles); diff --git a/api/src/user/schemas/user.schema.ts b/api/src/user/schemas/user.schema.ts index 21a4a19d..502036b1 100644 --- a/api/src/user/schemas/user.schema.ts +++ b/api/src/user/schemas/user.schema.ts @@ -112,7 +112,7 @@ export class User extends UserStub { roles: string[]; @Transform(({ obj }) => obj.avatar?.toString() || null) - avatar?: string; + avatar?: string | null; } @Schema({ timestamps: true }) diff --git a/api/src/user/services/auth.service.spec.ts b/api/src/user/services/auth.service.spec.ts index e142c177..a66fa8c9 100644 --- a/api/src/user/services/auth.service.spec.ts +++ b/api/src/user/services/auth.service.spec.ts @@ -82,7 +82,7 @@ describe('AuthService', () => { {}, undefined, ); - expect(result.id).toBe(user.id); + expect(result!.id).toBe(user!.id); }); it('should not validate user if the provided password is incorrect', async () => { const result = await authService.validateUser( diff --git a/api/src/user/services/invitation.service.spec.ts b/api/src/user/services/invitation.service.spec.ts index 003f605a..7672fc1d 100644 --- a/api/src/user/services/invitation.service.spec.ts +++ b/api/src/user/services/invitation.service.spec.ts @@ -150,7 +150,7 @@ describe('InvitationService', () => { const role = await roleRepository.findOne({}); const newInvitation: InvitationCreateDto = { email: 'test@testland.tst', - roles: [role.id.toString()], + roles: [role!.id.toString()], }; jest.spyOn(invitationRepository, 'create'); diff --git a/api/src/user/services/model.service.spec.ts b/api/src/user/services/model.service.spec.ts index d3a66f7d..ec1c13af 100644 --- a/api/src/user/services/model.service.spec.ts +++ b/api/src/user/services/model.service.spec.ts @@ -19,7 +19,7 @@ import { import { ModelRepository } from '../repositories/model.repository'; import { PermissionRepository } from '../repositories/permission.repository'; -import { Model, ModelModel } from '../schemas/model.schema'; +import { Model, ModelFull, ModelModel } from '../schemas/model.schema'; import { Permission, PermissionModel } from '../schemas/permission.schema'; import { ModelService } from './model.service'; @@ -28,7 +28,7 @@ describe('ModelService', () => { let modelService: ModelService; let modelRepository: ModelRepository; let permissionRepository: PermissionRepository; - let model: Model; + let model: Model | null; let permissions: Permission[]; beforeAll(async () => { @@ -49,7 +49,7 @@ describe('ModelService', () => { module.get(PermissionRepository); modelRepository = module.get(ModelRepository); model = await modelRepository.findOne({ name: 'ContentType' }); - permissions = await permissionRepository.find({ model: model.id }); + permissions = await permissionRepository.find({ model: model!.id }); }); afterAll(closeInMongodConnection); @@ -58,7 +58,7 @@ describe('ModelService', () => { describe('findOneAndPopulate', () => { it('should find a model and populate its permissions', async () => { - const result = await modelService.findOneAndPopulate(model.id); + const result = await modelService.findOneAndPopulate(model!.id); expect(result).toEqualPayload({ ...modelFixtures.find(({ name }) => name === 'ContentType'), permissions, @@ -74,12 +74,12 @@ describe('ModelService', () => { const modelsWithPermissions = models.reduce((acc, currModel) => { acc.push({ ...currModel, - permissions: permissions.filter((permission) => { - return permission.model === currModel.id; - }), + permissions: permissions.filter( + (permission) => permission.model === currModel.id, + ), }); return acc; - }, []); + }, [] as ModelFull[]); expect(modelRepository.findAndPopulate).toHaveBeenCalledWith( {}, undefined, diff --git a/api/src/user/services/passwordReset.service.spec.ts b/api/src/user/services/passwordReset.service.spec.ts index cdef472a..136e52f8 100644 --- a/api/src/user/services/passwordReset.service.spec.ts +++ b/api/src/user/services/passwordReset.service.spec.ts @@ -103,16 +103,15 @@ describe('PasswordResetService', () => { }).compile(); passwordResetService = module.get(PasswordResetService); - mailerService = module.get(MailerService); jwtService = module.get(JwtService); userModel = module.get>(getModelToken('User')); }); - afterAll(async () => { - await closeInMongodConnection(); - }); + + afterAll(closeInMongodConnection); afterEach(jest.clearAllMocks); + describe('requestReset', () => { it('should send an email with a token', async () => { const sendMailSpy = jest.spyOn(mailerService, 'sendMail'); @@ -152,8 +151,8 @@ describe('PasswordResetService', () => { expect(verifySpy).toHaveBeenCalled(); const user = await userModel.findOne({ email: users[0].email }); - expect(user.resetToken).toBeNull(); - expect(compareSync('newPassword', user.password)).toBeTruthy(); + expect(user!.resetToken).toBeNull(); + expect(compareSync('newPassword', user!.password)).toBeTruthy(); }); }); }); diff --git a/api/src/user/services/user.service.spec.ts b/api/src/user/services/user.service.spec.ts index 74b35676..7a122183 100644 --- a/api/src/user/services/user.service.spec.ts +++ b/api/src/user/services/user.service.spec.ts @@ -29,7 +29,7 @@ import { RoleRepository } from '../repositories/role.repository'; import { UserRepository } from '../repositories/user.repository'; import { PermissionModel } from '../schemas/permission.schema'; import { Role, RoleModel } from '../schemas/role.schema'; -import { User, UserModel } from '../schemas/user.schema'; +import { User, UserFull, UserModel } from '../schemas/user.schema'; import { PermissionService } from './permission.service'; import { RoleService } from './role.service'; @@ -39,7 +39,7 @@ describe('UserService', () => { let userService: UserService; let roleRepository: RoleRepository; let userRepository: UserRepository; - let user: User; + let user: User | null; let allRoles: Role[]; const FIELDS_TO_IGNORE: string[] = [ ...IGNORED_TEST_FIELDS, @@ -99,15 +99,15 @@ describe('UserService', () => { describe('findOneAndPopulate', () => { it('should find one user and populate its role', async () => { jest.spyOn(userRepository, 'findOneAndPopulate'); - const result = await userService.findOneAndPopulate(user.id); + const result = await userService.findOneAndPopulate(user!.id); expect(userRepository.findOneAndPopulate).toHaveBeenCalledWith( - user.id, + user!.id, undefined, ); expect(result).toEqualPayload( { ...userFixtures.find(({ username }) => username === 'admin'), - roles: allRoles.filter(({ id }) => user.roles.includes(id)), + roles: allRoles.filter(({ id }) => user!.roles.includes(id)), }, FIELDS_TO_IGNORE, ); @@ -120,13 +120,17 @@ describe('UserService', () => { jest.spyOn(userRepository, 'findPageAndPopulate'); const allUsers = await userRepository.findAll(); const result = await userService.findPageAndPopulate({}, pageQuery); - const usersWithRoles = allUsers.reduce((acc, currUser) => { - acc.push({ - ...currUser, - roles: allRoles.filter(({ id }) => user.roles.includes(id)), - }); - return acc; - }, []); + const usersWithRoles = allUsers.reduce( + (acc, { avatar: _avatar, roles: _roles, ...rest }) => { + acc.push({ + ...rest, + roles: allRoles.filter(({ id }) => user?.roles?.includes(id)), + avatar: null, + }); + return acc; + }, + [] as UserFull[], + ); expect(userRepository.findPageAndPopulate).toHaveBeenCalledWith( {}, diff --git a/api/src/utils/types/filter.types.ts b/api/src/utils/types/filter.types.ts index fd14b748..764d5542 100644 --- a/api/src/utils/types/filter.types.ts +++ b/api/src/utils/types/filter.types.ts @@ -55,7 +55,7 @@ export type RecursivePartial = { : T[P]; }; //base controller validator types -type TAllowedKeys = { +type TAllowedKeys = { [key in keyof Record< TFilterKeysOfType< TFilterPopulateFields, TStub>, @@ -69,13 +69,17 @@ export type TValidateProps = { dto: | Partial> | Partial>; - allowedIds: Partial & TAllowedKeys>; + allowedIds: TAllowedKeys & + TAllowedKeys; }; //populate types export type TFilterPopulateFields = Omit< T, - TFilterKeysOfType + TFilterKeysOfType< + TStub, + null | undefined | string | number | boolean | object + > >; //search filter types From 5bb4668a01bc7b740dd0b07223836aaf49213c64 Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 08:28:14 +0100 Subject: [PATCH 2/7] fix: adapt services constructors --- api/src/user/services/invitation.service.ts | 2 +- api/src/user/services/passwordReset.service.ts | 4 ++-- api/src/user/services/validate-account.service.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/user/services/invitation.service.ts b/api/src/user/services/invitation.service.ts index f4da2282..69a1cc75 100644 --- a/api/src/user/services/invitation.service.ts +++ b/api/src/user/services/invitation.service.ts @@ -39,10 +39,10 @@ export class InvitationService extends BaseService< @Inject(InvitationRepository) readonly repository: InvitationRepository, @Inject(JwtService) private readonly jwtService: JwtService, - @Optional() private readonly mailerService: MailerService | undefined, private logger: LoggerService, protected readonly i18n: I18nService, public readonly languageService: LanguageService, + @Optional() private readonly mailerService?: MailerService, ) { super(repository); } diff --git a/api/src/user/services/passwordReset.service.ts b/api/src/user/services/passwordReset.service.ts index 704e1a9b..8c5d7991 100644 --- a/api/src/user/services/passwordReset.service.ts +++ b/api/src/user/services/passwordReset.service.ts @@ -32,11 +32,11 @@ import { UserService } from './user.service'; export class PasswordResetService { constructor( @Inject(JwtService) private readonly jwtService: JwtService, - @Optional() private readonly mailerService: MailerService | undefined, private logger: LoggerService, private readonly userService: UserService, public readonly i18n: I18nService, public readonly languageService: LanguageService, + @Optional() private readonly mailerService: MailerService, ) {} public readonly jwtSignOptions: JwtSignOptions = { @@ -105,7 +105,7 @@ export class PasswordResetService { // first step is to check if the token has been used const user = await this.userService.findOne({ email: payload.email }); - if (!user.resetToken || compareSync(user.resetToken, token)) { + if (!user?.resetToken || compareSync(user.resetToken, token)) { throw new UnauthorizedException('Invalid token'); } diff --git a/api/src/user/services/validate-account.service.ts b/api/src/user/services/validate-account.service.ts index c7a7d203..6872ed2e 100644 --- a/api/src/user/services/validate-account.service.ts +++ b/api/src/user/services/validate-account.service.ts @@ -36,10 +36,10 @@ export class ValidateAccountService { constructor( @Inject(JwtService) private readonly jwtService: JwtService, private readonly userService: UserService, - @Optional() private readonly mailerService: MailerService | undefined, private logger: LoggerService, private readonly i18n: I18nService, private readonly languageService: LanguageService, + @Optional() private readonly mailerService?: MailerService, ) {} /** From 869d7d167d33d701de0ce9da00c91a98a1fee985 Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 08:31:42 +0100 Subject: [PATCH 3/7] fix: make mailerService optional --- api/src/user/services/passwordReset.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/user/services/passwordReset.service.ts b/api/src/user/services/passwordReset.service.ts index 8c5d7991..2a9a7742 100644 --- a/api/src/user/services/passwordReset.service.ts +++ b/api/src/user/services/passwordReset.service.ts @@ -36,7 +36,7 @@ export class PasswordResetService { private readonly userService: UserService, public readonly i18n: I18nService, public readonly languageService: LanguageService, - @Optional() private readonly mailerService: MailerService, + @Optional() private readonly mailerService?: MailerService, ) {} public readonly jwtSignOptions: JwtSignOptions = { From 88cb28169419f7f7d303131dd0d9136995368c58 Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 08:46:27 +0100 Subject: [PATCH 4/7] fix: adapt ability guard --- api/src/user/guards/ability.guard.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/api/src/user/guards/ability.guard.ts b/api/src/user/guards/ability.guard.ts index b60a8ac2..96fee534 100644 --- a/api/src/user/guards/ability.guard.ts +++ b/api/src/user/guards/ability.guard.ts @@ -53,6 +53,7 @@ export class Ability implements CanActivate { if (user?.roles?.length) { if ( + _parsedUrl.pathname && [ // Allow access to all routes available for authenticated users '/auth/logout', @@ -68,9 +69,9 @@ export class Ability implements CanActivate { ) { return true; } - const modelFromPathname = _parsedUrl.pathname - .split('/')[1] - .toLowerCase() as TModel; + const modelFromPathname = _parsedUrl?.pathname + ?.split('/')[1] + .toLowerCase() as TModel | undefined; const permissions = await this.permissionService.getPermissions(); @@ -80,6 +81,7 @@ export class Ability implements CanActivate { .map(([_, value]) => value); if ( + modelFromPathname && permissionsFromRoles.some((permission) => permission[modelFromPathname]?.includes(MethodToAction[method]), ) From cc2f5a16652e78d573925aaeffe533e46fbdd49d Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 08:46:45 +0100 Subject: [PATCH 5/7] fix: adapt passport logic --- api/src/user/passport/session.serializer.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/api/src/user/passport/session.serializer.ts b/api/src/user/passport/session.serializer.ts index 56de030e..3480cf0e 100644 --- a/api/src/user/passport/session.serializer.ts +++ b/api/src/user/passport/session.serializer.ts @@ -19,7 +19,10 @@ export class AuthSerializer extends PassportSerializer { super(); } - serializeUser(user: User, done: (err: Error, user: SessionUser) => void) { + serializeUser( + user: User, + done: (err: Error | null, user: SessionUser) => void, + ) { done(null, { id: user.id, first_name: user.first_name, @@ -29,9 +32,9 @@ export class AuthSerializer extends PassportSerializer { async deserializeUser( payload: SessionUser, - done: (err: Error, user: SessionUser) => void, + done: (err: Error | null, user: SessionUser | null) => void, ) { - const user = await this.userService.findOne(payload.id); + const user = payload.id ? await this.userService.findOne(payload.id) : null; user ? done(null, user) : done(null, null); } } From 8ab2f16c566b636f0a6fc694a4d2f9f7311fb7f2 Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 10:44:32 +0100 Subject: [PATCH 6/7] fix: make user avatar field required --- api/src/user/controllers/auth.controller.spec.ts | 3 +++ api/src/user/controllers/user.controller.spec.ts | 1 + api/src/user/controllers/user.controller.ts | 2 +- api/src/user/dto/user.dto.ts | 2 +- api/src/user/passport/session.serializer.ts | 2 +- api/src/user/schemas/user.schema.ts | 4 ++-- api/src/utils/test/fixtures/user.ts | 4 ++-- 7 files changed, 11 insertions(+), 7 deletions(-) diff --git a/api/src/user/controllers/auth.controller.spec.ts b/api/src/user/controllers/auth.controller.spec.ts index b35a5db7..15858d6e 100644 --- a/api/src/user/controllers/auth.controller.spec.ts +++ b/api/src/user/controllers/auth.controller.spec.ts @@ -137,6 +137,7 @@ describe('AuthController', () => { first_name: 'test', last_name: 'test', roles: [role!.id], + avatar: null, }; await invitationService.create(baseUser); }); @@ -157,6 +158,7 @@ describe('AuthController', () => { email: 'test@test.test', password: 'test', roles: ['invalid role value'], + avatar: null, }; await expect(authController.signup(userCreateDto)).rejects.toThrow( @@ -174,6 +176,7 @@ describe('AuthController', () => { email: 'test@test.test', password: 'test', roles: ['659564cb4aa383c0d0dbc688'], + avatar: null, }; const result = await authController.signup(userCreateDto); expect(userService.create).toHaveBeenCalledWith(userCreateDto); diff --git a/api/src/user/controllers/user.controller.spec.ts b/api/src/user/controllers/user.controller.spec.ts index b4c82fb3..c7f5d889 100644 --- a/api/src/user/controllers/user.controller.spec.ts +++ b/api/src/user/controllers/user.controller.spec.ts @@ -210,6 +210,7 @@ describe('UserController', () => { email: 'test@test.test', password: 'test', roles: [role!.id], + avatar: null, }; const result = await userController.create(userDto); expect(userService.create).toHaveBeenCalledWith(userDto); diff --git a/api/src/user/controllers/user.controller.ts b/api/src/user/controllers/user.controller.ts index f765b58e..5f8ff25d 100644 --- a/api/src/user/controllers/user.controller.ts +++ b/api/src/user/controllers/user.controller.ts @@ -250,7 +250,7 @@ export class ReadWriteUserController extends ReadOnlyUserController { .map((role) => role.id), avatar: user.avatar ? (await this.attachmentService.findOne(user.avatar))?.id - : undefined, + : null, }, }); return await this.userService.create(user); diff --git a/api/src/user/dto/user.dto.ts b/api/src/user/dto/user.dto.ts index 56d298ac..2b082b6e 100644 --- a/api/src/user/dto/user.dto.ts +++ b/api/src/user/dto/user.dto.ts @@ -60,7 +60,7 @@ export class UserCreateDto { @IsOptional() @IsString() @IsObjectId({ message: 'Avatar must be a valid ObjectId' }) - avatar?: string; + avatar: string | null = null; } export class UserEditProfileDto extends OmitType(PartialType(UserCreateDto), [ diff --git a/api/src/user/passport/session.serializer.ts b/api/src/user/passport/session.serializer.ts index 3480cf0e..9db750b4 100644 --- a/api/src/user/passport/session.serializer.ts +++ b/api/src/user/passport/session.serializer.ts @@ -35,6 +35,6 @@ export class AuthSerializer extends PassportSerializer { done: (err: Error | null, user: SessionUser | null) => void, ) { const user = payload.id ? await this.userService.findOne(payload.id) : null; - user ? done(null, user) : done(null, null); + done(null, user); } } diff --git a/api/src/user/schemas/user.schema.ts b/api/src/user/schemas/user.schema.ts index 502036b1..a97d2057 100644 --- a/api/src/user/schemas/user.schema.ts +++ b/api/src/user/schemas/user.schema.ts @@ -91,7 +91,7 @@ export class UserStub extends BaseSchema { ref: 'Attachment', default: null, }) - avatar?: unknown; + avatar: unknown; @Prop({ type: String, @@ -112,7 +112,7 @@ export class User extends UserStub { roles: string[]; @Transform(({ obj }) => obj.avatar?.toString() || null) - avatar?: string | null; + avatar: string | null; } @Schema({ timestamps: true }) diff --git a/api/src/utils/test/fixtures/user.ts b/api/src/utils/test/fixtures/user.ts index c5f9d5d9..55a1aaff 100644 --- a/api/src/utils/test/fixtures/user.ts +++ b/api/src/utils/test/fixtures/user.ts @@ -9,7 +9,7 @@ import mongoose from 'mongoose'; import { UserCreateDto } from '@/user/dto/user.dto'; -import { UserModel, User } from '@/user/schemas/user.schema'; +import { User, UserModel } from '@/user/schemas/user.schema'; import { hash } from '@/user/utilities/bcryptjs'; import { getFixturesWithDefaultValues } from '../defaultValues'; @@ -25,6 +25,7 @@ export const users: UserCreateDto[] = [ email: 'admin@admin.admin', password: 'adminadmin', roles: ['0', '1'], + avatar: null, }, ]; @@ -34,7 +35,6 @@ export const userDefaultValues: TFixturesDefaultValues = { timezone: 'Europe/Berlin', sendEmail: false, resetCount: 0, - avatar: null, }; export const getUserFixtures = (users: UserCreateDto[]) => From fb4c7feadb127bbb12a48829fca077cb1b2d34a6 Mon Sep 17 00:00:00 2001 From: yassinedorbozgithub Date: Thu, 9 Jan 2025 10:47:16 +0100 Subject: [PATCH 7/7] fix: update user seeds --- api/src/user/seeds/user.seed-model.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/api/src/user/seeds/user.seed-model.ts b/api/src/user/seeds/user.seed-model.ts index 1a650c49..9d02a3bf 100644 --- a/api/src/user/seeds/user.seed-model.ts +++ b/api/src/user/seeds/user.seed-model.ts @@ -17,6 +17,7 @@ export const userModels = (roles: string[]): UserCreateDto[] => { email: 'admin@admin.admin', password: 'adminadmin', roles, + avatar: null, }, ]; };