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

@@ -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);
     }
 

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(
+        if (Array.isArray(externalGroups)) {
-            userId,
+            let newGroups = await this.groupStore.getNewGroupsForExternalUser(
-            externalGroups,
+                userId,
-        );
+                externalGroups,
-        await this.groupStore.addUserToGroups(
+            );
-            userId,
+            await this.groupStore.addUserToGroups(
-            newGroups.map((g) => g.id),
+                userId,
-        );
+                newGroups.map((g) => g.id),
-        let oldGroups = await this.groupStore.getOldGroupsForExternalUser(
+            );
-            userId,
+            let oldGroups = await this.groupStore.getOldGroupsForExternalUser(
-            externalGroups,
+                userId,
-        );
+                externalGroups,
-        await this.groupStore.deleteUsersFromGroup(oldGroups);
+            );
+            await this.groupStore.deleteUsersFromGroup(oldGroups);
+        }
     }
 
     async getGroupsForUser(userId: number): Promise<IGroup[]> {

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');
+});