From f2767286881283f7a6bae15e17a62bd528ba4fed Mon Sep 17 00:00:00 2001
From: Simon Hornby <simon@getunleash.io>
Date: Wed, 14 Aug 2024 15:11:56 +0200
Subject: [PATCH] feat: allow editing root role/description on SCIM group
 (#7874)

---
 .../admin/groups/CreateGroup/CreateGroup.tsx  |  1 +
 .../admin/groups/EditGroup/EditGroup.tsx      |  3 +-
 .../component/admin/groups/Group/Group.tsx    |  1 -
 .../admin/groups/GroupForm/GroupForm.tsx      |  5 +-
 .../GroupCardActions/GroupCardActions.tsx     |  3 +-
 .../component/admin/groups/group-constants.ts |  2 +-
 src/lib/services/group-service.ts             | 26 +++++
 .../e2e/services/group-service.e2e.test.ts    | 98 +++++++++++++++++++
 8 files changed, 134 insertions(+), 5 deletions(-)

diff --git a/frontend/src/component/admin/groups/CreateGroup/CreateGroup.tsx b/frontend/src/component/admin/groups/CreateGroup/CreateGroup.tsx
index 6ad828df7046..24c0354f14e8 100644
--- a/frontend/src/component/admin/groups/CreateGroup/CreateGroup.tsx
+++ b/frontend/src/component/admin/groups/CreateGroup/CreateGroup.tsx
@@ -106,6 +106,7 @@ export const CreateGroup = () => {
                 handleSubmit={handleSubmit}
                 handleCancel={handleCancel}
                 mode={CREATE}
+                isScimGroup={false}
             >
                 <Button
                     type='submit'
diff --git a/frontend/src/component/admin/groups/EditGroup/EditGroup.tsx b/frontend/src/component/admin/groups/EditGroup/EditGroup.tsx
index 46cfd42dc1c1..5785920229f7 100644
--- a/frontend/src/component/admin/groups/EditGroup/EditGroup.tsx
+++ b/frontend/src/component/admin/groups/EditGroup/EditGroup.tsx
@@ -149,6 +149,7 @@ export const EditGroup = ({
                 handleSubmit={handleSubmit}
                 handleCancel={handleCancel}
                 mode={EDIT}
+                isScimGroup={isScimGroup}
             >
                 <Tooltip title={isScimGroup ? scimGroupTooltip : ''} arrow>
                     <div>
@@ -156,7 +157,7 @@ export const EditGroup = ({
                             type='submit'
                             variant='contained'
                             color='primary'
-                            disabled={isScimGroup || !isValid}
+                            disabled={!isValid}
                             data-testid={UG_SAVE_BTN_ID}
                         >
                             Save
diff --git a/frontend/src/component/admin/groups/Group/Group.tsx b/frontend/src/component/admin/groups/Group/Group.tsx
index 69d42107b2b2..721a5c6ca91e 100644
--- a/frontend/src/component/admin/groups/Group/Group.tsx
+++ b/frontend/src/component/admin/groups/Group/Group.tsx
@@ -261,7 +261,6 @@ export const Group: VFC = () => {
                                             ? scimGroupTooltip
                                             : 'Edit group',
                                     }}
-                                    disabled={isScimGroup}
                                 >
                                     <Edit />
                                 </PermissionIconButton>
diff --git a/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx b/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
index 945c621f2114..f1a11547fbb2 100644
--- a/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
+++ b/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
@@ -81,6 +81,7 @@ interface IGroupForm {
     mappingsSSO: string[];
     users: IGroupUser[];
     rootRole: number | null;
+    isScimGroup: boolean;
     setName: (name: string) => void;
     setDescription: React.Dispatch<React.SetStateAction<string>>;
     setMappingsSSO: React.Dispatch<React.SetStateAction<string[]>>;
@@ -99,6 +100,7 @@ export const GroupForm: FC<IGroupForm> = ({
     mappingsSSO,
     users,
     rootRole,
+    isScimGroup,
     setName,
     setDescription,
     setMappingsSSO,
@@ -138,6 +140,7 @@ export const GroupForm: FC<IGroupForm> = ({
                     onChange={(e) => setName(e.target.value)}
                     data-testid={UG_NAME_ID}
                     required
+                    disabled={isScimGroup}
                 />
                 <StyledInputDescription>
                     How would you describe your group?
@@ -152,7 +155,7 @@ export const GroupForm: FC<IGroupForm> = ({
                     data-testid={UG_DESC_ID}
                 />
                 <ConditionallyRender
-                    condition={isGroupSyncingEnabled}
+                    condition={isGroupSyncingEnabled && !isScimGroup}
                     show={
                         <>
                             <StyledInputDescription>
diff --git a/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions/GroupCardActions.tsx b/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions/GroupCardActions.tsx
index efc0f4e95506..6e9b0849a541 100644
--- a/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions/GroupCardActions.tsx
+++ b/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions/GroupCardActions.tsx
@@ -74,7 +74,6 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
                         aria-expanded={open ? 'true' : undefined}
                         onClick={handleClick}
                         type='button'
-                        disabled={isScimGroup}
                     >
                         <MoreVert />
                     </IconButton>
@@ -103,6 +102,7 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
                         </ListItemText>
                     </MenuItem>
                     <MenuItem
+                        disabled={isScimGroup}
                         onClick={() => {
                             onEditUsers();
                             handleClose();
@@ -122,6 +122,7 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
                             onRemove();
                             handleClose();
                         }}
+                        disabled={isScimGroup}
                     >
                         <ListItemIcon>
                             <Delete />
diff --git a/frontend/src/component/admin/groups/group-constants.ts b/frontend/src/component/admin/groups/group-constants.ts
index 3b548d68b585..e3ac8c774060 100644
--- a/frontend/src/component/admin/groups/group-constants.ts
+++ b/frontend/src/component/admin/groups/group-constants.ts
@@ -1,2 +1,2 @@
 export const scimGroupTooltip =
-    'This group is managed by your SCIM provider and cannot be changed manually';
+    'This group is managed by your SCIM provider, only the role and description can be changed manually';
diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts
index 285fd08f8dff..f83b74cd7b3e 100644
--- a/src/lib/services/group-service.ts
+++ b/src/lib/services/group-service.ts
@@ -30,6 +30,10 @@ import type { IUser } from '../types/user';
 import type EventService from '../features/events/event-service';
 import { SSO_SYNC_USER } from '../db/group-store';
 
+const setsAreEqual = (firstSet, secondSet) =>
+    firstSet.size === secondSet.size &&
+    [...firstSet].every((x) => secondSet.has(x));
+
 export class GroupService {
     private groupStore: IGroupStore;
 
@@ -231,6 +235,28 @@ export class GroupService {
                 throw new NameExistsError('Group name already exists');
             }
         }
+
+        if (existingGroup && Boolean(existingGroup.scimId)) {
+            if (existingGroup.name !== group.name) {
+                throw new BadDataError(
+                    'Cannot update the name of a SCIM group',
+                );
+            }
+
+            const existingUsers = new Set(
+                (
+                    await this.groupStore.getAllUsersByGroups([
+                        existingGroup.id,
+                    ])
+                ).map((g) => g.userId),
+            );
+
+            const newUsers = new Set(group.users?.map((g) => g.user.id) || []);
+
+            if (!setsAreEqual(existingUsers, newUsers)) {
+                throw new BadDataError('Cannot update users of a SCIM group');
+            }
+        }
     }
 
     async getRolesForProject(projectId: string): Promise<IGroupRole[]> {
diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts
index e7cc79b5ad33..d2118074e82d 100644
--- a/src/test/e2e/services/group-service.e2e.test.ts
+++ b/src/test/e2e/services/group-service.e2e.test.ts
@@ -262,3 +262,101 @@ test('adding a nonexistent role to a group should fail', async () => {
         'Request validation failed: your request body or params contain invalid data: Incorrect role id 100',
     );
 });
+
+test('trying to modify a SCIM group name should fail', async () => {
+    const group = await groupStore.create({
+        name: 'scim_group',
+        description: 'scim_group',
+    });
+
+    await db.rawDatabase('groups').where({ id: group.id }).update({
+        scim_id: 'some-faux-uuid',
+        scim_external_id: 'external-id',
+    });
+
+    await expect(() => {
+        return groupService.updateGroup(
+            {
+                id: group.id,
+                name: 'new_name',
+                users: [],
+                rootRole: 100,
+                createdAt: new Date(),
+                createdBy: 'test',
+            },
+            TEST_AUDIT_USER,
+        );
+    }).rejects.toThrow(
+        'Request validation failed: your request body or params contain invalid data: Cannot update the name of a SCIM group',
+    );
+});
+
+test('trying to modify users in a SCIM group should fail', async () => {
+    const group = await groupStore.create({
+        name: 'another_scim_group',
+        description: 'scim_group',
+    });
+
+    await db.rawDatabase('groups').where({ id: group.id }).update({
+        scim_id: 'some-other-faux-uuid',
+        scim_external_id: 'some-external-id',
+    });
+
+    await expect(() => {
+        return groupService.updateGroup(
+            {
+                id: group.id,
+                name: 'new_name',
+                users: [
+                    {
+                        user: {
+                            id: 1,
+                            isAPI: false,
+                            permissions: [],
+                        },
+                    },
+                ],
+                rootRole: 100,
+                createdAt: new Date(),
+                createdBy: 'test',
+            },
+            TEST_AUDIT_USER,
+        );
+    }).rejects.toThrow(
+        'Request validation failed: your request body or params contain invalid data: Cannot update the name of a SCIM group',
+    );
+});
+
+test('can modify a SCIM group description and root role', async () => {
+    const group = await groupStore.create({
+        name: 'yet_another_scim_group',
+        description: 'scim_group',
+    });
+
+    await db.rawDatabase('groups').where({ id: group.id }).update({
+        scim_id: 'completely-different-faux-uuid',
+        scim_external_id: 'totally-not-the-same-external-id',
+    });
+
+    await groupService.updateGroup(
+        {
+            id: group.id,
+            name: 'yet_another_scim_group',
+            description: 'new description',
+            users: [],
+            rootRole: 1,
+            createdAt: new Date(),
+            createdBy: 'test',
+        },
+        TEST_AUDIT_USER,
+    );
+
+    const updatedGroup = await groupService.getGroup(group.id);
+    expect(updatedGroup).toMatchObject({
+        description: 'new description',
+        id: group.id,
+        mappingsSSO: [],
+        name: 'yet_another_scim_group',
+        rootRole: 1,
+    });
+});