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)

src/lib/features/segment/segment-controller.ts

@@ -354,31 +354,6 @@ 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 mapChangeRequestStrategies = (strategy) => ({
-                ...(strategy.id ? { 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,
-                ),
-            });
-        } else {
         const segmentStrategies = strategies.strategies.map((strategy) => ({
             id: strategy.id,
             projectId: strategy.projectId,
@@ -387,6 +362,22 @@ export class SegmentsController extends Controller {
             environment: strategy.environment,
         }));
 
+        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: segmentStrategies,
+                changeRequestStrategies,
+            });
+        } else {
             res.json({ strategies: segmentStrategies });
         }
     }

src/lib/services/segment-service.ts

@@ -119,6 +119,10 @@ export class SegmentService implements ISegmentService {
         const strategies =
             await this.featureStrategiesStore.getStrategiesBySegment(id);
 
+        if (
+            this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests') &&
+            this.config.isEnterprise
+        ) {
             const changeRequestStrategies =
                 await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests(
                     id,
@@ -127,6 +131,9 @@ export class SegmentService implements ISegmentService {
             return { strategies, changeRequestStrategies };
         }
 
+        return { strategies, changeRequestStrategies: [] };
+    }
+
     async isInUse(id: number): Promise<boolean> {
         const { strategies, changeRequestStrategies } =
             await this.getAllStrategies(id);

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<StrategiesUsingSegment> =>
+        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);
+        await createFeatureToggle(enterpriseApp, toggle);
-        const [segment] = await fetchSegments();
+        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);
+        await createFeatureToggle(enterpriseApp, toggle);
-        const [segment] = await fetchSegments();
+        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);
+        await createFeatureToggle(enterpriseApp, toggle);
-        const [segment] = await fetchSegments();
+        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);
+        await createFeatureToggle(enterpriseApp, toggle);
-        const [segment] = await fetchSegments();
+        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 },
+        ]);
     });
 });