From ab913228ca59160cc548123fba40e297f1a0e298 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Fri, 24 Mar 2023 14:31:43 +0100 Subject: [PATCH] refactor: read model for change request access checking (#3380) --- src/lib/db/access-store.ts | 15 ------ .../change-request-access-read-model.ts | 13 +++++ .../createChangeRequestAccessReadModel.ts | 19 +++++++ .../fake-change-request-access-read-model.ts | 22 ++++++++ .../sql-change-request-access-read-model.ts | 52 +++++++++++++++++++ .../createFeatureToggleService.ts | 11 ++++ src/lib/features/index.ts | 1 + src/lib/services/access-service.ts | 7 --- src/lib/services/feature-toggle-service.ts | 24 ++++----- src/lib/services/index.ts | 8 +++ src/lib/types/stores/access-store.ts | 5 -- .../e2e/services/access-service.e2e.test.ts | 6 +++ .../services/api-token-service.e2e.test.ts | 6 +++ .../feature-toggle-service-v2.e2e.test.ts | 16 ++++++ .../e2e/services/playground-service.test.ts | 6 +++ .../project-health-service.e2e.test.ts | 6 +++ .../e2e/services/project-service.e2e.test.ts | 6 +++ src/test/fixtures/fake-access-store.ts | 8 --- 18 files changed, 184 insertions(+), 47 deletions(-) create mode 100644 src/lib/features/change-request-access-service/change-request-access-read-model.ts create mode 100644 src/lib/features/change-request-access-service/createChangeRequestAccessReadModel.ts create mode 100644 src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts create mode 100644 src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 5d32678c4e..ed6ea93a41 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -475,19 +475,4 @@ export class AccessStore implements IAccessStore { [destinationEnvironment, sourceEnvironment], ); } - - async isChangeRequestsEnabled( - project: string, - environment: string, - ): Promise { - const result = await this.db.raw( - `SELECT EXISTS(SELECT 1 - FROM ${T.CHANGE_REQUEST_SETTINGS} - WHERE environment = ? - and project = ?) AS present`, - [environment, project], - ); - const { present } = result.rows[0]; - return present; - } } diff --git a/src/lib/features/change-request-access-service/change-request-access-read-model.ts b/src/lib/features/change-request-access-service/change-request-access-read-model.ts new file mode 100644 index 0000000000..041b8c6abf --- /dev/null +++ b/src/lib/features/change-request-access-service/change-request-access-read-model.ts @@ -0,0 +1,13 @@ +import User from '../../types/user'; + +export interface IChangeRequestAccessReadModel { + canBypassChangeRequest( + project: string, + environment: string, + user?: User, + ): Promise; + isChangeRequestsEnabled( + project: string, + environment: string, + ): Promise; +} diff --git a/src/lib/features/change-request-access-service/createChangeRequestAccessReadModel.ts b/src/lib/features/change-request-access-service/createChangeRequestAccessReadModel.ts new file mode 100644 index 0000000000..4e7cb88e0b --- /dev/null +++ b/src/lib/features/change-request-access-service/createChangeRequestAccessReadModel.ts @@ -0,0 +1,19 @@ +import { Db, IUnleashConfig } from 'lib/server-impl'; +import { ChangeRequestAccessReadModel } from './sql-change-request-access-read-model'; +import { createAccessService } from '../access/createAccessService'; +import { FakeChangeRequestAccessReadModel } from './fake-change-request-access-read-model'; +import { IChangeRequestAccessReadModel } from './change-request-access-read-model'; + +export const createChangeRequestAccessReadModel = ( + db: Db, + config: IUnleashConfig, +): IChangeRequestAccessReadModel => { + const accessService = createAccessService(db, config); + + return new ChangeRequestAccessReadModel(db, accessService); +}; + +export const createFakeChangeRequestAccessService = + (): IChangeRequestAccessReadModel => { + return new FakeChangeRequestAccessReadModel(); + }; diff --git a/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts new file mode 100644 index 0000000000..036249db6f --- /dev/null +++ b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts @@ -0,0 +1,22 @@ +import { IChangeRequestAccessReadModel } from './change-request-access-read-model'; + +export class FakeChangeRequestAccessReadModel + implements IChangeRequestAccessReadModel +{ + private canBypass: boolean; + + private isChangeRequestEnabled: boolean; + + constructor(canBypass = true, isChangeRequestEnabled = true) { + this.canBypass = canBypass; + this.isChangeRequestEnabled = isChangeRequestEnabled; + } + + public async canBypassChangeRequest(): Promise { + return this.canBypass; + } + + public async isChangeRequestsEnabled(): Promise { + return this.isChangeRequestEnabled; + } +} diff --git a/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts new file mode 100644 index 0000000000..12edcddd3d --- /dev/null +++ b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts @@ -0,0 +1,52 @@ +import { SKIP_CHANGE_REQUEST } from '../../types'; +import { Db } from '../../db/db'; +import { AccessService } from '../../services'; +import User from '../../types/user'; +import { IChangeRequestAccessReadModel } from './change-request-access-read-model'; + +export class ChangeRequestAccessReadModel + implements IChangeRequestAccessReadModel +{ + private db: Db; + + private accessService: AccessService; + + constructor(db: Db, accessService: AccessService) { + this.db = db; + this.accessService = accessService; + } + + public async canBypassChangeRequest( + project: string, + environment: string, + user?: User, + ): Promise { + const [canSkipChangeRequest, changeRequestEnabled] = await Promise.all([ + user + ? this.accessService.hasPermission( + user, + SKIP_CHANGE_REQUEST, + project, + environment, + ) + : Promise.resolve(false), + this.isChangeRequestsEnabled(project, environment), + ]); + return !(changeRequestEnabled && !canSkipChangeRequest); + } + + public async isChangeRequestsEnabled( + project: string, + environment: string, + ): Promise { + const result = await this.db.raw( + `SELECT EXISTS(SELECT 1 + FROM change_request_settings + WHERE environment = ? + and project = ?) AS present`, + [environment, project], + ); + const { present } = result.rows[0]; + return present; + } +} diff --git a/src/lib/features/feature-toggle/createFeatureToggleService.ts b/src/lib/features/feature-toggle/createFeatureToggleService.ts index c1eabb586b..3323aac976 100644 --- a/src/lib/features/feature-toggle/createFeatureToggleService.ts +++ b/src/lib/features/feature-toggle/createFeatureToggleService.ts @@ -34,6 +34,10 @@ import FakeAccessStore from '../../../test/fixtures/fake-access-store'; import FakeRoleStore from '../../../test/fixtures/fake-role-store'; import FakeEnvironmentStore from '../../../test/fixtures/fake-environment-store'; import EventStore from '../../db/event-store'; +import { + createChangeRequestAccessReadModel, + createFakeChangeRequestAccessService, +} from '../change-request-access-service/createChangeRequestAccessReadModel'; export const createFeatureToggleService = ( db: Db, @@ -85,6 +89,10 @@ export const createFeatureToggleService = ( { segmentStore, featureStrategiesStore, eventStore }, config, ); + const changeRequestAccessReadMode = createChangeRequestAccessReadModel( + db, + config, + ); const featureToggleService = new FeatureToggleService( { featureStrategiesStore, @@ -99,6 +107,7 @@ export const createFeatureToggleService = ( { getLogger, flagResolver }, segmentService, accessService, + changeRequestAccessReadMode, ); return featureToggleService; }; @@ -134,6 +143,7 @@ export const createFakeFeatureToggleService = ( { segmentStore, featureStrategiesStore, eventStore }, config, ); + const changeRequestAccessReadMode = createFakeChangeRequestAccessService(); const featureToggleService = new FeatureToggleService( { featureStrategiesStore, @@ -148,6 +158,7 @@ export const createFakeFeatureToggleService = ( { getLogger, flagResolver }, segmentService, accessService, + changeRequestAccessReadMode, ); return featureToggleService; }; diff --git a/src/lib/features/index.ts b/src/lib/features/index.ts index 9edecf9be8..b257f4ea44 100644 --- a/src/lib/features/index.ts +++ b/src/lib/features/index.ts @@ -2,3 +2,4 @@ export * from './access/createAccessService'; export * from './export-import-toggles/createExportImportService'; export * from './feature-toggle/createFeatureToggleService'; export * from './project/createProjectService'; +export * from './change-request-access-service/createChangeRequestAccessReadModel'; diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 0a9da5c356..91e491ffd4 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -550,11 +550,4 @@ export class AccessService { await this.validateRoleIsUnique(role.name, existingId); return cleanedRole; } - - async isChangeRequestsEnabled( - project: string, - environment: string, - ): Promise { - return this.store.isChangeRequestsEnabled(project, environment); - } } diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 48fa442b5b..98f8db2bcf 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -83,6 +83,7 @@ import NoAccessError from '../error/no-access-error'; import { IFeatureProjectUserParams } from '../routes/admin-api/project/project-features'; import { unique } from '../util/unique'; import { ISegmentService } from 'lib/segments/segment-service-interface'; +import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model'; interface IFeatureContext { featureName: string; @@ -130,6 +131,8 @@ class FeatureToggleService { private flagResolver: IFlagResolver; + private changeRequestAccessReadModel: IChangeRequestAccessReadModel; + constructor( { featureStrategiesStore, @@ -157,6 +160,7 @@ class FeatureToggleService { }: Pick, segmentService: ISegmentService, accessService: AccessService, + changeRequestAccessReadModel: IChangeRequestAccessReadModel, ) { this.logger = getLogger('services/feature-toggle-service.ts'); this.featureStrategiesStore = featureStrategiesStore; @@ -170,6 +174,7 @@ class FeatureToggleService { this.segmentService = segmentService; this.accessService = accessService; this.flagResolver = flagResolver; + this.changeRequestAccessReadModel = changeRequestAccessReadModel; } async validateFeaturesContext( @@ -1734,18 +1739,13 @@ class FeatureToggleService { environment: string, user?: User, ) { - const [canSkipChangeRequest, changeRequestEnabled] = await Promise.all([ - user - ? this.accessService.hasPermission( - user, - SKIP_CHANGE_REQUEST, - project, - environment, - ) - : Promise.resolve(false), - this.accessService.isChangeRequestsEnabled(project, environment), - ]); - if (changeRequestEnabled && !canSkipChangeRequest) { + const canBypass = + await this.changeRequestAccessReadModel.canBypassChangeRequest( + project, + environment, + user, + ); + if (!canBypass) { throw new NoAccessError(SKIP_CHANGE_REQUEST); } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index ecaa092609..e14de8bf77 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -52,6 +52,10 @@ import { createFakeExportImportTogglesService, } from '../features/export-import-toggles/createExportImportService'; import { Db } from '../db/db'; +import { + createChangeRequestAccessReadModel, + createFakeChangeRequestAccessService, +} from '../features/change-request-access-service/createChangeRequestAccessReadModel'; // TODO: will be moved to scheduler feature directory export const scheduleServices = ( @@ -149,11 +153,15 @@ export const createServices = ( 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 featureToggleServiceV2 = new FeatureToggleService( stores, config, segmentService, accessService, + changeRequestAccessReadModel, ); 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 42aa71a961..157198191c 100644 --- a/src/lib/types/stores/access-store.ts +++ b/src/lib/types/stores/access-store.ts @@ -138,9 +138,4 @@ export interface IAccessStore extends Store { sourceEnvironment: string, destinationEnvironment: string, ): Promise; - - isChangeRequestsEnabled( - project: string, - environment: string, - ): Promise; } diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 85e1184df0..8bec72329f 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -15,6 +15,7 @@ import { ALL_PROJECTS } from '../../../lib/util/constants'; import { SegmentService } from '../../../lib/services/segment-service'; import { GroupService } from '../../../lib/services/group-service'; import { FavoritesService } from '../../../lib/services'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let db: ITestDb; let stores: IUnleashStores; @@ -216,11 +217,16 @@ beforeAll(async () => { editorRole = roles.find((r) => r.name === RoleName.EDITOR); adminRole = roles.find((r) => r.name === RoleName.ADMIN); readRole = roles.find((r) => r.name === RoleName.VIEWER); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); featureToggleService = new FeatureToggleService( stores, config, new SegmentService(stores, config), accessService, + changeRequestAccessReadModel, ); favoritesService = new FavoritesService(stores, config); projectService = new ProjectService( 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 edfcfb96b7..be1c513a25 100644 --- a/src/test/e2e/services/api-token-service.e2e.test.ts +++ b/src/test/e2e/services/api-token-service.e2e.test.ts @@ -11,6 +11,7 @@ import { AccessService } from '../../../lib/services/access-service'; import { SegmentService } from '../../../lib/services/segment-service'; import { GroupService } from '../../../lib/services/group-service'; import { FavoritesService } from '../../../lib/services'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let db; let stores; @@ -26,11 +27,16 @@ beforeAll(async () => { stores = db.stores; const groupService = new GroupService(stores, config); const accessService = new AccessService(stores, config, groupService); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); const featureToggleService = new FeatureToggleService( stores, config, new SegmentService(stores, config), accessService, + changeRequestAccessReadModel, ); 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 5611903b1a..ae8727287f 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 @@ -12,6 +12,7 @@ import EnvironmentService from '../../../lib/services/environment-service'; import { NoAccessError } from '../../../lib/error'; import { SKIP_CHANGE_REQUEST } from '../../../lib/types'; import { ISegmentService } from '../../../lib/segments/segment-service-interface'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let stores; let db; @@ -39,11 +40,16 @@ beforeAll(async () => { 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, + ); service = new FeatureToggleService( stores, config, segmentService, accessService, + changeRequestAccessReadModel, ); }); @@ -384,6 +390,10 @@ test('If change requests are enabled, cannot change variants without going via C unleashConfig, groupService, ); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); // Force all feature flags on to make sure we have Change requests on const customFeatureService = new FeatureToggleService( stores, @@ -395,6 +405,7 @@ test('If change requests are enabled, cannot change variants without going via C }, segmentService, accessService, + changeRequestAccessReadModel, ); const newVariant: IVariant = { @@ -462,6 +473,10 @@ test('If CRs are protected for any environment in the project stops bulk update unleashConfig, groupService, ); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); // Force all feature flags on to make sure we have Change requests on const customFeatureService = new FeatureToggleService( stores, @@ -473,6 +488,7 @@ test('If CRs are protected for any environment in the project stops bulk update }, segmentService, accessService, + changeRequestAccessReadModel, ); const toggle = await service.createFeatureToggle( diff --git a/src/test/e2e/services/playground-service.test.ts b/src/test/e2e/services/playground-service.test.ts index fbe69c50f1..1f9c27c8e4 100644 --- a/src/test/e2e/services/playground-service.test.ts +++ b/src/test/e2e/services/playground-service.test.ts @@ -21,6 +21,7 @@ import { PlaygroundSegmentSchema } from 'lib/openapi/spec/playground-segment-sch import { GroupService } from '../../../lib/services/group-service'; import { AccessService } from '../../../lib/services/access-service'; import { ISegmentService } from '../../../lib/segments/segment-service-interface'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let stores: IUnleashStores; let db: ITestDb; @@ -35,11 +36,16 @@ beforeAll(async () => { 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, + ); featureToggleService = new FeatureToggleService( stores, config, segmentService, accessService, + changeRequestAccessReadModel, ); 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 1ba3551ca0..94386413f8 100644 --- a/src/test/e2e/services/project-health-service.e2e.test.ts +++ b/src/test/e2e/services/project-health-service.e2e.test.ts @@ -10,6 +10,7 @@ import { IUser } from '../../../lib/server-impl'; import { SegmentService } from '../../../lib/services/segment-service'; import { GroupService } from '../../../lib/services/group-service'; import { FavoritesService } from '../../../lib/services'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let stores: IUnleashStores; let db: ITestDb; @@ -31,11 +32,16 @@ beforeAll(async () => { }); groupService = new GroupService(stores, config); accessService = new AccessService(stores, config, groupService); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); featureToggleService = new FeatureToggleService( stores, config, new SegmentService(stores, config), accessService, + changeRequestAccessReadModel, ); favoritesService = new FavoritesService(stores, config); diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index b9f2d8d0f5..91a4d6cf6e 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -14,6 +14,7 @@ import { GroupService } from '../../../lib/services/group-service'; import { FavoritesService } from '../../../lib/services'; import { FeatureEnvironmentEvent } from '../../../lib/types/events'; import { subDays } from 'date-fns'; +import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model'; let stores; let db: ITestDb; @@ -50,11 +51,16 @@ beforeAll(async () => { }); groupService = new GroupService(stores, config); accessService = new AccessService(stores, config, groupService); + const changeRequestAccessReadModel = new ChangeRequestAccessReadModel( + db.rawDatabase, + accessService, + ); featureToggleService = new FeatureToggleService( stores, config, new SegmentService(stores, config), accessService, + changeRequestAccessReadModel, ); favoritesService = new FavoritesService(stores, config); diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index 191051493a..dba3ba1b50 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -1,5 +1,4 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ -import noLoggerProvider from './no-logger'; import { IAccessInfo, IAccessStore, @@ -11,13 +10,6 @@ import { import { IPermission } from 'lib/types/model'; class AccessStoreMock implements IAccessStore { - isChangeRequestsEnabled( - project: string, - environment: string, - ): Promise { - throw new Error('Method not implemented.'); - } - addAccessToProject( users: IAccessInfo[], groups: IAccessInfo[],