diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index 7340a09297..1ccc4ed066 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/src/lib/routes/admin-api/feature.ts @@ -196,7 +196,12 @@ class FeatureController extends Controller { const projectId = await this.service.getProjectId(featureName); const value = await featureSchema.validateAsync(updatedFeature); - await this.service.updateFeatureToggle(projectId, value, userName); + await this.service.updateFeatureToggle( + projectId, + value, + userName, + featureName, + ); await this.service.removeAllStrategiesForEnv(featureName); diff --git a/src/lib/routes/admin-api/project/features.ts b/src/lib/routes/admin-api/project/features.ts index 0c88683d1c..a677a1c01e 100644 --- a/src/lib/routes/admin-api/project/features.ts +++ b/src/lib/routes/admin-api/project/features.ts @@ -178,16 +178,22 @@ export default class ProjectFeaturesController extends Controller { } async updateFeature( - req: IAuthRequest, + req: IAuthRequest< + { projectId: string; featureName: string }, + any, + FeatureToggleDTO, + any + >, res: Response, ): Promise { - const { projectId } = req.params; + const { projectId, featureName } = req.params; const data = req.body; const userName = extractUsername(req); const created = await this.featureService.updateFeatureToggle( projectId, data, userName, + featureName, ); res.status(200).json(created); } diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index b7baef6253..d237c87c62 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -118,7 +118,7 @@ class FeatureToggleService { const id = await this.featureToggleStore.getProjectId(featureName); if (id !== projectId) { throw new InvalidOperationError( - 'You can not change the projectId for an activation strategy.', + 'Project id does not match the project that the feature belongs to', ); } } @@ -162,6 +162,7 @@ class FeatureToggleService { project, newDocument, createdBy, + featureName, ); if (featureToggle.stale !== newDocument.stale) { @@ -559,8 +560,10 @@ class FeatureToggleService { projectId: string, updatedFeature: FeatureToggleDTO, userName: string, + featureName: string, ): Promise { - const featureName = updatedFeature.name; + await this.validateFeatureContext({ featureName, projectId }); + this.logger.info(`${userName} updates feature toggle ${featureName}`); const featureData = await featureMetadataSchema.validateAsync( @@ -569,10 +572,11 @@ class FeatureToggleService { const preData = await this.featureToggleStore.get(featureName); - const featureToggle = await this.featureToggleStore.update( - projectId, - featureData, - ); + const featureToggle = await this.featureToggleStore.update(projectId, { + ...featureData, + name: featureName, + }); + const tags = await this.tagStore.getAllTagsForFeature(featureName); await this.eventStore.store( diff --git a/src/test/e2e/api/admin/feature.e2e.test.ts b/src/test/e2e/api/admin/feature.e2e.test.ts index 98f22fcba8..ec4766d2fc 100644 --- a/src/test/e2e/api/admin/feature.e2e.test.ts +++ b/src/test/e2e/api/admin/feature.e2e.test.ts @@ -732,7 +732,6 @@ test('should hit validate and tags endpoint if legacy api is disabled', async () await appWithDisabledLegacyFeatures.request .get(`/api/admin/features/${feature.name}/tags`) .expect((res) => { - console.log(res.body); expect(res.body.tags[0].value).toBe('TeamGreen'); }); 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 d186982824..449a5d87f7 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -1914,3 +1914,72 @@ test(`a feature's variants should be sorted by name in increasing order`, async expect(res.body.variants[2].name).toBe('z'); }); }); + +test('should validate context when calling update with PUT', async () => { + const name = 'new.toggle.validate.context'; + await app.request + .post('/api/admin/projects/default/features') + .send({ name, description: 'some', type: 'release' }) + .expect(201); + + await app.request + .put(`/api/admin/projects/another-project/features/${name}`) + .send({ name, description: 'updated', type: 'kill-switch' }) + .expect((res) => { + expect(res.body.name).toBe('InvalidOperationError'); + }) + .expect(403); +}); + +test('should validate context when calling update with PATCH', async () => { + const name = 'new.toggle.validate.context2'; + await app.request + .post('/api/admin/projects/default/features') + .send({ name, description: 'some', type: 'release' }) + .expect(201); + + await app.request + .patch(`/api/admin/projects/another-project/features/${name}`) + .send([]) + .expect((res) => { + expect(res.body.name).toBe('InvalidOperationError'); + }) + .expect(403); +}); + +test('should not update project with PUT', async () => { + const name = 'new.toggle.validate.update.project.put'; + await app.request + .post('/api/admin/projects/default/features') + .send({ name, description: 'some', type: 'release' }) + .expect(201); + + await app.request + .put(`/api/admin/projects/default/features/${name}`) + .send({ + name, + description: 'updated', + type: 'kill-switch', + project: 'new-project', + }) + .expect((res) => { + expect(res.body.project).toBe('default'); + }) + .expect(200); +}); + +test('should not update project with PATCH', async () => { + const name = 'new.toggle.validate.update.project.patch'; + await app.request + .post('/api/admin/projects/default/features') + .send({ name, description: 'some', type: 'release' }) + .expect(201); + + await app.request + .patch(`/api/admin/projects/default/features/${name}`) + .send([{ op: 'replace', path: '/project', value: 'new-project' }]) + .expect((res) => { + expect(res.body.project).toBe('default'); + }) + .expect(200); +}); diff --git a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts index 706d89f99f..a7b7fd065f 100644 --- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts +++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts @@ -111,3 +111,41 @@ test('Should be able to get strategy by id', async () => { const fetchedConfig = await service.getStrategy(createdConfig.id); expect(fetchedConfig).toEqual(createdConfig); }); + +test('should ignore name in the body when updating feature toggle', async () => { + const featureName = 'body-name-update'; + const projectId = 'default'; + + const userName = 'strategy'; + const secondFeatureName = 'body-name-update2'; + + await service.createFeatureToggle( + projectId, + { + name: featureName, + description: 'First toggle', + }, + userName, + ); + + await service.createFeatureToggle( + projectId, + { + name: secondFeatureName, + description: 'Second toggle', + }, + userName, + ); + + const update = { + name: secondFeatureName, + description: "I'm changed", + }; + + await service.updateFeatureToggle(projectId, update, userName, featureName); + const featureOne = await service.getFeature(featureName); + const featureTwo = await service.getFeature(secondFeatureName); + + expect(featureOne.description).toBe(`I'm changed`); + expect(featureTwo.description).toBe('Second toggle'); +});