From 06d95419f47b1b8c5c4ac15c4024b9f971858faa Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Thu, 18 Aug 2022 13:31:02 +0200 Subject: [PATCH 01/16] add regeneration of tokens --- .../admin/server/controllers/api-token.js | 15 +++++++- .../core/admin/server/routes/api-tokens.js | 11 ++++++ .../services/__tests__/api-token.test.js | 35 +++++++++++++++++++ .../core/admin/server/services/api-token.js | 22 ++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/packages/core/admin/server/controllers/api-token.js b/packages/core/admin/server/controllers/api-token.js index 6ac27b28bb..2eaa8b4d50 100644 --- a/packages/core/admin/server/controllers/api-token.js +++ b/packages/core/admin/server/controllers/api-token.js @@ -37,6 +37,20 @@ module.exports = { ctx.created({ data: apiToken }); }, + async regenerate(ctx) { + const { body } = ctx.request; + const apiTokenService = getService('api-token'); + + const alreadyExists = await apiTokenService.exists({ name: body.id }); + if (!alreadyExists) { + ctx.notFound('API Token not found'); + return; + } + + const accessToken = await apiTokenService.regenerate(body.id); + ctx.created({ data: accessToken }); + }, + async list(ctx) { const apiTokenService = getService('api-token'); const apiTokens = await apiTokenService.list(); @@ -59,7 +73,6 @@ module.exports = { if (!apiToken) { ctx.notFound('API Token not found'); - return; } diff --git a/packages/core/admin/server/routes/api-tokens.js b/packages/core/admin/server/routes/api-tokens.js index 30147b0a32..acb34d9b45 100644 --- a/packages/core/admin/server/routes/api-tokens.js +++ b/packages/core/admin/server/routes/api-tokens.js @@ -56,4 +56,15 @@ module.exports = [ ], }, }, + { + method: 'PUT', + path: '/api-tokens/:id/regenerate', + handler: 'api-token.regenerate', + config: { + policies: [ + 'admin::isAuthenticatedAdmin', + { name: 'admin::hasPermissions', config: { actions: ['admin::api-tokens.update'] } }, + ], + }, + }, ]; diff --git a/packages/core/admin/server/services/__tests__/api-token.test.js b/packages/core/admin/server/services/__tests__/api-token.test.js index c79cdb7d95..bedfa0f3bb 100644 --- a/packages/core/admin/server/services/__tests__/api-token.test.js +++ b/packages/core/admin/server/services/__tests__/api-token.test.js @@ -263,6 +263,41 @@ describe('API Token', () => { expect(res).toEqual(attributes); }); }); + + describe('regenerate', () => { + test('It regenerates the accessKey', async () => { + const update = jest.fn(({ data }) => Promise.resolve(data)); + + global.strapi = { + query() { + return { update }; + }, + config: { + get: jest.fn(() => ''), + }, + }; + + const id = 1; + const attributes = { + name: 'api-token_tests-updated-name', + description: 'api-token_tests-description', + type: 'read-only', + }; + + const res = await apiTokenService.regenerate(id); + + expect(update).toHaveBeenCalledWith(id, { + select: ['id', 'accessKey'], + where: { id }, + data: { + ...attributes, + accessKey: expect.any(String), + }, + }); + expect(res).toEqual(attributes); + }); + }); + describe('getByName', () => { const token = { id: 1, diff --git a/packages/core/admin/server/services/api-token.js b/packages/core/admin/server/services/api-token.js index 5829a14df5..de1da814e9 100644 --- a/packages/core/admin/server/services/api-token.js +++ b/packages/core/admin/server/services/api-token.js @@ -71,6 +71,27 @@ const create = async (attributes) => { }; }; +/** + * @param {string|number} id + * + * @returns {Promise} + */ +const regenerate = async (id) => { + const accessKey = crypto.randomBytes(128).toString('hex'); + + const apiToken = await strapi.query('admin::api-token').update(id, { + select: ['id', 'accessKey'], + data: { + accessKey: hash(accessKey), + }, + }); + + return { + ...apiToken, + accessKey, + }; +}; + /** * @returns {void} */ @@ -162,6 +183,7 @@ const getBy = async (whereParams = {}) => { module.exports = { create, + regenerate, exists, checkSaltIsDefined, hash, From 9941198dac8df8599331863ecf75b60d5eb3e83e Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Thu, 18 Aug 2022 14:03:59 +0200 Subject: [PATCH 02/16] fix regeneration --- .../core/admin/server/controllers/api-token.js | 9 +++++---- packages/core/admin/server/routes/api-tokens.js | 2 +- .../server/services/__tests__/api-token.test.js | 3 ++- packages/core/admin/server/services/api-token.js | 3 ++- .../admin/server/tests/admin-api-token.test.e2e.js | 14 ++++++++++++++ 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/core/admin/server/controllers/api-token.js b/packages/core/admin/server/controllers/api-token.js index 8cde465fc1..71b719f97c 100644 --- a/packages/core/admin/server/controllers/api-token.js +++ b/packages/core/admin/server/controllers/api-token.js @@ -39,16 +39,17 @@ module.exports = { }, async regenerate(ctx) { - const { body } = ctx.request; + const { id } = ctx.params; const apiTokenService = getService('api-token'); - const alreadyExists = await apiTokenService.exists({ name: body.id }); - if (!alreadyExists) { + const apiTokenExists = await apiTokenService.getById(id); + if (!apiTokenExists) { ctx.notFound('API Token not found'); return; } - const accessToken = await apiTokenService.regenerate(body.id); + const accessToken = await apiTokenService.regenerate(id); + ctx.created({ data: accessToken }); }, diff --git a/packages/core/admin/server/routes/api-tokens.js b/packages/core/admin/server/routes/api-tokens.js index acb34d9b45..e994382db6 100644 --- a/packages/core/admin/server/routes/api-tokens.js +++ b/packages/core/admin/server/routes/api-tokens.js @@ -57,7 +57,7 @@ module.exports = [ }, }, { - method: 'PUT', + method: 'POST', path: '/api-tokens/:id/regenerate', handler: 'api-token.regenerate', config: { diff --git a/packages/core/admin/server/services/__tests__/api-token.test.js b/packages/core/admin/server/services/__tests__/api-token.test.js index 6669db03c7..789a9166f1 100644 --- a/packages/core/admin/server/services/__tests__/api-token.test.js +++ b/packages/core/admin/server/services/__tests__/api-token.test.js @@ -258,7 +258,8 @@ describe('API Token', () => { const id = 1; const res = await apiTokenService.regenerate(id); - expect(update).toHaveBeenCalledWith(id, { + expect(update).toHaveBeenCalledWith({ + where: { id }, select: ['id', 'accessKey'], data: { accessKey: apiTokenService.hash(mockedApiToken.hexedString), diff --git a/packages/core/admin/server/services/api-token.js b/packages/core/admin/server/services/api-token.js index 23f4f55bbe..cba015b91d 100644 --- a/packages/core/admin/server/services/api-token.js +++ b/packages/core/admin/server/services/api-token.js @@ -138,8 +138,9 @@ const create = async (attributes) => { const regenerate = async (id) => { const accessKey = crypto.randomBytes(128).toString('hex'); - const apiToken = await strapi.query('admin::api-token').update(id, { + const apiToken = await strapi.query('admin::api-token').update({ select: ['id', 'accessKey'], + where: { id }, data: { accessKey: hash(accessKey), }, diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index 2c49c6f722..3d4d77787b 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -608,4 +608,18 @@ describe('Admin API Token v2 CRUD (e2e)', () => { updatedAt: expect.any(String), }); }); + + test('Regenerates an api token access key)', async () => { + const token = await createValidToken(); + + const res = await rq({ + url: `/admin/api-tokens/${token.id}/regenerate`, + method: 'POST', + }); + + expect(res.statusCode).toBe(200); + expect(res.body.data).toMatchObject({ + accessKey: expect.any(String), + }); + }); }); From 7ce7db6604d160ad6f3f19667b2fbc8550aa5c8c Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Thu, 18 Aug 2022 14:12:34 +0200 Subject: [PATCH 03/16] update tests --- .../core/admin/server/tests/admin-api-token.test.e2e.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index 3d4d77787b..f3079f8979 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -609,7 +609,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); }); - test('Regenerates an api token access key)', async () => { + test('Regenerates an api token access key', async () => { const token = await createValidToken(); const res = await rq({ @@ -617,9 +617,13 @@ describe('Admin API Token v2 CRUD (e2e)', () => { method: 'POST', }); - expect(res.statusCode).toBe(200); + expect(res.statusCode).toBe(201); expect(res.body.data).toMatchObject({ accessKey: expect.any(String), }); }); + + test.todo('Regenerated access key works'); + test.todo('Tokens access content for which they are authorized'); + test.todo('Tokens fail to access content for which they are not authorized'); }); From dabbd42ca72ae8aed4f2f221be86c4fd93d209bf Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 22 Aug 2022 12:22:36 +0200 Subject: [PATCH 04/16] assert acessKey has changed --- packages/core/admin/server/tests/admin-api-token.test.e2e.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index f3079f8979..8d8197b6e1 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -621,6 +621,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.body.data).toMatchObject({ accessKey: expect.any(String), }); + expect(res.body.data.accessKey).not.toEqual(token.accessKey); }); test.todo('Regenerated access key works'); From aa5a1c7ed7711f90b8c21439026d442ac5aa8119 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 22 Aug 2022 12:33:00 +0200 Subject: [PATCH 05/16] regenerate throws on invalid id --- .../services/__tests__/api-token.test.js | 27 +++++++++++++++++++ .../core/admin/server/services/api-token.js | 4 +++ .../server/tests/admin-api-token.test.e2e.js | 13 +++++++++ 3 files changed, 44 insertions(+) diff --git a/packages/core/admin/server/services/__tests__/api-token.test.js b/packages/core/admin/server/services/__tests__/api-token.test.js index 789a9166f1..169405504e 100644 --- a/packages/core/admin/server/services/__tests__/api-token.test.js +++ b/packages/core/admin/server/services/__tests__/api-token.test.js @@ -1,5 +1,6 @@ 'use strict'; +const { NotFoundError } = require('@strapi/utils/lib/errors'); const crypto = require('crypto'); const { omit } = require('lodash/fp'); const apiTokenService = require('../api-token'); @@ -267,6 +268,32 @@ describe('API Token', () => { }); expect(res).toEqual({ accessKey: mockedApiToken.hexedString }); }); + + test('It throws a NotFound if the id is not found', async () => { + const update = jest.fn(() => Promise.resolve(null)); + + global.strapi = { + query() { + return { update }; + }, + config: { + get: jest.fn(() => ''), + }, + }; + + const id = 1; + await expect(async () => { + await apiTokenService.regenerate(id); + }).rejects.toThrowError(NotFoundError); + + expect(update).toHaveBeenCalledWith({ + where: { id }, + select: ['id', 'accessKey'], + data: { + accessKey: apiTokenService.hash(mockedApiToken.hexedString), + }, + }); + }); }); describe('update', () => { diff --git a/packages/core/admin/server/services/api-token.js b/packages/core/admin/server/services/api-token.js index cba015b91d..946554562f 100644 --- a/packages/core/admin/server/services/api-token.js +++ b/packages/core/admin/server/services/api-token.js @@ -146,6 +146,10 @@ const regenerate = async (id) => { }, }); + if (!apiToken) { + throw new NotFoundError('The provided token id does not exist'); + } + return { ...apiToken, accessKey, diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index 8d8197b6e1..13b42f5e31 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -624,6 +624,19 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.body.data.accessKey).not.toEqual(token.accessKey); }); + test('Regenerate throws a NotFound if provided an invalid id', async () => { + const res = await rq({ + url: `/admin/api-tokens/999999/regenerate`, + method: 'POST', + }); + + expect(res.statusCode).toBe(404); + expect(res.body.error).toMatchObject({ + name: 'NotFoundError', + status: 404, + }); + }); + test.todo('Regenerated access key works'); test.todo('Tokens access content for which they are authorized'); test.todo('Tokens fail to access content for which they are not authorized'); From 7a1de40bb68024d1804a8e36aae4eef99e955aa7 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 22 Aug 2022 17:31:15 +0200 Subject: [PATCH 06/16] register actions in contentapi bootstrap --- .../strapi/lib/services/content-api/index.js | 108 ++++++++---------- .../server/services/users-permissions.js | 4 +- 2 files changed, 51 insertions(+), 61 deletions(-) diff --git a/packages/core/strapi/lib/services/content-api/index.js b/packages/core/strapi/lib/services/content-api/index.js index d4c3f6dbe8..a7331d1ddf 100644 --- a/packages/core/strapi/lib/services/content-api/index.js +++ b/packages/core/strapi/lib/services/content-api/index.js @@ -2,64 +2,6 @@ const { uniq } = 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,54 @@ 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 syncActions = async () => { + // Register actions + 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.warn('Trying to add action that already exists', action); + }) + ) + ); + }; + + // create permission engine + const engine = permissions + .createPermissionEngine({ providers }) + .on('before-format::validate.permission', createValidatePermissionHandler(providers.action)); + + return { + permissions: { + engine, + providers, + syncActions, + }, + }; +}; + module.exports = createContentAPI; diff --git a/packages/plugins/users-permissions/server/services/users-permissions.js b/packages/plugins/users-permissions/server/services/users-permissions.js index 65eb9cafd0..41f256334b 100644 --- a/packages/plugins/users-permissions/server/services/users-permissions.js +++ b/packages/plugins/users-permissions/server/services/users-permissions.js @@ -171,9 +171,7 @@ 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)); + // NOTE: actions are registered in content API bootstrap syncActions await Promise.all( toDelete.map((action) => { From cb872000999aa015c67652af0f514e92ec825321 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 22 Aug 2022 18:50:32 +0200 Subject: [PATCH 07/16] use strapi.log --- packages/core/strapi/lib/services/content-api/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/strapi/lib/services/content-api/index.js b/packages/core/strapi/lib/services/content-api/index.js index a7331d1ddf..db5be6bf0a 100644 --- a/packages/core/strapi/lib/services/content-api/index.js +++ b/packages/core/strapi/lib/services/content-api/index.js @@ -50,7 +50,7 @@ const createContentAPI = (/* strapi */) => { Promise.all( uniq(actions).map((action) => providers.action.register(action).catch(() => { - console.warn('Trying to add action that already exists', action); + strapi.log.warn(`Trying to add action that already exists: ${action}`); }) ) ); From 6abea9cd85abf47e1b582f1bbc0a667e5cea6b0c Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 23 Aug 2022 16:02:45 +0200 Subject: [PATCH 08/16] Expose "get actions as map" & update sync actions Co-authored-by: Ben Irvin --- packages/core/strapi/lib/Strapi.js | 2 +- .../strapi/lib/services/content-api/index.js | 93 +++++++++++++------ .../permissions/providers/action.js | 4 +- .../server/services/users-permissions.js | 2 - 4 files changed, 70 insertions(+), 31 deletions(-) diff --git a/packages/core/strapi/lib/Strapi.js b/packages/core/strapi/lib/Strapi.js index 30c77ef821..f7c9c37357 100644 --- a/packages/core/strapi/lib/Strapi.js +++ b/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(); diff --git a/packages/core/strapi/lib/services/content-api/index.js b/packages/core/strapi/lib/services/content-api/index.js index db5be6bf0a..c17922bc25 100644 --- a/packages/core/strapi/lib/services/content-api/index.js +++ b/packages/core/strapi/lib/services/content-api/index.js @@ -1,6 +1,6 @@ 'use strict'; -const { uniq } = require('lodash'); +const _ = require('lodash'); const permissions = require('./permissions'); /** @@ -30,30 +30,70 @@ const createContentAPI = (/* strapi */) => { condition: permissions.providers.createConditionProvider(), }; - const syncActions = async () => { - // Register actions - 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(() => { - strapi.log.warn(`Trying to add action that already exists: ${action}`); - }) - ) - ); + 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 }) + .catch(() => { + strapi.log.warn(`Trying to add action that already exists: ${actionUID}`); + }); + }) + ); + } + } }; // create permission engine @@ -65,7 +105,8 @@ const createContentAPI = (/* strapi */) => { permissions: { engine, providers, - syncActions, + registerActions, + getActionsMap, }, }; }; diff --git a/packages/core/strapi/lib/services/content-api/permissions/providers/action.js b/packages/core/strapi/lib/services/content-api/permissions/providers/action.js index e44cce17a2..ead6855351 100644 --- a/packages/core/strapi/lib/services/content-api/permissions/providers/action.js +++ b/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); }, }; }; diff --git a/packages/plugins/users-permissions/server/services/users-permissions.js b/packages/plugins/users-permissions/server/services/users-permissions.js index 41f256334b..1389cd582e 100644 --- a/packages/plugins/users-permissions/server/services/users-permissions.js +++ b/packages/plugins/users-permissions/server/services/users-permissions.js @@ -171,8 +171,6 @@ module.exports = ({ strapi }) => ({ const toDelete = _.difference(permissionsFoundInDB, allActions); - // NOTE: actions are registered in content API bootstrap syncActions - await Promise.all( toDelete.map((action) => { return strapi.query('plugin::users-permissions.permission').delete({ where: { action } }); From 1d6b42efbe2e3d71b811068b481e1c775476bddd Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 23 Aug 2022 16:05:47 +0200 Subject: [PATCH 09/16] Use local instance of strapi instead of the global --- packages/core/strapi/lib/services/content-api/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/strapi/lib/services/content-api/index.js b/packages/core/strapi/lib/services/content-api/index.js index c17922bc25..f6f9f465d6 100644 --- a/packages/core/strapi/lib/services/content-api/index.js +++ b/packages/core/strapi/lib/services/content-api/index.js @@ -23,7 +23,7 @@ const createValidatePermissionHandler = /** * Create a content API container that holds logic, tools and utils. (eg: permissions, ...) */ -const createContentAPI = (/* strapi */) => { +const createContentAPI = (strapi) => { // Add providers const providers = { action: permissions.providers.createActionProvider(), From f3c1812e006d4ed95952e919fcbaaa321273ba90 Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 23 Aug 2022 17:09:48 +0200 Subject: [PATCH 10/16] Remove useless warning --- .../core/strapi/lib/services/content-api/index.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/core/strapi/lib/services/content-api/index.js b/packages/core/strapi/lib/services/content-api/index.js index f6f9f465d6..4f03728cf5 100644 --- a/packages/core/strapi/lib/services/content-api/index.js +++ b/packages/core/strapi/lib/services/content-api/index.js @@ -85,11 +85,12 @@ const createContentAPI = (strapi) => { actions.map((action) => { const actionUID = `${api}.${controller}.${action}`; - return providers.action - .register(actionUID, { api, controller, action, uid: actionUID }) - .catch(() => { - strapi.log.warn(`Trying to add action that already exists: ${actionUID}`); - }); + return providers.action.register(actionUID, { + api, + controller, + action, + uid: actionUID, + }); }) ); } From 3944677650c1711692757c30aaf0dbfd572590cb Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 23 Aug 2022 17:10:10 +0200 Subject: [PATCH 11/16] Add unit tests for the content-api permissions --- .../__tests__/content-api-permissions.test.js | 291 ++++++++++++++++++ 1 file changed, 291 insertions(+) create mode 100644 packages/core/strapi/lib/services/__tests__/content-api-permissions.test.js diff --git a/packages/core/strapi/lib/services/__tests__/content-api-permissions.test.js b/packages/core/strapi/lib/services/__tests__/content-api-permissions.test.js new file mode 100644 index 0000000000..06b327d636 --- /dev/null +++ b/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` + ); + }); + }); +}); From 97f91dd7a57ca7c69af1f8226f61e308a33e22eb Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 23 Aug 2022 17:24:29 +0200 Subject: [PATCH 12/16] fix failing tests --- .../server/tests/admin-api-token.test.e2e.js | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index 13b42f5e31..003deb49d7 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -1,6 +1,6 @@ 'use strict'; -const { omit, map, orderBy } = require('lodash'); +const { omit, map } = require('lodash'); const { createStrapiInstance } = require('../../../../../test/helpers/strapi'); const { createAuthRequest } = require('../../../../../test/helpers/request'); @@ -36,6 +36,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { const createValidToken = async (token = {}) => { const body = { type: 'read-only', + // eslint-disable-next-line no-plusplus name: `token_${String(currentTokens++)}`, description: 'generic description', ...token, @@ -130,7 +131,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(201); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ accessKey: expect.any(String), name: body.name, permissions: [], @@ -189,7 +190,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(201); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ accessKey: expect.any(String), name: body.name, permissions: [], @@ -217,7 +218,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(201); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ accessKey: expect.any(String), name: body.name, permissions: body.permissions, @@ -269,7 +270,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(201); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ accessKey: expect.any(String), name: body.name, permissions: [], @@ -296,7 +297,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(201); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ accessKey: expect.any(String), name: 'api-token_tests-spaces-at-the-end', permissions: [], @@ -331,9 +332,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.statusCode).toBe(200); expect(res.body.data.length).toBe(4); - expect(orderBy(res.body.data, ['id'])).toStrictEqual( - map(orderBy(tokens, ['id']), (t) => omit(t, ['accessKey'])) - ); + expect(res.body.data).toMatchObject(map(tokens, (t) => omit(t, ['accessKey']))); }); test('Deletes a token (successfully)', async () => { @@ -345,7 +344,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(200); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ name: token.name, permissions: token.permissions, description: token.description, @@ -378,7 +377,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(200); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ name: token.name, permissions: token.permissions, description: token.description, @@ -402,7 +401,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(res.statusCode).toBe(200); - expect(res.body.data).toStrictEqual({ + expect(res.body.data).toMatchObject({ name: token.name, permissions: token.permissions, description: token.description, @@ -460,7 +459,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { }); expect(updatedRes.statusCode).toBe(200); - expect(updatedRes.body.data).toStrictEqual({ + expect(updatedRes.body.data).toMatchObject({ name: updatedBody.name, permissions: [], description: updatedBody.description, From 205c9098855ec5e0c79e0834cbf0bce75175d79a Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 23 Aug 2022 17:59:06 +0200 Subject: [PATCH 13/16] fix failing test --- .../core/admin/server/tests/admin-api-token.test.e2e.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index bba57bdf15..aa0bd674d7 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -1,6 +1,6 @@ 'use strict'; -const { omit, map, orderBy } = require('lodash'); +const { omit, map } = require('lodash'); const { createStrapiInstance } = require('../../../../../test/helpers/strapi'); const { createAuthRequest } = require('../../../../../test/helpers/request'); @@ -36,6 +36,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { const createValidToken = async (token = {}) => { const body = { type: 'read-only', + // eslint-disable-next-line no-plusplus name: `token_${String(currentTokens++)}`, description: 'generic description', ...token, @@ -331,9 +332,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.statusCode).toBe(200); expect(res.body.data.length).toBe(4); - expect(orderBy(res.body.data, ['id'])).toStrictEqual( - map(orderBy(tokens, ['id']), (t) => omit(t, ['accessKey'])) - ); + expect(res.body.data).toMatchObject(map(tokens, (t) => omit(t, ['accessKey']))); }); test('Deletes a token (successfully)', async () => { From 353efdf84c4e87a6acb9b9b53ac1b350910f960c Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 23 Aug 2022 18:28:34 +0200 Subject: [PATCH 14/16] fix test --- .../admin/server/tests/admin-api-token.test.e2e.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index bba57bdf15..aa1bfbd41d 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -1,6 +1,6 @@ 'use strict'; -const { omit, map, orderBy } = require('lodash'); +const { omit, map } = require('lodash'); const { createStrapiInstance } = require('../../../../../test/helpers/strapi'); const { createAuthRequest } = require('../../../../../test/helpers/request'); @@ -36,6 +36,7 @@ describe('Admin API Token v2 CRUD (e2e)', () => { const createValidToken = async (token = {}) => { const body = { type: 'read-only', + // eslint-disable-next-line no-plusplus name: `token_${String(currentTokens++)}`, description: 'generic description', ...token, @@ -331,8 +332,14 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.statusCode).toBe(200); expect(res.body.data.length).toBe(4); - expect(orderBy(res.body.data, ['id'])).toStrictEqual( - map(orderBy(tokens, ['id']), (t) => omit(t, ['accessKey'])) + const mappedTokens = map(tokens, (t) => omit(t, ['accessKey'])); + expect(res.body.data).toEqual( + expect.arrayContaining([ + expect.objectContaining(mappedTokens[0]), + expect.objectContaining(mappedTokens[1]), + expect.objectContaining(mappedTokens[2]), + expect.objectContaining(mappedTokens[3]), + ]) ); }); From 5085f5ed45a8e7848b6b8a35e5c151e1698736a6 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 23 Aug 2022 19:19:56 +0200 Subject: [PATCH 15/16] actually fix test --- .../server/tests/admin-api-token.test.e2e.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index aa1bfbd41d..85d3e77350 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -1,6 +1,6 @@ 'use strict'; -const { omit, map } = require('lodash'); +const { omit } = require('lodash'); const { createStrapiInstance } = require('../../../../../test/helpers/strapi'); const { createAuthRequest } = require('../../../../../test/helpers/request'); @@ -332,15 +332,11 @@ describe('Admin API Token v2 CRUD (e2e)', () => { expect(res.statusCode).toBe(200); expect(res.body.data.length).toBe(4); - const mappedTokens = map(tokens, (t) => omit(t, ['accessKey'])); - expect(res.body.data).toEqual( - expect.arrayContaining([ - expect.objectContaining(mappedTokens[0]), - expect.objectContaining(mappedTokens[1]), - expect.objectContaining(mappedTokens[2]), - expect.objectContaining(mappedTokens[3]), - ]) - ); + // check that each token exists in data + tokens.forEach((token) => { + const t = res.body.data.find((t) => t.id === token.id); + expect(t).toMatchObject(omit(token, 'accessKey')); + }); }); test('Deletes a token (successfully)', async () => { From 0b8ecaaccb5777f24765bc7d3b5445326f8e644c Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 23 Aug 2022 20:08:45 +0200 Subject: [PATCH 16/16] sort permissions to compare --- .../core/admin/server/tests/admin-api-token.test.e2e.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/core/admin/server/tests/admin-api-token.test.e2e.js b/packages/core/admin/server/tests/admin-api-token.test.e2e.js index 85d3e77350..d66a42b3b0 100644 --- a/packages/core/admin/server/tests/admin-api-token.test.e2e.js +++ b/packages/core/admin/server/tests/admin-api-token.test.e2e.js @@ -335,7 +335,12 @@ describe('Admin API Token v2 CRUD (e2e)', () => { // check that each token exists in data tokens.forEach((token) => { const t = res.body.data.find((t) => t.id === token.id); - expect(t).toMatchObject(omit(token, 'accessKey')); + if (t.permissions) { + t.permissions = t.permissions.sort(); + // eslint-disable-next-line no-param-reassign + token.permissions = token.permissions.sort(); + } + expect(t).toMatchObject(omit(token, ['accessKey'])); }); });