task: Ban changes to variants through feature (#1130)

* task: Ban changes to variants through feature

After adding the new `/variants` endpoint for features we now have a way
to access control adding/modifying variants, so the /:featureName
endpoint should no longer allow editing/adding variants.

This removes variants as a known field from the featureMetadata schema
and tells joi to stripUnknown, thus making sure we never include
variants in the initial creation or future update calls.

For the old features v1 API we allow it to declare that it has already
validated the data coming with its own schema, so we should use the data
we get from it. Thus keeping the old v1 functionality intact

Co-authored-by: Simon Hornby <simon@getunleash.ai>

src/lib/error/operation-denied-error.ts

@@ -0,0 +1,21 @@
+export class OperationDeniedError extends Error {
+    constructor(message: string) {
+        super();
+        Error.captureStackTrace(this, this.constructor);
+
+        this.name = this.constructor.name;
+        this.message = message;
+    }
+
+    toJSON(): object {
+        return {
+            isJoi: true,
+            name: this.constructor.name,
+            details: [
+                {
+                    message: this.message,
+                },
+            ],
+        };
+    }
+}

src/lib/routes/admin-api/feature.ts

@@ -145,6 +145,7 @@ class FeatureController extends Controller {
             validatedToggle.project,
             validatedToggle,
             userName,
+            true,
         );
         const strategies = await Promise.all(
             toggle.strategies.map(async (s) =>

src/lib/routes/util.ts

@@ -30,32 +30,34 @@ export const handleErrors: (
     // eslint-disable-next-line no-param-reassign
     error.isJoi = true;
     switch (error.name) {
-        case 'NoAccessError':
-            return res.status(403).json(error).end();
-        case 'NotFoundError':
-            return res.status(404).json(error).end();
-        case 'InvalidOperationError':
-            return res.status(403).json(error).end();
-        case 'NameExistsError':
-            return res.status(409).json(error).end();
         case 'ValidationError':
             return res.status(400).json(error).end();
         case 'BadDataError':
             return res.status(400).json(error).end();
-        case 'FeatureHasTagError':
-            return res.status(409).json(error).end();
-        case 'UsedTokenError':
-            return res.status(403).json(error).end();
-        case 'InvalidTokenError':
-            return res.status(401).json(error).end();
         case 'OwaspValidationError':
             return res.status(400).json(error).end();
         case 'PasswordUndefinedError':
             return res.status(400).json(error).end();
-        case 'IncompatibleProjectError':
-            return res.status(403).json(error).end();
         case 'MinimumOneEnvironmentError':
             return res.status(400).json(error).end();
+        case 'InvalidTokenError':
+            return res.status(401).json(error).end();
+        case 'NoAccessError':
+            return res.status(403).json(error).end();
+        case 'UsedTokenError':
+            return res.status(403).json(error).end();
+        case 'InvalidOperationError':
+            return res.status(403).json(error).end();
+        case 'IncompatibleProjectError':
+            return res.status(403).json(error).end();
+        case 'OperationDeniedError':
+            return res.status(403).json(error).end();
+        case 'NotFoundError':
+            return res.status(404).json(error).end();
+        case 'NameExistsError':
+            return res.status(409).json(error).end();
+        case 'FeatureHasTagError':
+            return res.status(409).json(error).end();
         default:
             logger.error('Server failed executing request', error);
             return res.status(500).end();

src/lib/schema/feature-schema.ts

@@ -56,12 +56,6 @@ export const featureMetadataSchema = joi
         archived: joi.boolean().default(false),
         type: joi.string().default('release'),
         description: joi.string().allow('').allow(null).optional(),
-        variants: joi
-            .array()
-            .allow(null)
-            .unique((a, b) => a.name === b.name)
-            .optional()
-            .items(variantsSchema),
         createdAt: joi.date().optional().allow(null),
     })
     .options({ allowUnknown: false, stripUnknown: true, abortEarly: false });

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

@@ -51,6 +51,7 @@ import { IFeatureEnvironmentStore } from '../types/stores/feature-environment-st
 import { IFeatureToggleClientStore } from '../types/stores/feature-toggle-client-store';
 import { DEFAULT_ENV } from '../util/constants';
 import { applyPatch, deepClone, Operation } from 'fast-json-patch';
+import { OperationDeniedError } from '../error/operation-denied-error';
 
 interface IFeatureContext {
     featureName: string;
@@ -146,6 +147,11 @@ class FeatureToggleService {
     ): Promise<FeatureToggle> {
         const featureToggle = await this.getFeatureMetadata(featureName);
 
+        if (operations.some((op) => op.path.indexOf('/variants') >= 0)) {
+            throw new OperationDeniedError(
+                `Changing variants is done via PATCH operation to /api/admin/projects/:project/features/:feature/variants`,
+            );
+        }
         const { newDocument } = applyPatch(
             deepClone(featureToggle),
             operations,
@@ -455,14 +461,18 @@ class FeatureToggleService {
         projectId: string,
         value: FeatureToggleDTO,
         createdBy: string,
+        isValidated: boolean = false,
     ): Promise<FeatureToggle> {
         this.logger.info(`${createdBy} creates feature toggle ${value.name}`);
         await this.validateName(value.name);
         const exists = await this.projectStore.hasProject(projectId);
         if (exists) {
-            const featureData = await featureMetadataSchema.validateAsync(
-                value,
-            );
+            let featureData;
+            if (isValidated) {
+                featureData = value;
+            } else {
+                featureData = await featureMetadataSchema.validateAsync(value);
+            }
             const featureName = featureData.name;
             const createdToggle = await this.featureToggleStore.create(
                 projectId,

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

@@ -1,5 +1,5 @@
 import faker from 'faker';
-import { FeatureToggleDTO, IStrategyConfig } from 'lib/types/model';
+import { FeatureToggleDTO, IStrategyConfig, IVariant } from 'lib/types/model';
 import dbInit, { ITestDb } from '../../helpers/database-init';
 import { IUnleashTest, setupApp } from '../../helpers/test-helper';
 import getLogger from '../../../fixtures/no-logger';
@@ -35,6 +35,15 @@ beforeAll(async () => {
             username,
         );
     };
+    const createVariants = async (
+        featureName: string,
+        variants: IVariant[],
+    ) => {
+        await app.services.featureToggleServiceV2.saveVariants(
+            featureName,
+            variants,
+        );
+    };
 
     await createToggle({
         name: 'featureX',
@@ -127,23 +136,23 @@ beforeAll(async () => {
     await createToggle({
         name: 'feature.with.variants',
         description: 'A feature toggle with variants',
-        variants: [
-            {
-                name: 'control',
-                weight: 50,
-                weightType: 'variable',
-                overrides: [],
-                stickiness: 'default',
-            },
-            {
-                name: 'new',
-                weight: 50,
-                weightType: 'variable',
-                overrides: [],
-                stickiness: 'default',
-            },
-        ],
     });
+    await createVariants('feature.with.variants', [
+        {
+            name: 'control',
+            weight: 50,
+            weightType: 'variable',
+            overrides: [],
+            stickiness: 'default',
+        },
+        {
+            name: 'new',
+            weight: 50,
+            weightType: 'variable',
+            overrides: [],
+            stickiness: 'default',
+        },
+    ]);
 });
 
 afterAll(async () => {
@@ -191,7 +200,7 @@ test('creates new feature toggle', async () => {
 });
 
 test('creates new feature toggle with variants', async () => {
-    expect.assertions(0);
+    expect.assertions(1);
     return app.request
         .post('/api/admin/features')
         .send({
@@ -204,7 +213,10 @@ test('creates new feature toggle with variants', async () => {
             ],
         })
         .set('Content-Type', 'application/json')
-        .expect(201);
+        .expect(201)
+        .expect((res) => {
+            expect(res.body.variants).toHaveLength(2);
+        });
 });
 
 test('fetch feature toggle with variants', async () => {

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

@@ -560,6 +560,34 @@ test('Patching feature toggles to stale should trigger FEATURE_STALE_ON event',
     expect(updateForOurToggle).toBeTruthy();
 });
 
+test('Trying to patch variants on a feature toggle should trigger an OperationDeniedError', async () => {
+    const url = '/api/admin/projects/default/features';
+    const name = 'toggle.variants.on.patch';
+    await app.request
+        .post(url)
+        .send({ name, description: 'some', type: 'release', stale: false });
+    await app.request
+        .patch(`${url}/${name}`)
+        .send([
+            {
+                op: 'add',
+                path: '/variants/1',
+                value: {
+                    name: 'variant',
+                    weightType: 'variable',
+                    weight: 500,
+                    stickiness: 'default',
+                },
+            },
+        ])
+        .expect(403)
+        .expect((res) => {
+            expect(res.body.details[0].message).toEqual(
+                'Changing variants is done via PATCH operation to /api/admin/projects/:project/features/:feature/variants',
+            );
+        });
+});
+
 test('Patching feature toggles to active (turning stale to false) should trigger FEATURE_STALE_OFF event', async () => {
     const url = '/api/admin/projects/default/features';
     const name = 'toggle.stale.off.patch';