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

@@ -728,8 +728,8 @@ export default class ProjectService {
         ) {
             return true;
         }
-        return rolesBeingAdded.every((role) =>
+        return rolesBeingAdded.every((roleId) =>
-            userRoles.some((userRole) => userRole.id === role),
+            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);
         }
-
+        const isAllowedToAssignRoles = await this.isAllowedToAddAccess(
-        await this.accessService.setProjectRolesForUser(
+            createdByUserId,
             projectId,
-            userId,
             newRoles,
         );
-        await this.eventService.storeEvent(
+        if (isAllowedToAssignRoles) {
-            new ProjectAccessUserRolesUpdated({
+            await this.accessService.setProjectRolesForUser(
-                project: projectId,
+                projectId,
-                createdBy: createdByUserName,
+                userId,
-                createdByUserId,
+                newRoles,
-                data: {
+            );
-                    roles: newRoles,
+            await this.eventService.storeEvent(
-                    userId,
+                new ProjectAccessUserRolesUpdated({
-                },
+                    project: projectId,
-                preData: {
+                    createdBy: createdByUserName,
-                    roles: currentRoles,
+                    createdByUserId,
-                    userId,
+                    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);
         }
-
+        const isAllowedToAssignRoles = await this.isAllowedToAddAccess(
-        await this.accessService.setProjectRolesForGroup(
+            createdByUserId,
             projectId,
-            groupId,
             newRoles,
-            createdBy,
         );
-        await this.eventService.storeEvent(
+        if (isAllowedToAssignRoles) {
-            new ProjectAccessGroupRolesUpdated({
+            await this.accessService.setProjectRolesForGroup(
-                project: projectId,
+                projectId,
+                groupId,
+                newRoles,
                 createdBy,
-                createdByUserId,
+            );
-                data: {
+            await this.eventService.storeEvent(
-                    roles: newRoles,
+                new ProjectAccessGroupRolesUpdated({
-                    groupId,
+                    project: projectId,
-                },
+                    createdBy,
-                preData: {
+                    createdByUserId,
-                    roles: currentRoles,
+                    data: {
-                    groupId,
+                        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(

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',
+            ),
+        );
     });
 });