From 0b59bd61f6b24fcc44dc6c5e219be2b6c0479cdc Mon Sep 17 00:00:00 2001 From: Alexandre Bodin Date: Thu, 23 Jan 2020 16:59:50 +0100 Subject: [PATCH] Refactor policy util and error handling Signed-off-by: Alexandre Bodin --- .../services/Aggregator.js | 20 +- .../services/Mutation.js | 18 +- .../strapi-plugin-graphql/services/Query.js | 18 +- .../{parse-type.js => parse-type.test.js} | 0 .../strapi-utils/lib/__tests__/policy.test.js | 83 ++++++++ packages/strapi-utils/lib/policy.js | 195 ++++++++---------- packages/strapi/lib/core-api/index.js | 3 +- .../middlewares/router/utils/routerChecker.js | 52 +++-- 8 files changed, 220 insertions(+), 169 deletions(-) rename packages/strapi-utils/lib/__tests__/{parse-type.js => parse-type.test.js} (100%) create mode 100644 packages/strapi-utils/lib/__tests__/policy.test.js diff --git a/packages/strapi-plugin-graphql/services/Aggregator.js b/packages/strapi-plugin-graphql/services/Aggregator.js index 02a44d179e..7006eab1c8 100644 --- a/packages/strapi-plugin-graphql/services/Aggregator.js +++ b/packages/strapi-plugin-graphql/services/Aggregator.js @@ -526,13 +526,19 @@ const formatModelConnectionsGQL = function( }), ]; - policyUtils.get( - 'plugins.users-permissions.permissions', - plugin, - policiesFn, - `GraphQL connection "${name}" `, - name - ); + try { + policiesFn.push( + policyUtils.get( + 'plugins.users-permissions.permissions', + plugin, + name + ) + ); + } catch (error) { + strapi.stopWithError( + `Error building graphql connection "${queryName}": ${error.message}` + ); + } // Execute policies stack. const policy = await compose(policiesFn)(ctx); diff --git a/packages/strapi-plugin-graphql/services/Mutation.js b/packages/strapi-plugin-graphql/services/Mutation.js index cd54a825bc..386c48a751 100644 --- a/packages/strapi-plugin-graphql/services/Mutation.js +++ b/packages/strapi-plugin-graphql/services/Mutation.js @@ -155,15 +155,15 @@ module.exports = { } // Populate policies. - policies.forEach(policy => - policyUtils.get( - policy, - plugin, - policiesFn, - `GraphQL query "${queryName}"`, - name - ) - ); + policies.forEach(policyName => { + try { + policiesFn.push(policyUtils.get(policyName, plugin, name)); + } catch (error) { + strapi.stopWithError( + `Error building graphql mutation "${queryName}": ${error.message}` + ); + } + }); return async (obj, options, graphqlCtx) => { const { context } = graphqlCtx; diff --git a/packages/strapi-plugin-graphql/services/Query.js b/packages/strapi-plugin-graphql/services/Query.js index 51b6b776ac..a3e287079d 100644 --- a/packages/strapi-plugin-graphql/services/Query.js +++ b/packages/strapi-plugin-graphql/services/Query.js @@ -227,15 +227,15 @@ module.exports = { } // Populate policies. - policies.forEach(policy => - policyUtils.get( - policy, - plugin, - policiesFn, - `GraphQL query "${queryName}"`, - name - ) - ); + policies.forEach(policyName => { + try { + policiesFn.push(policyUtils.get(policyName, plugin, name)); + } catch (error) { + strapi.stopWithError( + `Error building graphql query "${queryName}": ${error.message}` + ); + } + }); return async (obj, options = {}, graphqlContext) => { const { context } = graphqlContext; diff --git a/packages/strapi-utils/lib/__tests__/parse-type.js b/packages/strapi-utils/lib/__tests__/parse-type.test.js similarity index 100% rename from packages/strapi-utils/lib/__tests__/parse-type.js rename to packages/strapi-utils/lib/__tests__/parse-type.test.js diff --git a/packages/strapi-utils/lib/__tests__/policy.test.js b/packages/strapi-utils/lib/__tests__/policy.test.js new file mode 100644 index 0000000000..589d85c97d --- /dev/null +++ b/packages/strapi-utils/lib/__tests__/policy.test.js @@ -0,0 +1,83 @@ +'use strict'; + +const policyUtils = require('../policy'); + +describe('Policy util', () => { + describe('Get policy', () => { + test('Throws on policy not found', () => { + expect(() => policyUtils.get('undefined')).toThrow(); + }); + + test('Retrieves global policy', () => { + const policyFn = () => {}; + + // init global strapi + global.strapi = { + config: { + policies: { + 'test-policy': policyFn, + }, + }, + }; + + expect(policyUtils.get('global.test-policy')).toBe(policyFn); + }); + + test('Retrieves a global plugin policy', () => { + const policyFn = () => {}; + + global.strapi = { + plugins: { + 'test-plugin': { + config: { + policies: { + 'test-policy': policyFn, + }, + }, + }, + }, + }; + + expect(() => policyUtils.get('test-plugin.test-policy')).toThrow(); + expect(policyUtils.get('plugins.test-plugin.test-policy')).toBe(policyFn); + }); + + test('Retrieves a plugin policy locally', () => { + const policyFn = () => {}; + + global.strapi = { + plugins: { + 'test-plugin': { + config: { + policies: { + 'test-policy': policyFn, + }, + }, + }, + }, + }; + + expect(policyUtils.get('test-policy', 'test-plugin')).toBe(policyFn); + }); + + test('Retrieves an api policy locally', () => { + const policyFn = () => {}; + + global.strapi = { + api: { + 'test-api': { + config: { + policies: { + 'test-policy': policyFn, + }, + }, + }, + }, + }; + + expect(policyUtils.get('test-policy', undefined, 'test-api')).toBe( + policyFn + ); + }); + }); +}); diff --git a/packages/strapi-utils/lib/policy.js b/packages/strapi-utils/lib/policy.js index 64719bd5a7..b3ec86c70e 100644 --- a/packages/strapi-utils/lib/policy.js +++ b/packages/strapi-utils/lib/policy.js @@ -1,121 +1,90 @@ -// Public dependencies. +/** + * Policies util + */ +'use strict'; + const _ = require('lodash'); -/* eslint-disable prefer-template */ -module.exports = { - get(policy, plugin, policies = [], endpoint, currentApiName) { - // Define global policy prefix. - const globalPolicyPrefix = 'global.'; - const pluginPolicyPrefix = 'plugins.'; - const policySplited = policy.split('.'); - // Looking for global policy or namespaced. - if ( - _.startsWith(policy, globalPolicyPrefix, 0) && - !_.isUndefined( - _.get( - strapi.config.policies, - policy.replace(globalPolicyPrefix, '').toLowerCase() - ) - ) - ) { - // Global policy. - return policies.push( - this.parsePolicy( - _.get( - strapi.config.policies, - policy.replace(globalPolicyPrefix, '').toLowerCase() - ) - ) - ); - } else if ( - _.startsWith(policy, pluginPolicyPrefix, 0) && - strapi.plugins[policySplited[1]] && - !_.isUndefined( - _.get( - strapi.plugins, - policySplited[1] + - '.config.policies.' + - policySplited[2].toLowerCase() - ) - ) - ) { - // Plugin's policies can be used from app APIs with a specific syntax (`plugins.pluginName.policyName`). - return policies.push( - this.parsePolicy( - _.get( - strapi.plugins, - policySplited[1] + - '.config.policies.' + - policySplited[2].toLowerCase() - ) - ) - ); - } else if ( - !_.startsWith(policy, globalPolicyPrefix, 0) && - plugin && - !_.isUndefined( - _.get( - strapi.plugins, - plugin + '.config.policies.' + policy.toLowerCase() - ) - ) - ) { - // Plugin policy used in the plugin itself. - return policies.push( - this.parsePolicy( - _.get( - strapi.plugins, - plugin + '.config.policies.' + policy.toLowerCase() - ) - ) - ); - } else if ( - !_.startsWith(policy, globalPolicyPrefix, 0) && - !_.isUndefined( - _.get( - strapi.api, - currentApiName + '.config.policies.' + policy.toLowerCase() - ) - ) - ) { - // API policy used in the API itself. - return policies.push( - this.parsePolicy( - _.get( - strapi.api, - currentApiName + '.config.policies.' + policy.toLowerCase() - ) - ) - ); - } +const get = (policy, plugin, apiName) => { + if (globalPolicyExists(policy)) { + return parsePolicy(getGlobalPolicy(policy)); + } - strapi.log.error( - `Ignored attempt to bind to ${endpoint} with unknown policy "${policy}"` - ); - }, + if (pluginPolicyExists(policy)) { + return parsePolicy(getPluginPolicy(policy)); + } - parsePolicy(policy) { - if (_.isFunction(policy)) { - return policy; - } + const pluginPolicy = `${PLUGIN_PREFIX}${plugin}.${policy}`; - return policy.handler; - }, + if (!isGlobal(policy) && plugin && pluginPolicyExists(pluginPolicy)) { + return parsePolicy(getPluginPolicy(pluginPolicy)); + } - // Middleware used for every routes. - // Expose the endpoint in `this`. - globalPolicy({ method, endpoint, controller, action, plugin }) { - return async (ctx, next) => { - ctx.request.route = { - endpoint: `${method} ${endpoint}`, - controller: _.toLower(controller), - action: _.toLower(action), - splittedEndpoint: endpoint, - verb: _.toLower(method), - plugin, - }; + const api = _.get(strapi.api, apiName); + if (!isGlobal(policy) && api && policyExistsIn(api, policy)) { + return parsePolicy(getPolicyIn(api, policy)); + } - await next(); - }; - }, + throw new Error(`Could not find policy "${policy}"`); +}; + +const globalPolicy = ({ method, endpoint, controller, action, plugin }) => { + return async (ctx, next) => { + ctx.request.route = { + endpoint: `${method} ${endpoint}`, + controller: _.toLower(controller), + action: _.toLower(action), + splittedEndpoint: endpoint, + verb: _.toLower(method), + plugin, + }; + + await next(); + }; +}; + +const parsePolicy = policy => { + if (_.isFunction(policy)) { + return policy; + } + + return policy.handler; +}; + +const GLOBAL_PREFIX = 'global.'; +const PLUGIN_PREFIX = 'plugins.'; + +const getPolicyIn = (container, policy) => { + return _.get(container, ['config', 'policies', policy.toLowerCase()]); +}; + +const policyExistsIn = (container, policy) => { + return !_.isUndefined(getPolicyIn(container, policy)); +}; + +const isGlobal = policy => _.startsWith(policy, GLOBAL_PREFIX); + +const getGlobalPolicy = policy => { + const strippedPolicy = policy.replace(GLOBAL_PREFIX, ''); + return getPolicyIn(strapi, strippedPolicy); +}; + +const globalPolicyExists = policy => { + return isGlobal(policy) && !_.isUndefined(getGlobalPolicy(policy)); +}; + +const getPluginPolicy = policy => { + const [, plugin = '', policyName = ''] = policy.split('.'); + return getPolicyIn(_.get(strapi, ['plugins', plugin]), policyName); +}; + +const pluginPolicyExists = policy => { + return isPluginPolicy(policy) && !_.isUndefined(getPluginPolicy(policy)); +}; + +const isPluginPolicy = policy => _.startsWith(policy, PLUGIN_PREFIX); + +module.exports = { + get, + globalPolicy, }; diff --git a/packages/strapi/lib/core-api/index.js b/packages/strapi/lib/core-api/index.js index ada487258a..2951f80149 100644 --- a/packages/strapi/lib/core-api/index.js +++ b/packages/strapi/lib/core-api/index.js @@ -25,8 +25,7 @@ function createCoreApi({ api, model }) { const controller = Object.assign( createController({ service, model }), - userController, - { identity: userController.identity || _.upperFirst(modelName) } + userController ); return { diff --git a/packages/strapi/lib/middlewares/router/utils/routerChecker.js b/packages/strapi/lib/middlewares/router/utils/routerChecker.js index 2561eceb3a..b0c5136940 100644 --- a/packages/strapi/lib/middlewares/router/utils/routerChecker.js +++ b/packages/strapi/lib/middlewares/router/utils/routerChecker.js @@ -41,40 +41,34 @@ module.exports = strapi => controller ); - // Init policies array. - const policies = []; - // Add the `globalPolicy`. - policies.push( - policyUtils.globalPolicy({ - controller: controllerKey, - action: actionName, - method, - endpoint, - plugin, - }) - ); + const globalPolicy = policyUtils.globalPolicy({ + controller: controllerKey, + action: actionName, + method, + endpoint, + plugin, + }); + + // Init policies array. + const policies = [globalPolicy]; + + let policyOption = _.get(value, 'config.policies'); // Allow string instead of array of policies. - if ( - !_.isArray(_.get(value, 'config.policies')) && - !_.isEmpty(_.get(value, 'config.policies')) - ) { - value.config.policies = [value.config.policies]; + if (_.isString(policyOption) && !_.isEmpty(policyOption)) { + policyOption = [policyOption]; } - if ( - _.isArray(_.get(value, 'config.policies')) && - !_.isEmpty(_.get(value, 'config.policies')) - ) { - _.forEach(value.config.policies, policy => { - policyUtils.get( - policy, - plugin, - policies, - `${method} ${endpoint}`, - currentApiName - ); + if (_.isArray(policyOption)) { + policyOption.forEach(policyName => { + try { + policies.push(policyUtils.get(policyName, plugin, currentApiName)); + } catch (error) { + strapi.stopWithError( + `Error creating endpoint ${method} ${endpoint}: ${error.message}` + ); + } }); }