From cfcf9de65a210352d81e7ba8fd8d37df8624bdc5 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Thu, 12 Oct 2023 08:38:03 +0200 Subject: [PATCH] feat: Protect archive feature (#5003) --- .../dependent-features-service.ts | 2 +- .../dependent-features-store-type.ts | 2 +- .../dependent-features-store.ts | 6 +- .../feature-toggle-controller.ts | 3 +- .../feature-toggle/feature-toggle-service.ts | 58 ++++++++++++++++--- .../feature-toggle-legacy-controller.ts | 3 +- .../admin-api/project/project-archive.ts | 3 +- src/test/e2e/api/client/feature.e2e.test.ts | 8 ++- .../api/client/feature.optimal304.e2e.test.ts | 8 ++- .../e2e/services/project-service.e2e.test.ts | 4 +- 10 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/lib/features/dependent-features/dependent-features-service.ts b/src/lib/features/dependent-features/dependent-features-service.ts index 12b536e65b96..3920524d4165 100644 --- a/src/lib/features/dependent-features/dependent-features-service.ts +++ b/src/lib/features/dependent-features/dependent-features-service.ts @@ -182,7 +182,7 @@ export class DependentFeaturesService { projectId: string, user: string, ): Promise { - await this.dependentFeaturesStore.deleteAll(feature); + await this.dependentFeaturesStore.deleteAll([feature]); await this.eventService.storeEvent({ type: 'feature-dependencies-removed', project: projectId, diff --git a/src/lib/features/dependent-features/dependent-features-store-type.ts b/src/lib/features/dependent-features/dependent-features-store-type.ts index 007bfd5ce28f..8d736f83b8c5 100644 --- a/src/lib/features/dependent-features/dependent-features-store-type.ts +++ b/src/lib/features/dependent-features/dependent-features-store-type.ts @@ -3,5 +3,5 @@ import { FeatureDependency, FeatureDependencyId } from './dependent-features'; export interface IDependentFeaturesStore { upsert(featureDependency: FeatureDependency): Promise; delete(dependency: FeatureDependencyId): Promise; - deleteAll(child?: string): Promise; + deleteAll(children?: string[]): Promise; } diff --git a/src/lib/features/dependent-features/dependent-features-store.ts b/src/lib/features/dependent-features/dependent-features-store.ts index 32ddb8ffc2c4..a14b0eb0e1dc 100644 --- a/src/lib/features/dependent-features/dependent-features-store.ts +++ b/src/lib/features/dependent-features/dependent-features-store.ts @@ -41,10 +41,10 @@ export class DependentFeaturesStore implements IDependentFeaturesStore { .del(); } - async deleteAll(feature: string | undefined): Promise { - if (feature) { + async deleteAll(features: string[] | undefined): Promise { + if (features) { await this.db('dependent_features') - .andWhere('child', feature) + .whereIn('child', features) .del(); } else { await this.db('dependent_features').del(); diff --git a/src/lib/features/feature-toggle/feature-toggle-controller.ts b/src/lib/features/feature-toggle/feature-toggle-controller.ts index 9ad0b3aeb9cb..fd62f4044a6b 100644 --- a/src/lib/features/feature-toggle/feature-toggle-controller.ts +++ b/src/lib/features/feature-toggle/feature-toggle-controller.ts @@ -777,10 +777,9 @@ export default class ProjectFeaturesController extends Controller { res: Response, ): Promise { const { featureName, projectId } = req.params; - const userName = extractUsername(req); await this.featureService.archiveToggle( featureName, - userName, + req.user, projectId, ); res.status(202).send(); diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index 8cc11fa80730..824262a74da2 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -71,6 +71,7 @@ import { import { DATE_OPERATORS, DEFAULT_ENV, + extractUsernameFromUser, NUM_OPERATORS, SEMVER_OPERATORS, STRING_OPERATORS, @@ -1465,6 +1466,25 @@ class FeatureToggleService { } async archiveToggle( + featureName: string, + user: User, + projectId?: string, + ): Promise { + if (projectId) { + await this.stopWhenChangeRequestsEnabled( + projectId, + undefined, + user, + ); + } + await this.unprotectedArchiveToggle( + featureName, + extractUsernameFromUser(user), + projectId, + ); + } + + async unprotectedArchiveToggle( featureName: string, createdBy: string, projectId?: string, @@ -1476,6 +1496,7 @@ class FeatureToggleService { featureName, projectId, }); + await this.validateNoOrphanParents([featureName]); } await this.validateNoChildren(featureName); @@ -1492,12 +1513,27 @@ class FeatureToggleService { } async archiveToggles( + featureNames: string[], + user: User, + projectId: string, + ): Promise { + await this.stopWhenChangeRequestsEnabled(projectId, undefined, user); + await this.unprotectedArchiveToggles( + featureNames, + extractUsernameFromUser(user), + projectId, + ); + } + + async unprotectedArchiveToggles( featureNames: string[], createdBy: string, projectId: string, ): Promise { - await this.validateFeaturesContext(featureNames, projectId); - await this.validateNoOrphanParents(featureNames); + await Promise.all([ + this.validateFeaturesContext(featureNames, projectId), + this.validateNoOrphanParents(featureNames), + ]); const features = await this.featureToggleStore.getAllByNames( featureNames, @@ -2176,15 +2212,19 @@ class FeatureToggleService { private async stopWhenChangeRequestsEnabled( project: string, - environment: string, + environment?: string, user?: User, ) { - const canBypass = - await this.changeRequestAccessReadModel.canBypassChangeRequest( - project, - environment, - user, - ); + const canBypass = environment + ? await this.changeRequestAccessReadModel.canBypassChangeRequest( + project, + environment, + user, + ) + : await this.changeRequestAccessReadModel.canBypassChangeRequestForProject( + project, + user, + ); if (!canBypass) { throw new PermissionError(SKIP_CHANGE_REQUEST); } diff --git a/src/lib/features/feature-toggle/legacy/feature-toggle-legacy-controller.ts b/src/lib/features/feature-toggle/legacy/feature-toggle-legacy-controller.ts index 9b364c6cbf9d..f259a3e5db21 100644 --- a/src/lib/features/feature-toggle/legacy/feature-toggle-legacy-controller.ts +++ b/src/lib/features/feature-toggle/legacy/feature-toggle-legacy-controller.ts @@ -489,9 +489,8 @@ class FeatureController extends Controller { async archiveToggle(req: IAuthRequest, res: Response): Promise { const { featureName } = req.params; - const userName = extractUsername(req); - await this.service.archiveToggle(featureName, userName); + await this.service.archiveToggle(featureName, req.user); res.status(200).end(); } } diff --git a/src/lib/routes/admin-api/project/project-archive.ts b/src/lib/routes/admin-api/project/project-archive.ts index 4ffd55d6c87c..7669e59a8b3f 100644 --- a/src/lib/routes/admin-api/project/project-archive.ts +++ b/src/lib/routes/admin-api/project/project-archive.ts @@ -140,9 +140,8 @@ export default class ProjectArchiveController extends Controller { ): Promise { const { features } = req.body; const { projectId } = req.params; - const userName = extractUsername(req); - await this.featureService.archiveToggles(features, userName, projectId); + await this.featureService.archiveToggles(features, req.user, projectId); res.status(202).end(); } } diff --git a/src/test/e2e/api/client/feature.e2e.test.ts b/src/test/e2e/api/client/feature.e2e.test.ts index aee5bdf75cdf..b2fe54eec0ba 100644 --- a/src/test/e2e/api/client/feature.e2e.test.ts +++ b/src/test/e2e/api/client/feature.e2e.test.ts @@ -5,9 +5,11 @@ import { import dbInit, { ITestDb } from '../../helpers/database-init'; import getLogger from '../../../fixtures/no-logger'; import { DEFAULT_ENV } from '../../../../lib/util/constants'; +import User from '../../../../lib/types/user'; let app: IUnleashTest; let db: ITestDb; +const testUser = { name: 'test' } as User; beforeAll(async () => { db = await dbInit('feature_api_client', getLogger, { @@ -69,7 +71,7 @@ beforeAll(async () => { await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedX', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( @@ -83,7 +85,7 @@ beforeAll(async () => { await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedY', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( 'default', @@ -95,7 +97,7 @@ beforeAll(async () => { ); await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedZ', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( 'default', diff --git a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts index 1effc282fe58..cf4020e118fc 100644 --- a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts +++ b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts @@ -4,10 +4,12 @@ import { } from '../../helpers/test-helper'; import dbInit, { ITestDb } from '../../helpers/database-init'; import getLogger from '../../../fixtures/no-logger'; +import User from '../../../../lib/types/user'; // import { DEFAULT_ENV } from '../../../../lib/util/constants'; let app: IUnleashTest; let db: ITestDb; +const testUser = { name: 'test' } as User; beforeAll(async () => { db = await dbInit('feature_304_api_client', getLogger); @@ -55,7 +57,7 @@ beforeAll(async () => { await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedX', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( @@ -69,7 +71,7 @@ beforeAll(async () => { await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedY', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( 'default', @@ -81,7 +83,7 @@ beforeAll(async () => { ); await app.services.featureToggleServiceV2.archiveToggle( 'featureArchivedZ', - 'test', + testUser, ); await app.services.featureToggleServiceV2.createFeatureToggle( 'default', diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index 8ade603eca42..821b1f7bbd38 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -1241,7 +1241,7 @@ test('should only count active feature toggles for project', async () => { enabled: false, }); - await featureToggleService.archiveToggle('only-active-t2', 'me'); + await featureToggleService.archiveToggle('only-active-t2', user); const projects = await projectService.getProjects(); const theProject = projects.find((p) => p.id === project.id); @@ -1265,7 +1265,7 @@ test('should list projects with all features archived', async () => { enabled: false, }); - await featureToggleService.archiveToggle('archived-toggle', 'me'); + await featureToggleService.archiveToggle('archived-toggle', user); const projects = await projectService.getProjects(); const theProject = projects.find((p) => p.id === project.id);