Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow editing root role/description on SCIM group #7874

Merged
merged 5 commits into from
Aug 14, 2024
Merged

feat: allow editing root role/description on SCIM group #7874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0dc5743
feat: expose root role and description to be edited on frontend
sighphyre Aug 14, 2024
9b1938d
feat: block updates to groups when static properties are updated on s…
sighphyre Aug 14, 2024
ddd0e93
fix: block updates to only name or users for groups
sighphyre Aug 14, 2024
2160d34
chore: tests
sighphyre Aug 14, 2024
892727e
chore: better message to the user about what you can change
sighphyre Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/src/component/admin/groups/CreateGroup/CreateGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export const CreateGroup = () => {
handleSubmit={handleSubmit}
handleCancel={handleCancel}
mode={CREATE}
isScimGroup={false}
Copy link
Member Author

@sighphyre sighphyre Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the create flow - can't be a SCIM group if SCIM didn't make it!

>
<Button
type='submit'
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/component/admin/groups/EditGroup/EditGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,15 @@ export const EditGroup = ({
handleSubmit={handleSubmit}
handleCancel={handleCancel}
mode={EDIT}
isScimGroup={isScimGroup}
>
<Tooltip title={isScimGroup ? scimGroupTooltip : ''} arrow>
<div>
<Button
type='submit'
variant='contained'
color='primary'
disabled={isScimGroup || !isValid}
disabled={!isValid}
data-testid={UG_SAVE_BTN_ID}
>
Save
Expand Down
1 change: 0 additions & 1 deletion frontend/src/component/admin/groups/Group/Group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,6 @@ export const Group: VFC = () => {
? scimGroupTooltip
: 'Edit group',
}}
disabled={isScimGroup}
>
<Edit />
</PermissionIconButton>
Expand Down
5 changes: 4 additions & 1 deletion frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[]>>;
Expand All @@ -99,6 +100,7 @@ export const GroupForm: FC<IGroupForm> = ({
mappingsSSO,
users,
rootRole,
isScimGroup,
setName,
setDescription,
setMappingsSSO,
Expand Down Expand Up @@ -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?
Expand All @@ -152,7 +155,7 @@ export const GroupForm: FC<IGroupForm> = ({
data-testid={UG_DESC_ID}
/>
<ConditionallyRender
condition={isGroupSyncingEnabled}
condition={isGroupSyncingEnabled && !isScimGroup}
show={
<>
<StyledInputDescription>
Expand Down
3 changes: 2 additions & 1 deletion ...end/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions/GroupCardActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
aria-expanded={open ? 'true' : undefined}
onClick={handleClick}
type='button'
disabled={isScimGroup}
>
<MoreVert />
</IconButton>
Expand Down Expand Up @@ -103,6 +102,7 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
</ListItemText>
</MenuItem>
<MenuItem
disabled={isScimGroup}
onClick={() => {
onEditUsers();
handleClose();
Expand All @@ -122,6 +122,7 @@ export const GroupCardActions: FC<IGroupCardActions> = ({
onRemove();
handleClose();
}}
disabled={isScimGroup}
>
<ListItemIcon>
<Delete />
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/component/admin/groups/group-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
26 changes: 26 additions & 0 deletions src/lib/services/group-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly and bloats the call site where it's actually relevant but only really useful in one place so I've pulled it out into a non exported function

firstSet.size === secondSet.size &&
[...firstSet].every((x) => secondSet.has(x));

export class GroupService {
private groupStore: IGroupStore;

Expand Down Expand Up @@ -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[]> {
Expand Down
98 changes: 98 additions & 0 deletions src/test/e2e/services/group-service.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});
Loading