From 4068e4749f86fb3c5c0dcb7ebda740d0cff924ad Mon Sep 17 00:00:00 2001
From: sellinjaanus <107852002+sellinjaanus@users.noreply.github.com>
Date: Mon, 17 Oct 2022 11:44:36 +0300
Subject: [PATCH] Fix all groups being removed, even when no external groups
 were defined (#2197)

* Syncing groups

* Add tests

* Fix all groups being removed

Co-authored-by: sjaanus <sellinjaanus@gmail.com>
---
 src/lib/db/group-store.ts                     |  4 ++-
 src/lib/services/group-service.ts             | 28 ++++++++++---------
 .../e2e/services/group-service.e2e.test.ts    | 28 +++++++++++++++++++
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts
index 732ebd113d..63315b7b5f 100644
--- a/src/lib/db/group-store.ts
+++ b/src/lib/db/group-store.ts
@@ -254,9 +254,11 @@ export default class GroupStore implements IGroupStore {
                     .select('id')
                     .whereRaw('mappings_sso \\?| :groups', {
                         groups: externalGroups,
-                    }),
+                    })
+                    .orWhereRaw('jsonb_array_length(mappings_sso) = 0'),
             )
             .where('gu.user_id', userId);
+
         return rows.map(rowToGroupUser);
     }
 
diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts
index 6287a64b56..46c5995afe 100644
--- a/src/lib/services/group-service.ts
+++ b/src/lib/services/group-service.ts
@@ -220,19 +220,21 @@ export class GroupService {
         userId: number,
         externalGroups: string[],
     ): Promise<void> {
-        let newGroups = await this.groupStore.getNewGroupsForExternalUser(
-            userId,
-            externalGroups,
-        );
-        await this.groupStore.addUserToGroups(
-            userId,
-            newGroups.map((g) => g.id),
-        );
-        let oldGroups = await this.groupStore.getOldGroupsForExternalUser(
-            userId,
-            externalGroups,
-        );
-        await this.groupStore.deleteUsersFromGroup(oldGroups);
+        if (Array.isArray(externalGroups)) {
+            let newGroups = await this.groupStore.getNewGroupsForExternalUser(
+                userId,
+                externalGroups,
+            );
+            await this.groupStore.addUserToGroups(
+                userId,
+                newGroups.map((g) => g.id),
+            );
+            let oldGroups = await this.groupStore.getOldGroupsForExternalUser(
+                userId,
+                externalGroups,
+            );
+            await this.groupStore.deleteUsersFromGroup(oldGroups);
+        }
     }
 
     async getGroupsForUser(userId: number): Promise<IGroup[]> {
diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts
index 18a3c6a128..0b23591bff 100644
--- a/src/test/e2e/services/group-service.e2e.test.ts
+++ b/src/test/e2e/services/group-service.e2e.test.ts
@@ -2,11 +2,13 @@ import dbInit, { ITestDb } from '../helpers/database-init';
 import getLogger from '../../fixtures/no-logger';
 import { createTestConfig } from '../../config/test-config';
 import { GroupService } from '../../../lib/services/group-service';
+import GroupStore from '../../../lib/db/group-store';
 
 let stores;
 let db: ITestDb;
 
 let groupService: GroupService;
+let groupStore: GroupStore;
 let user;
 
 beforeAll(async () => {
@@ -20,6 +22,7 @@ beforeAll(async () => {
         getLogger,
     });
     groupService = new GroupService(stores, config);
+    groupStore = stores.groupStore;
 
     await stores.groupStore.create({
         name: 'dev_group',
@@ -69,3 +72,28 @@ test('should add person to completely new group with new name', async () => {
     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);
+    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, []);
+    const groups = await groupService.getGroupsForUser(user.id);
+    expect(groups.length).toBe(0);
+});
+
+test('should not remove user from no SSO definition group', async () => {
+    const group = await groupStore.create({
+        name: 'no_mapping_group',
+        description: 'no_mapping_group',
+    });
+    await groupStore.addUserToGroups(user.id, [group.id]);
+    await groupService.syncExternalGroups(user.id, []);
+    const groups = await groupService.getGroupsForUser(user.id);
+    expect(groups.length).toBe(1);
+    expect(groups[0].name).toEqual('no_mapping_group');
+});