From 0887999dd063f44105c8f2a9a68f53a94b739cbb Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 29 Feb 2024 09:38:32 +0100 Subject: [PATCH] fix: setRolesForUser and setRolesForGroup role check (#6380) In order to stop privilege escalation via `/api/admin/projects/:project/users/:userId/roles` and `/api/admin/projects/:project/groups/:groupId/roles` this PR adds the same check we added to setAccess methods to the methods updating access for these two methods. Also adds tests that verify that we throw an exception if you try to assign roles you do not have. Thank you @nunogois for spotting this during testing. --- src/lib/features/project/project-service.ts | 97 +++++++++++-------- .../e2e/services/project-service.e2e.test.ts | 76 ++++++++++++++- 2 files changed, 133 insertions(+), 40 deletions(-) diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 812f01fb50..31db616405 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -728,8 +728,8 @@ export default class ProjectService { ) { return true; } - return rolesBeingAdded.every((role) => - userRoles.some((userRole) => userRole.id === role), + return rolesBeingAdded.every((roleId) => + userRoles.some((userRole) => userRole.id === roleId), ); } async addAccess( @@ -785,7 +785,6 @@ export default class ProjectService { projectId, userId, ); - const ownerRole = await this.accessService.getRoleByName( RoleName.OWNER, ); @@ -795,27 +794,37 @@ export default class ProjectService { if (hasOwnerRole && isRemovingOwnerRole) { await this.validateAtLeastOneOwner(projectId, ownerRole); } - - await this.accessService.setProjectRolesForUser( + const isAllowedToAssignRoles = await this.isAllowedToAddAccess( + createdByUserId, projectId, - userId, newRoles, ); - await this.eventService.storeEvent( - new ProjectAccessUserRolesUpdated({ - project: projectId, - createdBy: createdByUserName, - createdByUserId, - data: { - roles: newRoles, - userId, - }, - preData: { - roles: currentRoles, - userId, - }, - }), - ); + if (isAllowedToAssignRoles) { + await this.accessService.setProjectRolesForUser( + projectId, + userId, + newRoles, + ); + await this.eventService.storeEvent( + new ProjectAccessUserRolesUpdated({ + project: projectId, + createdBy: createdByUserName, + createdByUserId, + data: { + roles: newRoles, + userId, + }, + preData: { + roles: currentRoles, + userId, + }, + }), + ); + } else { + throw new InvalidOperationError( + 'User tried to assign a role they did not have access to', + ); + } } async setRolesForGroup( @@ -838,28 +847,38 @@ export default class ProjectService { if (hasOwnerRole && isRemovingOwnerRole) { await this.validateAtLeastOneOwner(projectId, ownerRole); } - - await this.accessService.setProjectRolesForGroup( + const isAllowedToAssignRoles = await this.isAllowedToAddAccess( + createdByUserId, projectId, - groupId, newRoles, - createdBy, ); - await this.eventService.storeEvent( - new ProjectAccessGroupRolesUpdated({ - project: projectId, + if (isAllowedToAssignRoles) { + await this.accessService.setProjectRolesForGroup( + projectId, + groupId, + newRoles, createdBy, - createdByUserId, - data: { - roles: newRoles, - groupId, - }, - preData: { - roles: currentRoles, - groupId, - }, - }), - ); + ); + await this.eventService.storeEvent( + new ProjectAccessGroupRolesUpdated({ + project: projectId, + createdBy, + createdByUserId, + data: { + roles: newRoles, + groupId, + }, + preData: { + roles: currentRoles, + groupId, + }, + }), + ); + } else { + throw new InvalidOperationError( + 'User tried to assign a role they did not have access to', + ); + } } async findProjectGroupRole( diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index c42b4ae4d9..cb58a6ea2a 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -25,6 +25,7 @@ import { SYSTEM_USER_ID, } from '../../../lib/types'; import { User } from '../../../lib/server-impl'; +import { InvalidOperationError } from '../../../lib/error'; let stores: IUnleashStores; let db: ITestDb; @@ -619,7 +620,80 @@ describe('Managing Project access', () => { projectUser.username, projectUser.id, ), - ).resolves.not.toThrow(); + ).resolves.not.toThrow( + new InvalidOperationError( + 'User tried to assign a role they did not have access to', + ), + ); + }); + test('Users can not assign roles they do not have to a user through explicit roles endpoint', async () => { + const project = { + id: 'user_fail_assign_to_user', + name: 'user_fail_assign_to_user', + description: '', + mode: 'open' as const, + defaultStickiness: 'clientId', + }; + await projectService.createProject(project, user); + const projectUser = await stores.userStore.insert({ + name: 'Some project user', + email: 'fail_assign_role_to_user@example.com', + }); + const secondUser = await stores.userStore.insert({ + name: 'Some other user', + email: 'otheruser_no_roles@example.com', + }); + const customRole = await stores.roleStore.create({ + name: 'role_that_noone_has', + roleType: 'custom', + description: + 'Used to prove that you can not assign a role you do not have via setRolesForUser', + }); + expect( + projectService.setRolesForUser( + project.id, + secondUser.id, + [customRole.id], + projectUser.username, + projectUser.id, + ), + ).rejects.toThrow(); + }); + test('Users can not assign roles they do not have to a group through explicit roles endpoint', async () => { + const project = { + id: 'user_fail_assign_to_group', + name: 'user_fail_assign_to_group', + description: '', + mode: 'open' as const, + defaultStickiness: 'clientId', + }; + await projectService.createProject(project, user); + const projectUser = await stores.userStore.insert({ + name: 'Some project user', + email: 'fail_assign_role_to_group@example.com', + }); + const group = await stores.groupStore.create({ + name: 'Some group_awaiting_role', + }); + const customRole = await stores.roleStore.create({ + name: 'role_that_noone_has_fail_assign_group', + roleType: 'custom', + description: + 'Used to prove that you can not assign a role you do not have via setRolesForGroup', + }); + expect( + projectService.setRolesForGroup( + project.id, + group.id, + [customRole.id], + projectUser.username, + projectUser.id, + ), + ).rejects.toThrow( + new InvalidOperationError( + 'User tried to assign a role they did not have access to', + ), + ); }); });