From 6b25b91fa413aa1c06382643ed26e848d65e4371 Mon Sep 17 00:00:00 2001 From: Naz Date: Mon, 20 Sep 2021 19:06:16 +0200 Subject: [PATCH] Removed method complexity in settings API controller refs https://github.com/TryGhost/Team/issues/694 refs https://linear.app/tryghost/issue/CORE-13 - The controller code is not meant to contain complex business logic. Removed complexity in settings.edit method - Have separated the regular editing from "Stripe Data" editing to keep the dependency on the members service still in the controller reducing coupling of the settings BREAD service to the minimum. - The stripeConnectData passed into `edit` method still feels out of place (maybe it should be passed as an array already that's ready to be merged with the rest of settings, but that was left for another refactor in the future) --- core/server/api/canary/settings.js | 74 ++------------ core/server/services/settings/index.js | 1 + .../settings/settings-bread-service.js | 96 ++++++++++++++++++- 3 files changed, 104 insertions(+), 67 deletions(-) diff --git a/core/server/api/canary/settings.js b/core/server/api/canary/settings.js index 792ca10515..3a9607ed8b 100644 --- a/core/server/api/canary/settings.js +++ b/core/server/api/canary/settings.js @@ -5,7 +5,7 @@ const models = require('../../models'); const routeSettings = require('../../services/route-settings'); const frontendSettings = require('../../../frontend/services/settings'); const i18n = require('../../../shared/i18n'); -const {BadRequestError, NoPermissionError, NotFoundError} = require('@tryghost/errors'); +const {BadRequestError, NoPermissionError} = require('@tryghost/errors'); const settingsService = require('../../services/settings'); const settingsCache = require('../../../shared/settings-cache'); const membersService = require('../../services/members'); @@ -208,76 +208,20 @@ module.exports = { } }, async query(frame) { + let stripeConnectData; const stripeConnectIntegrationToken = frame.data.settings.find(setting => setting.key === 'stripe_connect_integration_token'); - const settings = frame.data.settings.filter((setting) => { - // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. - return ![ - 'stripe_connect_integration_token', - 'stripe_connect_publishable_key', - 'stripe_connect_secret_key', - 'stripe_connect_livemode', - 'stripe_connect_account_id', - 'stripe_connect_display_name' - ].includes(setting.key) - // Remove obfuscated settings - && !(setting.value === settingsService.obfuscatedSetting && settingsService.isSecretSetting(setting)); - }); - - const getSetting = setting => settingsCache.get(setting.key, {resolve: false}); - - const firstUnknownSetting = settings.find(setting => !getSetting(setting)); - - if (firstUnknownSetting) { - throw new NotFoundError({ - message: i18n.t('errors.api.settings.problemFindingSetting', { - key: firstUnknownSetting.key - }) - }); - } - - if (!(frame.options.context && frame.options.context.internal)) { - const firstCoreSetting = settings.find(setting => getSetting(setting).group === 'core'); - if (firstCoreSetting) { - throw new NoPermissionError({ - message: i18n.t('errors.api.settings.accessCoreSettingFromExtReq') - }); - } - } - if (stripeConnectIntegrationToken && stripeConnectIntegrationToken.value) { const getSessionProp = prop => frame.original.session[prop]; - try { - const data = await membersService.stripeConnect.getStripeConnectTokenData(stripeConnectIntegrationToken.value, getSessionProp); - settings.push({ - key: 'stripe_connect_publishable_key', - value: data.public_key - }); - settings.push({ - key: 'stripe_connect_secret_key', - value: data.secret_key - }); - settings.push({ - key: 'stripe_connect_livemode', - value: data.livemode - }); - settings.push({ - key: 'stripe_connect_display_name', - value: data.display_name - }); - settings.push({ - key: 'stripe_connect_account_id', - value: data.account_id - }); - } catch (err) { - throw new BadRequestError({ - err, - message: 'The Stripe Connect token could not be parsed.' - }); - } + + stripeConnectData = await settingsBREADService.getStripeConnectData( + stripeConnectIntegrationToken, + getSessionProp, + membersService.stripeConnect.getStripeConnectTokenData + ); } - return models.Settings.edit(settings, frame.options); + return await settingsBREADService.edit(frame.data.settings, frame.options, stripeConnectData); } }, diff --git a/core/server/services/settings/index.js b/core/server/services/settings/index.js index abf0d08281..1bb7902521 100644 --- a/core/server/services/settings/index.js +++ b/core/server/services/settings/index.js @@ -29,6 +29,7 @@ function hideValueIfSecret(setting) { */ const getSettingsBREADServiceInstance = () => { return new SettingsBREADService({ + SettingsModel: models.Settings, settingsCache: SettingsCache }); }; diff --git a/core/server/services/settings/settings-bread-service.js b/core/server/services/settings/settings-bread-service.js index 954c00fbe2..80b1f076cc 100644 --- a/core/server/services/settings/settings-bread-service.js +++ b/core/server/services/settings/settings-bread-service.js @@ -1,5 +1,5 @@ const tpl = require('@tryghost/tpl'); -const {NotFoundError, NoPermissionError} = require('@tryghost/errors'); +const {NotFoundError, NoPermissionError, BadRequestError} = require('@tryghost/errors'); const messages = { problemFindingSetting: 'Problem finding setting: {key}', @@ -26,9 +26,11 @@ class SettingsBREADService { /** * * @param {Object} options + * @param {Object} options.SettingsModel * @param {Object} options.settingsCache - SettingsCache instance */ - constructor({settingsCache}) { + constructor({SettingsModel, settingsCache}) { + this.SettingsModel = SettingsModel; this.settingsCache = settingsCache; } @@ -76,6 +78,96 @@ class SettingsBREADService { [key]: setting }; } + + /** + * + * @param {Object[]} settings + * @param {Object} options + * @param {Object} [options.context] + * @param {Object} [stripeConnectData] + * @returns + */ + async edit(settings, options, stripeConnectData) { + const filteredSettings = settings.filter((setting) => { + // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. + return ![ + 'stripe_connect_integration_token', + 'stripe_connect_publishable_key', + 'stripe_connect_secret_key', + 'stripe_connect_livemode', + 'stripe_connect_account_id', + 'stripe_connect_display_name' + ].includes(setting.key) + // Remove obfuscated settings + && !(setting.value === obfuscatedSetting && isSecretSetting(setting)); + }); + + const getSetting = setting => this.settingsCache.get(setting.key, {resolve: false}); + + const firstUnknownSetting = filteredSettings.find(setting => !getSetting(setting)); + + if (firstUnknownSetting) { + throw new NotFoundError({ + message: tpl(messages.problemFindingSetting, { + key: firstUnknownSetting.key + }) + }); + } + + if (!(options.context && options.context.internal)) { + const firstCoreSetting = filteredSettings.find(setting => getSetting(setting).group === 'core'); + if (firstCoreSetting) { + throw new NoPermissionError({ + message: tpl(messages.accessCoreSettingFromExtReq) + }); + } + } + + if (stripeConnectData) { + filteredSettings.push({ + key: 'stripe_connect_publishable_key', + value: stripeConnectData.public_key + }); + filteredSettings.push({ + key: 'stripe_connect_secret_key', + value: stripeConnectData.secret_key + }); + filteredSettings.push({ + key: 'stripe_connect_livemode', + value: stripeConnectData.livemode + }); + filteredSettings.push({ + key: 'stripe_connect_display_name', + value: stripeConnectData.display_name + }); + filteredSettings.push({ + key: 'stripe_connect_account_id', + value: stripeConnectData.account_id + }); + } + + return this.SettingsModel.edit(filteredSettings, options); + } + + /** + * + * @param {Object} stripeConnectIntegrationToken + * @param {Function} getSessionProp sync function fetching property from session store + * @param {Function} getStripeConnectTokenData async function retreiving Stripe Connect data for settings + * @returns {Promise} resolves with an object with following keys: public_key, secret_key, livemode, display_name, account_id + */ + async getStripeConnectData(stripeConnectIntegrationToken, getSessionProp, getStripeConnectTokenData) { + if (stripeConnectIntegrationToken && stripeConnectIntegrationToken.value) { + try { + return await getStripeConnectTokenData(stripeConnectIntegrationToken.value, getSessionProp); + } catch (err) { + throw new BadRequestError({ + err, + message: 'The Stripe Connect token could not be parsed.' + }); + } + } + } } module.exports = SettingsBREADService;