From 48d36b6a48efad0ad65ca6931e6b934b8a5d95dd Mon Sep 17 00:00:00 2001 From: Naz Date: Mon, 7 Jun 2021 17:56:52 +0400 Subject: [PATCH] Disallowed aditing "labs" settings in v2/v3 APIs refs https://github.com/TryGhost/Team/issues/757 - There is no usecase for editing "labs" settings outside of canary/v4 API versions. Removing support for older versions makes the supported API surface smaller (easy maintenance). --- .../v2/utils/serializers/input/settings.js | 19 +++---------------- .../v3/utils/serializers/input/settings.js | 19 +++---------------- test/regression/api/v2/admin/settings_spec.js | 10 ++-------- test/regression/api/v3/admin/settings_spec.js | 10 ++-------- 4 files changed, 10 insertions(+), 48 deletions(-) diff --git a/core/server/api/v2/utils/serializers/input/settings.js b/core/server/api/v2/utils/serializers/input/settings.js index 5a0a9f8477..9b9453e72a 100644 --- a/core/server/api/v2/utils/serializers/input/settings.js +++ b/core/server/api/v2/utils/serializers/input/settings.js @@ -2,7 +2,6 @@ const _ = require('lodash'); const url = require('./utils/url'); const typeGroupMapper = require('../../../../shared/serializers/input/utils/settings-filter-type-group-mapper'); const settingsCache = require('../../../../../services/settings/cache'); -const {WRITABLE_KEYS_ALLOWLIST} = require('../../../../../services/labs'); const DEPRECATED_SETTINGS = [ 'bulk_email_settings', @@ -92,11 +91,12 @@ module.exports = { const settings = settingsCache.getAll(); - // Ignore and drop all values with Read-only flag frame.data.settings = frame.data.settings.filter((setting) => { const settingFlagsStr = settings[setting.key] ? settings[setting.key].flags : ''; const settingFlagsArr = settingFlagsStr ? settingFlagsStr.split(',') : []; - return !settingFlagsArr.includes('RO'); + + // Ignore and drop all values with Read-only flag AND 'labs' setting + return !settingFlagsArr.includes('RO') && (setting.key !== 'labs'); }); frame.data.settings.push(...getMappedDeprecatedSettings(frame.data.settings)); @@ -139,19 +139,6 @@ module.exports = { setting.value = JSON.parse(setting.value).isActive; } - if (setting.key === 'labs') { - const inputLabsValue = JSON.parse(setting.value); - const filteredLabsValue = {}; - - for (const value in inputLabsValue) { - if (WRITABLE_KEYS_ALLOWLIST.includes(value)) { - filteredLabsValue[value] = inputLabsValue[value]; - } - } - - setting.value = JSON.stringify(filteredLabsValue); - } - setting = url.forSetting(setting); }); diff --git a/core/server/api/v3/utils/serializers/input/settings.js b/core/server/api/v3/utils/serializers/input/settings.js index 3a19fe195c..ee8e307d64 100644 --- a/core/server/api/v3/utils/serializers/input/settings.js +++ b/core/server/api/v3/utils/serializers/input/settings.js @@ -2,7 +2,6 @@ const _ = require('lodash'); const url = require('./utils/url'); const typeGroupMapper = require('../../../../shared/serializers/input/utils/settings-filter-type-group-mapper'); const settingsCache = require('../../../../../services/settings/cache'); -const {WRITABLE_KEYS_ALLOWLIST} = require('../../../../../services/labs'); const DEPRECATED_SETTINGS = [ 'bulk_email_settings', @@ -95,11 +94,12 @@ module.exports = { } const settings = settingsCache.getAll(); - // Ignore and drop all values with Read-only flag frame.data.settings = frame.data.settings.filter((setting) => { const settingFlagsStr = settings[setting.key] ? settings[setting.key].flags : ''; const settingFlagsArr = settingFlagsStr ? settingFlagsStr.split(',') : []; - return !settingFlagsArr.includes('RO'); + + // Ignore and drop all values with Read-only flag AND 'labs' setting + return !settingFlagsArr.includes('RO') && (setting.key !== 'labs'); }); const mappedDeprecatedSettings = getMappedDeprecatedSettings(frame.data.settings); @@ -155,19 +155,6 @@ module.exports = { setting.value = JSON.parse(setting.value).isActive; } - if (setting.key === 'labs') { - const inputLabsValue = JSON.parse(setting.value); - const filteredLabsValue = {}; - - for (const value in inputLabsValue) { - if (WRITABLE_KEYS_ALLOWLIST.includes(value)) { - filteredLabsValue[value] = inputLabsValue[value]; - } - } - - setting.value = JSON.stringify(filteredLabsValue); - } - setting = url.forSetting(setting); }); diff --git a/test/regression/api/v2/admin/settings_spec.js b/test/regression/api/v2/admin/settings_spec.js index d936d164a9..9c9319cf50 100644 --- a/test/regression/api/v2/admin/settings_spec.js +++ b/test/regression/api/v2/admin/settings_spec.js @@ -521,7 +521,7 @@ describe('Settings API (v2)', function () { }); }); - it('Can edit only allowed labs keys', async function () { + it('Cannot edit labs keys', async function () { const settingToChange = { settings: [{ key: 'labs', @@ -544,13 +544,7 @@ describe('Settings API (v2)', function () { should.exist(jsonResponse); should.exist(jsonResponse.settings); - jsonResponse.settings.length.should.eql(1); - testUtils.API.checkResponseValue(jsonResponse.settings[0], ['id', 'key', 'value', 'type', 'flags', 'created_at', 'updated_at']); - jsonResponse.settings[0].key.should.eql('labs'); - - jsonResponse.settings[0].value.should.eql(JSON.stringify({ - activitypub: true - })); + jsonResponse.settings.length.should.eql(0); }); it('Can\'t edit non existent setting', function () { diff --git a/test/regression/api/v3/admin/settings_spec.js b/test/regression/api/v3/admin/settings_spec.js index 2fa618899d..632a238a79 100644 --- a/test/regression/api/v3/admin/settings_spec.js +++ b/test/regression/api/v3/admin/settings_spec.js @@ -464,7 +464,7 @@ describe('Settings API (v3)', function () { }); }); - it('Can edit only allowed labs keys', async function () { + it('Cannot edit labs keys', async function () { const settingToChange = { settings: [{ key: 'labs', @@ -487,13 +487,7 @@ describe('Settings API (v3)', function () { should.exist(jsonResponse); should.exist(jsonResponse.settings); - jsonResponse.settings.length.should.eql(1); - testUtils.API.checkResponseValue(jsonResponse.settings[0], ['id', 'group', 'key', 'value', 'type', 'flags', 'created_at', 'updated_at']); - jsonResponse.settings[0].key.should.eql('labs'); - - jsonResponse.settings[0].value.should.eql(JSON.stringify({ - activitypub: true - })); + jsonResponse.settings.length.should.eql(0); }); it('Can\'t read non existent setting', function (done) {