From 40ebb7ef95edf0c23619352bd3e9453a4dfe46da Mon Sep 17 00:00:00 2001
From: David Leek <david@getunleash.io>
Date: Thu, 5 Oct 2023 13:22:46 +0200
Subject: [PATCH] fix: only delete SSO-synced group membership where membership
 was added 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?
---
 src/lib/db/group-store.ts                       |  2 +-
 src/test/e2e/services/group-service.e2e.test.ts | 16 ++++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts
index 055a7c2a350e..835b6eb416b7 100644
--- a/src/lib/db/group-store.ts
+++ b/src/lib/db/group-store.ts
@@ -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);
     }
diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts
index a4eab6bb6256..ec28d284209e 100644
--- a/src/test/e2e/services/group-service.e2e.test.ts
+++ b/src/test/e2e/services/group-service.e2e.test.ts
@@ -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);
 });
@@ -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');