From 42b87679bb767b12310df7e18d2291a480d8b4b5 Mon Sep 17 00:00:00 2001
From: Alexandre Bodin <bodin.alex@gmail.com>
Date: Fri, 5 Nov 2021 12:19:49 +0100
Subject: [PATCH] Refactor policies to have a consistent factory API like
 middlewares

---
 .../src/api/address/policies/address.js       |  2 +-
 .../src/api/address/routes/address.js         |  2 +-
 .../src/middlewares/test-middleware.js        |  2 +-
 .../getstarted/src/policies/test-policy.js    | 19 +++-
 .../admin/server/policies/hasPermissions.js   | 30 +++---
 .../server/policies/isAuthenticatedAdmin.js   |  4 +-
 .../__tests__/has-draft-and-publish.test.js   |  8 +-
 .../server/policies/has-draft-and-publish.js  |  2 +-
 .../server/policies/hasPermissions.js         | 16 +--
 .../lib/services/server/compose-endpoint.js   |  3 +-
 .../core/strapi/lib/services/server/policy.js | 14 +--
 packages/core/utils/lib/policy.js             | 99 ++++++++++---------
 .../generators/lib/templates/policy.js.hbs    | 18 ++--
 .../server/services/content-api/policy.js     | 18 ++--
 14 files changed, 127 insertions(+), 110 deletions(-)

