Merge pull request #14164 from strapi/api-token-v2/fix-syncActions

Api token v2/fix sync actions

packages/core/strapi/lib/Strapi.js

@@ -454,7 +454,7 @@ class Strapi {
     await this.runLifecyclesFunctions(LIFECYCLES.BOOTSTRAP);
 
     // TODO: is this the best place for this?
-    await this.contentAPI.permissions.syncActions();
+    await this.contentAPI.permissions.registerActions();
 
     this.cron.start();
 

packages/core/strapi/lib/services/__tests__/content-api-permissions.test.js

@@ -0,0 +1,291 @@
+'use strict';
+
+const createContentAPI = require('../content-api');
+
+describe('Content API - Permissions', () => {
+  const bindToContentAPI = (action) => {
+    Object.assign(action, { [Symbol.for('__type__')]: ['content-api'] });
+    return action;
+  };
+
+  describe('Get Actions Map', () => {
+    test('When no API are defined, it should return an empty object', () => {
+      global.strapi = {};
+
+      const contentAPI = createContentAPI(global.strapi);
+      const actionsMap = contentAPI.permissions.getActionsMap();
+
+      expect(actionsMap).toEqual({});
+    });
+
+    test('When no controller are defined for an API, it should ignore the API', () => {
+      global.strapi = {
+        api: {
+          foo: {},
+          bar: {},
+        },
+      };
+
+      const contentAPI = createContentAPI(global.strapi);
+      const actionsMap = contentAPI.permissions.getActionsMap();
+
+      expect(actionsMap).toEqual({});
+    });
+
+    test(`Do not register controller if they're not bound to the content API`, () => {
+      const actionC = () => {};
+      Object.assign(actionC, { [Symbol.for('__type__')]: ['admin-api'] });
+
+      global.strapi = {
+        api: {
+          foo: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB() {},
+                actionC,
+              },
+            },
+          },
+        },
+      };
+
+      const contentAPI = createContentAPI(global.strapi);
+      const actionsMap = contentAPI.permissions.getActionsMap();
+
+      expect(actionsMap).toEqual({
+        'api::foo': { controllers: { controllerA: ['actionA'] } },
+      });
+    });
+
+    test('Creates and populate a map of actions from APIs and plugins', () => {
+      global.strapi = {
+        api: {
+          foo: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+            },
+          },
+          bar: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+            },
+          },
+          foobar: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+              controllerB: {
+                actionC: bindToContentAPI(() => {}),
+                actionD: bindToContentAPI(() => {}),
+              },
+            },
+          },
+        },
+        plugins: {
+          foo: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+            },
+          },
+          bar: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+            },
+          },
+        },
+      };
+
+      const contentAPI = createContentAPI(global.strapi);
+      const actionsMap = contentAPI.permissions.getActionsMap();
+
+      expect(actionsMap).toEqual({
+        'api::foo': { controllers: { controllerA: ['actionA', 'actionB'] } },
+        'api::bar': { controllers: { controllerA: ['actionA', 'actionB'] } },
+        'api::foobar': {
+          controllers: {
+            controllerA: ['actionA', 'actionB'],
+            controllerB: ['actionC', 'actionD'],
+          },
+        },
+        'plugin::foo': { controllers: { controllerA: ['actionA', 'actionB'] } },
+        'plugin::bar': { controllers: { controllerA: ['actionA', 'actionB'] } },
+      });
+    });
+  });
+
+  describe('Register Actions', () => {
+    beforeEach(() => {
+      global.strapi = {
+        api: {
+          foo: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+                actionB: bindToContentAPI(() => {}),
+              },
+              controllerB: {
+                actionC: bindToContentAPI(() => {}),
+                actionD: bindToContentAPI(() => {}),
+              },
+            },
+          },
+        },
+        plugins: {
+          foo: {
+            controllers: {
+              controllerA: {
+                actionA: bindToContentAPI(() => {}),
+              },
+            },
+          },
+        },
+      };
+    });
+
+    test('The action provider should holds every action from APIs and plugins', async () => {
+      const contentAPI = createContentAPI(global.strapi);
+
+      await contentAPI.permissions.registerActions();
+
+      const values = contentAPI.permissions.providers.action.values();
+
+      expect(values).toEqual([
+        {
+          uid: 'api::foo.controllerA.actionA',
+          api: 'api::foo',
+          controller: 'controllerA',
+          action: 'actionA',
+        },
+        {
+          uid: 'api::foo.controllerA.actionB',
+          api: 'api::foo',
+          controller: 'controllerA',
+          action: 'actionB',
+        },
+        {
+          uid: 'api::foo.controllerB.actionC',
+          api: 'api::foo',
+          controller: 'controllerB',
+          action: 'actionC',
+        },
+        {
+          uid: 'api::foo.controllerB.actionD',
+          api: 'api::foo',
+          controller: 'controllerB',
+          action: 'actionD',
+        },
+        {
+          uid: 'plugin::foo.controllerA.actionA',
+          api: 'plugin::foo',
+          controller: 'controllerA',
+          action: 'actionA',
+        },
+      ]);
+    });
+
+    test('Call registerActions twice should throw a duplicate error', async () => {
+      const contentAPI = createContentAPI(global.strapi);
+
+      await contentAPI.permissions.registerActions();
+
+      expect(() => contentAPI.permissions.registerActions()).rejects.toThrowError(
+        'Duplicated item key: api::foo.controllerA.actionA'
+      );
+    });
+  });
+
+  describe('Providers', () => {
+    test('You should not be able to register action once strapi is loaded', () => {
+      global.strapi.isLoaded = true;
+
+      const contentAPI = createContentAPI(global.strapi);
+
+      // Actions
+      expect(() =>
+        contentAPI.permissions.providers.action.register('foo', {})
+      ).rejects.toThrowError(`You can't register new actions outside the bootstrap function.`);
+
+      // Conditions
+      expect(() =>
+        contentAPI.permissions.providers.condition.register({ name: 'myCondition' })
+      ).rejects.toThrowError(`You can't register new conditions outside the bootstrap function.`);
+
+      // Register Actions
+      expect(() => contentAPI.permissions.registerActions()).rejects.toThrowError(
+        `You can't register new actions outside the bootstrap function.`
+      );
+    });
+  });
+
+  describe('Engine', () => {
+    test('Engine warns when registering an unknown action', async () => {
+      global.strapi = {
+        log: {
+          debug: jest.fn(),
+        },
+      };
+
+      const contentAPI = createContentAPI();
+
+      const ability = await contentAPI.permissions.engine.generateAbility([{ action: 'foo' }]);
+
+      expect(ability.rules).toHaveLength(0);
+      expect(global.strapi.log.debug).toHaveBeenCalledWith(
+        `Unknown action "foo" supplied when registering a new permission`
+      );
+    });
+
+    test('Engine filter out invalid action when generating an ability', async () => {
+      global.strapi = {
+        log: {
+          debug: jest.fn(),
+        },
+
+        api: {
+          foo: {
+            controllers: {
+              bar: { foobar: bindToContentAPI(() => {}) },
+            },
+          },
+        },
+      };
+
+      const contentAPI = createContentAPI(global.strapi);
+
+      await contentAPI.permissions.registerActions();
+
+      const ability = await contentAPI.permissions.engine.generateAbility([
+        { action: 'foo' },
+        { action: 'api::foo.bar.foobar' },
+      ]);
+
+      expect(ability.rules).toHaveLength(1);
+      expect(ability.rules).toEqual([
+        {
+          action: 'api::foo.bar.foobar',
+          subject: 'all',
+        },
+      ]);
+
+      expect(global.strapi.log.debug).toHaveBeenCalledTimes(1);
+      expect(global.strapi.log.debug).toHaveBeenCalledWith(
+        `Unknown action "foo" supplied when registering a new permission`
+      );
+    });
+  });
+});

