From cb64234ab1c4f62a46bbfa7037d6b00fab360624 Mon Sep 17 00:00:00 2001 From: Emnaghz Date: Wed, 18 Sep 2024 17:29:44 +0100 Subject: [PATCH 1/4] fix: prevent user from deleting their own roles --- .../user/controllers/role.controller.spec.ts | 54 ++++++++++++++++--- api/src/user/controllers/role.controller.ts | 29 +++++++--- frontend/src/components/roles/index.tsx | 4 +- 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/api/src/user/controllers/role.controller.spec.ts b/api/src/user/controllers/role.controller.spec.ts index adb758c..fae89e6 100644 --- a/api/src/user/controllers/role.controller.spec.ts +++ b/api/src/user/controllers/role.controller.spec.ts @@ -8,10 +8,11 @@ */ import { CACHE_MANAGER } from '@nestjs/cache-manager'; -import { NotFoundException } from '@nestjs/common'; +import { ForbiddenException, NotFoundException } from '@nestjs/common'; import { EventEmitter2 } from '@nestjs/event-emitter'; import { MongooseModule } from '@nestjs/mongoose'; import { Test, TestingModule } from '@nestjs/testing'; +import { Session as ExpressSession } from 'express-session'; import { AttachmentRepository } from '@/attachment/repositories/attachment.repository'; import { AttachmentModel } from '@/attachment/schemas/attachment.schema'; @@ -42,7 +43,6 @@ describe('RoleController', () => { let roleService: RoleService; let permissionService: PermissionService; let userService: UserService; - let notFoundId: string; let roleAdmin: Role; let rolePublic: Role; @@ -190,17 +190,48 @@ describe('RoleController', () => { }); describe('deleteOne', () => { - it('should delete role by id', async () => { - const result = await roleController.deleteOne(roleAdmin.id); - notFoundId = roleAdmin.id; - expect(result).toEqual({ acknowledged: true, deletedCount: 1 }); + it("should throw ForbiddenException if the role is part of the user's roles", async () => { + const session = { passport: { user: { id: 'user1' } } } as ExpressSession; + const roleId = 'role1'; + + userService.findOneAndPopulate = jest.fn().mockResolvedValue({ + roles: [{ id: roleId }], + }); + + await expect(roleController.deleteOne(roleId, session)).rejects.toThrow( + ForbiddenException, + ); }); - it('should throw a NotFoundException when attempting to delete a role by id', async () => { - await expect(roleController.deleteOne(notFoundId)).rejects.toThrow( + it('should throw NotFoundException if the role is not found', async () => { + const session = { passport: { user: { id: 'user1' } } } as ExpressSession; + const roleId = 'role2'; + + userService.findOneAndPopulate = jest.fn().mockResolvedValue({ + roles: [{ id: 'role1' }], + }); + + roleService.deleteOne = jest.fn().mockResolvedValue({ deletedCount: 0 }); + + await expect(roleController.deleteOne(roleId, session)).rejects.toThrow( NotFoundException, ); }); + + it('should return the result if the role is successfully deleted', async () => { + const session = { passport: { user: { id: 'user1' } } } as ExpressSession; + const roleId = 'role2'; + + userService.findOneAndPopulate = jest.fn().mockResolvedValue({ + roles: [{ id: 'role1' }], + }); + + const deleteResult = { deletedCount: 1 }; + roleService.deleteOne = jest.fn().mockResolvedValue(deleteResult); + + const result = await roleController.deleteOne(roleId, session); + expect(result).toEqual(deleteResult); + }); }); describe('updateOne', () => { @@ -225,6 +256,13 @@ describe('RoleController', () => { }); it('should throw a NotFoundException when attempting to modify a role', async () => { + const notFoundId = 'nonexistentRoleId'; + const roleUpdateDto = { name: 'newRoleName' }; + + roleService.updateOne = jest + .fn() + .mockRejectedValue(new NotFoundException()); + await expect( roleController.updateOne(notFoundId, roleUpdateDto), ).rejects.toThrow(NotFoundException); diff --git a/api/src/user/controllers/role.controller.ts b/api/src/user/controllers/role.controller.ts index 5ae2919..7fe36c3 100644 --- a/api/src/user/controllers/role.controller.ts +++ b/api/src/user/controllers/role.controller.ts @@ -19,8 +19,11 @@ import { Patch, Query, UseInterceptors, + Session, + ForbiddenException, } from '@nestjs/common'; import { CsrfCheck } from '@tekuconcept/nestjs-csrf'; +import { Session as ExpressSession } from 'express-session'; import { TFilterQuery } from 'mongoose'; import { CsrfInterceptor } from '@/interceptors/csrf.interceptor'; @@ -34,6 +37,7 @@ import { SearchFilterPipe } from '@/utils/pipes/search-filter.pipe'; import { RoleCreateDto, RoleUpdateDto } from '../dto/role.dto'; import { Role, RoleStub } from '../schemas/role.schema'; import { RoleService } from '../services/role.service'; +import { UserService } from '../services/user.service'; @UseInterceptors(CsrfInterceptor) @Controller('role') @@ -41,6 +45,7 @@ export class RoleController extends BaseController { constructor( private readonly roleService: RoleService, private readonly logger: LoggerService, + private readonly userService: UserService, ) { super(roleService); } @@ -142,12 +147,24 @@ export class RoleController extends BaseController { @CsrfCheck(true) @Delete(':id') @HttpCode(204) - async deleteOne(@Param('id') id: string) { - const result = await this.roleService.deleteOne(id); - if (result.deletedCount === 0) { - this.logger.warn(`Unable to delete Role by id ${id}`); - throw new NotFoundException(`Role with ID ${id} not found`); + async deleteOne(@Param('id') id: string, @Session() session: ExpressSession) { + const roles = ( + await this.userService.findOneAndPopulate(session.passport?.user?.id, [ + 'roles', + ]) + ).roles.map((role) => role.id); + if (roles.includes(id)) { + throw new ForbiddenException("Your account's role can't be deleted"); + } else { + try { + const result = await this.roleService.deleteOne(id); + if (result.deletedCount === 0) { + throw new NotFoundException(`Role with ID ${id} not found`); + } + return result; + } catch (error) { + throw new NotFoundException(`Role with ID ${id} not found`); + } } - return result; } } diff --git a/frontend/src/components/roles/index.tsx b/frontend/src/components/roles/index.tsx index 380728f..4ae6b6f 100644 --- a/frontend/src/components/roles/index.tsx +++ b/frontend/src/components/roles/index.tsx @@ -57,8 +57,8 @@ export const Roles = () => { }, ); const { mutateAsync: deleteRole } = useDelete(EntityType.ROLE, { - onError: () => { - toast.error(t("message.internal_server_error")); + onError: (error) => { + toast.error(t(error.message || "message.internal_server_error")); }, onSuccess() { deleteDialogCtl.closeDialog(); From 16f10c50f7f24b1428f8394277d7db1945ffbe7a Mon Sep 17 00:00:00 2001 From: Emnaghz Date: Sat, 21 Sep 2024 12:38:24 +0100 Subject: [PATCH 2/4] fix: improve code + add requested changes --- api/src/user/controllers/role.controller.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/api/src/user/controllers/role.controller.ts b/api/src/user/controllers/role.controller.ts index 7fe36c3..d159534 100644 --- a/api/src/user/controllers/role.controller.ts +++ b/api/src/user/controllers/role.controller.ts @@ -19,8 +19,8 @@ import { Patch, Query, UseInterceptors, - Session, ForbiddenException, + Session, } from '@nestjs/common'; import { CsrfCheck } from '@tekuconcept/nestjs-csrf'; import { Session as ExpressSession } from 'express-session'; @@ -148,11 +148,16 @@ export class RoleController extends BaseController { @Delete(':id') @HttpCode(204) async deleteOne(@Param('id') id: string, @Session() session: ExpressSession) { - const roles = ( - await this.userService.findOneAndPopulate(session.passport?.user?.id, [ - 'roles', - ]) - ).roles.map((role) => role.id); + const currentUser = await this.userService.findOneAndPopulate( + session.passport.user.id, + ['roles'], + ); + if (!currentUser) { + throw new NotFoundException('User not found'); + } + + const roles = currentUser.roles.map((role) => role.id); + if (roles.includes(id)) { throw new ForbiddenException("Your account's role can't be deleted"); } else { From d2f61eebcdf768e9266b4f3c836fa45198edf040 Mon Sep 17 00:00:00 2001 From: Emnaghz Date: Mon, 23 Sep 2024 11:17:19 +0100 Subject: [PATCH 3/4] fix: use req + update unit tests --- .../user/controllers/role.controller.spec.ts | 43 ++++++++++--------- api/src/user/controllers/role.controller.ts | 24 +++++------ 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/api/src/user/controllers/role.controller.spec.ts b/api/src/user/controllers/role.controller.spec.ts index fae89e6..ce6f786 100644 --- a/api/src/user/controllers/role.controller.spec.ts +++ b/api/src/user/controllers/role.controller.spec.ts @@ -12,7 +12,7 @@ import { ForbiddenException, NotFoundException } from '@nestjs/common'; import { EventEmitter2 } from '@nestjs/event-emitter'; import { MongooseModule } from '@nestjs/mongoose'; import { Test, TestingModule } from '@nestjs/testing'; -import { Session as ExpressSession } from 'express-session'; +import { Request } from 'express'; import { AttachmentRepository } from '@/attachment/repositories/attachment.repository'; import { AttachmentModel } from '@/attachment/schemas/attachment.schema'; @@ -191,45 +191,48 @@ describe('RoleController', () => { describe('deleteOne', () => { it("should throw ForbiddenException if the role is part of the user's roles", async () => { - const session = { passport: { user: { id: 'user1' } } } as ExpressSession; + const req = { user: { roles: ['role1'] } } as unknown as Request; const roleId = 'role1'; - userService.findOneAndPopulate = jest.fn().mockResolvedValue({ - roles: [{ id: roleId }], - }); + userService.findOne = jest.fn().mockResolvedValue(null); - await expect(roleController.deleteOne(roleId, session)).rejects.toThrow( + await expect(roleController.deleteOne(roleId, req)).rejects.toThrow( + ForbiddenException, + ); + }); + + it('should throw ForbiddenException if the role is associated with other users', async () => { + const req = { user: { roles: ['role2'] } } as unknown as Request; + const roleId = 'role1'; + + userService.findOne = jest.fn().mockResolvedValue({ id: 'user2' }); + + await expect(roleController.deleteOne(roleId, req)).rejects.toThrow( ForbiddenException, ); }); it('should throw NotFoundException if the role is not found', async () => { - const session = { passport: { user: { id: 'user1' } } } as ExpressSession; - const roleId = 'role2'; - - userService.findOneAndPopulate = jest.fn().mockResolvedValue({ - roles: [{ id: 'role1' }], - }); + const req = { user: { roles: ['role2'] } } as unknown as Request; + const roleId = 'role1'; + userService.findOne = jest.fn().mockResolvedValue(null); roleService.deleteOne = jest.fn().mockResolvedValue({ deletedCount: 0 }); - await expect(roleController.deleteOne(roleId, session)).rejects.toThrow( + await expect(roleController.deleteOne(roleId, req)).rejects.toThrow( NotFoundException, ); }); it('should return the result if the role is successfully deleted', async () => { - const session = { passport: { user: { id: 'user1' } } } as ExpressSession; - const roleId = 'role2'; - - userService.findOneAndPopulate = jest.fn().mockResolvedValue({ - roles: [{ id: 'role1' }], - }); + const req = { user: { roles: ['role2'] } } as unknown as Request; + const roleId = 'role1'; + userService.findOne = jest.fn().mockResolvedValue(null); const deleteResult = { deletedCount: 1 }; roleService.deleteOne = jest.fn().mockResolvedValue(deleteResult); - const result = await roleController.deleteOne(roleId, session); + const result = await roleController.deleteOne(roleId, req); expect(result).toEqual(deleteResult); }); }); diff --git a/api/src/user/controllers/role.controller.ts b/api/src/user/controllers/role.controller.ts index b54b62a..ba92870 100644 --- a/api/src/user/controllers/role.controller.ts +++ b/api/src/user/controllers/role.controller.ts @@ -20,10 +20,10 @@ import { Query, UseInterceptors, ForbiddenException, - Session, + Req, } from '@nestjs/common'; import { CsrfCheck } from '@tekuconcept/nestjs-csrf'; -import { Session as ExpressSession } from 'express-session'; +import { Request } from 'express'; import { TFilterQuery } from 'mongoose'; import { CsrfInterceptor } from '@/interceptors/csrf.interceptor'; @@ -36,6 +36,7 @@ import { SearchFilterPipe } from '@/utils/pipes/search-filter.pipe'; import { RoleCreateDto, RoleUpdateDto } from '../dto/role.dto'; import { Role, RoleFull, RolePopulate, RoleStub } from '../schemas/role.schema'; +import { User } from '../schemas/user.schema'; import { RoleService } from '../services/role.service'; import { UserService } from '../services/user.service'; @@ -152,19 +153,16 @@ export class RoleController extends BaseController< @CsrfCheck(true) @Delete(':id') @HttpCode(204) - async deleteOne(@Param('id') id: string, @Session() session: ExpressSession) { - const currentUser = await this.userService.findOneAndPopulate( - session.passport.user.id, - ['roles'], - ); - if (!currentUser) { - throw new NotFoundException('User not found'); - } + async deleteOne(@Param('id') id: string, @Req() req: Request) { + const userRoles = (req.user as User).roles; - const roles = currentUser.roles.map((role) => role.id); - - if (roles.includes(id)) { + const associatedUser = await this.userService.findOne({ + roles: { $in: [id] }, + }); + if (userRoles.includes(id)) { throw new ForbiddenException("Your account's role can't be deleted"); + } else if (associatedUser) { + throw new ForbiddenException('Role is associated with other users'); } else { try { const result = await this.roleService.deleteOne(id); From e93c649115aa3e2d7b374090ac73fb3dafb40d1c Mon Sep 17 00:00:00 2001 From: Emnaghz Date: Mon, 23 Sep 2024 16:58:19 +0100 Subject: [PATCH 4/4] fix: improve code --- api/src/user/controllers/role.controller.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/api/src/user/controllers/role.controller.ts b/api/src/user/controllers/role.controller.ts index ba92870..06a366d 100644 --- a/api/src/user/controllers/role.controller.ts +++ b/api/src/user/controllers/role.controller.ts @@ -164,15 +164,11 @@ export class RoleController extends BaseController< } else if (associatedUser) { throw new ForbiddenException('Role is associated with other users'); } else { - try { - const result = await this.roleService.deleteOne(id); - if (result.deletedCount === 0) { - throw new NotFoundException(`Role with ID ${id} not found`); - } - return result; - } catch (error) { + const result = await this.roleService.deleteOne(id); + if (result.deletedCount === 0) { throw new NotFoundException(`Role with ID ${id} not found`); } + return result; } } }