fix: do not allow creation/update of feature toggle with invalid strategy name (#4555)

src/lib/features/feature-toggle/createFeatureToggleService.ts

@@ -39,6 +39,8 @@ import {
     createFakeSegmentService,
     createSegmentService,
 } from '../segment/createSegmentService';
+import StrategyStore from '../../db/strategy-store';
+import FakeStrategiesStore from '../../../test/fixtures/fake-strategies-store';
 
 export const createFeatureToggleService = (
     db: Db,
@@ -76,6 +78,7 @@ export const createFeatureToggleService = (
         flagResolver,
     );
     const groupStore = new GroupStore(db);
+    const strategyStore = new StrategyStore(db, getLogger);
     const accountStore = new AccountStore(db, getLogger);
     const accessStore = new AccessStore(db, eventBus, getLogger);
     const roleStore = new RoleStore(db, eventBus, getLogger);
@@ -105,6 +108,7 @@ export const createFeatureToggleService = (
             featureTagStore,
             featureEnvironmentStore,
             contextFieldStore,
+            strategyStore,
         },
         { getLogger, flagResolver },
         segmentService,
@@ -119,6 +123,7 @@ export const createFakeFeatureToggleService = (
 ): FeatureToggleService => {
     const { getLogger, flagResolver } = config;
     const eventStore = new FakeEventStore();
+    const strategyStore = new FakeStrategiesStore();
     const featureStrategiesStore = new FakeFeatureStrategiesStore();
     const featureToggleStore = new FakeFeatureToggleStore();
     const featureToggleClientStore = new FakeFeatureToggleClientStore();
@@ -152,6 +157,7 @@ export const createFakeFeatureToggleService = (
             featureTagStore,
             featureEnvironmentStore,
             contextFieldStore,
+            strategyStore,
         },
         { getLogger, flagResolver },
         segmentService,

src/lib/services/feature-toggle-service.ts

@@ -42,6 +42,7 @@ import {
     WeightType,
     StrategiesOrderChangedEvent,
     PotentiallyStaleOnEvent,
+    IStrategyStore,
 } from '../types';
 import { Logger } from '../logger';
 import BadDataError from '../error/bad-data-error';
@@ -118,6 +119,8 @@ class FeatureToggleService {
 
     private featureStrategiesStore: IFeatureStrategiesStore;
 
+    private strategyStore: IStrategyStore;
+
     private featureToggleStore: IFeatureToggleStore;
 
     private featureToggleClientStore: IFeatureToggleClientStore;
@@ -150,6 +153,7 @@ class FeatureToggleService {
             featureTagStore,
             featureEnvironmentStore,
             contextFieldStore,
+            strategyStore,
         }: Pick<
             IUnleashStores,
             | 'featureStrategiesStore'
@@ -160,6 +164,7 @@ class FeatureToggleService {
             | 'featureTagStore'
             | 'featureEnvironmentStore'
             | 'contextFieldStore'
+            | 'strategyStore'
         >,
         {
             getLogger,
@@ -171,6 +176,7 @@ class FeatureToggleService {
     ) {
         this.logger = getLogger('services/feature-toggle-service.ts');
         this.featureStrategiesStore = featureStrategiesStore;
+        this.strategyStore = strategyStore;
         this.featureToggleStore = featureToggleStore;
         this.featureToggleClientStore = featureToggleClientStore;
         this.tagStore = featureTagStore;
@@ -225,24 +231,24 @@ class FeatureToggleService {
 
     validateUpdatedProperties(
         { featureName, projectId }: IFeatureContext,
-        strategy: IFeatureStrategy,
+        existingStrategy: IFeatureStrategy,
     ): void {
-        if (strategy.projectId !== projectId) {
+        if (existingStrategy.projectId !== projectId) {
             throw new InvalidOperationError(
                 'You can not change the projectId for an activation strategy.',
             );
         }
 
-        if (strategy.featureName !== featureName) {
+        if (existingStrategy.featureName !== featureName) {
             throw new InvalidOperationError(
                 'You can not change the featureName for an activation strategy.',
             );
         }
 
         if (
-            strategy.parameters &&
+            existingStrategy.parameters &&
-            'stickiness' in strategy.parameters &&
+            'stickiness' in existingStrategy.parameters &&
-            strategy.parameters.stickiness === ''
+            existingStrategy.parameters.stickiness === ''
         ) {
             throw new InvalidOperationError(
                 'You can not have an empty string for stickiness.',
@@ -271,6 +277,19 @@ class FeatureToggleService {
         }
     }
 
+    async validateStrategyType(
+        strategyName: string | undefined,
+    ): Promise<void> {
+        if (strategyName !== undefined) {
+            const exists = await this.strategyStore.exists(strategyName);
+            if (!exists) {
+                throw new BadDataError(
+                    `Could not find strategy type with name ${strategyName}`,
+                );
+            }
+        }
+    }
+
     async validateConstraints(
         constraints: IConstraint[],
     ): Promise<IConstraint[]> {
@@ -502,6 +521,7 @@ class FeatureToggleService {
         const { featureName, projectId, environment } = context;
         await this.validateFeatureBelongsToProject(context);
 
+        await this.validateStrategyType(strategyConfig.name);
         await this.validateProjectCanAccessSegments(
             projectId,
             strategyConfig.segments,
@@ -602,7 +622,7 @@ class FeatureToggleService {
      */
     async updateStrategy(
         id: string,
-        updates: Partial<IFeatureStrategy>,
+        updates: Partial<IStrategyConfig>,
         context: IFeatureStrategyContext,
         userName: string,
         user?: User,
@@ -640,13 +660,15 @@ class FeatureToggleService {
 
     async unprotectedUpdateStrategy(
         id: string,
-        updates: Partial<IFeatureStrategy>,
+        updates: Partial<IStrategyConfig>,
         context: IFeatureStrategyContext,
         userName: string,
     ): Promise<Saved<IStrategyConfig>> {
         const { projectId, environment, featureName } = context;
         const existingStrategy = await this.featureStrategiesStore.get(id);
+
         this.validateUpdatedProperties(context, existingStrategy);
+        await this.validateStrategyType(updates.name);
         await this.validateProjectCanAccessSegments(
             projectId,
             updates.segments,

src/test/e2e/api/admin/project/features.e2e.test.ts

@@ -57,26 +57,28 @@ const createSegment = async (segmentName: string) => {
 const createStrategy = async (
     featureName: string,
     payload: IStrategyConfig,
+    expectedCode = 200,
 ) => {
     return app.request
         .post(
             `/api/admin/projects/default/features/${featureName}/environments/default/strategies`,
         )
         .send(payload)
-        .expect(200);
+        .expect(expectedCode);
 };
 
 const updateStrategy = async (
     featureName: string,
     strategyId: string,
     payload: IStrategyConfig,
+    expectedCode = 200,
 ) => {
     const { body } = await app.request
         .put(
             `/api/admin/projects/default/features/${featureName}/environments/default/strategies/${strategyId}`,
         )
         .send(payload)
-        .expect(200);
+        .expect(expectedCode);
 
     return body;
 };
@@ -110,8 +112,6 @@ afterEach(async () => {
                 ),
             ),
     );
-
-    await db.stores.strategyStore.deleteAll();
 });
 
 afterAll(async () => {
@@ -2355,7 +2355,7 @@ test('should handle strategy variants', async () => {
     await app.createFeature(feature.name);
 
     const strategyWithInvalidVariant = {
-        name: uuidv4(),
+        name: 'flexibleRollout',
         constraints: [],
         variants: [
             {
@@ -2386,7 +2386,7 @@ test('should handle strategy variants', async () => {
         stickiness: 'default',
     };
     const strategyWithValidVariant = {
-        name: uuidv4(),
+        name: 'flexibleRollout',
         constraints: [],
         variants: [variant],
     };
@@ -2438,7 +2438,7 @@ test('should reject invalid constraint values for multi-valued constraints', asy
     });
 
     const mockStrategy = (values: string[]) => ({
-        name: uuidv4(),
+        name: 'flexibleRollout',
         constraints: [{ contextName: 'userId', operator: 'IN', values }],
     });
 
@@ -2497,7 +2497,7 @@ test('should add default constraint values for single-valued constraints', async
     };
 
     const mockStrategy = (constraint: unknown) => ({
-        name: uuidv4(),
+        name: 'flexibleRollout',
         constraints: [constraint],
     });
 
@@ -2544,7 +2544,7 @@ test('should allow long parameter values', async () => {
     });
 
     const strategy = {
-        name: uuidv4(),
+        name: 'flexibleRollout',
         parameters: { a: 'b'.repeat(500) },
     };
 
@@ -2582,7 +2582,7 @@ test('should change strategy sort order when payload is valid', async () => {
             `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`,
         )
         .send({
-            name: 'gradualrollout',
+            name: 'flexibleRollout',
             parameters: {
                 userId: 'string',
             },
@@ -2668,7 +2668,7 @@ test('should return strategies in correct order when new strategies are added',
             `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`,
         )
         .send({
-            name: 'gradualrollout',
+            name: 'flexibleRollout',
             parameters: {
                 userId: 'string',
             },
@@ -2705,7 +2705,7 @@ test('should return strategies in correct order when new strategies are added',
             `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`,
         )
         .send({
-            name: 'gradualrollout',
+            name: 'flexibleRollout',
             parameters: {
                 userId: 'string',
             },
@@ -2717,7 +2717,7 @@ test('should return strategies in correct order when new strategies are added',
             `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`,
         )
         .send({
-            name: 'gradualrollout',
+            name: 'flexibleRollout',
             parameters: {
                 userId: 'string',
             },
@@ -2993,7 +2993,7 @@ test('should return disabled strategies', async () => {
             `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`,
         )
         .send({
-            name: 'gradualrollout',
+            name: 'flexibleRollout',
             parameters: {
                 userId: 'string',
             },
@@ -3355,3 +3355,42 @@ test('Updating feature strategy sort-order should trigger a an event', async ()
             expect(res.body.events[0].type).toBe('strategy-order-changed');
         });
 });
+
+test('should not be allowed to create with invalid strategy type name', async () => {
+    const feature = { name: uuidv4(), impressionData: false };
+    await app.createFeature(feature.name);
+    await createStrategy(
+        feature.name,
+        {
+            name: 'random-type',
+            parameters: {
+                userId: 'string',
+            },
+        },
+        400,
+    );
+});
+
+test('should not be allowed to update with invalid strategy type name', async () => {
+    const feature = { name: uuidv4(), impressionData: false };
+    await app.createFeature(feature.name);
+    const { body: strategyOne } = await createStrategy(feature.name, {
+        name: 'default',
+        parameters: {
+            userId: 'string',
+        },
+    });
+
+    await updateStrategy(
+        feature.name,
+        strategyOne.id,
+        {
+            name: 'random-type',
+            parameters: {
+                userId: 'string',
+            },
+            segments: [],
+        },
+        400,
+    );
+});

src/test/e2e/api/admin/state.e2e.test.ts

@@ -382,21 +382,6 @@ test(`should not delete api_tokens on import when drop-flag is set`, async () =>
         },
         userName,
     );
-    await app.services.featureToggleServiceV2.createStrategy(
-        {
-            name: 'default',
-            constraints: [
-                { contextName: 'userId', operator: 'IN', values: ['123'] },
-            ],
-            parameters: {},
-        },
-        {
-            projectId,
-            featureName,
-            environment,
-        },
-        userName,
-    );
     await app.services.apiTokenService.createApiTokenWithProjects({
         tokenName: apiTokenName,
         type: ApiTokenType.CLIENT,

src/test/e2e/api/admin/strategy.e2e.test.ts

@@ -29,7 +29,7 @@ test('gets all strategies', async () => {
         .expect('Content-Type', /json/)
         .expect(200)
         .expect((res) => {
-            expect(res.body.strategies).toHaveLength(2);
+            expect(res.body.strategies).toHaveLength(3);
         });
 });
 

src/test/e2e/api/client/feature.e2e.test.ts

@@ -217,7 +217,7 @@ test('Can get strategies for specific environment', async () => {
             `/api/admin/projects/default/features/${featureName}/environments/testing/strategies`,
         )
         .send({
-            name: 'custom1',
+            name: 'default',
         })
         .expect(200);
 
@@ -229,7 +229,7 @@ test('Can get strategies for specific environment', async () => {
             expect(res.body.name).toBe(featureName);
             expect(res.body.strategies).toHaveLength(1);
             expect(
-                res.body.strategies.find((s) => s.name === 'custom1'),
+                res.body.strategies.find((s) => s.name === 'default'),
             ).toBeDefined();
         });
 });

src/test/e2e/api/client/feature.token.access.e2e.test.ts

@@ -61,7 +61,7 @@ beforeAll(async () => {
     );
     await featureToggleServiceV2.createStrategy(
         {
-            name: 'custom-testing',
+            name: 'flexibleRollout',
             constraints: [],
             parameters: {},
         },
@@ -152,7 +152,7 @@ test('returns feature toggle with testing environment config', async () => {
 
             expect(features).toHaveLength(2);
             expect(f1.strategies).toHaveLength(1);
-            expect(f1.strategies[0].name).toBe('custom-testing');
+            expect(f1.strategies[0].name).toBe('flexibleRollout');
             expect(f2.strategies).toHaveLength(1);
             expect(query.project[0]).toBe(project);
             expect(query.environment).toBe(environment);

src/test/e2e/api/client/segment.e2e.test.ts

@@ -66,7 +66,7 @@ const updateSegment = (
 
 const mockStrategy = (segments: number[] = []) => {
     return {
-        name: randomId(),
+        name: 'flexibleRollout',
         parameters: {},
         constraints: [],
         segments,

src/test/e2e/api/proxy/proxy.e2e.test.ts

@@ -463,18 +463,10 @@ test('should filter features by strategies', async () => {
         enabled: false,
         strategies: [],
     });
-    await createFeatureToggle({
-        name: 'featureWithUnknownStrategy',
-        enabled: true,
-        strategies: [{ name: 'unknown', constraints: [], parameters: {} }],
-    });
     await createFeatureToggle({
         name: 'featureWithMultipleStrategies',
         enabled: true,
-        strategies: [
+        strategies: [{ name: 'default', constraints: [], parameters: {} }],
-            { name: 'default', constraints: [], parameters: {} },
-            { name: 'unknown', constraints: [], parameters: {} },
-        ],
     });
     await app.request
         .get('/api/frontend')

src/test/e2e/helpers/database.json

@@ -14,6 +14,30 @@
           "type": "string"
         }
       ]
+    },
+    {
+      "name": "flexibleRollout",
+      "description": "Roll out to a percentage of your userbase, and ensure that the experience is the same for the user on each visit.",
+      "parameters": [
+        {
+          "name": "rollout",
+          "type": "percentage",
+          "description": "",
+          "required": false
+        },
+        {
+          "name": "stickiness",
+          "type": "string",
+          "description": "Used define stickiness. Possible values: default, userId, sessionId, random",
+          "required": true
+        },
+        {
+          "name": "groupId",
+          "type": "string",
+          "description": "Used to define a activation groups, which allows you to correlate across feature toggles.",
+          "required": true
+        }
+      ]
     }
   ],
   "contextFields": [