From 7e8d4b5a47e1289808157d8cc9db46c72b0ced8e Mon Sep 17 00:00:00 2001 From: kwasniew Date: Tue, 10 Oct 2023 15:00:09 +0200 Subject: [PATCH 1/2] feat: prevent adding dependency to archived or removed parent --- .../createDependentFeaturesService.ts | 14 +++++-- .../dependent-features-read-model-type.ts | 1 + .../dependent-features-read-model.ts | 9 +++++ .../dependent-features-service.ts | 37 +++++++++++++++---- .../dependent.features.e2e.test.ts | 31 ++++++++++++++++ .../fake-dependent-features-read-model.ts | 4 ++ .../fake-features-read-model.ts | 7 ++++ .../features-read-model-type.ts | 3 ++ .../feature-toggle/features-read-model.ts | 19 ++++++++++ 9 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 src/lib/features/feature-toggle/fake-features-read-model.ts create mode 100644 src/lib/features/feature-toggle/features-read-model-type.ts create mode 100644 src/lib/features/feature-toggle/features-read-model.ts diff --git a/src/lib/features/dependent-features/createDependentFeaturesService.ts b/src/lib/features/dependent-features/createDependentFeaturesService.ts index f00fa202b8f8..c040c8e51376 100644 --- a/src/lib/features/dependent-features/createDependentFeaturesService.ts +++ b/src/lib/features/dependent-features/createDependentFeaturesService.ts @@ -14,6 +14,8 @@ import { createChangeRequestAccessReadModel, createFakeChangeRequestAccessService, } from '../change-request-access-service/createChangeRequestAccessReadModel'; +import { FeaturesReadModel } from '../feature-toggle/features-read-model'; +import { FakeFeaturesReadModel } from '../feature-toggle/fake-features-read-model'; export const createDependentFeaturesService = ( db: Db, @@ -35,12 +37,14 @@ export const createDependentFeaturesService = ( db, config, ); - return new DependentFeaturesService( + const featuresReadModel = new FeaturesReadModel(db); + return new DependentFeaturesService({ dependentFeaturesStore, dependentFeaturesReadModel, changeRequestAccessReadModel, + featuresReadModel, eventService, - ); + }); }; export const createFakeDependentFeaturesService = ( @@ -58,11 +62,13 @@ export const createFakeDependentFeaturesService = ( const dependentFeaturesStore = new FakeDependentFeaturesStore(); const dependentFeaturesReadModel = new FakeDependentFeaturesReadModel(); const changeRequestAccessReadModel = createFakeChangeRequestAccessService(); + const featuresReadModel = new FakeFeaturesReadModel(); - return new DependentFeaturesService( + return new DependentFeaturesService({ dependentFeaturesStore, dependentFeaturesReadModel, changeRequestAccessReadModel, + featuresReadModel, eventService, - ); + }); }; diff --git a/src/lib/features/dependent-features/dependent-features-read-model-type.ts b/src/lib/features/dependent-features/dependent-features-read-model-type.ts index e08c950010f1..f65b2aaf5740 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model-type.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model-type.ts @@ -1,6 +1,7 @@ import { IDependency } from '../../types'; export interface IDependentFeaturesReadModel { + featureExists(parent: string): Promise; getChildren(parents: string[]): Promise; // given a list of parents and children verifies if some children would be orphaned after deletion // we're interested in the list of parents, not orphans diff --git a/src/lib/features/dependent-features/dependent-features-read-model.ts b/src/lib/features/dependent-features/dependent-features-read-model.ts index fb03a64016f4..15463bdedd17 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model.ts @@ -43,6 +43,15 @@ export class DependentFeaturesReadModel implements IDependentFeaturesReadModel { })); } + async featureExists(parent: string): Promise { + const rows = await this.db('features') + .where('name', parent) + .andWhere('archived_at', null) + .select('name'); + + return rows.length > 0; + } + async getParentOptions(child: string): Promise { const result = await this.db('features') .where('features.name', child) diff --git a/src/lib/features/dependent-features/dependent-features-service.ts b/src/lib/features/dependent-features/dependent-features-service.ts index b33bf73c4434..12b536e65b96 100644 --- a/src/lib/features/dependent-features/dependent-features-service.ts +++ b/src/lib/features/dependent-features/dependent-features-service.ts @@ -8,6 +8,15 @@ import { User } from '../../server-impl'; import { SKIP_CHANGE_REQUEST } from '../../types'; import { IChangeRequestAccessReadModel } from '../change-request-access-service/change-request-access-read-model'; import { extractUsernameFromUser } from '../../util'; +import { IFeaturesReadModel } from '../feature-toggle/features-read-model-type'; + +interface IDependentFeaturesServiceDeps { + dependentFeaturesStore: IDependentFeaturesStore; + dependentFeaturesReadModel: IDependentFeaturesReadModel; + changeRequestAccessReadModel: IChangeRequestAccessReadModel; + featuresReadModel: IFeaturesReadModel; + eventService: EventService; +} export class DependentFeaturesService { private dependentFeaturesStore: IDependentFeaturesStore; @@ -16,17 +25,21 @@ export class DependentFeaturesService { private changeRequestAccessReadModel: IChangeRequestAccessReadModel; + private featuresReadModel: IFeaturesReadModel; + private eventService: EventService; - constructor( - dependentFeaturesStore: IDependentFeaturesStore, - dependentFeaturesReadModel: IDependentFeaturesReadModel, - changeRequestAccessReadModel: IChangeRequestAccessReadModel, - eventService: EventService, - ) { + constructor({ + featuresReadModel, + dependentFeaturesReadModel, + dependentFeaturesStore, + eventService, + changeRequestAccessReadModel, + }: IDependentFeaturesServiceDeps) { this.dependentFeaturesStore = dependentFeaturesStore; this.dependentFeaturesReadModel = dependentFeaturesReadModel; this.changeRequestAccessReadModel = changeRequestAccessReadModel; + this.featuresReadModel = featuresReadModel; this.eventService = eventService; } @@ -77,15 +90,23 @@ export class DependentFeaturesService { ): Promise { const { enabled, feature: parent, variants } = dependentFeature; - const children = await this.dependentFeaturesReadModel.getChildren([ - child, + const [children, parentExists] = await Promise.all([ + this.dependentFeaturesReadModel.getChildren([child]), + this.featuresReadModel.featureExists(parent), ]); + if (children.length > 0) { throw new InvalidOperationError( 'Transitive dependency detected. Cannot add a dependency to the feature that other features depend on.', ); } + if (!parentExists) { + throw new InvalidOperationError( + `No active feature ${parent} exists`, + ); + } + const featureDependency: FeatureDependency = enabled === false ? { diff --git a/src/lib/features/dependent-features/dependent.features.e2e.test.ts b/src/lib/features/dependent-features/dependent.features.e2e.test.ts index be378a58fa26..5d1980780068 100644 --- a/src/lib/features/dependent-features/dependent.features.e2e.test.ts +++ b/src/lib/features/dependent-features/dependent.features.e2e.test.ts @@ -136,3 +136,34 @@ test('should not allow to add a parent dependency to a feature that already has 403, ); }); + +test('should not allow to add non-existent parent dependency', async () => { + const grandparent = uuidv4(); + const parent = uuidv4(); + const child = uuidv4(); + await app.createFeature(child); + + await addFeatureDependency( + child, + { + feature: parent, + }, + 403, + ); +}); + +test('should not allow to add archived parent dependency', async () => { + const parent = uuidv4(); + const child = uuidv4(); + await app.createFeature(child); + await app.createFeature(parent); + await app.archiveFeature(parent); + + await addFeatureDependency( + child, + { + feature: parent, + }, + 403, + ); +}); diff --git a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts index 75657618bf59..a034ee3a28cd 100644 --- a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts @@ -23,4 +23,8 @@ export class FakeDependentFeaturesReadModel getOrphanParents(parentsAndChildren: string[]): Promise { return Promise.resolve([]); } + + featureExists(): Promise { + return Promise.resolve(false); + } } diff --git a/src/lib/features/feature-toggle/fake-features-read-model.ts b/src/lib/features/feature-toggle/fake-features-read-model.ts new file mode 100644 index 000000000000..402f07019ff1 --- /dev/null +++ b/src/lib/features/feature-toggle/fake-features-read-model.ts @@ -0,0 +1,7 @@ +import { IFeaturesReadModel } from './features-read-model-type'; + +export class FakeFeaturesReadModel implements IFeaturesReadModel { + featureExists(): Promise { + return Promise.resolve(false); + } +} diff --git a/src/lib/features/feature-toggle/features-read-model-type.ts b/src/lib/features/feature-toggle/features-read-model-type.ts new file mode 100644 index 000000000000..2455c4ed30ee --- /dev/null +++ b/src/lib/features/feature-toggle/features-read-model-type.ts @@ -0,0 +1,3 @@ +export interface IFeaturesReadModel { + featureExists(parent: string): Promise; +} diff --git a/src/lib/features/feature-toggle/features-read-model.ts b/src/lib/features/feature-toggle/features-read-model.ts new file mode 100644 index 000000000000..0e5c2e381767 --- /dev/null +++ b/src/lib/features/feature-toggle/features-read-model.ts @@ -0,0 +1,19 @@ +import { Db } from '../../db/db'; +import { IFeaturesReadModel } from './features-read-model-type'; + +export class FeaturesReadModel implements IFeaturesReadModel { + private db: Db; + + constructor(db: Db) { + this.db = db; + } + + async featureExists(parent: string): Promise { + const rows = await this.db('features') + .where('name', parent) + .andWhere('archived_at', null) + .select('name'); + + return rows.length > 0; + } +} From d9b8bb99374767faaafb1b2e799c91dcaceb2cb3 Mon Sep 17 00:00:00 2001 From: kwasniew Date: Tue, 10 Oct 2023 15:01:27 +0200 Subject: [PATCH 2/2] feat: prevent adding dependency to archived or removed parent --- .../dependent-features-read-model-type.ts | 1 - .../dependent-features/dependent-features-read-model.ts | 9 --------- .../fake-dependent-features-read-model.ts | 4 ---- 3 files changed, 14 deletions(-) diff --git a/src/lib/features/dependent-features/dependent-features-read-model-type.ts b/src/lib/features/dependent-features/dependent-features-read-model-type.ts index f65b2aaf5740..e08c950010f1 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model-type.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model-type.ts @@ -1,7 +1,6 @@ import { IDependency } from '../../types'; export interface IDependentFeaturesReadModel { - featureExists(parent: string): Promise; getChildren(parents: string[]): Promise; // given a list of parents and children verifies if some children would be orphaned after deletion // we're interested in the list of parents, not orphans diff --git a/src/lib/features/dependent-features/dependent-features-read-model.ts b/src/lib/features/dependent-features/dependent-features-read-model.ts index 15463bdedd17..fb03a64016f4 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model.ts @@ -43,15 +43,6 @@ export class DependentFeaturesReadModel implements IDependentFeaturesReadModel { })); } - async featureExists(parent: string): Promise { - const rows = await this.db('features') - .where('name', parent) - .andWhere('archived_at', null) - .select('name'); - - return rows.length > 0; - } - async getParentOptions(child: string): Promise { const result = await this.db('features') .where('features.name', child) diff --git a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts index a034ee3a28cd..75657618bf59 100644 --- a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts @@ -23,8 +23,4 @@ export class FakeDependentFeaturesReadModel getOrphanParents(parentsAndChildren: string[]): Promise { return Promise.resolve([]); } - - featureExists(): Promise { - return Promise.resolve(false); - } }