feat: protect segment operations for change requests (#4417)

src/lib/features/change-request-access-service/change-request-access-read-model.ts

@@ -6,6 +6,10 @@ export interface IChangeRequestAccessReadModel {
         environment: string,
         user?: User,
     ): Promise<boolean>;
+    canBypassChangeRequestForProject(
+        project: string,
+        user?: User,
+    ): Promise<boolean>;
     isChangeRequestsEnabled(
         project: string,
         environment: string,

src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts

@@ -16,6 +16,10 @@ export class FakeChangeRequestAccessReadModel
         return this.canBypass;
     }
 
+    public async canBypassChangeRequestForProject(): Promise<boolean> {
+        return this.canBypass;
+    }
+
     public async isChangeRequestsEnabled(): Promise<boolean> {
         return this.isChangeRequestEnabled;
     }

src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts

@@ -32,7 +32,24 @@ export class ChangeRequestAccessReadModel
                 : Promise.resolve(false),
             this.isChangeRequestsEnabled(project, environment),
         ]);
-        return !(changeRequestEnabled && !canSkipChangeRequest);
+        return canSkipChangeRequest || !changeRequestEnabled;
+    }
+
+    public async canBypassChangeRequestForProject(
+        project: string,
+        user?: User,
+    ): Promise<boolean> {
+        const [canSkipChangeRequest, changeRequestEnabled] = await Promise.all([
+            user
+                ? this.accessService.hasPermission(
+                      user,
+                      SKIP_CHANGE_REQUEST,
+                      project,
+                  )
+                : Promise.resolve(false),
+            this.isChangeRequestsEnabledForProject(project),
+        ]);
+        return canSkipChangeRequest || !changeRequestEnabled;
     }
 
     public async isChangeRequestsEnabled(

src/lib/features/feature-toggle/createFeatureToggleService.ts

@@ -2,7 +2,6 @@ import {
     AccessService,
     FeatureToggleService,
     GroupService,
-    SegmentService,
 } from '../../services';
 import FeatureStrategiesStore from '../../db/feature-strategy-store';
 import FeatureToggleStore from '../../db/feature-toggle-store';
@@ -10,7 +9,6 @@ import FeatureToggleClientStore from '../../db/feature-toggle-client-store';
 import ProjectStore from '../../db/project-store';
 import FeatureTagStore from '../../db/feature-tag-store';
 import { FeatureEnvironmentStore } from '../../db/feature-environment-store';
-import SegmentStore from '../../db/segment-store';
 import ContextFieldStore from '../../db/context-field-store';
 import GroupStore from '../../db/group-store';
 import { AccountStore } from '../../db/account-store';
@@ -26,7 +24,6 @@ import FakeFeatureToggleClientStore from '../../../test/fixtures/fake-feature-to
 import FakeProjectStore from '../../../test/fixtures/fake-project-store';
 import FakeFeatureTagStore from '../../../test/fixtures/fake-feature-tag-store';
 import FakeFeatureEnvironmentStore from '../../../test/fixtures/fake-feature-environment-store';
-import FakeSegmentStore from '../../../test/fixtures/fake-segment-store';
 import FakeContextFieldStore from '../../../test/fixtures/fake-context-field-store';
 import FakeGroupStore from '../../../test/fixtures/fake-group-store';
 import { FakeAccountStore } from '../../../test/fixtures/fake-account-store';
@@ -38,6 +35,10 @@ import {
     createChangeRequestAccessReadModel,
     createFakeChangeRequestAccessService,
 } from '../change-request-access-service/createChangeRequestAccessReadModel';
+import {
+    createFakeSegmentService,
+    createSegmentService,
+} from '../segment/createSegmentService';
 
 export const createFeatureToggleService = (
     db: Db,
@@ -69,12 +70,6 @@ export const createFeatureToggleService = (
         eventBus,
         getLogger,
     );
-    const segmentStore = new SegmentStore(
-        db,
-        eventBus,
-        getLogger,
-        flagResolver,
-    );
     const contextFieldStore = new ContextFieldStore(
         db,
         getLogger,
@@ -95,11 +90,8 @@ export const createFeatureToggleService = (
         { getLogger, flagResolver },
         groupService,
     );
-    const segmentService = new SegmentService(
+    const segmentService = createSegmentService(db, config);
-        { segmentStore, featureStrategiesStore, eventStore },
+    const changeRequestAccessReadModel = createChangeRequestAccessReadModel(
-        config,
-    );
-    const changeRequestAccessReadMode = createChangeRequestAccessReadModel(
         db,
         config,
     );
@@ -117,7 +109,7 @@ export const createFeatureToggleService = (
         { getLogger, flagResolver },
         segmentService,
         accessService,
-        changeRequestAccessReadMode,
+        changeRequestAccessReadModel,
     );
     return featureToggleService;
 };
@@ -133,7 +125,6 @@ export const createFakeFeatureToggleService = (
     const projectStore = new FakeProjectStore();
     const featureTagStore = new FakeFeatureTagStore();
     const featureEnvironmentStore = new FakeFeatureEnvironmentStore();
-    const segmentStore = new FakeSegmentStore();
     const contextFieldStore = new FakeContextFieldStore();
     const groupStore = new FakeGroupStore();
     const accountStore = new FakeAccountStore();
@@ -149,11 +140,8 @@ export const createFakeFeatureToggleService = (
         { getLogger, flagResolver },
         groupService,
     );
-    const segmentService = new SegmentService(
+    const segmentService = createFakeSegmentService(config);
-        { segmentStore, featureStrategiesStore, eventStore },
+    const changeRequestAccessReadModel = createFakeChangeRequestAccessService();
-        config,
-    );
-    const changeRequestAccessReadMode = createFakeChangeRequestAccessService();
     const featureToggleService = new FeatureToggleService(
         {
             featureStrategiesStore,
@@ -168,7 +156,7 @@ export const createFakeFeatureToggleService = (
         { getLogger, flagResolver },
         segmentService,
         accessService,
-        changeRequestAccessReadMode,
+        changeRequestAccessReadModel,
     );
     return featureToggleService;
 };

src/lib/features/segment/createSegmentService.ts

@@ -7,11 +7,15 @@ import FeatureStrategiesStore from '../../db/feature-strategy-store';
 import SegmentStore from '../../db/segment-store';
 import FakeSegmentStore from '../../../test/fixtures/fake-segment-store';
 import FakeFeatureStrategiesStore from '../../../test/fixtures/fake-feature-strategies-store';
+import {
+    createChangeRequestAccessReadModel,
+    createFakeChangeRequestAccessService,
+} from '../change-request-access-service/createChangeRequestAccessReadModel';
 
 export const createSegmentService = (
     db: Db,
     config: IUnleashConfig,
-): ISegmentService => {
+): SegmentService => {
     const { eventBus, getLogger, flagResolver } = config;
     const eventStore = new EventStore(db, getLogger);
     const segmentStore = new SegmentStore(
@@ -26,9 +30,14 @@ export const createSegmentService = (
         getLogger,
         flagResolver,
     );
+    const changeRequestAccessReadModel = createChangeRequestAccessReadModel(
+        db,
+        config,
+    );
 
     return new SegmentService(
         { segmentStore, featureStrategiesStore, eventStore },
+        changeRequestAccessReadModel,
         config,
     );
 };
@@ -39,9 +48,11 @@ export const createFakeSegmentService = (
     const eventStore = new FakeEventStore();
     const segmentStore = new FakeSegmentStore();
     const featureStrategiesStore = new FakeFeatureStrategiesStore();
+    const changeRequestAccessReadModel = createFakeChangeRequestAccessService();
 
     return new SegmentService(
         { segmentStore, featureStrategiesStore, eventStore },
+        changeRequestAccessReadModel,
         config,
     );
 };

src/lib/segments/segment-service-interface.ts

@@ -34,8 +34,16 @@ export interface ISegmentService {
         user: Partial<Pick<IUser, 'username' | 'email'>>,
     ): Promise<void>;
 
+    unprotectedUpdate(
+        id: number,
+        data: UpsertSegmentSchema,
+        user: Partial<Pick<IUser, 'username' | 'email'>>,
+    ): Promise<void>;
+
     delete(id: number, user: IUser): Promise<void>;
 
+    unprotectedDelete(id: number, user: IUser): Promise<void>;
+
     removeFromStrategy(id: number, strategyId: string): Promise<void>;
 
     cloneStrategySegments(

src/lib/services/index.ts

@@ -176,10 +176,14 @@ export const createServices = (
     const versionService = new VersionService(stores, config);
     const healthService = new HealthService(stores, config);
     const userFeedbackService = new UserFeedbackService(stores, config);
-    const segmentService = new SegmentService(stores, config);
     const changeRequestAccessReadModel = db
         ? createChangeRequestAccessReadModel(db, config)
         : createFakeChangeRequestAccessService();
+    const segmentService = new SegmentService(
+        stores,
+        changeRequestAccessReadModel,
+        config,
+    );
     const featureToggleServiceV2 = new FeatureToggleService(
         stores,
         config,

src/lib/services/segment-service.ts

@@ -1,6 +1,11 @@
 import { IUnleashConfig } from '../types/option';
 import { IEventStore } from '../types/stores/event-store';
-import { IClientSegment, IUnleashStores } from '../types';
+import {
+    IClientSegment,
+    IFlagResolver,
+    IUnleashStores,
+    SKIP_CHANGE_REQUEST,
+} from '../types';
 import { Logger } from '../logger';
 import NameExistsError from '../error/name-exists-error';
 import { ISegmentStore } from '../types/stores/segment-store';
@@ -15,6 +20,8 @@ import User from '../types/user';
 import { IFeatureStrategiesStore } from '../types/stores/feature-strategies-store';
 import BadDataError from '../error/bad-data-error';
 import { ISegmentService } from '../segments/segment-service-interface';
+import { PermissionError } from '../error';
+import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model';
 
 export class SegmentService implements ISegmentService {
     private logger: Logger;
@@ -25,8 +32,12 @@ export class SegmentService implements ISegmentService {
 
     private eventStore: IEventStore;
 
+    private changeRequestAccessReadModel: IChangeRequestAccessReadModel;
+
     private config: IUnleashConfig;
 
+    private flagResolver: IFlagResolver;
+
     constructor(
         {
             segmentStore,
@@ -36,12 +47,15 @@ export class SegmentService implements ISegmentService {
             IUnleashStores,
             'segmentStore' | 'featureStrategiesStore' | 'eventStore'
         >,
+        changeRequestAccessReadModel: IChangeRequestAccessReadModel,
         config: IUnleashConfig,
     ) {
         this.segmentStore = segmentStore;
         this.featureStrategiesStore = featureStrategiesStore;
         this.eventStore = eventStore;
+        this.changeRequestAccessReadModel = changeRequestAccessReadModel;
         this.logger = config.getLogger('services/segment-service.ts');
+        this.flagResolver = config.flagResolver;
         this.config = config;
     }
 
@@ -82,17 +96,25 @@ export class SegmentService implements ISegmentService {
 
         await this.eventStore.store({
             type: SEGMENT_CREATED,
-            createdBy: user.email || user.username,
+            createdBy: user.email || user.username || 'unknown',
             data: segment,
         });
 
         return segment;
     }
 
-    async update(
+    async update(id: number, data: unknown, user: User): Promise<void> {
+        if (this.flagResolver.isEnabled('segmentChangeRequests')) {
+            const input = await segmentSchema.validateAsync(data);
+            await this.stopWhenChangeRequestsEnabled(input.project, user);
+        }
+        return this.unprotectedUpdate(id, data, user);
+    }
+
+    async unprotectedUpdate(
         id: number,
         data: unknown,
-        user: Partial<Pick<User, 'username' | 'email'>>,
+        user: User,
     ): Promise<void> {
         const input = await segmentSchema.validateAsync(data);
         this.validateSegmentValuesLimit(input);
@@ -108,13 +130,26 @@ export class SegmentService implements ISegmentService {
 
         await this.eventStore.store({
             type: SEGMENT_UPDATED,
-            createdBy: user.email || user.username,
+            createdBy: user.email || user.username || 'unknown',
             data: segment,
             preData,
         });
     }
 
     async delete(id: number, user: User): Promise<void> {
+        const segment = await this.segmentStore.get(id);
+        if (this.flagResolver.isEnabled('segmentChangeRequests')) {
+            await this.stopWhenChangeRequestsEnabled(segment.project, user);
+        }
+        await this.segmentStore.delete(id);
+        await this.eventStore.store({
+            type: SEGMENT_DELETED,
+            createdBy: user.email || user.username,
+            data: segment,
+        });
+    }
+
+    async unprotectedDelete(id: number, user: User): Promise<void> {
         const segment = await this.segmentStore.get(id);
         await this.segmentStore.delete(id);
         await this.eventStore.store({
@@ -248,4 +283,16 @@ export class SegmentService implements ISegmentService {
             );
         }
     }
+
+    private async stopWhenChangeRequestsEnabled(project?: string, user?: User) {
+        if (!project) return;
+        const canBypass =
+            await this.changeRequestAccessReadModel.canBypassChangeRequestForProject(
+                project,
+                user,
+            );
+        if (!canBypass) {
+            throw new PermissionError(SKIP_CHANGE_REQUEST);
+        }
+    }
 }

src/test/e2e/services/access-service.e2e.test.ts

@@ -231,7 +231,7 @@ beforeAll(async () => {
     featureToggleService = new FeatureToggleService(
         stores,
         config,
-        new SegmentService(stores, config),
+        new SegmentService(stores, changeRequestAccessReadModel, config),
         accessService,
         changeRequestAccessReadModel,
     );

src/test/e2e/services/api-token-service.e2e.test.ts

@@ -34,7 +34,7 @@ beforeAll(async () => {
     const featureToggleService = new FeatureToggleService(
         stores,
         config,
-        new SegmentService(stores, config),
+        new SegmentService(stores, changeRequestAccessReadModel, config),
         accessService,
         changeRequestAccessReadModel,
     );

src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts

@@ -40,13 +40,18 @@ beforeAll(async () => {
     );
     unleashConfig = config;
     stores = db.stores;
-    segmentService = new SegmentService(stores, config);
+
     const groupService = new GroupService(stores, config);
     const accessService = new AccessService(stores, config, groupService);
     const changeRequestAccessReadModel = new ChangeRequestAccessReadModel(
         db.rawDatabase,
         accessService,
     );
+    segmentService = new SegmentService(
+        stores,
+        changeRequestAccessReadModel,
+        config,
+    );
     service = new FeatureToggleService(
         stores,
         config,

src/test/e2e/services/playground-service.test.ts

@@ -36,13 +36,17 @@ beforeAll(async () => {
     const config = createTestConfig();
     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);
     const changeRequestAccessReadModel = new ChangeRequestAccessReadModel(
         db.rawDatabase,
         accessService,
     );
+    segmentService = new SegmentService(
+        stores,
+        changeRequestAccessReadModel,
+        config,
+    );
     featureToggleService = new FeatureToggleService(
         stores,
         config,

src/test/e2e/services/project-health-service.e2e.test.ts

@@ -39,7 +39,7 @@ beforeAll(async () => {
     featureToggleService = new FeatureToggleService(
         stores,
         config,
-        new SegmentService(stores, config),
+        new SegmentService(stores, changeRequestAccessReadModel, config),
         accessService,
         changeRequestAccessReadModel,
     );

src/test/e2e/services/project-service.e2e.test.ts

@@ -58,7 +58,7 @@ beforeAll(async () => {
     featureToggleService = new FeatureToggleService(
         stores,
         config,
-        new SegmentService(stores, config),
+        new SegmentService(stores, changeRequestAccessReadModel, config),
         accessService,
         changeRequestAccessReadModel,
     );

src/test/fixtures/fake-project-store.ts

@@ -177,7 +177,7 @@ export default class FakeProjectStore implements IProjectStore {
         projectId: string,
         // eslint-disable-next-line @typescript-eslint/no-unused-vars
         environment: string,
-    ): Promise<CreateFeatureStrategySchema | undefined> {
+    ): Promise<CreateFeatureStrategySchema | null> {
         throw new Error('Method not implemented.');
     }