diff --git a/src/lib/error/operation-denied-error.ts b/src/lib/error/operation-denied-error.ts new file mode 100644 index 0000000000..3e4b3a4443 --- /dev/null +++ b/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, + }, + ], + }; + } +} diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index ac9bf9894c..24ab695f9e 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/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) => diff --git a/src/lib/routes/util.ts b/src/lib/routes/util.ts index 50dc5d12fd..f8b5051328 100644 --- a/src/lib/routes/util.ts +++ b/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(); diff --git a/src/lib/schema/feature-schema.ts b/src/lib/schema/feature-schema.ts index f2501b0492..21fbb5c328 100644 --- a/src/lib/schema/feature-schema.ts +++ b/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 }); diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 2dcd774823..0e31a06960 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/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 { 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 { 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, diff --git a/src/test/e2e/api/admin/feature.e2e.test.ts b/src/test/e2e/api/admin/feature.e2e.test.ts index f1ad2f9392..287fb8d622 100644 --- a/src/test/e2e/api/admin/feature.e2e.test.ts +++ b/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 () => { diff --git a/src/test/e2e/api/admin/project/features.e2e.test.ts b/src/test/e2e/api/admin/project/features.e2e.test.ts index 261f1e3cc4..df87a15ccd 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/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';