Skip to content

Commit

Permalink
fix: only delete SSO-synced group membership where membership was add…
Browse files Browse the repository at this point in the history
…ed by SSO sync (#4929)

Fixes an issue where SSO group sync would delete a syncable group that a
user was manually added to

## Discussion points

Is this the longterm fix for this? Or would we want another column in
the mapping table for future-proofing this?
  • Loading branch information
daveleek authored Oct 5, 2023
1 parent 8d0e947 commit 40ebb7e
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 7 deletions.
2 changes: 1 addition & 1 deletion src/lib/db/group-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ export default class GroupStore implements IGroupStore {
})
.orWhereRaw('jsonb_array_length(mappings_sso) = 0'),
)
.where('gu.user_id', userId);
.where({ 'gu.user_id': userId, 'gu.created_by': 'SSO' });

return rows.map(rowToGroupUser);
}
Expand Down
16 changes: 10 additions & 6 deletions src/test/e2e/services/group-service.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,34 +57,38 @@ test('should have three group', async () => {
});

test('should add person to 2 groups', async () => {
await groupService.syncExternalGroups(user.id, ['dev', 'maintainer']);
await groupService.syncExternalGroups(
user.id,
['dev', 'maintainer'],
'SSO',
);
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(2);
});

test('should remove person from one group', async () => {
await groupService.syncExternalGroups(user.id, ['maintainer']);
await groupService.syncExternalGroups(user.id, ['maintainer'], 'SSO');
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(1);
expect(groups[0].name).toEqual('maintainer_group');
});

test('should add person to completely new group with new name', async () => {
await groupService.syncExternalGroups(user.id, ['dev']);
await groupService.syncExternalGroups(user.id, ['dev'], 'SSO');
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(1);
expect(groups[0].name).toEqual('dev_group');
});

test('should not update groups when not string array ', async () => {
await groupService.syncExternalGroups(user.id, 'Everyone' as any);
await groupService.syncExternalGroups(user.id, 'Everyone' as any, 'SSO');
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(1);
expect(groups[0].name).toEqual('dev_group');
});

test('should clear groups when empty array ', async () => {
await groupService.syncExternalGroups(user.id, []);
await groupService.syncExternalGroups(user.id, [], 'SSO');
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(0);
});
Expand All @@ -95,7 +99,7 @@ test('should not remove user from no SSO definition group', async () => {
description: 'no_mapping_group',
});
await groupStore.addUserToGroups(user.id, [group.id]);
await groupService.syncExternalGroups(user.id, []);
await groupService.syncExternalGroups(user.id, [], 'SSO');
const groups = await groupService.getGroupsForUser(user.id);
expect(groups.length).toBe(1);
expect(groups[0].name).toEqual('no_mapping_group');
Expand Down

0 comments on commit 40ebb7e

Please sign in to comment.