From e5aa1a81cb74af3064c603f5088c3ca3c884ac35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 27 May 2024 11:58:32 +0200 Subject: [PATCH] feat: add remote ip to all events (2) (#7149) ## About the changes This aligns us with the requirement of having ip in all events. After tackling the enterprise part we will be able to make the ip field mandatory here: https://github.com/Unleash/unleash/blob/2c66a4ace44e3553195f45a8eafa0d19ca26113c/src/lib/types/events.ts#L362 --- src/lib/db/group-store.ts | 4 +- .../metrics/instance/instance-service.ts | 1 + src/lib/services/access-service.test.ts | 2 +- src/lib/services/addon-service.test.ts | 55 +++++++++++++------ .../feature-service-potentially-stale.test.ts | 4 +- src/lib/services/group-service.ts | 3 +- src/test/e2e/api/admin/event.e2e.test.ts | 4 ++ .../e2e/services/group-service.e2e.test.ts | 35 +++++++++--- src/test/e2e/stores/event-store.e2e.test.ts | 6 ++ src/test/fixtures/fake-event-store.ts | 18 ++++-- 10 files changed, 97 insertions(+), 35 deletions(-) diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts index 77107c1624..f353be131a 100644 --- a/src/lib/db/group-store.ts +++ b/src/lib/db/group-store.ts @@ -33,6 +33,8 @@ const GROUP_COLUMNS = [ 'scim_id', ]; +export const SSO_SYNC_USER = 'SSO'; + const rowToGroup = (row) => { if (!row) { throw new NotFoundError('No group found'); @@ -332,7 +334,7 @@ export default class GroupStore implements IGroupStore { }) .orWhereRaw('jsonb_array_length(mappings_sso) = 0'), ) - .where({ 'gu.user_id': userId, 'gu.created_by': 'SSO' }); + .where({ 'gu.user_id': userId, 'gu.created_by': SSO_SYNC_USER }); return rows.map(rowToGroupUser); } diff --git a/src/lib/features/metrics/instance/instance-service.ts b/src/lib/features/metrics/instance/instance-service.ts index fca6c96ee5..05b4c136c7 100644 --- a/src/lib/features/metrics/instance/instance-service.ts +++ b/src/lib/features/metrics/instance/instance-service.ts @@ -118,6 +118,7 @@ export default class ClientInstanceService { createdBy: app.createdBy || SYSTEM_USER.username!, data: app, createdByUserId: app.createdByUserId || SYSTEM_USER.id, + ip: '', // TODO: fix this, how do we get the ip from the client? This comes from a row in the DB })); await this.eventStore.batchStore(events); } diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts index 5324dcf742..c7ff960f25 100644 --- a/src/lib/services/access-service.test.ts +++ b/src/lib/services/access-service.test.ts @@ -201,7 +201,7 @@ test('user with custom root role should get a user root role', async () => { expect(role.name).toBe('custom-root-role'); const events = await eventStore.getEvents(); expect(events).toHaveLength(1); - expect(events[0]).toEqual({ + expect(events[0]).toMatchObject({ type: ROLE_CREATED, createdBy: SYSTEM_USER_AUDIT.username, createdByUserId: SYSTEM_USER.id, diff --git a/src/lib/services/addon-service.test.ts b/src/lib/services/addon-service.test.ts index ab630d487a..50d8aba708 100644 --- a/src/lib/services/addon-service.test.ts +++ b/src/lib/services/addon-service.test.ts @@ -117,6 +117,7 @@ test('should trigger simple-addon eventHandler', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; @@ -144,7 +145,7 @@ test('should not trigger event handler if project of event is different from add await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: 'someotherproject', data: { @@ -152,6 +153,7 @@ test('should not trigger event handler if project of event is different from add enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; // @ts-expect-error @@ -178,7 +180,7 @@ test('should trigger event handler if project for event is one of the desired pr await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProject, data: { @@ -186,10 +188,11 @@ test('should trigger event handler if project for event is one of the desired pr enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: otherProject, data: { @@ -197,6 +200,7 @@ test('should trigger event handler if project for event is one of the desired pr enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; // @ts-expect-error @@ -225,7 +229,7 @@ test('should trigger events for multiple projects if addon is setup to filter mu await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[0], data: { @@ -233,10 +237,11 @@ test('should trigger events for multiple projects if addon is setup to filter mu enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: otherProject, data: { @@ -244,10 +249,11 @@ test('should trigger events for multiple projects if addon is setup to filter mu enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[1], data: { @@ -255,6 +261,7 @@ test('should trigger events for multiple projects if addon is setup to filter mu enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; // @ts-expect-error @@ -286,7 +293,7 @@ test('should filter events on environment if addon is setup to filter for it', a await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredEnvironment, environment: desiredEnvironment, @@ -295,10 +302,11 @@ test('should filter events on environment if addon is setup to filter for it', a enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, environment: otherEnvironment, data: { @@ -306,6 +314,7 @@ test('should filter events on environment if addon is setup to filter for it', a enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; // @ts-expect-error @@ -333,7 +342,7 @@ test('should not filter out global events (no specific environment) even if addo const globalEventWithNoEnvironment = { type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: 'some-project', data: { @@ -341,6 +350,7 @@ test('should not filter out global events (no specific environment) even if addo enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }; await addonService.createAddon(config, TEST_AUDIT_USER); @@ -371,13 +381,14 @@ test('should not filter out global events (no specific project) even if addon is const globalEventWithNoProject = { type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, data: { name: 'some-toggle', enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }; await addonService.createAddon(config, TEST_AUDIT_USER); @@ -409,7 +420,7 @@ test('should support wildcard option for filtering addons', async () => { await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[0], data: { @@ -417,10 +428,11 @@ test('should support wildcard option for filtering addons', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: otherProject, data: { @@ -428,10 +440,11 @@ test('should support wildcard option for filtering addons', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[1], data: { @@ -439,6 +452,7 @@ test('should support wildcard option for filtering addons', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; // @ts-expect-error @@ -476,7 +490,7 @@ test('Should support filtering by both project and environment', async () => { await addonService.createAddon(config, TEST_AUDIT_USER); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[0], environment: desiredEnvironments[0], @@ -485,10 +499,11 @@ test('Should support filtering by both project and environment', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[0], environment: 'wrongenvironment', @@ -497,10 +512,11 @@ test('Should support filtering by both project and environment', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[2], environment: desiredEnvironments[1], @@ -509,10 +525,11 @@ test('Should support filtering by both project and environment', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: desiredProjects[2], environment: desiredEnvironments[2], @@ -521,10 +538,11 @@ test('Should support filtering by both project and environment', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, - createdBy: SYSTEM_USER.username, + createdBy: SYSTEM_USER.username!, createdByUserId: SYSTEM_USER.id, project: 'wrongproject', environment: desiredEnvironments[0], @@ -533,6 +551,7 @@ test('Should support filtering by both project and environment', async () => { enabled: false, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }); const simpleProvider = addonService.addonProviders.simple; diff --git a/src/lib/services/feature-service-potentially-stale.test.ts b/src/lib/services/feature-service-potentially-stale.test.ts index 960adb5588..2108950ef1 100644 --- a/src/lib/services/feature-service-potentially-stale.test.ts +++ b/src/lib/services/feature-service-potentially-stale.test.ts @@ -1,6 +1,6 @@ import { FEATURE_POTENTIALLY_STALE_ON, - type IEvent, + type IBaseEvent, type IUnleashConfig, type IUnleashStores, } from '../types'; @@ -28,7 +28,7 @@ test('Should only store events for potentially stale on', async () => { { // @ts-expect-error eventStore: { - batchStore: async (events: IEvent[]) => { + batchStore: async (events: IBaseEvent[]) => { expect(events.length).toBe(1); const [event1] = events; diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index 1394cbf471..1a5da221fd 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -29,6 +29,7 @@ import NameExistsError from '../error/name-exists-error'; import type { IAccountStore } from '../types/stores/account-store'; import type { IUser } from '../types/user'; import type EventService from '../features/events/event-service'; +import { SSO_SYNC_USER } from '../db/group-store'; export class GroupService { private groupStore: IGroupStore; @@ -302,7 +303,7 @@ export class GroupService { await this.groupStore.addUserToGroups( userId, newGroups.map((g) => g.id), - auditUser.username, + SSO_SYNC_USER, ); const oldGroups = await this.groupStore.getOldGroupsForExternalUser( userId, diff --git a/src/test/e2e/api/admin/event.e2e.test.ts b/src/test/e2e/api/admin/event.e2e.test.ts index 04c64f6a1a..bd9ff120a9 100644 --- a/src/test/e2e/api/admin/event.e2e.test.ts +++ b/src/test/e2e/api/admin/event.e2e.test.ts @@ -61,6 +61,7 @@ test('Can filter by project', async () => { tags: [], createdBy: 'test-user', createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', }); await eventService.storeEvent({ type: FEATURE_CREATED, @@ -70,6 +71,7 @@ test('Can filter by project', async () => { createdBy: 'test-user', environment: 'test', createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', }); await app.request .get('/api/admin/events?project=default') @@ -89,6 +91,7 @@ test('can search for events', async () => { tags: [], createdBy: randomId(), createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', }, { type: FEATURE_CREATED, @@ -98,6 +101,7 @@ test('can search for events', async () => { tags: [{ type: 'simple', value: randomId() }], createdBy: randomId(), createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', }, ]; diff --git a/src/test/e2e/services/group-service.e2e.test.ts b/src/test/e2e/services/group-service.e2e.test.ts index 85ed9ae335..08c77f0faf 100644 --- a/src/test/e2e/services/group-service.e2e.test.ts +++ b/src/test/e2e/services/group-service.e2e.test.ts @@ -8,6 +8,7 @@ import { type IUnleashStores, type IUser, TEST_AUDIT_USER, + SYSTEM_USER_AUDIT, } from '../../../lib/types'; let stores: IUnleashStores; @@ -69,10 +70,10 @@ test('should have three group', async () => { }); test('should add person to 2 groups', async () => { - await groupService.syncExternalGroups( + await groupService.syncExternalGroupsWithAudit( user.id, ['dev', 'maintainer'], - 'SSO', + SYSTEM_USER_AUDIT, ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(2); @@ -98,7 +99,11 @@ 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'); + await groupService.syncExternalGroupsWithAudit( + user.id, + ['maintainer'], + SYSTEM_USER_AUDIT, + ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('maintainer_group'); @@ -119,7 +124,11 @@ 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'); + await groupService.syncExternalGroupsWithAudit( + user.id, + ['dev'], + SYSTEM_USER_AUDIT, + ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('dev_group'); @@ -144,7 +153,11 @@ 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.syncExternalGroups(user.id, 'Everyone' as any, 'SSO'); + await groupService.syncExternalGroupsWithAudit( + user.id, + 'Everyone' as any, + SYSTEM_USER_AUDIT, + ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('dev_group'); @@ -155,7 +168,11 @@ 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.syncExternalGroups(user.id, [], 'SSO'); + await groupService.syncExternalGroupsWithAudit( + user.id, + [], + SYSTEM_USER_AUDIT, + ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(0); expect(removedGroups).toHaveLength(1); @@ -176,7 +193,11 @@ 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, [], 'SSO'); + await groupService.syncExternalGroupsWithAudit( + user.id, + [], + SYSTEM_USER_AUDIT, + ); const groups = await groupService.getGroupsForUser(user.id); expect(groups.length).toBe(1); expect(groups[0].name).toEqual('no_mapping_group'); diff --git a/src/test/e2e/stores/event-store.e2e.test.ts b/src/test/e2e/stores/event-store.e2e.test.ts index f66d38f664..bedbfaf7e1 100644 --- a/src/test/e2e/stores/event-store.e2e.test.ts +++ b/src/test/e2e/stores/event-store.e2e.test.ts @@ -100,6 +100,7 @@ test('Should be able to store multiple events at once', async () => { clientIp: '127.0.0.1', appName: 'test1', }, + ip: '127.0.0.1', }; const event2 = { type: APPLICATION_CREATED, @@ -109,6 +110,7 @@ test('Should be able to store multiple events at once', async () => { clientIp: '127.0.0.1', appName: 'test2', }, + ip: '127.0.0.1', }; const event3 = { type: APPLICATION_CREATED, @@ -119,6 +121,7 @@ test('Should be able to store multiple events at once', async () => { appName: 'test3', }, tags: [{ type: 'simple', value: 'mytest' }], + ip: '127.0.0.1', }; const seen: IEvent[] = []; eventStore.on(APPLICATION_CREATED, (e) => seen.push(e)); @@ -142,6 +145,7 @@ test('Should get all stored events', async () => { enabled: true, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }; await eventStore.store(event); const events = await eventStore.getAll(); @@ -161,6 +165,7 @@ test('Should delete stored event', async () => { enabled: true, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }; await eventStore.store(event); await eventStore.store(event); @@ -185,6 +190,7 @@ test('Should get stored event by id', async () => { enabled: true, strategies: [{ name: 'default' }], }, + ip: '127.0.0.1', }; await eventStore.store(event); const events = await eventStore.getAll(); diff --git a/src/test/fixtures/fake-event-store.ts b/src/test/fixtures/fake-event-store.ts index c1c9abbfd7..153f885f04 100644 --- a/src/test/fixtures/fake-event-store.ts +++ b/src/test/fixtures/fake-event-store.ts @@ -1,5 +1,5 @@ import type { IEventStore } from '../../lib/types/stores/event-store'; -import type { IEvent } from '../../lib/types/events'; +import type { IBaseEvent, IEvent } from '../../lib/types/events'; import { sharedEventEmitter } from '../../lib/util/anyEventEmitter'; import type { IQueryOperations } from '../../lib/features/events/event-store'; import type { SearchEventsSchema } from '../../lib/openapi'; @@ -19,15 +19,23 @@ class FakeEventStore implements IEventStore { return Promise.resolve(1); } - store(event: IEvent): Promise { - this.events.push(event); + store(event: IBaseEvent): Promise { + this.events.push({ + ...event, + id: this.events.length, + createdAt: new Date(), + }); this.eventEmitter.emit(event.type, event); return Promise.resolve(); } - batchStore(events: IEvent[]): Promise { + batchStore(events: IBaseEvent[]): Promise { events.forEach((event) => { - this.events.push(event); + this.events.push({ + ...event, + id: this.events.length, + createdAt: new Date(), + }); this.eventEmitter.emit(event.type, event); }); return Promise.resolve();