Skip to content

Commit

Permalink
chore: move enterprise check further left, prevent OSS from seeing CR…
Browse files Browse the repository at this point in the history 
… usage (#5431)

This PR checks that the unleash instance is an enterprise instance
before fetching change request data. This is to prevent Change Request
usage from preventing OSS users from deleting segments (when they don't
have access to change requests).

This PR also does a little bit of refactoring (which we can remove if
you want)
  • Loading branch information
@thomasheartman
thomasheartman authored Nov 27, 2023
1 parent c0369b7 commit 1a75432
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 74 deletions.
47 changes: 19 additions & 28 deletions src/lib/features/segment/segment-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,39 +354,30 @@ export class SegmentsController extends Controller {
user.id,
);

if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) {
const mapStrategies = (strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
});
const segmentStrategies = strategies.strategies.map((strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
}));

const mapChangeRequestStrategies = (strategy) => ({
...(strategy.id ? { id: strategy.id } : {}),
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
changeRequest: strategy.changeRequest,
});
if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) {
const changeRequestStrategies =
strategies.changeRequestStrategies.map((strategy) => ({
...('id' in strategy ? { id: strategy.id } : {}),
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
changeRequest: strategy.changeRequest,
}));

res.json({
strategies: strategies.strategies.map(mapStrategies),
changeRequestStrategies: strategies.changeRequestStrategies.map(
mapChangeRequestStrategies,
),
strategies: segmentStrategies,
changeRequestStrategies,
});
} else {
const segmentStrategies = strategies.strategies.map((strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
}));

res.json({ strategies: segmentStrategies });
}
}
Expand Down
17 changes: 12 additions & 5 deletions src/lib/services/segment-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,19 @@ export class SegmentService implements ISegmentService {
const strategies =
await this.featureStrategiesStore.getStrategiesBySegment(id);

const changeRequestStrategies =
await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests(
id,
);
if (
this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests') &&
this.config.isEnterprise
) {
const changeRequestStrategies =
await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests(
id,
);

return { strategies, changeRequestStrategies };
}

return { strategies, changeRequestStrategies };
return { strategies, changeRequestStrategies: [] };
}

