diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 8df8f770c9..97bd4c7fcc 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -71,6 +71,7 @@ exports[`should create default config 1`] = ` "anonymiseEventLog": false, "batchMetrics": false, "caseInsensitiveInOperators": false, + "crOnVariants": false, "embedProxy": true, "embedProxyFrontend": true, "featuresExportImport": false, @@ -92,6 +93,7 @@ exports[`should create default config 1`] = ` "anonymiseEventLog": false, "batchMetrics": false, "caseInsensitiveInOperators": false, + "crOnVariants": false, "embedProxy": true, "embedProxyFrontend": true, "featuresExportImport": false, diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 873293f9c5..822bd360eb 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -481,8 +481,10 @@ export class AccessStore implements IAccessStore { environment: string, ): Promise { const result = await this.db.raw( - `SELECT EXISTS(SELECT 1 FROM ${T.CHANGE_REQUEST_SETTINGS} - WHERE environment = ? and project = ?) AS present`, + `SELECT EXISTS(SELECT 1 + FROM ${T.CHANGE_REQUEST_SETTINGS} + WHERE environment = ? + and project = ?) AS present`, [environment, project], ); const { present } = result.rows[0]; diff --git a/src/lib/routes/admin-api/project/variants.ts b/src/lib/routes/admin-api/project/variants.ts index afab6ba7e6..1c003fdc36 100644 --- a/src/lib/routes/admin-api/project/variants.ts +++ b/src/lib/routes/admin-api/project/variants.ts @@ -189,12 +189,11 @@ export default class VariantsController extends Controller { res: Response, ): Promise { const { projectId, featureName } = req.params; - const userName = extractUsername(req); const updatedFeature = await this.featureService.updateVariants( featureName, projectId, req.body, - userName, + req.user, ); res.status(200).json({ version: 1, @@ -231,7 +230,6 @@ export default class VariantsController extends Controller { ): Promise { const { projectId, featureName } = req.params; const { environments, variants } = req.body; - const userName = extractUsername(req); if (environments === undefined || environments.length === 0) { throw new BadDataError('No environments provided'); @@ -250,12 +248,12 @@ export default class VariantsController extends Controller { ...variant, })); - await this.featureService.setVariantsOnEnvs( + await this.featureService.crProtectedSetVariantsOnEnvs( projectId, featureName, environments, variantsWithDefaults, - userName, + req.user, ); res.status(200).json({ version: 1, @@ -303,14 +301,13 @@ export default class VariantsController extends Controller { res: Response, ): Promise { const { projectId, featureName, environment } = req.params; - const userName = extractUsername(req); const variants = await this.featureService.updateVariantsOnEnv( featureName, projectId, environment, req.body, - userName, + req.user, ); res.status(200).json({ version: 1, @@ -323,13 +320,12 @@ export default class VariantsController extends Controller { res: Response, ): Promise { const { featureName, environment, projectId } = req.params; - const userName = extractUsername(req); - const variants = await this.featureService.saveVariantsOnEnv( + const variants = await this.featureService.crProtectedSaveVariantsOnEnv( projectId, featureName, environment, req.body, - userName, + req.user, ); res.status(200).json({ version: 1, diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 6b4629ddc1..2c2747b929 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -1,5 +1,5 @@ import { IUnleashConfig } from '../types/option'; -import { IUnleashStores } from '../types'; +import { IFlagResolver, IUnleashStores } from '../types'; import { Logger } from '../logger'; import BadDataError from '../error/bad-data-error'; import NameExistsError from '../error/name-exists-error'; @@ -127,6 +127,8 @@ class FeatureToggleService { private accessService: AccessService; + private flagResolver: IFlagResolver; + constructor( { featureStrategiesStore, @@ -148,7 +150,10 @@ class FeatureToggleService { | 'featureEnvironmentStore' | 'contextFieldStore' >, - { getLogger }: Pick, + { + getLogger, + flagResolver, + }: Pick, segmentService: SegmentService, accessService: AccessService, ) { @@ -163,6 +168,7 @@ class FeatureToggleService { this.contextFieldStore = contextFieldStore; this.segmentService = segmentService; this.accessService = accessService; + this.flagResolver = flagResolver; } async validateFeatureContext({ @@ -1248,7 +1254,7 @@ class FeatureToggleService { featureName: string, project: string, newVariants: Operation[], - createdBy: string, + user: User, ): Promise { const ft = await this.featureStrategiesStore.getFeatureToggleWithVariantEnvs( @@ -1260,7 +1266,7 @@ class FeatureToggleService { project, env.name, newVariants, - createdBy, + user, ).then((resultingVariants) => (env.variants = resultingVariants)), ); await Promise.all(promises); @@ -1273,19 +1279,19 @@ class FeatureToggleService { project: string, environment: string, newVariants: Operation[], - createdBy: string, + user: User, ): Promise { const oldVariants = await this.getVariantsForEnv( featureName, environment, ); const { newDocument } = await applyPatch(oldVariants, newVariants); - return this.saveVariantsOnEnv( + return this.crProtectedSaveVariantsOnEnv( project, featureName, environment, newDocument, - createdBy, + user, oldVariants, ); } @@ -1357,6 +1363,52 @@ class FeatureToggleService { return fixedVariants; } + async crProtectedSaveVariantsOnEnv( + projectId: string, + featureName: string, + environment: string, + newVariants: IVariant[], + user: User, + oldVariants?: IVariant[], + ): Promise { + if (this.flagResolver.isEnabled('crOnVariants')) { + await this.stopWhenChangeRequestsEnabled( + projectId, + environment, + user, + ); + } + return this.saveVariantsOnEnv( + projectId, + featureName, + environment, + newVariants, + user.username, + oldVariants, + ); + } + + async crProtectedSetVariantsOnEnvs( + projectId: string, + featureName: string, + environments: string[], + newVariants: IVariant[], + user: User, + ): Promise { + if (this.flagResolver.isEnabled('crOnVariants')) { + for (const env of environments) { + await this.stopWhenChangeRequestsEnabled(projectId, env); + } + } + return this.setVariantsOnEnvs( + projectId, + featureName, + environments, + newVariants, + user.username, + ); + } + async setVariantsOnEnvs( projectId: string, featureName: string, @@ -1390,7 +1442,6 @@ class FeatureToggleService { }), ), ); - await this.featureEnvironmentStore.setVariantsToFeatureEnvironments( featureName, environments, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index aa4bbb88cc..d80fc6140b 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -66,6 +66,10 @@ const flags = { process.env.UNLEASH_EXPERIMENTAL_CASE_INSENSITIVE_IN_OPERATORS, false, ), + crOnVariants: parseEnvVarBoolean( + process.env.UNLEASH_EXPERIMENTAL_CR_ON_VARIANTS, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = { 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 f79dd8f8f7..6c4269927b 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 @@ -5,10 +5,12 @@ import { DEFAULT_ENV } from '../../../lib/util/constants'; 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 { IConstraint, IVariant } from '../../../lib/types/model'; import { AccessService } from '../../../lib/services/access-service'; import { GroupService } from '../../../lib/services/group-service'; import EnvironmentService from '../../../lib/services/environment-service'; +import { NoAccessError } from '../../../lib/error'; +import { SKIP_CHANGE_REQUEST } from '../../../lib/types'; let stores; let db; @@ -367,3 +369,165 @@ test('Cloning a feature toggle also clones segments correctly', async () => { .segments, ).toHaveLength(1); }); + +test('If change requests are enabled, cannot change variants without going via CR', async () => { + const featureName = 'feature-with-variants-per-env-and-cr'; + await service.createFeatureToggle( + 'default', + { name: featureName }, + 'test-user', + ); + const groupService = new GroupService(stores, unleashConfig); + const accessService = new AccessService( + stores, + unleashConfig, + groupService, + ); + // Force all feature flags on to make sure we have Change requests on + const customFeatureService = new FeatureToggleService( + stores, + { + ...unleashConfig, + flagResolver: { + isEnabled: () => true, + }, + }, + segmentService, + accessService, + ); + + const newVariant: IVariant = { + name: 'cr-enabled', + weight: 100, + weightType: 'variable', + stickiness: 'default', + }; + await db.rawDatabase('change_request_settings').insert({ + project: 'default', + environment: 'default', + }); + return expect(async () => + customFeatureService.crProtectedSaveVariantsOnEnv( + 'default', + featureName, + 'default', + [newVariant], + { + createdAt: undefined, + email: '', + id: 0, + imageUrl: '', + loginAttempts: 0, + name: '', + permissions: [], + seenAt: undefined, + username: '', + generateImageUrl(): string { + return ''; + }, + isAPI: true, + }, + [], + ), + ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST)); +}); + +test('If CRs are protected for any environment in the project stops bulk update of variants', async () => { + const project = await stores.projectStore.create({ + id: 'crOnVariantsProject', + name: 'crOnVariantsProject', + }); + const enabledEnv = await stores.environmentStore.create({ + name: 'crenabledenv', + type: 'production', + }); + const disabledEnv = await stores.environmentStore.create({ + name: 'crdisabledenv', + type: 'production', + }); + + await stores.projectStore.addEnvironmentToProject( + project.id, + enabledEnv.name, + ); + await stores.projectStore.addEnvironmentToProject( + project.id, + disabledEnv.name, + ); + const groupService = new GroupService(stores, unleashConfig); + const accessService = new AccessService( + stores, + unleashConfig, + groupService, + ); + // Force all feature flags on to make sure we have Change requests on + const customFeatureService = new FeatureToggleService( + stores, + { + ...unleashConfig, + flagResolver: { + isEnabled: () => true, + }, + }, + segmentService, + accessService, + ); + + const toggle = await service.createFeatureToggle( + project.id, + { name: 'crOnVariantToggle' }, + 'test-user', + ); + + const variant: IVariant = { + name: 'cr-enabled', + weight: 100, + weightType: 'variable', + stickiness: 'default', + }; + await db.rawDatabase('change_request_settings').insert({ + project: project.id, + environment: enabledEnv.name, + }); + + await customFeatureService.setVariantsOnEnvs( + project.id, + toggle.name, + [enabledEnv.name, disabledEnv.name], + [variant], + 'test-user', + ); + + const newVariants = [ + { ...variant, weight: 500 }, + { + name: 'cr-enabled-2', + weight: 500, + weightType: 'fix', + stickiness: 'default', + }, + ]; + return expect(async () => + customFeatureService.crProtectedSetVariantsOnEnvs( + project.id, + toggle.name, + [enabledEnv.name, disabledEnv.name], + newVariants, + { + createdAt: undefined, + email: '', + id: 0, + imageUrl: '', + loginAttempts: 0, + name: '', + permissions: [], + seenAt: undefined, + username: '', + generateImageUrl(): string { + return ''; + }, + isAPI: true, + }, + ), + ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST)); +});