From 07ef4a114ffd2f2d05fe27d8bbb4867ce7cd9dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Thu, 30 May 2024 11:47:30 +0200 Subject: [PATCH] chore: sync user groups is a system action (#7214) ## About the changes After an internal conversation, we concluded that syncExternalGroups is an action that Unleash performs as a system, not something triggered by the user. We keep the method and just write the event log that the action was performed by the system user. --- src/lib/services/group-service.ts | 62 ++----------------- .../e2e/services/group-service.e2e.test.ts | 37 ++--------- 2 files changed, 11 insertions(+), 88 deletions(-) diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index 1a5da221fd..285fd08f8d 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -10,6 +10,7 @@ import type { import { GroupDeletedEvent, GroupUpdatedEvent, + SYSTEM_USER_AUDIT, type IAuditUser, type IUnleashConfig, type IUnleashStores, @@ -19,8 +20,6 @@ import type { Logger } from '../logger'; import BadDataError from '../error/bad-data-error'; import { GROUP_CREATED, - GROUP_USER_ADDED, - GROUP_USER_REMOVED, GroupUserAdded, GroupUserRemoved, type IBaseEvent, @@ -238,62 +237,11 @@ export class GroupService { return this.groupStore.getProjectGroupRoles(projectId); } - /** @deprecated use syncExternalGroupsWithAudit */ async syncExternalGroups( userId: number, externalGroups: string[], - createdBy?: string, - createdByUserId?: number, - ): Promise { - if (Array.isArray(externalGroups)) { - const newGroups = await this.groupStore.getNewGroupsForExternalUser( - userId, - externalGroups, - ); - await this.groupStore.addUserToGroups( - userId, - newGroups.map((g) => g.id), - createdBy, - ); - const oldGroups = await this.groupStore.getOldGroupsForExternalUser( - userId, - externalGroups, - ); - await this.groupStore.deleteUsersFromGroup(oldGroups); - - const events: IBaseEvent[] = []; - for (const group of newGroups) { - events.push({ - type: GROUP_USER_ADDED, - createdBy: createdBy ?? 'unknown', - createdByUserId: createdByUserId ?? -9999, - data: { - groupId: group.id, - userId, - }, - }); - } - - for (const group of oldGroups) { - events.push({ - type: GROUP_USER_REMOVED, - createdBy: createdBy ?? 'unknown', - createdByUserId: createdByUserId ?? -9999, - preData: { - groupId: group.groupId, - userId, - }, - }); - } - - await this.eventService.storeEvents(events); - } - } - - async syncExternalGroupsWithAudit( - userId: number, - externalGroups: string[], - auditUser: IAuditUser, + createdBy?: string, // deprecated + createdByUserId?: number, // deprecated ): Promise { if (Array.isArray(externalGroups)) { const newGroups = await this.groupStore.getNewGroupsForExternalUser( @@ -317,7 +265,7 @@ export class GroupService { new GroupUserAdded({ userId, groupId: group.id, - auditUser, + auditUser: SYSTEM_USER_AUDIT, }), ); } @@ -327,7 +275,7 @@ export class GroupService { new GroupUserRemoved({ userId, groupId: group.groupId, - auditUser, + auditUser: SYSTEM_USER_AUDIT, }), ); } diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts index 08c77f0faf..e7cc79b5ad 100644 --- a/src/test/e2e/services/group-service.e2e.test.ts +++ b/src/test/e2e/services/group-service.e2e.test.ts @@ -8,7 +8,6 @@ import { type IUnleashStores, type IUser, TEST_AUDIT_USER, - SYSTEM_USER_AUDIT, } from '../../../lib/types'; let stores: IUnleashStores; @@ -70,11 +69,7 @@ test('should have three group', async () => { }); test('should add person to 2 groups', async () => { - await groupService.syncExternalGroupsWithAudit( - user.id, - ['dev', 'maintainer'], - SYSTEM_USER_AUDIT, - ); + await groupService.syncExternalGroups(user.id, ['dev', 'maintainer']); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(2); const events = await getTestEvents(); @@ -99,11 +94,7 @@ test('should remove person from one group', async () => { const removedGroups = (await groupService.getGroupsForUser(user.id)).filter( (g) => !g.mappingsSSO?.includes('maintainer'), ); - await groupService.syncExternalGroupsWithAudit( - user.id, - ['maintainer'], - SYSTEM_USER_AUDIT, - ); + await groupService.syncExternalGroups(user.id, ['maintainer']); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('maintainer_group'); @@ -124,11 +115,7 @@ test('should add person to completely new group with new name', async () => { const removedGroups = (await groupService.getGroupsForUser(user.id)).filter( (g) => !g.mappingsSSO?.includes('dev'), ); - await groupService.syncExternalGroupsWithAudit( - user.id, - ['dev'], - SYSTEM_USER_AUDIT, - ); + await groupService.syncExternalGroups(user.id, ['dev']); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('dev_group'); @@ -153,11 +140,7 @@ test('should add person to completely new group with new name', async () => { test('should not update groups when not string array ', async () => { const beforeEvents = await getTestEvents(); - await groupService.syncExternalGroupsWithAudit( - user.id, - 'Everyone' as any, - SYSTEM_USER_AUDIT, - ); + 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'); @@ -168,11 +151,7 @@ test('should not update groups when not string array ', async () => { // this test depends on the other tests being executed test('should clear groups when empty array ', async () => { const removedGroups = await groupService.getGroupsForUser(user.id); - await groupService.syncExternalGroupsWithAudit( - user.id, - [], - SYSTEM_USER_AUDIT, - ); + await groupService.syncExternalGroups(user.id, []); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(0); expect(removedGroups).toHaveLength(1); @@ -193,11 +172,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.syncExternalGroupsWithAudit( - user.id, - [], - SYSTEM_USER_AUDIT, - ); + 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');