async isInUse(id: number): Promise<boolean> {
Expand Down
119 changes: 78 additions & 41 deletions src/test/e2e/api/admin/segment.e2e.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,43 @@ describe('detect strategy usage in change requests', () => {
const CR_ID = 54321;
let user;

// Change request data is only counted for enterprise
// instances, so we'll instantiate our own version of the app
// for that.
let enterpriseApp;

// likewise, we want to fetch from the right app to make sure
// we get the right data
const enterpriseFetchSegments = () =>
enterpriseApp.request
.get(SEGMENTS_BASE_PATH)
.expect(200)
.then((res) => res.body.segments);

const enterpriseFetchSegmentStrategies = (
segmentId: number,
): Promise<StrategiesUsingSegment> =>
enterpriseApp.request
.get(`${SEGMENTS_BASE_PATH}/${segmentId}/strategies`)
.expect(200)
.then((res) => res.body);

beforeAll(async () => {
enterpriseApp = await setupAppWithCustomConfig(
db.stores,
{
enterpriseVersion: '5.3.0',
ui: { environment: 'Enterprise' },
isEnterprise: true,
experimental: {
flags: {
detectSegmentUsageInChangeRequests: true,
},
},
},
db.rawDatabase,
);

user = await db.stores.userStore.insert({
username: 'test',
});
Expand Down Expand Up @@ -463,8 +499,8 @@ describe('detect strategy usage in change requests', () => {
test('should not delete segments used by strategies in CRs', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await createFeatureToggle(enterpriseApp, toggle);
const [segment] = await enterpriseFetchSegments();

await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
Expand All @@ -487,20 +523,25 @@ describe('detect strategy usage in change requests', () => {
created_by: user.id,
});

expect((await fetchSegments()).length).toEqual(1);
expect((await enterpriseFetchSegments()).length).toEqual(1);

await app.request
await enterpriseApp.request
.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`)
.expect(409);

expect((await fetchSegments()).length).toEqual(1);
expect((await enterpriseFetchSegments()).length).toEqual(1);

// check that it can be deleted in OSS
await app.request
.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`)
.expect(204);
});

test('Should show segment usage in addStrategy events', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await createFeatureToggle(enterpriseApp, toggle);
const [segment] = await enterpriseFetchSegments();

await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
Expand All @@ -524,7 +565,7 @@ describe('detect strategy usage in change requests', () => {
});

const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
await enterpriseFetchSegmentStrategies(segment.id);

expect(changeRequestStrategies).toMatchObject([
{
Expand All @@ -536,16 +577,21 @@ describe('detect strategy usage in change requests', () => {
},
]);
expect(strategies).toStrictEqual([]);

// check that OSS gets no CR strategies
const ossResult = await fetchSegmentStrategies(segment.id);
expect(ossResult.strategies).toStrictEqual([]);
expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]);
});

test('Should show segment usage in updateStrategy events', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await createFeatureToggle(enterpriseApp, toggle);
const [segment] = await enterpriseFetchSegments();

await addStrategyToFeatureEnv(
app,
enterpriseApp,
{ ...toggle.strategies[0] },
'default',
toggle.name,
Expand Down Expand Up @@ -578,7 +624,7 @@ describe('detect strategy usage in change requests', () => {
});

const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
await enterpriseFetchSegmentStrategies(segment.id);

expect(changeRequestStrategies).toMatchObject([
{
Expand All @@ -587,16 +633,21 @@ describe('detect strategy usage in change requests', () => {
},
]);
expect(strategies).toStrictEqual([]);

// check that OSS gets no CR strategies
const ossResult = await fetchSegmentStrategies(segment.id);
expect(ossResult.strategies).toStrictEqual([]);
expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]);
});

test('If a segment is used in an existing strategy and in a CR for the same strategy, the strategy should be listed both places', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await createFeatureToggle(enterpriseApp, toggle);
const [segment] = await enterpriseFetchSegments();

await addStrategyToFeatureEnv(
app,
enterpriseApp,
{ ...toggle.strategies[0] },
'default',
toggle.name,
Expand Down Expand Up @@ -630,39 +681,19 @@ describe('detect strategy usage in change requests', () => {
});

const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
await enterpriseFetchSegmentStrategies(segment.id);

expect(strategies).toMatchObject([{ id: strategyId }]);

expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]);

// check that OSS gets no CR strategies
const ossResult = await fetchSegmentStrategies(segment.id);
expect(ossResult.strategies).toMatchObject([{ id: strategyId }]);
expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]);
});

test('Should show usage in features and projects in CRs', async () => {
// Change request data is only counted for enterprise
// instances, so we'll instantiate our own version of the app
// for that.
const enterpriseApp = await setupAppWithCustomConfig(
db.stores,
{
enterpriseVersion: '5.3.0',
ui: { environment: 'Enterprise' },
experimental: {
flags: {
detectSegmentUsageInChangeRequests: true,
},
},
},
db.rawDatabase,
);

// likewise, we want to fetch from the right app to make sure
// we get the right data
const enterpriseFetchSegments = () =>
enterpriseApp.request
.get(SEGMENTS_BASE_PATH)
.expect(200)
.then((res) => res.body.segments);

// because they use the same db, we can use the regular app
// (through `createSegment` and `createFeatureToggle`) to
// create the segment and the flag
Expand Down Expand Up @@ -698,5 +729,11 @@ describe('detect strategy usage in change requests', () => {
expect(segments).toMatchObject([
{ usedInFeatures: 1, usedInProjects: 1 },
]);

// check that OSS gets no CR usage
const ossSegments = await fetchSegments();
expect(ossSegments).toMatchObject([
{ usedInFeatures: 0, usedInProjects: 0 },
]);
});
});

0 comments on commit 1a75432

Please sign in to comment.