From 818b8e7813064e35ec20a24a8e868f4ac0875aab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 12 Jun 2023 15:07:18 +0200 Subject: [PATCH] fix: feature not found in project yields 404 (#3958) ## About the changes When a feature is not found in a project we should fail with a NotFound error. If the feature belongs to a different project, it should not be a permission issue, because the user might not be aware (lack of permissions/visibility) of that other project, so even in this case the error should be NotFound (this also works if we ever allow the same feature name in different projects) Fixes #3726 --------- Co-authored-by: Thomas Heartman --- src/lib/services/feature-toggle-service.ts | 8 ++++-- src/lib/types/stores/feature-toggle-store.ts | 2 +- .../api/admin/project/features.e2e.test.ts | 28 +++++++++---------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 8796d2f462..90a9c39008 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -206,8 +206,12 @@ class FeatureToggleService { const id = await this.featureToggleStore.getProjectId(featureName); if (id !== projectId) { - throw new InvalidOperationError( - `The operation could not be completed. The feature exists, but the provided project id ("${projectId}") does not match the project that the feature belongs to ("${id}"). Try using "${id}" in the request URL instead of "${projectId}".`, + throw new NotFoundError( + `There's no feature named "${featureName}" in project "${projectId}"${ + id === undefined + ? '.' + : `, but there's a feature with that name in project "${id}"` + }`, ); } } diff --git a/src/lib/types/stores/feature-toggle-store.ts b/src/lib/types/stores/feature-toggle-store.ts index 441f9db0e1..2fa195513d 100644 --- a/src/lib/types/stores/feature-toggle-store.ts +++ b/src/lib/types/stores/feature-toggle-store.ts @@ -11,7 +11,7 @@ export interface IFeatureToggleQuery { export interface IFeatureToggleStore extends Store { count(query?: Partial): Promise; setLastSeen(toggleNames: string[]): Promise; - getProjectId(name: string): Promise; + getProjectId(name: string): Promise; create(project: string, data: FeatureToggleDTO): Promise; update(project: string, data: FeatureToggleDTO): Promise; archive(featureName: string): Promise; 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 e7cf9f4f36..f46f3f73a0 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -575,7 +575,7 @@ test('Trying to toggle environment that does not exist yields 404', async () => test('Getting feature that does not exist should yield 404', async () => { await app.request .get('/api/admin/projects/default/features/non.existing.feature') - .expect(403); + .expect(404); }); describe('Interacting with features using project IDs that belong to other projects', () => { @@ -617,34 +617,34 @@ describe('Interacting with features using project IDs that belong to other proje await db.stores.userStore.deleteAll(); }); - test("Getting a feature yields 403 if the provided project id doesn't match the feature's project", async () => { + test("Getting a feature yields 404 if the provided project id doesn't match the feature's project", async () => { await app.request .get(`/api/admin/projects/${otherProject}/features/${featureName}`) - .expect(403); + .expect(404); }); - test("Getting a feature yields 403 if the provided project doesn't exist", async () => { + test("Getting a feature yields 404 if the provided project doesn't exist", async () => { await app.request .get( `/api/admin/projects/${nonExistingProject}/features/${featureName}`, ) - .expect(403); + .expect(404); }); - test("Archiving a feature yields 403 if the provided project id doesn't match the feature's project", async () => { + test("Archiving a feature yields 404 if the provided project id doesn't match the feature's project", async () => { await app.request .delete( `/api/admin/projects/${otherProject}/features/${featureName}`, ) - .expect(403); + .expect(404); }); - test("Archiving a feature yields 403 if the provided project doesn't exist", async () => { + test("Archiving a feature yields 404 if the provided project doesn't exist", async () => { await app.request .delete( `/api/admin/projects/${nonExistingProject}/features/${featureName}`, ) - .expect(403); + .expect(404); }); test("Trying to archive a feature that doesn't exist should yield a 404, regardless of whether the project exists or not.", async () => { @@ -1055,7 +1055,7 @@ test('add strategy cannot use wrong projectId', async () => { userIds: '', }, }) - .expect(403); + .expect(404); }); test('update strategy on feature toggle cannot use wrong projectId', async () => { @@ -2215,9 +2215,9 @@ test('should validate context when calling update with PUT', async () => { .put(`/api/admin/projects/another-project/features/${name}`) .send({ name, description: 'updated', type: 'kill-switch' }) .expect((res) => { - expect(res.body.name).toBe('InvalidOperationError'); + expect(res.body.name).toBe('NotFoundError'); }) - .expect(403); + .expect(404); }); test('should validate context when calling update with PATCH', async () => { @@ -2231,9 +2231,9 @@ test('should validate context when calling update with PATCH', async () => { .patch(`/api/admin/projects/another-project/features/${name}`) .send([]) .expect((res) => { - expect(res.body.name).toBe('InvalidOperationError'); + expect(res.body.name).toBe('NotFoundError'); }) - .expect(403); + .expect(404); }); test('should not update project with PUT', async () => {