From cf58140c425363bde2ebbd8d5b13c738d3acb1d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 28 Nov 2023 17:02:51 +0100 Subject: [PATCH] feat: report users on group change (#5445) ## About the changes Add user ids to group changes. This also modifies the payload of group created to include only the user id and creates events for SSO sync functionality --- src/lib/services/group-service.ts | 65 ++++++++-- src/lib/types/events.ts | 4 + .../e2e/services/group-service.e2e.test.ts | 113 +++++++++++++++++- 3 files changed, 165 insertions(+), 17 deletions(-) diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index 218d9afd13..c0264f14e7 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -11,7 +11,14 @@ import { IUnleashConfig, IUnleashStores } from '../types'; import { IGroupStore } from '../types/stores/group-store'; import { Logger } from '../logger'; import BadDataError from '../error/bad-data-error'; -import { GROUP_CREATED, GROUP_DELETED, GROUP_UPDATED } from '../types/events'; +import { + GROUP_CREATED, + GROUP_DELETED, + GROUP_UPDATED, + GROUP_USER_ADDED, + GROUP_USER_REMOVED, + IBaseEvent, +} from '../types/events'; import NameExistsError from '../error/name-exists-error'; import { IAccountStore } from '../types/stores/account-store'; import { IUser } from '../types/user'; @@ -92,25 +99,28 @@ export class GroupService { const newGroup = await this.groupStore.create(group); - await this.groupStore.addUsersToGroup( - newGroup.id, - group.users, - userName, - ); + if (group.users) { + await this.groupStore.addUsersToGroup( + newGroup.id, + group.users, + userName, + ); + } + const newUserIds = group.users?.map((g) => g.user.id); await this.eventService.storeEvent({ type: GROUP_CREATED, createdBy: userName, - data: group, + data: { ...group, users: newUserIds }, }); return newGroup; } async updateGroup(group: IGroupModel, userName: string): Promise { - const preData = await this.groupStore.get(group.id); + const existingGroup = await this.groupStore.get(group.id); - await this.validateGroup(group, preData); + await this.validateGroup(group, existingGroup); const newGroup = await this.groupStore.update(group); @@ -135,11 +145,12 @@ export class GroupService { userName, ); + const newUserIds = group.users.map((g) => g.user.id); await this.eventService.storeEvent({ type: GROUP_UPDATED, createdBy: userName, - data: newGroup, - preData, + data: { ...newGroup, users: newUserIds }, + preData: { ...existingGroup, users: existingUserIds }, }); return newGroup; @@ -175,12 +186,17 @@ export class GroupService { async deleteGroup(id: number, userName: string): Promise { const group = await this.groupStore.get(id); + const existingUsers = await this.groupStore.getAllUsersByGroups([ + group.id, + ]); + const existingUserIds = existingUsers.map((g) => g.userId); + await this.groupStore.delete(id); await this.eventService.storeEvent({ type: GROUP_DELETED, createdBy: userName, - preData: group, + preData: { ...group, users: existingUserIds }, }); } @@ -246,6 +262,31 @@ export class GroupService { externalGroups, ); await this.groupStore.deleteUsersFromGroup(oldGroups); + + const events: IBaseEvent[] = []; + for (const group of newGroups) { + events.push({ + type: GROUP_USER_ADDED, + createdBy: createdBy ?? 'unknown', + data: { + groupId: group.id, + userId, + }, + }); + } + + for (const group of oldGroups) { + events.push({ + type: GROUP_USER_REMOVED, + createdBy: createdBy ?? 'unknown', + preData: { + groupId: group.groupId, + userId, + }, + }); + } + + await this.eventService.storeEvents(events); } } diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index f37f27b0bb..e5df9457f3 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -105,6 +105,8 @@ export const SEGMENT_DELETED = 'segment-deleted' as const; export const GROUP_CREATED = 'group-created' as const; export const GROUP_UPDATED = 'group-updated' as const; export const GROUP_DELETED = 'group-deleted' as const; +export const GROUP_USER_ADDED = 'group-user-added' as const; +export const GROUP_USER_REMOVED = 'group-user-removed' as const; export const SETTING_CREATED = 'setting-created' as const; export const SETTING_UPDATED = 'setting-updated' as const; export const SETTING_DELETED = 'setting-deleted' as const; @@ -252,6 +254,8 @@ export const IEventTypes = [ GROUP_CREATED, GROUP_UPDATED, GROUP_DELETED, + GROUP_USER_ADDED, + GROUP_USER_REMOVED, SETTING_CREATED, SETTING_UPDATED, SETTING_DELETED, diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts index ec28d28420..becbda94de 100644 --- a/src/test/e2e/services/group-service.e2e.test.ts +++ b/src/test/e2e/services/group-service.e2e.test.ts @@ -51,6 +51,13 @@ afterAll(async () => { afterEach(async () => {}); +// Note: events come in reverse order, the first in the list is the last pushed +const getTestEvents = async () => { + return (await eventService.getEvents()).events.filter( + (e) => e.createdBy !== 'migration', + ); +}; + test('should have three group', async () => { const project = await groupService.getAll(); expect(project.length).toBe(3); @@ -64,36 +71,101 @@ test('should add person to 2 groups', async () => { ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(2); + const events = await getTestEvents(); + expect(events[0]).toMatchObject({ + type: 'group-user-added', + data: { + userId: user.id, + groupId: groups[1].id, + }, + }); + expect(events[1]).toMatchObject({ + type: 'group-user-added', + data: { + userId: user.id, + groupId: groups[0].id, + }, + }); }); +// this test depends on the other tests being executed test('should remove person from one group', async () => { + const removedGroups = (await groupService.getGroupsForUser(user.id)).filter( + (g) => !g.mappingsSSO?.includes('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'); + + expect(removedGroups).toHaveLength(1); + const events = await getTestEvents(); + expect(events[0]).toMatchObject({ + type: 'group-user-removed', + preData: { + userId: user.id, + groupId: removedGroups[0].id, + }, + }); }); +// this test depends on the other tests being executed 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.syncExternalGroups(user.id, ['dev'], 'SSO'); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('dev_group'); + + const events = await getTestEvents(); + expect(removedGroups).toHaveLength(1); + expect(events[0]).toMatchObject({ + type: 'group-user-removed', + preData: { + userId: user.id, + groupId: removedGroups[0].id, + }, + }); + expect(events[1]).toMatchObject({ + type: 'group-user-added', + data: { + userId: user.id, + groupId: groups[0].id, + }, + }); }); test('should not update groups when not string array ', async () => { + const beforeEvents = await getTestEvents(); 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'); + const afterEvents = await getTestEvents(); + expect(beforeEvents).toHaveLength(afterEvents.length); }); +// 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.syncExternalGroups(user.id, [], 'SSO'); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(0); + expect(removedGroups).toHaveLength(1); + const events = await getTestEvents(); + expect(events[0]).toMatchObject({ + type: 'group-user-removed', + preData: { + userId: user.id, + groupId: removedGroups[0].id, + }, + }); }); test('should not remove user from no SSO definition group', async () => { + const beforeEvents = await getTestEvents(); const group = await groupStore.create({ name: 'no_mapping_group', description: 'no_mapping_group', @@ -103,13 +175,18 @@ test('should not remove user from no SSO definition group', async () => { const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('no_mapping_group'); + const afterEvents = await getTestEvents(); + expect(beforeEvents).toHaveLength(afterEvents.length); }); test('adding a root role to a group with a project role should not fail', async () => { - const group = await groupStore.create({ - name: 'root_group', - description: 'root_group', - }); + const group = await groupService.createGroup( + { + name: 'root_group', + description: 'root_group', + }, + 'test', + ); await stores.accessStore.addGroupToRole(group.id, 1, 'test', 'default'); @@ -133,7 +210,33 @@ test('adding a root role to a group with a project role should not fail', async rootRole: 1, }); - expect.assertions(1); + const events = await getTestEvents(); + expect(events[1]).toMatchObject({ + type: 'group-created', + data: { + description: group.description, + name: group.name, + }, + }); + expect(events[0]).toMatchObject({ + type: 'group-updated', + data: { + description: group.description, + id: group.id, + mappingsSSO: [], + name: group.name, + rootRole: 1, + }, + preData: { + description: group.description, + id: group.id, + mappingsSSO: [], + name: group.name, + rootRole: null, + }, + }); + + expect.assertions(3); }); test('adding a nonexistent role to a group should fail', async () => {