From a09c6313b1b773feedabc30a028eb9e6c333fa7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Wed, 5 Oct 2022 23:33:36 +0200 Subject: [PATCH] fix: auto add stratgy when enabling empty env. (#2137) --- frontend/yarn | 0 src/lib/routes/admin-api/project/features.ts | 2 ++ src/lib/services/feature-toggle-service.ts | 30 ++++++++++++++-- src/lib/services/index.ts | 1 + src/lib/types/stores/access-store.ts | 2 +- src/lib/util/feature-evaluator/helpers.ts | 13 +++++++ .../admin/project/features.auth.e2e.test.ts | 34 +++++++++++++++++++ .../api/admin/project/features.e2e.test.ts | 6 ++-- .../e2e/services/access-service.e2e.test.ts | 1 + .../services/api-token-service.e2e.test.ts | 1 + .../feature-toggle-service-v2.e2e.test.ts | 11 +++++- .../e2e/services/playground-service.test.ts | 5 +++ .../project-health-service.e2e.test.ts | 1 + .../e2e/services/project-service.e2e.test.ts | 1 + 14 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 frontend/yarn diff --git a/frontend/yarn b/frontend/yarn new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/lib/routes/admin-api/project/features.ts b/src/lib/routes/admin-api/project/features.ts index c2cc297744..bb8c917fc5 100644 --- a/src/lib/routes/admin-api/project/features.ts +++ b/src/lib/routes/admin-api/project/features.ts @@ -527,6 +527,7 @@ export default class ProjectFeaturesController extends Controller { environment, true, extractUsername(req), + req.user, ); res.status(200).end(); } @@ -542,6 +543,7 @@ export default class ProjectFeaturesController extends Controller { environment, false, extractUsername(req), + req.user, ); res.status(200).end(); } diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index f6be89c954..2afd2a82a6 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -71,6 +71,11 @@ import { IContextFieldStore } from 'lib/types/stores/context-field-store'; import { Saved, Unsaved } from '../types/saved'; import { SegmentService } from './segment-service'; import { SetStrategySortOrderSchema } from 'lib/openapi/spec/set-strategy-sort-order-schema'; +import { getDefaultStrategy } from '../util/feature-evaluator/helpers'; +import { AccessService } from './access-service'; +import { User } from '../server-impl'; +import { CREATE_FEATURE_STRATEGY } from '../types/permissions'; +import NoAccessError from '../error/no-access-error'; interface IFeatureContext { featureName: string; @@ -106,6 +111,8 @@ class FeatureToggleService { private segmentService: SegmentService; + private accessService: AccessService; + constructor( { featureStrategiesStore, @@ -129,6 +136,7 @@ class FeatureToggleService { >, { getLogger }: Pick, segmentService: SegmentService, + accessService: AccessService, ) { this.logger = getLogger('services/feature-toggle-service.ts'); this.featureStrategiesStore = featureStrategiesStore; @@ -140,6 +148,7 @@ class FeatureToggleService { this.featureEnvironmentStore = featureEnvironmentStore; this.contextFieldStore = contextFieldStore; this.segmentService = segmentService; + this.accessService = accessService; } async validateFeatureContext({ @@ -834,6 +843,7 @@ class FeatureToggleService { environment: string, enabled: boolean, createdBy: string, + user?: User, ): Promise { const hasEnvironment = await this.featureEnvironmentStore.featureHasEnvironment( @@ -849,9 +859,23 @@ class FeatureToggleService { environment, ); if (strategies.length === 0) { - throw new InvalidOperationError( - 'You can not enable the environment before it has strategies', - ); + const canAddStrategies = + user && + (await this.accessService.hasPermission( + user, + CREATE_FEATURE_STRATEGY, + project, + environment, + )); + if (canAddStrategies) { + await this.createStrategy( + getDefaultStrategy(featureName), + { environment, projectId: project, featureName }, + createdBy, + ); + } else { + throw new NoAccessError(CREATE_FEATURE_STRATEGY); + } } } const updatedEnvironmentStatus = diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 87722810de..eded5eebb0 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -71,6 +71,7 @@ export const createServices = ( stores, config, segmentService, + accessService, ); const environmentService = new EnvironmentService(stores, config); const featureTagService = new FeatureTagService(stores, config); diff --git a/src/lib/types/stores/access-store.ts b/src/lib/types/stores/access-store.ts index 8b8a2cea52..b6bc5ca876 100644 --- a/src/lib/types/stores/access-store.ts +++ b/src/lib/types/stores/access-store.ts @@ -125,6 +125,6 @@ export interface IAccessStore extends Store { removePermissionFromRole( roleId: number, permission: string, - projectId?: string, + environment?: string, ): Promise; } diff --git a/src/lib/util/feature-evaluator/helpers.ts b/src/lib/util/feature-evaluator/helpers.ts index 68d4931f03..79aa382164 100644 --- a/src/lib/util/feature-evaluator/helpers.ts +++ b/src/lib/util/feature-evaluator/helpers.ts @@ -1,3 +1,4 @@ +import { IStrategyConfig } from '../../types/model'; import { FeatureStrategiesEvaluationResult } from './client'; import { Context } from './context'; @@ -38,3 +39,15 @@ export function resolveContextValue( export function safeName(str: string = ''): string { return str.replace(/\//g, '_'); } + +export function getDefaultStrategy(featureName: string): IStrategyConfig { + return { + name: 'flexibleRollout', + constraints: [], + parameters: { + rollout: '100', + stickiness: 'default', + groupId: featureName, + }, + }; +} diff --git a/src/test/e2e/api/admin/project/features.auth.e2e.test.ts b/src/test/e2e/api/admin/project/features.auth.e2e.test.ts index a60442d7bc..afa29f0c7b 100644 --- a/src/test/e2e/api/admin/project/features.auth.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.auth.e2e.test.ts @@ -3,6 +3,7 @@ import { IUnleashTest, setupAppWithAuth } from '../../../helpers/test-helper'; import getLogger from '../../../../fixtures/no-logger'; import { DEFAULT_ENV } from '../../../../../lib/util/constants'; import { RoleName } from '../../../../../lib/server-impl'; +import { CREATE_FEATURE_STRATEGY } from '../../../../../lib/types/permissions'; let app: IUnleashTest; let db: ITestDb; @@ -76,3 +77,36 @@ test('Should be possible to update feature toggle with permission', async () => .send({ name, description: 'updated', type: 'kill-switch' }) .expect(200); }); + +test('Should not be possible auto-enable feature toggle without CREATE_FEATURE_STRATEGY permission', async () => { + const email = 'user33@mail.com'; + const url = '/api/admin/projects/default/features'; + const name = 'auth.toggle.enable'; + + await app.services.featureToggleServiceV2.createFeatureToggle( + 'default', + { name }, + 'me', + true, + ); + + await app.services.userService.createUser({ + email, + rootRole: RoleName.EDITOR, + }); + + await app.request.post('/auth/demo/login').send({ + email, + }); + + const role = await db.stores.roleStore.getRoleByName(RoleName.EDITOR); + + await db.stores.accessStore.removePermissionFromRole( + role.id, + CREATE_FEATURE_STRATEGY, + 'default', + ); + await app.request + .post(`${url}/${name}/environments/default/on`) + .expect(403); +}); 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 02b9960396..c8b8eb276e 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -1166,7 +1166,7 @@ test('Trying to get a non existing feature strategy should yield 404', async () .expect(404); }); -test('Can not enable environment for feature without strategies', async () => { +test('Can enable environment for feature without strategies', async () => { const environment = 'some-env'; const featureName = 'com.test.enable.environment.disabled'; @@ -1194,7 +1194,7 @@ test('Can not enable environment for feature without strategies', async () => { `/api/admin/projects/default/features/${featureName}/environments/${environment}/on`, ) .set('Content-Type', 'application/json') - .expect(403); + .expect(200); await app.request .get(`/api/admin/projects/default/features/${featureName}`) .expect(200) @@ -1203,7 +1203,7 @@ test('Can not enable environment for feature without strategies', async () => { const enabledFeatureEnv = res.body.environments.find( (e) => e.name === environment, ); - expect(enabledFeatureEnv.enabled).toBe(false); + expect(enabledFeatureEnv.enabled).toBe(true); expect(enabledFeatureEnv.type).toBe('test'); }); }); diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 148ea63956..1a21fa1842 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -218,6 +218,7 @@ beforeAll(async () => { stores, config, new SegmentService(stores, config), + accessService, ); projectService = new ProjectService( stores, diff --git a/src/test/e2e/services/api-token-service.e2e.test.ts b/src/test/e2e/services/api-token-service.e2e.test.ts index 8988ada823..67d6c2c8f3 100644 --- a/src/test/e2e/services/api-token-service.e2e.test.ts +++ b/src/test/e2e/services/api-token-service.e2e.test.ts @@ -28,6 +28,7 @@ beforeAll(async () => { stores, config, new SegmentService(stores, config), + accessService, ); const project = { id: 'test-project', 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 75dc6085ae..ba53d411e7 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 @@ -6,6 +6,8 @@ import { SegmentService } from '../../../lib/services/segment-service'; import { FeatureStrategySchema } from '../../../lib/openapi/spec/feature-strategy-schema'; import User from '../../../lib/types/user'; import { IConstraint } from '../../../lib/types/model'; +import { AccessService } from '../../../lib/services/access-service'; +import { GroupService } from '../../../lib/services/group-service'; let stores; let db; @@ -28,7 +30,14 @@ beforeAll(async () => { ); stores = db.stores; segmentService = new SegmentService(stores, config); - service = new FeatureToggleService(stores, config, segmentService); + const groupService = new GroupService(stores, config); + const accessService = new AccessService(stores, config, groupService); + service = new FeatureToggleService( + stores, + config, + segmentService, + accessService, + ); }); afterAll(async () => { diff --git a/src/test/e2e/services/playground-service.test.ts b/src/test/e2e/services/playground-service.test.ts index 16b38fa97e..2c66fbf7ac 100644 --- a/src/test/e2e/services/playground-service.test.ts +++ b/src/test/e2e/services/playground-service.test.ts @@ -18,6 +18,8 @@ import { SdkContextSchema } from 'lib/openapi/spec/sdk-context-schema'; import { SegmentSchema } from 'lib/openapi/spec/segment-schema'; import { playgroundStrategyEvaluation } from '../../../lib/openapi/spec/playground-strategy-schema'; import { PlaygroundSegmentSchema } from 'lib/openapi/spec/playground-segment-schema'; +import { GroupService } from '../../../lib/services/group-service'; +import { AccessService } from '../../../lib/services/access-service'; let stores: IUnleashStores; let db: ITestDb; @@ -30,10 +32,13 @@ beforeAll(async () => { db = await dbInit('playground_service_serial', config.getLogger); stores = db.stores; segmentService = new SegmentService(stores, config); + const groupService = new GroupService(stores, config); + const accessService = new AccessService(stores, config, groupService); featureToggleService = new FeatureToggleService( stores, config, segmentService, + accessService, ); service = new PlaygroundService(config, { featureToggleServiceV2: featureToggleService, diff --git a/src/test/e2e/services/project-health-service.e2e.test.ts b/src/test/e2e/services/project-health-service.e2e.test.ts index e8b202ea4d..d6155d7ce1 100644 --- a/src/test/e2e/services/project-health-service.e2e.test.ts +++ b/src/test/e2e/services/project-health-service.e2e.test.ts @@ -33,6 +33,7 @@ beforeAll(async () => { stores, config, new SegmentService(stores, config), + accessService, ); projectService = new ProjectService( stores, diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index 57bc0fc355..89ece175f9 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -40,6 +40,7 @@ beforeAll(async () => { stores, config, new SegmentService(stores, config), + accessService, ); environmentService = new EnvironmentService(stores, config); projectService = new ProjectService(