diff --git a/examples/getstarted/src/api/address/policies/address.js b/examples/getstarted/src/api/address/policies/address.js
index 3061fe18cd..dddee23d97 100644
--- a/examples/getstarted/src/api/address/policies/address.js
+++ b/examples/getstarted/src/api/address/policies/address.js
@@ -1,3 +1,3 @@
-module.exports = ctx => {
+module.exports = (policyCtx, config, { strapi }) => {
   return true;
 };
diff --git a/examples/getstarted/src/api/address/routes/address.js b/examples/getstarted/src/api/address/routes/address.js
index b433b2dd38..7b50ed63ea 100755
--- a/examples/getstarted/src/api/address/routes/address.js
+++ b/examples/getstarted/src/api/address/routes/address.js
@@ -8,7 +8,7 @@ module.exports = {
       handler: 'address.find',
       config: {
         middlewares: ['api::address.address-middleware'],
-        policies: ['address'],
+        policies: ['global::test-policy', 'api::address.address'],
       },
     },
     {
diff --git a/examples/getstarted/src/middlewares/test-middleware.js b/examples/getstarted/src/middlewares/test-middleware.js
index a4e8aecf8c..d21482355c 100644
--- a/examples/getstarted/src/middlewares/test-middleware.js
+++ b/examples/getstarted/src/middlewares/test-middleware.js
@@ -7,7 +7,7 @@
 module.exports = (config, { strapi }) => {
   // Add your own logic here.
   return async (ctx, next) => {
-    strapi.log.info('In test-middleware middleware.');
+    strapi.log.info('In application test-middleware middleware.');
 
     await next();
   };
diff --git a/examples/getstarted/src/policies/test-policy.js b/examples/getstarted/src/policies/test-policy.js
index 3061fe18cd..819a728708 100644
--- a/examples/getstarted/src/policies/test-policy.js
+++ b/examples/getstarted/src/policies/test-policy.js
@@ -1,3 +1,18 @@
-module.exports = ctx => {
-  return true;
+'use strict';
+
+/**
+ * `test-policy` policy.
+ */
+
+module.exports = (policyCtx, config, { strapi }) => {
+  // Add your own logic here.
+  strapi.log.info('In test-policy policy.');
+
+  const canDoSomething = true;
+
+  if (canDoSomething) {
+    return true;
+  }
+
+  return false;
 };
diff --git a/packages/core/admin/server/policies/hasPermissions.js b/packages/core/admin/server/policies/hasPermissions.js
index 8576f0f48d..29ffb7a2e9 100644
--- a/packages/core/admin/server/policies/hasPermissions.js
+++ b/packages/core/admin/server/policies/hasPermissions.js
@@ -1,9 +1,7 @@
 'use strict';
 
 const _ = require('lodash');
-const {
-  policy: { createPolicyFactory },
-} = require('@strapi/utils');
+const { createPolicy } = require('@strapi/utils').policy;
 const { validateHasPermissionsInput } = require('../validation/policies/hasPermissions');
 
 const inputModifiers = [
@@ -22,28 +20,24 @@ const inputModifiers = [
   },
 ];
 
-module.exports = createPolicyFactory(
-  config => {
+module.exports = createPolicy({
+  name: 'admin::hasPermissions',
+  validator: validateHasPermissionsInput,
+  handler(ctx, config) {
     const { actions } = config;
 
     const permissions = actions.map(action =>
       inputModifiers.find(modifier => modifier.check(action)).transform(action)
     );
 
-    return ctx => {
-      const { userAbility: ability, isAuthenticated } = ctx.state;
+    const { userAbility: ability, isAuthenticated } = ctx.state;
 
-      if (!isAuthenticated || !ability) {
-        return true;
-      }
+    if (!isAuthenticated || !ability) {
+      return true;
+    }
 
-      const isAuthorized = permissions.every(({ action, subject }) => ability.can(action, subject));
+    const isAuthorized = permissions.every(({ action, subject }) => ability.can(action, subject));
 
-      return isAuthorized;
-    };
+    return isAuthorized;
   },
-  {
-    validator: validateHasPermissionsInput,
-    name: 'admin::hasPermissions',
-  }
-);
+});
diff --git a/packages/core/admin/server/policies/isAuthenticatedAdmin.js b/packages/core/admin/server/policies/isAuthenticatedAdmin.js
index 5cb6142107..fbebca70dd 100644
--- a/packages/core/admin/server/policies/isAuthenticatedAdmin.js
+++ b/packages/core/admin/server/policies/isAuthenticatedAdmin.js
@@ -1,5 +1,5 @@
 'use strict';
 
-module.exports = ctx => {
-  return Boolean(ctx.state.isAuthenticated);
+module.exports = policyCtx => {
+  return Boolean(policyCtx.state.isAuthenticated);
 };
diff --git a/packages/core/content-manager/server/policies/__tests__/has-draft-and-publish.test.js b/packages/core/content-manager/server/policies/__tests__/has-draft-and-publish.test.js
index 73887ed415..16242eb20a 100644
--- a/packages/core/content-manager/server/policies/__tests__/has-draft-and-publish.test.js
+++ b/packages/core/content-manager/server/policies/__tests__/has-draft-and-publish.test.js
@@ -29,7 +29,7 @@ describe('hasDraftAndPublish policy', () => {
 
   test('It should succeed when the model has draft & publish enabled', () => {
     const ctx = { params: { model: 'foo' } };
-    const res = hasDraftAndPublish(ctx, { strapi: global.strapi });
+    const res = hasDraftAndPublish(ctx, {}, { strapi: global.strapi });
 
     expect(res).toBe(true);
   });
@@ -37,21 +37,21 @@ describe('hasDraftAndPublish policy', () => {
   test(`It should fail when the model has draft & publish disabled`, () => {
     const ctx = { params: { model: 'bar' } };
 
-    const res = hasDraftAndPublish(ctx, { strapi: global.strapi });
+    const res = hasDraftAndPublish(ctx, {}, { strapi: global.strapi });
     expect(res).toBe(false);
   });
 
   test(`It should fail when the model doesn't exists`, () => {
     const ctx = { params: { model: 'foobar' } };
 
-    const res = hasDraftAndPublish(ctx, { strapi: global.strapi });
+    const res = hasDraftAndPublish(ctx, {}, { strapi: global.strapi });
     expect(res).toBe(false);
   });
 
   test(`It should fail when params.model isn't provided`, () => {
     const ctx = { params: {} };
 
-    const res = hasDraftAndPublish(ctx, { strapi: global.strapi });
+    const res = hasDraftAndPublish(ctx, {}, { strapi: global.strapi });
     expect(res).toBe(false);
   });
 });
diff --git a/packages/core/content-manager/server/policies/has-draft-and-publish.js b/packages/core/content-manager/server/policies/has-draft-and-publish.js
index 580f7a7b2c..8f5303cd46 100644
--- a/packages/core/content-manager/server/policies/has-draft-and-publish.js
+++ b/packages/core/content-manager/server/policies/has-draft-and-publish.js
@@ -4,7 +4,7 @@ const {
   contentTypes: { hasDraftAndPublish },
 } = require('@strapi/utils');
 
-module.exports = (ctx, { strapi }) => {
+module.exports = (ctx, config, { strapi }) => {
   const { model: modelUID } = ctx.params;
 
   const model = strapi.contentTypes[modelUID];
diff --git a/packages/core/content-manager/server/policies/hasPermissions.js b/packages/core/content-manager/server/policies/hasPermissions.js
index f0b66edace..24cd3e2731 100644
--- a/packages/core/content-manager/server/policies/hasPermissions.js
+++ b/packages/core/content-manager/server/policies/hasPermissions.js
@@ -1,12 +1,16 @@
 'use strict';
 
 const {
-  policy: { createPolicyFactory },
+  policy: { createPolicy },
 } = require('@strapi/utils');
 const { validateHasPermissionsInput } = require('../validation/policies/hasPermissions');
 
-module.exports = createPolicyFactory(
-  ({ actions = [], hasAtLeastOne = false } = {}) => ctx => {
+module.exports = createPolicy({
+  name: 'plugin::content-manager.hasPermissions',
+  validator: validateHasPermissionsInput,
+  handler(ctx, config = {}) {
+    const { actions = [], hasAtLeastOne = false } = config;
+
     const {
       state: { userAbility, isAuthenticatedAdmin },
       params: { model },
@@ -22,8 +26,4 @@ module.exports = createPolicyFactory(
 
     return isAuthorized;
   },
-  {
-    validator: validateHasPermissionsInput,
-    name: 'plugin::content-manager.hasPermissions',
-  }
-);
+});
diff --git a/packages/core/strapi/lib/services/server/compose-endpoint.js b/packages/core/strapi/lib/services/server/compose-endpoint.js
index 57bd30848c..419258a20e 100644
--- a/packages/core/strapi/lib/services/server/compose-endpoint.js
+++ b/packages/core/strapi/lib/services/server/compose-endpoint.js
@@ -82,7 +82,8 @@ module.exports = strapi => {
 
       router[method](path, routeHandler);
     } catch (error) {
-      throw new Error(`Error creating endpoint ${route.method} ${route.path}: ${error.message}`);
+      error.message = `Error creating endpoint ${route.method} ${route.path}: ${error.message}`;
+      throw error;
     }
   };
 };
diff --git a/packages/core/strapi/lib/services/server/policy.js b/packages/core/strapi/lib/services/server/policy.js
index ad24cb4a79..300f45b166 100644
--- a/packages/core/strapi/lib/services/server/policy.js
+++ b/packages/core/strapi/lib/services/server/policy.js
@@ -1,24 +1,20 @@
 'use strict';
 
 const { propOr } = require('lodash/fp');
-const policy = require('@strapi/utils/lib/policy');
 const { ForbiddenError } = require('@strapi/utils').errors;
+const { policy: policyUtils } = require('@strapi/utils');
 
 const getPoliciesConfig = propOr([], 'config.policies');
 
 const resolvePolicies = route => {
-  const { pluginName, apiName } = route.info || {};
   const policiesConfig = getPoliciesConfig(route);
-
-  const resolvedPolicies = policiesConfig.map(policyConfig =>
-    policy.get(policyConfig, { pluginName, apiName })
-  );
+  const resolvedPolicies = policyUtils.resolve(policiesConfig, route.info);
 
   const policiesMiddleware = async (ctx, next) => {
-    const context = policy.createPolicyContext('koa', ctx);
+    const context = policyUtils.createPolicyContext('koa', ctx);
 
-    for (const resolvedPolicy of resolvedPolicies) {
-      const result = await resolvedPolicy(context, { strapi });
+    for (const { handler, config } of resolvedPolicies) {
+      const result = await handler(context, config, { strapi });
 
       if (![true, undefined].includes(result)) {
         throw new ForbiddenError('Policies failed.');
diff --git a/packages/core/utils/lib/policy.js b/packages/core/utils/lib/policy.js
index 249e416fe0..5474e9c1ec 100644
--- a/packages/core/utils/lib/policy.js
+++ b/packages/core/utils/lib/policy.js
@@ -9,36 +9,22 @@ const { eq } = require('lodash/fp');
 const PLUGIN_PREFIX = 'plugin::';
 const API_PREFIX = 'api::';
 
-const createPolicy = (policyName, config) => ({ policyName, config });
-
-const resolveHandler = policy => {
-  return _.has('handler', policy) ? policy.handler : policy;
-};
-
 const parsePolicy = policy => {
   if (typeof policy === 'string') {
-    return createPolicy(policy);
+    return { policyName: policy, config: {} };
   }
 
-  const { name, config = {} } = policy;
-  return createPolicy(name, config);
-};
-
-const resolvePolicy = policyName => {
-  const policy = strapi.policy(policyName);
-
-  return resolveHandler(policy);
+  const { name, config } = policy;
+  return { policyName: name, config };
 };
 
 const searchLocalPolicy = (policyName, { pluginName, apiName }) => {
   if (pluginName) {
-    const policy = strapi.policy(`${PLUGIN_PREFIX}${pluginName}.${policyName}`);
-    return resolveHandler(policy);
+    return strapi.policy(`${PLUGIN_PREFIX}${pluginName}.${policyName}`);
   }
 
   if (apiName) {
-    const policy = strapi.policy(`${API_PREFIX}${apiName}.${policyName}`);
-    return resolveHandler(policy);
+    return strapi.policy(`${API_PREFIX}${apiName}.${policyName}`);
   }
 };
 
@@ -56,45 +42,68 @@ const globalPolicy = ({ method, endpoint, controller, action, plugin }) => {
   };
 };
 
-const get = (policy, { pluginName, apiName } = {}) => {
-  if (typeof policy === 'function') {
-    return policy;
-  }
+const resolvePolicies = (config, { pluginName, apiName } = {}) => {
+  return config.map(policyConfig => {
+    return {
+      handler: getPolicy(policyConfig, { pluginName, apiName }),
+      config: policyConfig.config || {},
+    };
+  });
+};
 
-  const { policyName, config } = parsePolicy(policy);
-
-  const resolvedPolicy = resolvePolicy(policyName);
+const findPolicy = (name, { pluginName, apiName } = {}) => {
+  const resolvedPolicy = strapi.policy(name);
 
   if (resolvedPolicy !== undefined) {
-    return _.isPlainObject(policy) ? resolvedPolicy(config) : resolvedPolicy;
+    return resolvedPolicy;
   }
 
-  const localPolicy = searchLocalPolicy(policy, { pluginName, apiName });
+  const localPolicy = searchLocalPolicy(name, { pluginName, apiName });
 
   if (localPolicy !== undefined) {
     return localPolicy;
   }
 
-  throw new Error(`Could not find policy "${policy}"`);
+  throw new Error(`Could not find policy "${name}"`);
 };
 
-const createPolicyFactory = (factoryCallback, options) => {
-  const { validator, name = 'unnamed' } = options;
+const getPolicy = (policyConfig, { pluginName, apiName } = {}) => {
+  if (typeof policyConfig === 'function') {
+    return policyConfig;
+  }
 
-  const validate = (...args) => {
-    try {
-      validator(...args);
-    } catch (e) {
-      throw new Error(`Invalid objects submitted to "${name}" policy.`);
+  const { policyName, config } = parsePolicy(policyConfig);
+
+  const policy = findPolicy(policyName, { pluginName, apiName });
+
+  if (typeof policy === 'function') {
+    return policy;
+  }
+
+  if (policy.validator) {
+    policy.validator(config);
+  }
+
+  return policy.handler;
+};
+
+const createPolicy = options => {
+  const { name = 'unnamed', validator, handler } = options;
+
+  const wrappedValidator = config => {
+    if (validator) {
+      try {
+        validator(config);
+      } catch (e) {
+        throw new Error(`Invalid config passed to "${name}" policy.`);
+      }
     }
   };
 
-  return config => {
-    if (validator) {
-      validate(config);
-    }
-
-    return factoryCallback(config);
+  return {
+    name,
+    validator: wrappedValidator,
+    handler,
   };
 };
 
@@ -102,7 +111,6 @@ const createPolicyContext = (type, ctx) => {
   return Object.assign(
     {
       is: eq(type),
-
       get type() {
         return type;
       },
@@ -112,8 +120,9 @@ const createPolicyContext = (type, ctx) => {
 };
 
 module.exports = {
-  get,
+  get: getPolicy,
+  resolve: resolvePolicies,
   globalPolicy,
-  createPolicyFactory,
+  createPolicy,
   createPolicyContext,
 };
diff --git a/packages/generators/generators/lib/templates/policy.js.hbs b/packages/generators/generators/lib/templates/policy.js.hbs
index 703df2f67b..de4271491d 100644
--- a/packages/generators/generators/lib/templates/policy.js.hbs
+++ b/packages/generators/generators/lib/templates/policy.js.hbs
@@ -4,15 +4,17 @@
  * `{{id}}` policy.
  */
 
-module.exports = async (ctx) => {
-  // Add your own logic here.
-  console.log('In {{id}} policy.');
+module.exports = (config, { strapi }) => {
+  return policyCtx => {
+    // Add your own logic here.
+    strapi.log.info('In {{id}} policy.');
 
-  const canDoSomething = true;
+    const canDoSomething = true;
 
-  if (canDoSomething) {
-    return true;
-  }
+    if (canDoSomething) {
+      return true;
+    }
 
-  return false;
+    return false;
+  };
 };
diff --git a/packages/plugins/graphql/server/services/content-api/policy.js b/packages/plugins/graphql/server/services/content-api/policy.js
index a8e387d5dc..7219bdd7c6 100644
--- a/packages/plugins/graphql/server/services/content-api/policy.js
+++ b/packages/plugins/graphql/server/services/content-api/policy.js
@@ -1,25 +1,25 @@
 'use strict';
 
-const { getOr } = require('lodash/fp');
+const { propOr } = require('lodash/fp');
 const { policy: policyUtils } = require('@strapi/utils');
 const { ForbiddenError } = require('@strapi/utils').errors;
 
+const getPoliciesConfig = propOr([], 'policies');
+
 const createPoliciesMiddleware = (resolverConfig, { strapi }) => {
+  const resolverPolicies = getPoliciesConfig(resolverConfig);
+  const policies = policyUtils.resolve(resolverPolicies);
+
   return async (resolve, ...rest) => {
-    const resolverPolicies = getOr([], 'policies', resolverConfig);
-
-    // Transform every policy into a unique format
-    const policies = resolverPolicies.map(policy => policyUtils.get(policy));
-
     // Create a graphql policy context
     const context = createGraphQLPolicyContext(...rest);
 
     // Run policies & throw an error if one of them fails
-    for (const policy of policies) {
-      const result = await policy(context, { strapi });
+    for (const { handler, config } of policies) {
+      const result = await handler(context, config, { strapi });
 
       if (![true, undefined].includes(result)) {
-        throw new ForbiddenError();
+        throw new ForbiddenError('Policies failed.');
       }
     }