From df73c65073b6fe5121cd2cdcfc9071dfc84977c2 Mon Sep 17 00:00:00 2001 From: Jaanus Sellin Date: Wed, 21 Aug 2024 13:59:24 +0300 Subject: [PATCH] feat: filter projectless events for normal users (#7914) Now events that do not have project ( for example user creation, segment creation etc), will not be displayed to non root admins. --- .../features/access/access-read-model-type.ts | 3 + src/lib/features/access/access-read-model.ts | 28 ++++ .../features/access/createAccessReadModel.ts | 24 +++ .../features/access/createAccessService.ts | 1 - .../features/events/createEventsService.ts | 8 + .../events/event-created-by-migration.test.ts | 11 +- src/lib/features/events/event-service.ts | 15 ++ .../features/feature-search/search-utils.ts | 7 +- .../feature-toggle-strategies-store-type.ts | 2 +- src/lib/features/segment/segment-service.ts | 2 +- src/lib/services/access-service.test.ts | 19 ++- src/lib/services/access-service.ts | 12 -- .../feature-service-potentially-stale.test.ts | 1 + src/lib/types/events.ts | 4 +- .../e2e/api/admin/event-search.e2e.test.ts | 153 ++++++++++++++++-- 15 files changed, 246 insertions(+), 44 deletions(-) create mode 100644 src/lib/features/access/access-read-model-type.ts create mode 100644 src/lib/features/access/access-read-model.ts create mode 100644 src/lib/features/access/createAccessReadModel.ts diff --git a/src/lib/features/access/access-read-model-type.ts b/src/lib/features/access/access-read-model-type.ts new file mode 100644 index 000000000000..0516327bc267 --- /dev/null +++ b/src/lib/features/access/access-read-model-type.ts @@ -0,0 +1,3 @@ +export interface IAccessReadModel { + isRootAdmin(userId: number): Promise; +} diff --git a/src/lib/features/access/access-read-model.ts b/src/lib/features/access/access-read-model.ts new file mode 100644 index 000000000000..eb709950c53d --- /dev/null +++ b/src/lib/features/access/access-read-model.ts @@ -0,0 +1,28 @@ +import { + ADMIN_TOKEN_USER, + type IAccessStore, + type IUnleashStores, + SYSTEM_USER_ID, +} from '../../types'; +import type { IAccessReadModel } from './access-read-model-type'; +import * as permissions from '../../types/permissions'; + +const { ADMIN } = permissions; + +export class AccessReadModel implements IAccessReadModel { + private store: IAccessStore; + + constructor({ accessStore }: Pick) { + this.store = accessStore; + } + + async isRootAdmin(userId: number): Promise { + if (userId === SYSTEM_USER_ID || userId === ADMIN_TOKEN_USER.id) { + return true; + } + const roles = await this.store.getRolesForUserId(userId); + return roles.some( + (role) => role.name.toLowerCase() === ADMIN.toLowerCase(), + ); + } +} diff --git a/src/lib/features/access/createAccessReadModel.ts b/src/lib/features/access/createAccessReadModel.ts new file mode 100644 index 000000000000..f0fc23c6505d --- /dev/null +++ b/src/lib/features/access/createAccessReadModel.ts @@ -0,0 +1,24 @@ +import type { Db, IUnleashConfig } from '../../server-impl'; +import type { IAccessReadModel } from './access-read-model-type'; +import { AccessReadModel } from './access-read-model'; +import { AccessStore } from '../../db/access-store'; +import FakeRoleStore from '../../../test/fixtures/fake-role-store'; +import FakeAccessStore from '../../../test/fixtures/fake-access-store'; +import type { IAccessStore } from '../../types'; + +export const createAccessReadModel = ( + db: Db, + config: IUnleashConfig, +): IAccessReadModel => { + const { eventBus, getLogger } = config; + const accessStore = new AccessStore(db, eventBus, getLogger); + return new AccessReadModel({ accessStore }); +}; + +export const createFakeAccessReadModel = ( + accessStore?: IAccessStore, +): IAccessReadModel => { + const roleStore = new FakeRoleStore(); + const finalAccessStore = accessStore ?? new FakeAccessStore(roleStore); + return new AccessReadModel({ accessStore: finalAccessStore }); +}; diff --git a/src/lib/features/access/createAccessService.ts b/src/lib/features/access/createAccessService.ts index b04b37fa8b0f..26791fc82fac 100644 --- a/src/lib/features/access/createAccessService.ts +++ b/src/lib/features/access/createAccessService.ts @@ -33,7 +33,6 @@ export const createAccessService = ( { getLogger }, eventService, ); - return new AccessService( { accessStore, accountStore, roleStore, environmentStore }, { getLogger }, diff --git a/src/lib/features/events/createEventsService.ts b/src/lib/features/events/createEventsService.ts index 3cfd82c51932..40ac16a3c36b 100644 --- a/src/lib/features/events/createEventsService.ts +++ b/src/lib/features/events/createEventsService.ts @@ -13,6 +13,10 @@ import { createFakePrivateProjectChecker, createPrivateProjectChecker, } from '../private-project/createPrivateProjectChecker'; +import { + createAccessReadModel, + createFakeAccessReadModel, +} from '../access/createAccessReadModel'; export const createEventsService: ( db: Db, @@ -25,10 +29,12 @@ export const createEventsService: ( config.getLogger, ); const privateProjectChecker = createPrivateProjectChecker(db, config); + const accessReadModel = createAccessReadModel(db, config); return new EventService( { eventStore, featureTagStore }, config, privateProjectChecker, + accessReadModel, ); }; @@ -43,9 +49,11 @@ export const createFakeEventsService: ( const featureTagStore = stores?.featureTagStore || new FakeFeatureTagStore(); const fakePrivateProjectChecker = createFakePrivateProjectChecker(); + const fakeAccessReadModel = createFakeAccessReadModel(); return new EventService( { eventStore, featureTagStore }, config, fakePrivateProjectChecker, + fakeAccessReadModel, ); }; diff --git a/src/lib/features/events/event-created-by-migration.test.ts b/src/lib/features/events/event-created-by-migration.test.ts index bf33a74df839..113ad7debe60 100644 --- a/src/lib/features/events/event-created-by-migration.test.ts +++ b/src/lib/features/events/event-created-by-migration.test.ts @@ -5,7 +5,7 @@ import { EventEmitter } from 'stream'; import { EVENTS_CREATED_BY_PROCESSED } from '../../metric-events'; import type { IUnleashConfig } from '../../types'; import { createTestConfig } from '../../../test/config/test-config'; -import EventService from './event-service'; +import { createEventsService } from './createEventsService'; let db: ITestDb; @@ -126,14 +126,11 @@ test('emits events with details on amount of updated rows', async () => { const store = new EventStore(db.rawDatabase, getLogger); const eventBus = new EventEmitter(); - const service = new EventService( - { eventStore: store, featureTagStore: db.stores.featureTagStore }, - { getLogger, eventBus }, - {} as any, - ); + const config = createTestConfig(); + const service = createEventsService(db.rawDatabase, config); let triggered = false; - eventBus.on(EVENTS_CREATED_BY_PROCESSED, ({ updated }) => { + config.eventBus.on(EVENTS_CREATED_BY_PROCESSED, ({ updated }) => { expect(updated).toBe(2); triggered = true; }); diff --git a/src/lib/features/events/event-service.ts b/src/lib/features/events/event-service.ts index 919870ed35aa..72b5e09c9147 100644 --- a/src/lib/features/events/event-service.ts +++ b/src/lib/features/events/event-service.ts @@ -16,6 +16,7 @@ import { parseSearchOperatorValue } from '../feature-search/search-utils'; import { endOfDay, formatISO } from 'date-fns'; import type { IPrivateProjectChecker } from '../private-project/privateProjectCheckerType'; import type { ProjectAccess } from '../private-project/privateProjectStore'; +import type { IAccessReadModel } from '../access/access-read-model-type'; export default class EventService { private logger: Logger; @@ -24,6 +25,8 @@ export default class EventService { private featureTagStore: IFeatureTagStore; + private accessReadModel: IAccessReadModel; + private privateProjectChecker: IPrivateProjectChecker; private eventBus: EventEmitter; @@ -35,12 +38,14 @@ export default class EventService { }: Pick, { getLogger, eventBus }: Pick, privateProjectChecker: IPrivateProjectChecker, + accessReadModel: IAccessReadModel, ) { this.logger = getLogger('services/event-service.ts'); this.eventStore = eventStore; this.privateProjectChecker = privateProjectChecker; this.featureTagStore = featureTagStore; this.eventBus = eventBus; + this.accessReadModel = accessReadModel; } async getEvents(): Promise { @@ -77,6 +82,8 @@ export default class EventService { ); const queryParams = this.convertToDbParams(search); + const projectFilter = await this.getProjectFilterForNonAdmins(userId); + queryParams.push(...projectFilter); const totalEvents = await this.eventStore.searchEventsCount( { @@ -222,6 +229,14 @@ export default class EventService { async getEventCreators() { return this.eventStore.getEventCreators(); } + + async getProjectFilterForNonAdmins(userId: number): Promise { + const isRootAdmin = await this.accessReadModel.isRootAdmin(userId); + if (!isRootAdmin) { + return [{ field: 'project', operator: 'IS_NOT', values: [null] }]; + } + return []; + } } export const filterAccessibleProjects = ( diff --git a/src/lib/features/feature-search/search-utils.ts b/src/lib/features/feature-search/search-utils.ts index 026f4ee04da3..a9dbe49d0abc 100644 --- a/src/lib/features/feature-search/search-utils.ts +++ b/src/lib/features/feature-search/search-utils.ts @@ -50,6 +50,7 @@ export const applyGenericQueryParams = ( queryParams: IQueryParam[], ): void => { queryParams.forEach((param) => { + const isSingleParam = param.values.length === 1; switch (param.operator) { case 'IS': case 'IS_ANY_OF': @@ -57,7 +58,11 @@ export const applyGenericQueryParams = ( break; case 'IS_NOT': case 'IS_NONE_OF': - query.whereNotIn(param.field, param.values); + if (isSingleParam) { + query.whereNot(param.field, param.values[0]); + } else { + query.whereNotIn(param.field, param.values); + } break; case 'IS_BEFORE': query.where(param.field, '<', param.values[0]); diff --git a/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts b/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts index 4226de53b8d4..6e9cc96b295a 100644 --- a/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts +++ b/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts @@ -55,7 +55,7 @@ export type IQueryOperator = export interface IQueryParam { field: string; operator: IQueryOperator; - values: string[]; + values: (string | null)[]; } export interface IFeatureStrategiesStore diff --git a/src/lib/features/segment/segment-service.ts b/src/lib/features/segment/segment-service.ts index e1c83ed59b70..6ce561ca5deb 100644 --- a/src/lib/features/segment/segment-service.ts +++ b/src/lib/features/segment/segment-service.ts @@ -154,7 +154,7 @@ export class SegmentService implements ISegmentService { await this.eventService.storeEvent( new SegmentCreatedEvent({ data: segment, - project: segment.project || 'no-project', + project: segment.project, auditUser, }), ); diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts index c7ff960f25f7..ac170a008695 100644 --- a/src/lib/services/access-service.test.ts +++ b/src/lib/services/access-service.test.ts @@ -23,13 +23,22 @@ import { } from '../../lib/types'; import BadDataError from '../../lib/error/bad-data-error'; import { createFakeEventsService } from '../../lib/features/events/createEventsService'; +import { createFakeAccessReadModel } from '../features/access/createAccessReadModel'; function getSetup() { const config = createTestConfig({ getLogger, }); - return createFakeAccessService(config); + const { accessService, eventStore, accessStore } = + createFakeAccessService(config); + + return { + accessService, + eventStore, + accessStore, + accessReadModel: createFakeAccessReadModel(accessStore), + }; } test('should fail when name exists', async () => { @@ -287,28 +296,28 @@ describe('addAccessToProject', () => { }); test('should return true if user has admin role', async () => { - const { accessService, accessStore } = getSetup(); + const { accessReadModel, accessStore } = getSetup(); const userId = 1; accessStore.getRolesForUserId = jest .fn() .mockResolvedValue([{ id: 1, name: 'ADMIN', type: 'custom' }]); - const result = await accessService.isRootAdmin(userId); + const result = await accessReadModel.isRootAdmin(userId); expect(result).toBe(true); expect(accessStore.getRolesForUserId).toHaveBeenCalledWith(userId); }); test('should return false if user does not have admin role', async () => { - const { accessService, accessStore } = getSetup(); + const { accessReadModel, accessStore } = getSetup(); const userId = 2; accessStore.getRolesForUserId = jest .fn() .mockResolvedValue([{ id: 2, name: 'user', type: 'custom' }]); - const result = await accessService.isRootAdmin(userId); + const result = await accessReadModel.isRootAdmin(userId); expect(result).toBe(false); expect(accessStore.getRolesForUserId).toHaveBeenCalledWith(userId); diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 9ab3978eeab7..9842085b7807 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -41,13 +41,11 @@ import BadDataError from '../error/bad-data-error'; import type { IGroup } from '../types/group'; import type { GroupService } from './group-service'; import { - ADMIN_TOKEN_USER, type IUnleashConfig, type IUserAccessOverview, RoleCreatedEvent, RoleDeletedEvent, RoleUpdatedEvent, - SYSTEM_USER_ID, } from '../types'; import type EventService from '../features/events/event-service'; @@ -889,14 +887,4 @@ export class AccessService { async getUserAccessOverview(): Promise { return this.store.getUserAccessOverview(); } - - async isRootAdmin(userId: number): Promise { - if (userId === SYSTEM_USER_ID || userId === ADMIN_TOKEN_USER.id) { - return true; - } - const roles = await this.store.getRolesForUserId(userId); - return roles.some( - (role) => role.name.toLowerCase() === ADMIN.toLowerCase(), - ); - } } diff --git a/src/lib/services/feature-service-potentially-stale.test.ts b/src/lib/services/feature-service-potentially-stale.test.ts index afc30c95a931..c42f64c6ef29 100644 --- a/src/lib/services/feature-service-potentially-stale.test.ts +++ b/src/lib/services/feature-service-potentially-stale.test.ts @@ -44,6 +44,7 @@ test('Should only store events for potentially stale on', async () => { }, config, {}, + {}, ); const featureToggleService = new FeatureToggleService( diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index f23db6c57e6c..26db0b636c26 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -1930,12 +1930,12 @@ export class AddonConfigDeletedEvent extends BaseEvent { } export class SegmentCreatedEvent extends BaseEvent { - readonly project: string; + readonly project: string | undefined; readonly data: any; constructor(eventData: { auditUser: IAuditUser; - project: string; + project: string | undefined; data: any; }) { super(SEGMENT_CREATED, eventData.auditUser); diff --git a/src/test/e2e/api/admin/event-search.e2e.test.ts b/src/test/e2e/api/admin/event-search.e2e.test.ts index c6c2abbf1022..5281290acdfb 100644 --- a/src/test/e2e/api/admin/event-search.e2e.test.ts +++ b/src/test/e2e/api/admin/event-search.e2e.test.ts @@ -1,34 +1,97 @@ import type { EventSearchQueryParameters } from '../../../../lib/openapi/spec/event-search-query-parameters'; import dbInit, { type ITestDb } from '../../helpers/database-init'; -import { FEATURE_CREATED, type IUnleashConfig } from '../../../../lib/types'; -import type { EventService } from '../../../../lib/services'; -import getLogger from '../../../fixtures/no-logger'; import { - type IUnleashTest, - setupAppWithCustomConfig, -} from '../../helpers/test-helper'; + FEATURE_CREATED, + type IUnleashConfig, + type IUnleashStores, + RoleName, + USER_CREATED, +} from '../../../../lib/types'; +import type { AccessService, EventService } from '../../../../lib/services'; +import getLogger from '../../../fixtures/no-logger'; +import { type IUnleashTest, setupAppWithAuth } from '../../helpers/test-helper'; import { createEventsService } from '../../../../lib/features'; import { createTestConfig } from '../../../config/test-config'; +import type { IRole } from '../../../../lib/types/stores/access-store'; let app: IUnleashTest; let db: ITestDb; let eventService: EventService; const TEST_USER_ID = -9999; +const regularUserName = 'import-user'; +const adminUserName = 'admin-user'; const config: IUnleashConfig = createTestConfig(); +let adminRole: IRole; +let stores: IUnleashStores; +let accessService: AccessService; + +const loginRegularUser = () => + app.request + .post(`/auth/demo/login`) + .send({ + email: `${regularUserName}@getunleash.io`, + }) + .expect(200); + +const loginAdminUser = () => + app.request + .post(`/auth/demo/login`) + .send({ + email: `${adminUserName}@getunleash.io`, + }) + .expect(200); + +const createUserEditorAccess = async (name, email) => { + const { userStore } = stores; + const user = await userStore.insert({ + name, + email, + }); + return user; +}; + +const createUserAdminAccess = async (name, email) => { + const { userStore } = stores; + const user = await userStore.insert({ + name, + email, + }); + await accessService.addUserToRole(user.id, adminRole.id, 'default'); + return user; +}; beforeAll(async () => { db = await dbInit('event_search', getLogger); - app = await setupAppWithCustomConfig(db.stores, { - experimental: { - flags: { - strictSchemaValidation: true, + stores = db.stores; + app = await setupAppWithAuth( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + }, }, }, - }); + db.rawDatabase, + ); eventService = createEventsService(db.rawDatabase, config); + + accessService = app.services.accessService; + + const roles = await accessService.getRootRoles(); + adminRole = roles.find((role) => role.name === RoleName.ADMIN)!; + + await createUserEditorAccess( + regularUserName, + `${regularUserName}@getunleash.io`, + ); + await createUserAdminAccess( + adminUserName, + `${adminUserName}@getunleash.io`, + ); }); afterAll(async () => { @@ -37,6 +100,7 @@ afterAll(async () => { }); beforeEach(async () => { + await loginAdminUser(); await db.stores.featureToggleStore.deleteAll(); await db.stores.segmentStore.deleteAll(); await db.stores.eventStore.deleteAll(); @@ -182,6 +246,7 @@ test('should filter events by type', async () => { test('should filter events by created by', async () => { await eventService.storeEvent({ type: FEATURE_CREATED, + project: 'default', createdBy: 'admin1@example.com', createdByUserId: TEST_USER_ID + 1, ip: '127.0.0.1', @@ -189,6 +254,7 @@ test('should filter events by created by', async () => { await eventService.storeEvent({ type: FEATURE_CREATED, + project: 'default', createdBy: 'admin2@example.com', createdByUserId: TEST_USER_ID, ip: '127.0.0.1', @@ -311,8 +377,7 @@ test('should filter events by feature using IS_ANY_OF', async () => { }); await eventService.storeEvent({ - type: FEATURE_CREATED, - project: 'default', + type: USER_CREATED, featureName: 'my_feature_b', createdBy: 'test-user', createdByUserId: TEST_USER_ID, @@ -335,7 +400,7 @@ test('should filter events by feature using IS_ANY_OF', async () => { expect(body).toMatchObject({ events: [ { - type: 'feature-created', + type: 'user-created', featureName: 'my_feature_b', }, { @@ -390,3 +455,63 @@ test('should filter events by project using IS_ANY_OF', async () => { total: 2, }); }); + +test('should not show user creation events for non-admins', async () => { + await loginRegularUser(); + await eventService.storeEvent({ + type: USER_CREATED, + createdBy: 'test-user', + createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', + }); + + await eventService.storeEvent({ + type: FEATURE_CREATED, + project: 'default', + createdBy: 'test-user', + createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', + }); + + const { body } = await searchEvents({}); + + expect(body).toMatchObject({ + events: [ + { + type: FEATURE_CREATED, + }, + ], + total: 1, + }); +}); + +test('should show user creation events for admins', async () => { + await eventService.storeEvent({ + type: USER_CREATED, + createdBy: 'test-user', + createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', + }); + + await eventService.storeEvent({ + type: FEATURE_CREATED, + project: 'default', + createdBy: 'test-user', + createdByUserId: TEST_USER_ID, + ip: '127.0.0.1', + }); + + const { body } = await searchEvents({}); + + expect(body).toMatchObject({ + events: [ + { + type: FEATURE_CREATED, + }, + { + type: USER_CREATED, + }, + ], + total: 2, + }); +});