packages/core/strapi/lib/services/content-api/index.js

@@ -1,65 +1,7 @@
 'use strict';
 
-const { uniq } = require('lodash');
+const _ = require('lodash');
 const permissions = require('./permissions');
-/**
- * Create a content API container that holds logic, tools and utils. (eg: permissions, ...)
- */
-const createContentAPI = (/* strapi */) => {
-  const syncActions = async () => {
-    /**
-     * NOTE: For some reason, this doesn't seem to be necessary because all the routes exist
-     * createActionProvider uses a providerFactory, which seems to already include everything, and when we try
-     * to register our actions we get an error that the keys already exist
-     * Could providerFactory not be providing a new provider, and instead sharing the registry with everything that uses it?
-     *
-     * If this isn't an issue to fix and is expected, we don't need the route registration code below and it should be removed
-     * */
-    // Start of route registration
-    const apiRoutesName = Object.values(strapi.api)
-      .map((api) => api.routes)
-      .reduce((acc, routesMap) => {
-        const routes = Object.values(routesMap)
-          // Only content api routes
-          .filter((p) => p.type === 'content-api')
-          // Resolve every handler name for each route
-          .reduce((a, p) => a.concat(p.routes.map((i) => i.handler)), []);
-        return acc.concat(routes);
-      }, []);
-    const pluginsRoutesname = Object.values(strapi.plugins)
-      .map((plugin) => plugin.routes['content-api'] || {})
-      .map((p) => (p.routes || []).map((i) => i.handler))
-      .flat();
-    const actions = apiRoutesName.concat(pluginsRoutesname);
-    Promise.all(
-      uniq(actions).map((action) =>
-        providers.action.register(action).catch(() => {
-          // console.log('Key already exists', action);
-        })
-      )
-    );
-  };
-  // End of route registration
-
-  // Add providers
-  const providers = {
-    action: permissions.providers.createActionProvider(),
-    condition: permissions.providers.createConditionProvider(),
-  };
-
-  // create permission engine
-  const engine = permissions
-    .createPermissionEngine({ providers })
-    .on('before-format::validate.permission', createValidatePermissionHandler(providers.action));
-
-  return {
-    permissions: {
-      engine,
-      providers,
-      syncActions,
-    },
-  };
-};
 
 /**
  * Creates an handler which check that the permission's action exists in the action registry
@@ -78,4 +20,96 @@ const createValidatePermissionHandler =
     }
   };
 
+/**
+ * Create a content API container that holds logic, tools and utils. (eg: permissions, ...)
+ */
+const createContentAPI = (strapi) => {
+  // Add providers
+  const providers = {
+    action: permissions.providers.createActionProvider(),
+    condition: permissions.providers.createConditionProvider(),
+  };
+
+  const getActionsMap = () => {
+    const actionMap = {};
+
+    const isContentApi = (action) => {
+      if (!_.has(action, Symbol.for('__type__'))) {
+        return false;
+      }
+
+      return action[Symbol.for('__type__')].includes('content-api');
+    };
+
+    const registerAPIsActions = (apis, source) => {
+      _.forEach(apis, (api, apiName) => {
+        const controllers = _.reduce(
+          api.controllers,
+          (acc, controller, controllerName) => {
+            const contentApiActions = _.pickBy(controller, isContentApi);
+
+            if (_.isEmpty(contentApiActions)) {
+              return acc;
+            }
+
+            acc[controllerName] = Object.keys(contentApiActions);
+
+            return acc;
+          },
+          {}
+        );
+
+        if (!_.isEmpty(controllers)) {
+          actionMap[`${source}::${apiName}`] = { controllers };
+        }
+      });
+    };
+
+    registerAPIsActions(strapi.api, 'api');
+    registerAPIsActions(strapi.plugins, 'plugin');
+
+    return actionMap;
+  };
+
+  const registerActions = async () => {
+    const actionsMap = getActionsMap();
+
+    // For each API
+    for (const [api, value] of Object.entries(actionsMap)) {
+      const { controllers } = value;
+
+      // Register controllers methods as actions
+      for (const [controller, actions] of Object.entries(controllers)) {
+        // Register each action individually
+        await Promise.all(
+          actions.map((action) => {
+            const actionUID = `${api}.${controller}.${action}`;
+
+            return providers.action.register(actionUID, {
+              api,
+              controller,
+              action,
+              uid: actionUID,
+            });
+          })
+        );
+      }
+    }
+  };
+
+  // create permission engine
+  const engine = permissions
+    .createPermissionEngine({ providers })
+    .on('before-format::validate.permission', createValidatePermissionHandler(providers.action));
+
+  return {
+    permissions: {
+      engine,
+      providers,
+      registerActions,
+      getActionsMap,
+    },
+  };
+};
+
 module.exports = createContentAPI;

packages/core/strapi/lib/services/content-api/permissions/providers/action.js

@@ -8,12 +8,12 @@ module.exports = (options = {}) => {
   return {
     ...provider,
 
-    async register(action) {
+    async register(action, payload) {
       if (strapi.isLoaded) {
         throw new Error(`You can't register new actions outside the bootstrap function.`);
       }
 
-      return provider.register(action, { name: action });
+      return provider.register(action, payload);
     },
   };
 };

packages/plugins/users-permissions/server/services/users-permissions.js

@@ -171,10 +171,6 @@ module.exports = ({ strapi }) => ({
 
     const toDelete = _.difference(permissionsFoundInDB, allActions);
 
-    // Register actions into the content API action provider
-    // TODO: do this in the content API bootstrap phase instead
-    allActions.forEach((action) => strapi.contentAPI.permissions.providers.action.register(action));
-
     await Promise.all(
       toDelete.map((action) => {
         return strapi.query('plugin::users-permissions.permission').delete({ where: { action } });