From 4af2e0a55265580cb5dc527868fa284fe56a7574 Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Tue, 29 Oct 2024 17:59:58 -0700 Subject: [PATCH] Fixes create/update duplicate Switcher key - allow moving Switcher --- src/routers/config.js | 6 ++++-- src/services/config.js | 44 ++++++++++++++++++++++++-------------- tests/config.test.js | 40 ++++++++++++++++++++++++++++++++-- tests/fixtures/db_api.js | 11 ++++++++++ tests/group-config.test.js | 6 +++--- 5 files changed, 84 insertions(+), 23 deletions(-) diff --git a/src/routers/config.js b/src/routers/config.js index e72b49c..e063498 100644 --- a/src/routers/config.js +++ b/src/routers/config.js @@ -129,9 +129,11 @@ router.delete('/config/:id', auth, [ }); router.patch('/config/:id', auth, [ - check('id').isMongoId() + check('id').isMongoId(), + check('key').optional().isLength({ min: 3, max: 50 }), + check('group').optional().isMongoId(), ], validate, verifyInputUpdateParameters([ - 'key', 'description', 'relay', 'disable_metrics' + 'key', 'description', 'group', 'relay', 'disable_metrics' ]), async (req, res) => { try { const config = await Services.updateConfig(req.params.id, req.body, req.admin); diff --git a/src/services/config.js b/src/services/config.js index a574f85..1ee30b3 100644 --- a/src/services/config.js +++ b/src/services/config.js @@ -59,6 +59,8 @@ export async function populateAdmin(configs) { } export async function createConfig(args, admin) { + args.key = formatInput(args.key, { toUpper: true, autoUnderscore: true }); + // validates account plan permissions const group = await getGroupConfigById(args.group); await checkSwitcher(group); @@ -70,7 +72,7 @@ export async function createConfig(args, admin) { }); if (config) { - throw new BadRequestError(`Config ${args.key} already exists`); + throw new BadRequestError(`Config ${config.key} already exists`); } config = new Config({ @@ -79,8 +81,6 @@ export async function createConfig(args, admin) { owner: admin._id }); - config.key = formatInput(config.key, { toUpper: true, autoUnderscore: true }); - // validates permissions config = await verifyOwnership(admin, config, group.domain, ActionTypes.CREATE, RouterTypes.CONFIG); @@ -112,19 +112,12 @@ export async function updateConfig(id, args, admin) { // validates existing switcher key if (args.key) { - const duplicatedKey = await getConfig({ - key: args.key, - group: config.group, - domain: config.domain - }); - - if (duplicatedKey) { - throw new BadRequestError(`Config ${args.key} already exists`); - } + args = await updateConfigKey(args, config); + } - // resets permission cache - permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.group); - permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.key); + // validates existing group + if (args.group) { + await getGroupConfigById(args.group); } // validates existing environment @@ -139,13 +132,32 @@ export async function updateConfig(id, args, admin) { config.updatedBy = admin.email; const updates = Object.keys(args); updates.forEach((update) => config[update] = args[update]); - config.key = formatInput(config.key, { toUpper: true, autoUnderscore: true }); + await config.save(); updateDomainVersion(config.domain); return config; } +async function updateConfigKey(args, config) { + args.key = formatInput(args.key, { toUpper: true, autoUnderscore: true }); + + const duplicatedKey = await getConfig({ + key: args.key, + domain: config.domain + }); + + if (duplicatedKey) { + throw new BadRequestError(`Config ${duplicatedKey.key} already exists`); + } + + // resets permission cache + permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.group); + permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.key); + + return args; +} + export async function updateConfigRelay(id, args, admin) { let config = await getConfigById(id); isRelayValid(args); diff --git a/tests/config.test.js b/tests/config.test.js index 45d28f0..e962e23 100644 --- a/tests/config.test.js +++ b/tests/config.test.js @@ -15,6 +15,7 @@ import { adminMasterAccountToken, domainId, groupConfigId, + groupConfigId2, configId1, config1Document, configId2, @@ -52,7 +53,7 @@ describe('Testing configuration insertion', () => { .post('/config/create') .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send({ - key: 'NEW_CONFIG', + key: 'new_config', // case insensitive - it will be converted to uppercase description: 'Description of my new Config', group: groupConfigId }).expect(400); @@ -277,7 +278,7 @@ describe('Testing update info', () => { const response = await request(app) .patch('/config/' + configId1) .set('Authorization', `Bearer ${adminMasterAccountToken}`) - .send({ key: 'NEWKEY' }).expect(400); + .send({ key: 'newkey' }).expect(400); expect(response.body.error).toEqual('Config NEWKEY already exists'); }); @@ -324,6 +325,41 @@ describe('Testing update info', () => { }); }); +describe('Testing moving Config from GroupConfig to another', () => { + beforeAll(setupDatabase); + + test('CONFIG_SUITE - Should move Config to a different GroupConfig', async () => { + await request(app) + .patch('/config/' + configId1) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send({ + group: groupConfigId2 + }).expect(200); + + // DB validation - verify flag updated + const config = await Config.findById(configId1).lean().exec(); + expect(config).not.toBeNull(); + expect(config.group).toEqual(groupConfigId2); + }); + + test('CONFIG_SUITE - Should NOT move Config to a different GroupConfig - GroupConfig not found', async () => { + await request(app) + .patch('/config/' + configId1) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send({ + group: new mongoose.Types.ObjectId() + }).expect(404); + + await request(app) + .patch('/config/' + configId1) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send({ + group: 'INVALID_VALUE_ID' + }).expect(422); + }); + +}); + describe('Testing Environment status change', () => { beforeAll(setupDatabase); diff --git a/tests/fixtures/db_api.js b/tests/fixtures/db_api.js index 7a5cd14..3227ed6 100644 --- a/tests/fixtures/db_api.js +++ b/tests/fixtures/db_api.js @@ -122,6 +122,16 @@ export const groupConfigDocument = { domain: domainId }; +export const groupConfigId2 = new mongoose.Types.ObjectId(); +export const groupConfig2Document = { + _id: groupConfigId2, + name: 'Group Test 2', + description: 'Test Group 2', + activated: new Map().set(EnvType.DEFAULT, true), + owner: adminMasterAccountId, + domain: domainId +}; + export const configId1 = new mongoose.Types.ObjectId(); export const config1Document = { _id: configId1, @@ -265,6 +275,7 @@ export const setupDatabase = async () => { await new Environment(environment3).save(); await new Domain(domainDocument).save(); await new GroupConfig(groupConfigDocument).save(); + await new GroupConfig(groupConfig2Document).save(); await new Config(config1Document).save(); await new Config(config2Document).save(); await new ConfigStrategy(configStrategyDocument).save(); diff --git a/tests/group-config.test.js b/tests/group-config.test.js index 999061a..4abb369 100644 --- a/tests/group-config.test.js +++ b/tests/group-config.test.js @@ -92,7 +92,7 @@ describe('Testing fetch Group info', () => { .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send().expect(200); - expect(response.body.length).toEqual(1); + expect(response.body.length).toBeGreaterThan(0); expect(String(response.body[0]._id)).toEqual(String(groupConfigDocument._id)); expect(response.body[0].name).toEqual(groupConfigDocument.name); @@ -119,7 +119,7 @@ describe('Testing fetch Group info', () => { .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send().expect(200); - expect(response.body.length).toEqual(2); + expect(response.body.length).toBeGreaterThan(1); // Query filter tests response = await request(app) @@ -134,7 +134,7 @@ describe('Testing fetch Group info', () => { .set('Authorization', `Bearer ${adminMasterAccountToken}`) .send().expect(200); - expect(response.body.length).toEqual(2); + expect(response.body.length).toBeGreaterThan(1); }); test('GROUP_SUITE - Should get Group Config information - only fields (name, activated.default)', async () => {