From e20e7df10f52611f62adda9ac05029f9933d058d Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Fri, 4 Aug 2023 12:23:19 +0200 Subject: [PATCH] feat: protect segment operations for change requests (#4417) --- .../change-request-access-read-model.ts | 4 ++ .../fake-change-request-access-read-model.ts | 4 ++ .../sql-change-request-access-read-model.ts | 19 ++++++- .../createFeatureToggleService.ts | 32 ++++------- .../features/segment/createSegmentService.ts | 13 ++++- src/lib/segments/segment-service-interface.ts | 8 +++ src/lib/services/index.ts | 6 +- src/lib/services/segment-service.ts | 57 +++++++++++++++++-- .../e2e/services/access-service.e2e.test.ts | 2 +- .../services/api-token-service.e2e.test.ts | 2 +- .../feature-toggle-service-v2.e2e.test.ts | 7 ++- .../e2e/services/playground-service.test.ts | 6 +- .../project-health-service.e2e.test.ts | 2 +- .../e2e/services/project-service.e2e.test.ts | 2 +- src/test/fixtures/fake-project-store.ts | 2 +- 15 files changed, 129 insertions(+), 37 deletions(-) diff --git a/src/lib/features/change-request-access-service/change-request-access-read-model.ts b/src/lib/features/change-request-access-service/change-request-access-read-model.ts index 8ca9d977e4..0258992d66 100644 --- a/src/lib/features/change-request-access-service/change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/change-request-access-read-model.ts @@ -6,6 +6,10 @@ export interface IChangeRequestAccessReadModel { environment: string, user?: User, ): Promise; + canBypassChangeRequestForProject( + project: string, + user?: User, + ): Promise; isChangeRequestsEnabled( project: string, environment: string, diff --git a/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts index 53773dbf87..bceed13cb3 100644 --- a/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts @@ -16,6 +16,10 @@ export class FakeChangeRequestAccessReadModel return this.canBypass; } + public async canBypassChangeRequestForProject(): Promise { + return this.canBypass; + } + public async isChangeRequestsEnabled(): Promise { return this.isChangeRequestEnabled; } diff --git a/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts index a4cde718e5..b0270987bf 100644 --- a/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts @@ -32,7 +32,24 @@ export class ChangeRequestAccessReadModel : Promise.resolve(false), this.isChangeRequestsEnabled(project, environment), ]); - return !(changeRequestEnabled && !canSkipChangeRequest); + return canSkipChangeRequest || !changeRequestEnabled; + } + + public async canBypassChangeRequestForProject( + project: string, + user?: User, + ): Promise { + const [canSkipChangeRequest, changeRequestEnabled] = await Promise.all([ + user + ? this.accessService.hasPermission( + user, + SKIP_CHANGE_REQUEST, + project, + ) + : Promise.resolve(false), + this.isChangeRequestsEnabledForProject(project), + ]); + return canSkipChangeRequest || !changeRequestEnabled; } public async isChangeRequestsEnabled( diff --git a/src/lib/features/feature-toggle/createFeatureToggleService.ts b/src/lib/features/feature-toggle/createFeatureToggleService.ts index faa220ae77..e7af06cd4c 100644 --- a/src/lib/features/feature-toggle/createFeatureToggleService.ts +++ b/src/lib/features/feature-toggle/createFeatureToggleService.ts @@ -2,7 +2,6 @@ import { AccessService, FeatureToggleService, GroupService, - SegmentService, } from '../../services'; import FeatureStrategiesStore from '../../db/feature-strategy-store'; import FeatureToggleStore from '../../db/feature-toggle-store'; @@ -10,7 +9,6 @@ import FeatureToggleClientStore from '../../db/feature-toggle-client-store'; import ProjectStore from '../../db/project-store'; import FeatureTagStore from '../../db/feature-tag-store'; import { FeatureEnvironmentStore } from '../../db/feature-environment-store'; -import SegmentStore from '../../db/segment-store'; import ContextFieldStore from '../../db/context-field-store'; import GroupStore from '../../db/group-store'; import { AccountStore } from '../../db/account-store'; @@ -26,7 +24,6 @@ import FakeFeatureToggleClientStore from '../../../test/fixtures/fake-feature-to import FakeProjectStore from '../../../test/fixtures/fake-project-store'; import FakeFeatureTagStore from '../../../test/fixtures/fake-feature-tag-store'; import FakeFeatureEnvironmentStore from '../../../test/fixtures/fake-feature-environment-store'; -import FakeSegmentStore from '../../../test/fixtures/fake-segment-store'; import FakeContextFieldStore from '../../../test/fixtures/fake-context-field-store'; import FakeGroupStore from '../../../test/fixtures/fake-group-store'; import { FakeAccountStore } from '../../../test/fixtures/fake-account-store'; @@ -38,6 +35,10 @@ import { createChangeRequestAccessReadModel, createFakeChangeRequestAccessService, } from '../change-request-access-service/createChangeRequestAccessReadModel'; +import { + createFakeSegmentService, + createSegmentService, +} from '../segment/createSegmentService'; export const createFeatureToggleService = ( db: Db, @@ -69,12 +70,6 @@ export const createFeatureToggleService = ( eventBus, getLogger, ); - const segmentStore = new SegmentStore( - db, - eventBus, - getLogger, - flagResolver, - ); const contextFieldStore = new ContextFieldStore( db, getLogger, @@ -95,11 +90,8 @@ export const createFeatureToggleService = ( { getLogger, flagResolver }, groupService, ); - const segmentService = new SegmentService( - { segmentStore, featureStrategiesStore, eventStore }, - config, - ); - const changeRequestAccessReadMode = createChangeRequestAccessReadModel( + const segmentService = createSegmentService(db, config); + const changeRequestAccessReadModel = createChangeRequestAccessReadModel( db, config, ); @@ -117,7 +109,7 @@ export const createFeatureToggleService = ( { getLogger, flagResolver }, segmentService, accessService, - changeRequestAccessReadMode, + changeRequestAccessReadModel, ); return featureToggleService; }; @@ -133,7 +125,6 @@ export const createFakeFeatureToggleService = ( const projectStore = new FakeProjectStore(); const featureTagStore = new FakeFeatureTagStore(); const featureEnvironmentStore = new FakeFeatureEnvironmentStore(); - const segmentStore = new FakeSegmentStore(); const contextFieldStore = new FakeContextFieldStore(); const groupStore = new FakeGroupStore(); const accountStore = new FakeAccountStore(); @@ -149,11 +140,8 @@ export const createFakeFeatureToggleService = ( { getLogger, flagResolver }, groupService, ); - const segmentService = new SegmentService( - { segmentStore, featureStrategiesStore, eventStore }, - config, - ); - const changeRequestAccessReadMode = createFakeChangeRequestAccessService(); + const segmentService = createFakeSegmentService(config); + const changeRequestAccessReadModel = createFakeChangeRequestAccessService(); const featureToggleService = new FeatureToggleService( { featureStrategiesStore, @@ -168,7 +156,7 @@ export const createFakeFeatureToggleService = ( { getLogger, flagResolver }, segmentService, accessService, - changeRequestAccessReadMode, + changeRequestAccessReadModel, ); return featureToggleService; }; diff --git a/src/lib/features/segment/createSegmentService.ts b/src/lib/features/segment/createSegmentService.ts index 4fe3af204d..bd3d441f24 100644 --- a/src/lib/features/segment/createSegmentService.ts +++ b/src/lib/features/segment/createSegmentService.ts @@ -7,11 +7,15 @@ import FeatureStrategiesStore from '../../db/feature-strategy-store'; import SegmentStore from '../../db/segment-store'; import FakeSegmentStore from '../../../test/fixtures/fake-segment-store'; import FakeFeatureStrategiesStore from '../../../test/fixtures/fake-feature-strategies-store'; +import { + createChangeRequestAccessReadModel, + createFakeChangeRequestAccessService, +} from '../change-request-access-service/createChangeRequestAccessReadModel'; export const createSegmentService = ( db: Db, config: IUnleashConfig, -): ISegmentService => { +): SegmentService => { const { eventBus, getLogger, flagResolver } = config; const eventStore = new EventStore(db, getLogger); const segmentStore = new SegmentStore( @@ -26,9 +30,14 @@ export const createSegmentService = ( getLogger, flagResolver, ); + const changeRequestAccessReadModel = createChangeRequestAccessReadModel( + db, + config, + ); return new SegmentService( { segmentStore, featureStrategiesStore, eventStore }, + changeRequestAccessReadModel, config, ); }; @@ -39,9 +48,11 @@ export const createFakeSegmentService = ( const eventStore = new FakeEventStore(); const segmentStore = new FakeSegmentStore(); const featureStrategiesStore = new FakeFeatureStrategiesStore(); + const changeRequestAccessReadModel = createFakeChangeRequestAccessService(); return new SegmentService( { segmentStore, featureStrategiesStore, eventStore }, + changeRequestAccessReadModel, config, ); }; diff --git a/src/lib/segments/segment-service-interface.ts b/src/lib/segments/segment-service-interface.ts index 2a547e66b0..40a7c2a226 100644 --- a/src/lib/segments/segment-service-interface.ts +++ b/src/lib/segments/segment-service-interface.ts @@ -34,8 +34,16 @@ export interface ISegmentService { user: Partial>, ): Promise; + unprotectedUpdate( + id: number, + data: UpsertSegmentSchema, + user: Partial>, + ): Promise; + delete(id: number, user: IUser): Promise; + unprotectedDelete(id: number, user: IUser): Promise; + removeFromStrategy(id: number, strategyId: string): Promise; cloneStrategySegments( diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 20af6cb6d5..161e8339a7 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -176,10 +176,14 @@ export const createServices = ( const versionService = new VersionService(stores, config); const healthService = new HealthService(stores, config); const userFeedbackService = new UserFeedbackService(stores, config); - const segmentService = new SegmentService(stores, config); const changeRequestAccessReadModel = db ? createChangeRequestAccessReadModel(db, config) : createFakeChangeRequestAccessService(); + const segmentService = new SegmentService( + stores, + changeRequestAccessReadModel, + config, + ); const featureToggleServiceV2 = new FeatureToggleService( stores, config, diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index 72a1d87ad6..cbfca28669 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -1,6 +1,11 @@ import { IUnleashConfig } from '../types/option'; import { IEventStore } from '../types/stores/event-store'; -import { IClientSegment, IUnleashStores } from '../types'; +import { + IClientSegment, + IFlagResolver, + IUnleashStores, + SKIP_CHANGE_REQUEST, +} from '../types'; import { Logger } from '../logger'; import NameExistsError from '../error/name-exists-error'; import { ISegmentStore } from '../types/stores/segment-store'; @@ -15,6 +20,8 @@ import User from '../types/user'; import { IFeatureStrategiesStore } from '../types/stores/feature-strategies-store'; import BadDataError from '../error/bad-data-error'; import { ISegmentService } from '../segments/segment-service-interface'; +import { PermissionError } from '../error'; +import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model'; export class SegmentService implements ISegmentService { private logger: Logger; @@ -25,8 +32,12 @@ export class SegmentService implements ISegmentService { private eventStore: IEventStore; + private changeRequestAccessReadModel: IChangeRequestAccessReadModel; + private config: IUnleashConfig; + private flagResolver: IFlagResolver; + constructor( { segmentStore, @@ -36,12 +47,15 @@ export class SegmentService implements ISegmentService { IUnleashStores, 'segmentStore' | 'featureStrategiesStore' | 'eventStore' >, + changeRequestAccessReadModel: IChangeRequestAccessReadModel, config: IUnleashConfig, ) { this.segmentStore = segmentStore; this.featureStrategiesStore = featureStrategiesStore; this.eventStore = eventStore; + this.changeRequestAccessReadModel = changeRequestAccessReadModel; this.logger = config.getLogger('services/segment-service.ts'); + this.flagResolver = config.flagResolver; this.config = config; } @@ -82,17 +96,25 @@ export class SegmentService implements ISegmentService { await this.eventStore.store({ type: SEGMENT_CREATED, - createdBy: user.email || user.username, + createdBy: user.email || user.username || 'unknown', data: segment, }); return segment; } - async update( + async update(id: number, data: unknown, user: User): Promise { + if (this.flagResolver.isEnabled('segmentChangeRequests')) { + const input = await segmentSchema.validateAsync(data); + await this.stopWhenChangeRequestsEnabled(input.project, user); + } + return this.unprotectedUpdate(id, data, user); + } + + async unprotectedUpdate( id: number, data: unknown, - user: Partial>, + user: User, ): Promise { const input = await segmentSchema.validateAsync(data); this.validateSegmentValuesLimit(input); @@ -108,13 +130,26 @@ export class SegmentService implements ISegmentService { await this.eventStore.store({ type: SEGMENT_UPDATED, - createdBy: user.email || user.username, + createdBy: user.email || user.username || 'unknown', data: segment, preData, }); } async delete(id: number, user: User): Promise { + const segment = await this.segmentStore.get(id); + if (this.flagResolver.isEnabled('segmentChangeRequests')) { + await this.stopWhenChangeRequestsEnabled(segment.project, user); + } + await this.segmentStore.delete(id); + await this.eventStore.store({ + type: SEGMENT_DELETED, + createdBy: user.email || user.username, + data: segment, + }); + } + + async unprotectedDelete(id: number, user: User): Promise { const segment = await this.segmentStore.get(id); await this.segmentStore.delete(id); await this.eventStore.store({ @@ -248,4 +283,16 @@ export class SegmentService implements ISegmentService { ); } } + + private async stopWhenChangeRequestsEnabled(project?: string, user?: User) { + if (!project) return; + const canBypass = + await this.changeRequestAccessReadModel.canBypassChangeRequestForProject( + project, + user, + ); + if (!canBypass) { + throw new PermissionError(SKIP_CHANGE_REQUEST); + } + } } diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index bf313aa45c..ceb7f59422 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -231,7 +231,7 @@ beforeAll(async () => { featureToggleService = new FeatureToggleService( stores, config, - new SegmentService(stores, config), + new SegmentService(stores, changeRequestAccessReadModel, config), accessService, changeRequestAccessReadModel, ); diff --git a/src/test/e2e/services/api-token-service.e2e.test.ts b/src/test/e2e/services/api-token-service.e2e.test.ts index f6ce28d92b..8bef71f323 100644 --- a/src/test/e2e/services/api-token-service.e2e.test.ts +++ b/src/test/e2e/services/api-token-service.e2e.test.ts @@ -34,7 +34,7 @@ beforeAll(async () => { const featureToggleService = new FeatureToggleService( stores, config, - new SegmentService(stores, config), + new SegmentService(stores, changeRequestAccessReadModel, config), accessService, changeRequestAccessReadModel, ); diff --git a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts index 5c0dfcbe04..0e3da6e685 100644 --- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts +++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts @@ -40,13 +40,18 @@ beforeAll(async () => { ); unleashConfig = config; stores = db.stores; - segmentService = new SegmentService(stores, config); + const groupService = new GroupService(stores, config); const accessService = new AccessService(stores, config, groupService); const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( db.rawDatabase, accessService, ); + segmentService = new SegmentService( + stores, + changeRequestAccessReadModel, + config, + ); service = new FeatureToggleService( stores, config, diff --git a/src/test/e2e/services/playground-service.test.ts b/src/test/e2e/services/playground-service.test.ts index 091df88c0b..2c8c7edd68 100644 --- a/src/test/e2e/services/playground-service.test.ts +++ b/src/test/e2e/services/playground-service.test.ts @@ -36,13 +36,17 @@ beforeAll(async () => { const config = createTestConfig(); db = await dbInit('playground_service_serial', config.getLogger); stores = db.stores; - segmentService = new SegmentService(stores, config); const groupService = new GroupService(stores, config); const accessService = new AccessService(stores, config, groupService); const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( db.rawDatabase, accessService, ); + segmentService = new SegmentService( + stores, + changeRequestAccessReadModel, + config, + ); featureToggleService = new FeatureToggleService( stores, config, diff --git a/src/test/e2e/services/project-health-service.e2e.test.ts b/src/test/e2e/services/project-health-service.e2e.test.ts index 94386413f8..6fca2c1ca6 100644 --- a/src/test/e2e/services/project-health-service.e2e.test.ts +++ b/src/test/e2e/services/project-health-service.e2e.test.ts @@ -39,7 +39,7 @@ beforeAll(async () => { featureToggleService = new FeatureToggleService( stores, config, - new SegmentService(stores, config), + new SegmentService(stores, changeRequestAccessReadModel, config), accessService, changeRequestAccessReadModel, ); diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index b4af1b9a7b..bd6f68321f 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -58,7 +58,7 @@ beforeAll(async () => { featureToggleService = new FeatureToggleService( stores, config, - new SegmentService(stores, config), + new SegmentService(stores, changeRequestAccessReadModel, config), accessService, changeRequestAccessReadModel, ); diff --git a/src/test/fixtures/fake-project-store.ts b/src/test/fixtures/fake-project-store.ts index bd3ae7bea4..0645cda6a9 100644 --- a/src/test/fixtures/fake-project-store.ts +++ b/src/test/fixtures/fake-project-store.ts @@ -177,7 +177,7 @@ export default class FakeProjectStore implements IProjectStore { projectId: string, // eslint-disable-next-line @typescript-eslint/no-unused-vars environment: string, - ): Promise { + ): Promise { throw new Error('Method not implemented.'); }