diff --git a/packages/strapi-admin/config/functions/bootstrap.js b/packages/strapi-admin/config/functions/bootstrap.js index 1079c554e9..d985eb14cd 100644 --- a/packages/strapi-admin/config/functions/bootstrap.js +++ b/packages/strapi-admin/config/functions/bootstrap.js @@ -29,9 +29,14 @@ const cleanPermissionInDatabase = async () => { const registerAdminConditions = () => { const { conditionProvider } = strapi.admin.services.permission; - conditionProvider.registerMany({ - 'strapi-admin::isOwner': user => ({ 'strapi_created_by.id': user.id }), - }); + conditionProvider.registerMany([ + { + name: 'isOwner', + plugin: 'admin', + category: 'default', + handler: user => ({ 'strapi_created_by.id': user.id }), + }, + ]); }; module.exports = async () => { diff --git a/packages/strapi-admin/controllers/permission.js b/packages/strapi-admin/controllers/permission.js index 54fdf6a92b..e2536cd9a8 100644 --- a/packages/strapi-admin/controllers/permission.js +++ b/packages/strapi-admin/controllers/permission.js @@ -1,5 +1,6 @@ 'use strict'; +const _ = require('lodash'); const { formatActionsBySections } = require('./formatters'); const { validateCheckPermissionsInput } = require('../validation/permission'); @@ -36,7 +37,9 @@ module.exports = { ctx.body = { data: { - conditions, + conditions: conditions.map(condition => + _.pick(condition, 'id', 'name', 'plugin', 'category') + ), sections: formatActionsBySections(allActions), }, }; diff --git a/packages/strapi-admin/domain/condition.js b/packages/strapi-admin/domain/condition.js new file mode 100644 index 0000000000..a91e32d1d5 --- /dev/null +++ b/packages/strapi-admin/domain/condition.js @@ -0,0 +1,24 @@ +'use strict'; + +const getConditionId = ({ name, plugin }) => { + let id; + + if (plugin === 'admin') { + id = `admin::${name}`; + } else if (plugin) { + id = `plugins::${plugin}.${name}`; + } else { + id = `application::${name}`; + } + return id; +}; + +const createCondition = condition => ({ + ...condition, + id: getConditionId(condition), +}); + +module.exports = { + getConditionId, + createCondition, +}; diff --git a/packages/strapi-admin/services/__tests__/permissions.conditions-provider.test.js b/packages/strapi-admin/services/__tests__/permissions.conditions-provider.test.js index 45e0fc083c..b1e107762a 100644 --- a/packages/strapi-admin/services/__tests__/permissions.conditions-provider.test.js +++ b/packages/strapi-admin/services/__tests__/permissions.conditions-provider.test.js @@ -1,11 +1,31 @@ 'use strict'; +const _ = require('lodash'); const createConditionProvider = require('../permission/condition-provider'); +const { createCondition, getConditionId } = require('../../domain/condition'); describe('Condition Provider', () => { let provider; + const localTestData = { + conditions: [ + { + name: 'foo', + plugin: 'test', + category: 'default', + handler: jest.fn(() => true), + }, + { + name: 'john', + plugin: 'test', + category: 'default', + handler: jest.fn(() => false), + }, + ], + }; beforeEach(() => { + global.strapi = { isLoaded: false }; + provider = createConditionProvider(); jest.spyOn(provider, 'register'); @@ -17,22 +37,33 @@ describe('Condition Provider', () => { }); describe('Register', () => { + test('Cannot register if strapi is already loaded', () => { + global.strapi.isLoaded = true; + + const condition = localTestData.conditions[0]; + + const registerFn = () => provider.register(condition); + + expect(registerFn).toThrowError(); + }); + test('Successfully register a new condition', () => { - const condition = { key: 'conditionName', value: jest.fn(() => true) }; + const condition = localTestData.conditions[0]; - provider.register(condition.key, condition.value); + provider.register(condition); - const res = provider.get(condition.key); + const res = provider.get(condition.name, condition.plugin); - expect(provider.has).toHaveBeenCalledWith(condition.key); - expect(res).toBe(condition.value); - expect(res()).toBeTruthy(); - expect(condition.value).toHaveBeenCalled(); + expect(provider.has).toHaveBeenCalledWith(condition.name, condition.plugin); + expect(res).toMatchObject(condition); + expect(res.handler()).toBe(true); + expect(condition.handler).toHaveBeenCalled(); }); test('The condition already exists', () => { - const key = 'conditionName'; - const registerFn = () => provider.register(key, {}); + const condition = localTestData.conditions[0]; + + const registerFn = () => provider.register(condition); registerFn(); @@ -43,34 +74,28 @@ describe('Condition Provider', () => { describe('Registers Many', () => { test('Registers many conditions successfully', () => { - const conditions = { - foo: jest.fn(() => 'bar'), - john: jest.fn(() => 'doe'), - }; + const conditions = localTestData.conditions; provider.registerMany(conditions); - const resFoo = provider.get('foo'); - const resJohn = provider.get('john'); + const resFoo = provider.get('foo', 'test'); + const resJohn = provider.get('john', 'test'); expect(provider.register).toHaveBeenCalledTimes(2); expect(provider.has).toHaveBeenCalledTimes(2); - expect(resFoo).toBe(conditions.foo); - expect(resJohn).toBe(conditions.john); + expect(resFoo).toMatchObject(createCondition(conditions[0])); + expect(resJohn).toMatchObject(createCondition(conditions[1])); - expect(resFoo()).toBe('bar'); - expect(resJohn()).toBe('doe'); + expect(resFoo.handler()).toBe(true); + expect(resJohn.handler()).toBe(false); - expect(conditions.foo).toHaveBeenCalled(); - expect(conditions.john).toHaveBeenCalled(); + expect(conditions[0].handler).toHaveBeenCalled(); + expect(conditions[1].handler).toHaveBeenCalled(); }); test('Fails to register already existing conditions', () => { - const conditions = { - foo: {}, - john: {}, - }; + const conditions = localTestData.conditions; const registerFn = () => provider.registerMany(conditions); @@ -83,54 +108,46 @@ describe('Condition Provider', () => { describe('Conditions', () => { test('Returns an array of all the conditions key', () => { - const conditions = { - foo: {}, - bar: {}, - }; - const expected = ['bar', 'foo']; + const conditions = localTestData.conditions; + + const expected = ['plugins::test.foo', 'plugins::test.john']; provider.registerMany(conditions); - expect(provider.getAll().sort()).toMatchObject(expected); + expect( + provider + .getAll() + .map(_.property('id')) + .sort() + ).toMatchObject(expected); }); }); describe('Has', () => { test('The key exists', () => { - const key = 'foo'; - provider.register(key, {}); + const condition = localTestData.conditions[0]; - expect(provider.has(key)).toBeTruthy(); + provider.register(condition); + + expect(provider.has(condition.name, condition.plugin)).toBeTruthy(); }); test(`The key doesn't exists`, () => { - const key = 'foo'; + const { name, plugin } = localTestData.conditions[1]; - expect(provider.has(key)).toBeFalsy(); + expect(provider.has(name, plugin)).toBeFalsy(); }); }); - describe('Delete', () => { - test('Delete existing condition', () => { - const key = 'foo'; + describe('GetById', () => { + test('Successfully get a condition by its ID', () => { + const condition = localTestData.conditions[0]; - provider.register(key); + provider.register(condition); - expect(provider.getAll()).toHaveLength(1); + const res = provider.getById(getConditionId(condition)); - provider.delete(key); - - expect(provider.has).toHaveBeenCalledWith(key); - expect(provider.getAll()).toHaveLength(0); - }); - - test('Do nothing when the key does not exists', () => { - const key = 'foo'; - - provider.delete(key); - - expect(provider.has).toHaveBeenCalledWith(key); - expect(provider.getAll()).toHaveLength(0); + expect(res).toMatchObject(createCondition(condition)); }); }); }); diff --git a/packages/strapi-admin/services/__tests__/permissions.engine.test.js b/packages/strapi-admin/services/__tests__/permissions.engine.test.js index 14749eb02a..f2982a2b30 100644 --- a/packages/strapi-admin/services/__tests__/permissions.engine.test.js +++ b/packages/strapi-admin/services/__tests__/permissions.engine.test.js @@ -30,40 +30,74 @@ describe('Permissions Engine', () => { roles: { 1: { permissions: [ - { action: 'read', subject: 'article', fields: ['**'], conditions: ['isBob'] }, - { action: 'read', subject: 'user', fields: ['title'], conditions: ['isAdmin'] }, + { + action: 'read', + subject: 'article', + fields: ['**'], + conditions: ['plugins::test.isBob'], + }, + { + action: 'read', + subject: 'user', + fields: ['title'], + conditions: ['plugins::test.isAdmin'], + }, ], }, 2: { - permissions: [{ action: 'post', subject: 'article', fields: ['*'], conditions: ['isBob'] }], + permissions: [ + { + action: 'post', + subject: 'article', + fields: ['*'], + conditions: ['plugins::test.isBob'], + }, + ], }, 3: { permissions: [ - { action: 'read', subject: 'user', fields: ['title'], conditions: ['isContainedIn'] }, + { + action: 'read', + subject: 'user', + fields: ['title'], + conditions: ['plugins::test.isContainedIn'], + }, ], }, }, - conditions: { - isBob: async user => new Promise(resolve => resolve(user.firstname === 'Bob')), - isAdmin: user => user.title === 'admin', - isCreatedBy: user => ({ created_by: user.firstname }), - isContainedIn: { firstname: { $in: ['Alice', 'Foo'] } }, - }, + conditions: [ + { + plugin: 'test', + name: 'isBob', + category: 'default', + handler: async user => new Promise(resolve => resolve(user.firstname === 'Bob')), + }, + { + plugin: 'test', + name: 'isAdmin', + category: 'default', + handler: user => user.title === 'admin', + }, + { + plugin: 'test', + name: 'isCreatedBy', + category: 'default', + handler: user => ({ created_by: user.firstname }), + }, + { + plugin: 'test', + name: 'isContainedIn', + category: 'default', + handler: { firstname: { $in: ['Alice', 'Foo'] } }, + }, + ], }; const getUser = name => localTestData.users[name]; beforeEach(() => { - conditionProvider = createConditionProvider(); - conditionProvider.registerMany(localTestData.conditions); - - engine = createPermissionsEngine(conditionProvider); - - jest.spyOn(engine, 'evaluatePermission'); - jest.spyOn(engine, 'createRegisterFunction'); - jest.spyOn(engine, 'generateAbilityCreatorFor'); - global.strapi = { + isLoaded: false, admin: { services: { permission: { @@ -82,6 +116,15 @@ describe('Permissions Engine', () => { }, }, }; + + conditionProvider = createConditionProvider(); + conditionProvider.registerMany(localTestData.conditions); + + engine = createPermissionsEngine(conditionProvider); + + jest.spyOn(engine, 'evaluatePermission'); + jest.spyOn(engine, 'createRegisterFunction'); + jest.spyOn(engine, 'generateAbilityCreatorFor'); }); afterEach(() => { @@ -211,7 +254,7 @@ describe('Permissions Engine', () => { action: 'read', subject: 'article', fields: ['title'], - conditions: ['isAdmin'], + conditions: ['plugins::test.isAdmin'], }; const user = getUser('alice'); const registerFn = jest.fn(); @@ -231,7 +274,7 @@ describe('Permissions Engine', () => { action: 'read', subject: 'article', fields: ['title'], - conditions: ['isBob'], + conditions: ['plugins::test.isBob'], }; const user = getUser('alice'); const registerFn = jest.fn(); @@ -246,7 +289,7 @@ describe('Permissions Engine', () => { action: 'read', subject: 'article', fields: ['title'], - conditions: ['isCreatedBy'], + conditions: ['plugins::test.isCreatedBy'], }; const user = getUser('alice'); const registerFn = jest.fn(); diff --git a/packages/strapi-admin/services/permission/condition-provider.js b/packages/strapi-admin/services/permission/condition-provider.js index 85841c249f..178cf4e336 100644 --- a/packages/strapi-admin/services/permission/condition-provider.js +++ b/packages/strapi-admin/services/permission/condition-provider.js @@ -1,70 +1,75 @@ 'use strict'; const _ = require('lodash'); +const { getConditionId, createCondition } = require('../../domain/condition'); module.exports = () => { const _registry = new Map(); return { /** - * Register a new condition with its associated unique key. - * @throws Error if the key already exists - * @param name + * Register a new condition + * @throws Error if the conditionId already exists * @param condition */ - register(name, condition) { - if (this.has(name)) { - throw new Error( - `Error while trying to add condition "${name}" to the registry. "${name}" already exists.` - ); + register(condition) { + const conditionId = getConditionId(condition); + + if (strapi.isLoaded) { + throw new Error(`You can't register new conditions outside of the bootstrap function.`); } - _registry.set(name, condition); + if (this.has(condition.name, condition.plugin)) { + throw new Error(`Duplicated condition id: ${getConditionId(condition)}.`); + } + + _registry.set(conditionId, createCondition(condition)); }, /** * Shorthand for batch-register operations. - * Internally calls `register` for each key/value couple. - * @param conditionsMap + * Internally calls `register` for each condition. + * @param conditions */ - registerMany(conditionsMap) { - _.each(conditionsMap, (value, key) => this.register(key, value)); - }, - - /** - * Deletes a condition by its key - * @param key - */ - delete(key) { - if (this.has(key)) { - _registry.delete(key); - } - }, - - /** - * Returns the keys of the conditions registry. - * @returns {string[]} - */ - conditions() { - return Array.from(_registry.keys()); - }, - - /** - * Get a condition by its key - * @param name - * @returns {any} - */ - get(name) { - return _registry.get(name); + registerMany(conditions) { + _.each(conditions, this.register.bind(this)); }, /** * Check if a key is already present in the registry * @param name - * @returns {boolean} true if the key is present in the registry, false otherwise. + * @param plugin + * @returns {boolean} true if the condition is present in the registry, false otherwise. */ - has(name) { - return _registry.has(name); + has(name, plugin) { + return _registry.has(getConditionId({ name, plugin })); + }, + + /** + * Get a condition by its name and plugin + * @param {string} name + * @param {string} plugin + * @returns {any} + */ + get(name, plugin) { + return _registry.get(getConditionId({ name, plugin })); + }, + + /** + * Get a condition by its id + * @param {string} id + * @returns {any} + */ + getById(id) { + return _registry.get(id); + }, + + /** + * Returns all the registered conditions. + * @returns {any[]} + */ + getAll() { + return Array.from(_registry.values()); }, }; }; diff --git a/packages/strapi-admin/services/permission/engine.js b/packages/strapi-admin/services/permission/engine.js index 8ecec4c382..4fe4eee082 100644 --- a/packages/strapi-admin/services/permission/engine.js +++ b/packages/strapi-admin/services/permission/engine.js @@ -51,7 +51,10 @@ module.exports = conditionProvider => ({ } // Replace each condition name by its associated value - const resolveConditions = map(conditionProvider.get); + const resolveConditions = map(conditionProvider.getById); + + // Only keep the handler of each condition + const pickHandlers = map(_.property('handler')); // Filter conditions, only keeps objects and functions const filterValidConditions = filter(_.isObject); @@ -76,6 +79,7 @@ module.exports = conditionProvider => ({ await Promise.resolve(conditions) .then(resolveConditions) + .then(pickHandlers) .then(filterValidConditions) .then(evaluateConditions) .then(filterValidResults)