From 1a754325decf9821c8dabaefebe15beff68d84ff Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 27 Nov 2023 14:16:06 +0100 Subject: [PATCH] chore: move enterprise check further left, prevent OSS from seeing CR usage (#5431) This PR checks that the unleash instance is an enterprise instance before fetching change request data. This is to prevent Change Request usage from preventing OSS users from deleting segments (when they don't have access to change requests). This PR also does a little bit of refactoring (which we can remove if you want) --- .../features/segment/segment-controller.ts | 47 +++---- src/lib/services/segment-service.ts | 17 ++- src/test/e2e/api/admin/segment.e2e.test.ts | 119 ++++++++++++------ 3 files changed, 109 insertions(+), 74 deletions(-) diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index 8ecf1b5e8045..7c96513395dc 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -354,39 +354,30 @@ export class SegmentsController extends Controller { user.id, ); - if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { - const mapStrategies = (strategy) => ({ - id: strategy.id, - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - }); + const segmentStrategies = strategies.strategies.map((strategy) => ({ + id: strategy.id, + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + })); - const mapChangeRequestStrategies = (strategy) => ({ - ...(strategy.id ? { id: strategy.id } : {}), - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - changeRequest: strategy.changeRequest, - }); + if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { + const changeRequestStrategies = + strategies.changeRequestStrategies.map((strategy) => ({ + ...('id' in strategy ? { id: strategy.id } : {}), + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + changeRequest: strategy.changeRequest, + })); res.json({ - strategies: strategies.strategies.map(mapStrategies), - changeRequestStrategies: strategies.changeRequestStrategies.map( - mapChangeRequestStrategies, - ), + strategies: segmentStrategies, + changeRequestStrategies, }); } else { - const segmentStrategies = strategies.strategies.map((strategy) => ({ - id: strategy.id, - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - })); - res.json({ strategies: segmentStrategies }); } } diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index f42609e97263..7565a29ab239 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -119,12 +119,19 @@ export class SegmentService implements ISegmentService { const strategies = await this.featureStrategiesStore.getStrategiesBySegment(id); - const changeRequestStrategies = - await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests( - id, - ); + if ( + this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests') && + this.config.isEnterprise + ) { + const changeRequestStrategies = + await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests( + id, + ); + + return { strategies, changeRequestStrategies }; + } - return { strategies, changeRequestStrategies }; + return { strategies, changeRequestStrategies: [] }; } async isInUse(id: number): Promise { diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index 438e1efe4646..8dd98d1c2549 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -435,7 +435,43 @@ describe('detect strategy usage in change requests', () => { const CR_ID = 54321; let user; + // Change request data is only counted for enterprise + // instances, so we'll instantiate our own version of the app + // for that. + let enterpriseApp; + + // likewise, we want to fetch from the right app to make sure + // we get the right data + const enterpriseFetchSegments = () => + enterpriseApp.request + .get(SEGMENTS_BASE_PATH) + .expect(200) + .then((res) => res.body.segments); + + const enterpriseFetchSegmentStrategies = ( + segmentId: number, + ): Promise => + enterpriseApp.request + .get(`${SEGMENTS_BASE_PATH}/${segmentId}/strategies`) + .expect(200) + .then((res) => res.body); + beforeAll(async () => { + enterpriseApp = await setupAppWithCustomConfig( + db.stores, + { + enterpriseVersion: '5.3.0', + ui: { environment: 'Enterprise' }, + isEnterprise: true, + experimental: { + flags: { + detectSegmentUsageInChangeRequests: true, + }, + }, + }, + db.rawDatabase, + ); + user = await db.stores.userStore.insert({ username: 'test', }); @@ -463,8 +499,8 @@ describe('detect strategy usage in change requests', () => { test('should not delete segments used by strategies in CRs', async () => { await createSegment({ name: 'a', constraints: [] }); const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); + await createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await db.rawDatabase.table('change_request_events').insert({ feature: toggle.name, @@ -487,20 +523,25 @@ describe('detect strategy usage in change requests', () => { created_by: user.id, }); - expect((await fetchSegments()).length).toEqual(1); + expect((await enterpriseFetchSegments()).length).toEqual(1); - await app.request + await enterpriseApp.request .delete(`${SEGMENTS_BASE_PATH}/${segment.id}`) .expect(409); - expect((await fetchSegments()).length).toEqual(1); + expect((await enterpriseFetchSegments()).length).toEqual(1); + + // check that it can be deleted in OSS + await app.request + .delete(`${SEGMENTS_BASE_PATH}/${segment.id}`) + .expect(204); }); test('Should show segment usage in addStrategy events', async () => { await createSegment({ name: 'a', constraints: [] }); const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); + await createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await db.rawDatabase.table('change_request_events').insert({ feature: toggle.name, @@ -524,7 +565,7 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(changeRequestStrategies).toMatchObject([ { @@ -536,16 +577,21 @@ describe('detect strategy usage in change requests', () => { }, ]); expect(strategies).toStrictEqual([]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toStrictEqual([]); + expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]); }); test('Should show segment usage in updateStrategy events', async () => { await createSegment({ name: 'a', constraints: [] }); const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); + await createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await addStrategyToFeatureEnv( - app, + enterpriseApp, { ...toggle.strategies[0] }, 'default', toggle.name, @@ -578,7 +624,7 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(changeRequestStrategies).toMatchObject([ { @@ -587,16 +633,21 @@ describe('detect strategy usage in change requests', () => { }, ]); expect(strategies).toStrictEqual([]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toStrictEqual([]); + expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]); }); test('If a segment is used in an existing strategy and in a CR for the same strategy, the strategy should be listed both places', async () => { await createSegment({ name: 'a', constraints: [] }); const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); + await createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await addStrategyToFeatureEnv( - app, + enterpriseApp, { ...toggle.strategies[0] }, 'default', toggle.name, @@ -630,39 +681,19 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(strategies).toMatchObject([{ id: strategyId }]); expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toMatchObject([{ id: strategyId }]); + expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]); }); test('Should show usage in features and projects in CRs', async () => { - // Change request data is only counted for enterprise - // instances, so we'll instantiate our own version of the app - // for that. - const enterpriseApp = await setupAppWithCustomConfig( - db.stores, - { - enterpriseVersion: '5.3.0', - ui: { environment: 'Enterprise' }, - experimental: { - flags: { - detectSegmentUsageInChangeRequests: true, - }, - }, - }, - db.rawDatabase, - ); - - // likewise, we want to fetch from the right app to make sure - // we get the right data - const enterpriseFetchSegments = () => - enterpriseApp.request - .get(SEGMENTS_BASE_PATH) - .expect(200) - .then((res) => res.body.segments); - // because they use the same db, we can use the regular app // (through `createSegment` and `createFeatureToggle`) to // create the segment and the flag @@ -698,5 +729,11 @@ describe('detect strategy usage in change requests', () => { expect(segments).toMatchObject([ { usedInFeatures: 1, usedInProjects: 1 }, ]); + + // check that OSS gets no CR usage + const ossSegments = await fetchSegments(); + expect(ossSegments).toMatchObject([ + { usedInFeatures: 0, usedInProjects: 0 }, + ]); }); });