fix: group roles assumption, refactor group types (#4576)

Does what it says on the tin, should help with cleaning up
https://github.com/Unleash/unleash/pull/4512 and respective schema
changes.

---------

Co-authored-by: Gastón Fournier <gaston@getunleash.io>

frontend/cypress/integration/projects/access.spec.ts

@@ -47,19 +47,6 @@ describe('project-access', () => {
             id: groupAndProjectName,
             name: groupAndProjectName,
         });
-
-        cy.intercept('GET', `${baseUrl}/api/admin/ui-config`, req => {
-            req.headers['cache-control'] =
-                'no-cache, no-store, must-revalidate';
-            req.on('response', res => {
-                if (res.body) {
-                    res.body.flags = {
-                        ...res.body.flags,
-                        multipleRoles: true,
-                    };
-                }
-            });
-        });
     });
 
     after(() => {
@@ -78,6 +65,20 @@ describe('project-access', () => {
 
     beforeEach(() => {
         cy.login_UI();
+
+        cy.intercept('GET', `${baseUrl}/api/admin/ui-config`, req => {
+            req.headers['cache-control'] =
+                'no-cache, no-store, must-revalidate';
+            req.on('response', res => {
+                if (res.body) {
+                    res.body.flags = {
+                        ...res.body.flags,
+                        multipleRoles: true,
+                    };
+                }
+            });
+        });
+
         cy.visit(`/projects/${groupAndProjectName}/settings/access`);
         if (document.querySelector("[data-testid='CLOSE_SPLASH']")) {
             cy.get("[data-testid='CLOSE_SPLASH']").click();

frontend/src/component/project/ProjectAccess/ProjectAccessTable/ProjectAccessTable.tsx

@@ -151,11 +151,13 @@ export const ProjectAccessTable: VFC = () => {
                 id: 'role',
                 Header: 'Role',
                 accessor: (row: IProjectAccess) =>
-                    row.entity.roles.length > 1
+                    row.entity.roles
-                        ? `${row.entity.roles.length} roles`
+                        ? row.entity.roles.length > 1
-                        : access?.roles.find(
+                            ? `${row.entity.roles.length} roles`
-                              ({ id }) => id === row.entity.roleId
+                            : access?.roles.find(
-                          )?.name,
+                                  ({ id }) => id === row.entity.roleId
+                              )?.name
+                        : 'No Roles!',
                 Cell: ({
                     value,
                     row: { original: row },

src/lib/db/group-store.ts

@@ -1,9 +1,9 @@
 import { IGroupStore, IStoreGroup } from '../types/stores/group-store';
 import NotFoundError from '../error/notfound-error';
 import Group, {
-    ICreateGroupModel,
     ICreateGroupUserModel,
     IGroup,
+    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
@@ -82,7 +82,7 @@ export default class GroupStore implements IGroupStore {
         return groups.map(rowToGroup);
     }
 
-    async update(group: ICreateGroupModel): Promise<IGroup> {
+    async update(group: IGroupModel): Promise<IGroup> {
         try {
             const rows = await this.db(T.GROUPS)
                 .where({ id: group.id })

src/lib/services/access-service.test.ts

@@ -190,7 +190,7 @@ test('throws error when trying to delete a project role in use by group', async
     const eventStore = new FakeEventStore();
     const groupStore = new FakeGroupStore();
     groupStore.getAllWithId = async (): Promise<IGroup[]> => {
-        return [{ name: 'group' }];
+        return [{ id: 1, name: 'group' }];
     };
     const accountStore = new FakeAccountStore();
     const roleStore = new FakeRoleStore();

src/lib/services/group-service.ts

@@ -105,10 +105,7 @@ export class GroupService {
         return newGroup;
     }
 
-    async updateGroup(
+    async updateGroup(group: IGroupModel, userName: string): Promise<IGroup> {
-        group: ICreateGroupModel,
-        userName: string,
-    ): Promise<IGroup> {
         const preData = await this.groupStore.get(group.id);
 
         await this.validateGroup(group, preData);
@@ -153,10 +150,10 @@ export class GroupService {
 
         if (projectGroups.length > 0) {
             const groups = await this.groupStore.getAllWithId(
-                projectGroups.map((g) => g.id!),
+                projectGroups.map((g) => g.id),
             );
             const groupUsers = await this.groupStore.getAllUsersByGroups(
-                groups.map((g) => g.id!),
+                groups.map((g) => g.id),
             );
             const users = await this.accountStore.getAllWithId(
                 groupUsers.map((u) => u.userId),
@@ -178,7 +175,7 @@ export class GroupService {
     }
 
     async validateGroup(
-        group: ICreateGroupModel,
+        group: IGroupModel | ICreateGroupModel,
         existingGroup?: IGroup,
     ): Promise<void> {
         if (!group.name) {
@@ -190,16 +187,6 @@ export class GroupService {
                 throw new NameExistsError('Group name already exists');
             }
         }
-
-        if (
-            group.id &&
-            group.rootRole &&
-            (await this.groupStore.hasProjectRole(group.id))
-        ) {
-            throw new BadDataError(
-                'This group already has a project role and cannot also be given a root role',
-            );
-        }
     }
 
     async getRolesForProject(projectId: string): Promise<IGroupRole[]> {

src/lib/types/group.ts

@@ -2,7 +2,7 @@ import Joi, { ValidationError } from 'joi';
 import { IUser } from './user';
 
 export interface IGroup {
-    id?: number;
+    id: number;
     name: string;
     description?: string;
     mappingsSSO?: string[];
@@ -33,7 +33,7 @@ export interface IGroupModel extends IGroup {
     projects?: string[];
 }
 
-export interface ICreateGroupModel extends IGroup {
+export interface ICreateGroupModel extends Omit<IGroup, 'id'> {
     users?: ICreateGroupUserModel[];
     projects?: string[];
 }

src/lib/types/stores/group-store.ts

@@ -1,8 +1,8 @@
 import { Store } from './store';
 import Group, {
-    ICreateGroupModel,
     ICreateGroupUserModel,
     IGroup,
+    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
@@ -48,7 +48,7 @@ export interface IGroupStore extends Store<IGroup, number> {
 
     deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise<void>;
 
-    update(group: ICreateGroupModel): Promise<IGroup>;
+    update(group: IGroupModel): Promise<IGroup>;
 
     getAllUsersByGroups(groupIds: number[]): Promise<IGroupUser[]>;
 

src/test/e2e/services/access-service.e2e.test.ts

@@ -63,7 +63,7 @@ const createGroup = async ({
         name: `Group ${groupIndex}`,
         rootRole: role,
     });
-    if (users) await groupStore.addUsersToGroup(group.id!, users, 'Admin');
+    if (users) await groupStore.addUsersToGroup(group.id, users, 'Admin');
     return group;
 };
 
@@ -1292,7 +1292,7 @@ test('if group has two roles user has union of permissions from the two roles',
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [firstRole.id, secondRole.id],
         'testusr',
     );
@@ -1347,7 +1347,7 @@ test('calling set for group overwrites existing roles', async () => {
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [firstRole.id, secondRole.id],
         'testusr',
     );
@@ -1363,7 +1363,7 @@ test('calling set for group overwrites existing roles', async () => {
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [firstRole.id],
         'testusr',
     );
@@ -1404,7 +1404,7 @@ test('group with root role can be assigned a project specific role', async () =>
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [firstRole.id],
         'testusr',
     );
@@ -1638,7 +1638,7 @@ test('calling set roles for group with invalid project role ids should not assig
 
     accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [adminRootRole.id, 9999],
         'admin',
     );
@@ -1669,7 +1669,7 @@ test('calling set roles for group with empty role array removes all roles', asyn
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [role.id],
         'admin',
     );
@@ -1682,7 +1682,7 @@ test('calling set roles for group with empty role array removes all roles', asyn
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        emptyGroup.id!,
+        emptyGroup.id,
         [],
         'admin',
     );
@@ -1719,7 +1719,7 @@ test('calling set roles for group with empty role array should not remove root r
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        group.id!,
+        group.id,
         [role.id],
         'admin',
     );
@@ -1732,7 +1732,7 @@ test('calling set roles for group with empty role array should not remove root r
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        group.id!,
+        group.id,
         [],
         'admin',
     );
@@ -1778,7 +1778,7 @@ test('remove group access should remove all project roles', async () => {
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        group.id!,
+        group.id,
         [firstRole.id, secondRole.id],
         'admin',
     );
@@ -1789,7 +1789,7 @@ test('remove group access should remove all project roles', async () => {
 
     expect(assignedPermissions.length).toBe(3);
 
-    await accessService.removeGroupAccess(projectName, group.id!);
+    await accessService.removeGroupAccess(projectName, group.id);
 
     const newAssignedPermissions = await accessService.getPermissionsForUser(
         emptyUser,
@@ -1831,7 +1831,7 @@ test('remove group access should remove all project roles, while leaving root ro
 
     await accessService.setProjectRolesForGroup(
         projectName,
-        group.id!,
+        group.id,
         [firstRole.id, secondRole.id],
         'admin',
     );
@@ -1842,7 +1842,7 @@ test('remove group access should remove all project roles, while leaving root ro
 
     expect(assignedPermissions.length).toBe(4);
 
-    await accessService.removeGroupAccess(projectName, group.id!);
+    await accessService.removeGroupAccess(projectName, group.id);
 
     const newAssignedPermissions = await accessService.getPermissionsForUser(
         adminUser,

src/test/e2e/services/group-service.e2e.test.ts

@@ -98,7 +98,7 @@ test('should not remove user from no SSO definition group', async () => {
     expect(groups[0].name).toEqual('no_mapping_group');
 });
 
-test('adding a root role to a group with a project role should fail', async () => {
+test('adding a root role to a group with a project role should not fail', async () => {
     const group = await groupStore.create({
         name: 'root_group',
         description: 'root_group',
@@ -118,9 +118,7 @@ test('adding a root role to a group with a project role should fail', async () =
             },
             'test',
         );
-    }).rejects.toThrow(
+    }).not.toThrow();
-        'This group already has a project role and cannot also be given a root role',
-    );
 
     expect.assertions(1);
 });

src/test/fixtures/fake-group-store.ts

@@ -1,8 +1,8 @@
 import { IGroupStore, IStoreGroup } from '../../lib/types/stores/group-store';
 import Group, {
-    ICreateGroupModel,
     ICreateGroupUserModel,
     IGroup,
+    IGroupModel,
     IGroupProject,
     IGroupRole,
     IGroupUser,
@@ -63,7 +63,7 @@ export default class FakeGroupStore implements IGroupStore {
         throw new Error('Method not implemented.');
     }
 
-    update(group: ICreateGroupModel): Promise<IGroup> {
+    update(group: IGroupModel): Promise<IGroup> {
         throw new Error('Method not implemented.');
     }