From b511ef3d8f40fbef21cd64258810877e43a9d9e8 Mon Sep 17 00:00:00 2001 From: ChengLei Shao Date: Sun, 24 Apr 2022 10:14:46 +0800 Subject: [PATCH] Fix acl target action error (#311) * fix: field association resource name * chore: resourceCollection fields unique index * fix: test * feat: allowConfigure permission skip * feat: skip with array type actionNames * chore: rename acl skip to allow * fix: type * chore: rename SkipManager to AllowManager --- .../__tests__/{skip.test.ts => allow.test.ts} | 8 ++-- packages/core/acl/src/acl.ts | 22 ++++----- .../src/{skip-manager.ts => allow-manager.ts} | 29 ++++++++---- .../acl/src/__tests__/configuration.test.ts | 45 +++++++++++++++++++ .../acl/src/collections/rolesResources.ts | 6 +++ .../acl/src/model/RoleResourceActionModel.ts | 7 ++- .../acl/src/model/RoleResourceModel.ts | 1 + packages/plugins/acl/src/server.ts | 12 ++--- packages/plugins/china-region/src/server.ts | 2 +- .../plugins/collection-manager/src/plugin.ts | 3 +- packages/plugins/file-manager/src/server.ts | 2 +- .../plugins/ui-schema-storage/src/server.ts | 4 +- .../users/src/__tests__/fields.test.ts | 1 + packages/plugins/users/src/server.ts | 4 +- 14 files changed, 109 insertions(+), 37 deletions(-) rename packages/core/acl/src/__tests__/{skip.test.ts => allow.test.ts} (90%) rename packages/core/acl/src/{skip-manager.ts => allow-manager.ts} (66%) create mode 100644 packages/plugins/acl/src/__tests__/configuration.test.ts diff --git a/packages/core/acl/src/__tests__/skip.test.ts b/packages/core/acl/src/__tests__/allow.test.ts similarity index 90% rename from packages/core/acl/src/__tests__/skip.test.ts rename to packages/core/acl/src/__tests__/allow.test.ts index 154b1dfbc..d1895b7ec 100644 --- a/packages/core/acl/src/__tests__/skip.test.ts +++ b/packages/core/acl/src/__tests__/allow.test.ts @@ -26,7 +26,7 @@ describe('skip', () => { await middlewareFunc(ctx, nextFunc); expect(nextFunc).toHaveBeenCalledTimes(0); - acl.skip('users', 'login'); + acl.allow('users', 'login'); await middlewareFunc(ctx, nextFunc); expect(nextFunc).toHaveBeenCalledTimes(1); @@ -49,7 +49,7 @@ describe('skip', () => { const nextFunc = jest.fn(); let skip = false; - acl.skip('users', 'login', (ctx) => { + acl.allow('users', 'login', (ctx) => { return skip; }); @@ -65,7 +65,7 @@ describe('skip', () => { const middlewareFunc = acl.middleware(); const conditionFn = jest.fn(); - acl.skipManager.registerSkipCondition('superUser', () => { + acl.allowManager.registerAllowCondition('superUser', async () => { conditionFn(); return true; }); @@ -84,7 +84,7 @@ describe('skip', () => { const nextFunc = jest.fn(); - acl.skip('users', 'login', 'superUser'); + acl.allow('users', 'login', 'superUser'); await middlewareFunc(ctx, nextFunc); expect(nextFunc).toHaveBeenCalledTimes(1); diff --git a/packages/core/acl/src/acl.ts b/packages/core/acl/src/acl.ts index 20c9ba109..2f31704d3 100644 --- a/packages/core/acl/src/acl.ts +++ b/packages/core/acl/src/acl.ts @@ -5,7 +5,7 @@ import lodash from 'lodash'; import { AclAvailableAction, AvailableActionOptions } from './acl-available-action'; import { ACLAvailableStrategy, AvailableStrategyOptions, predicate } from './acl-available-strategy'; import { ACLRole, RoleActionParams } from './acl-role'; -import { SkipManager } from './skip-manager'; +import { AllowManager } from './allow-manager'; const parse = require('json-templates'); interface CanResult { @@ -47,7 +47,7 @@ export class ACL extends EventEmitter { protected availableStrategy = new Map(); protected middlewares = []; - public skipManager = new SkipManager(this); + public allowManager = new AllowManager(this); roles = new Map(); @@ -85,7 +85,7 @@ export class ACL extends EventEmitter { } }); - this.middlewares.push(this.skipManager.aclMiddleware()); + this.middlewares.push(this.allowManager.aclMiddleware()); } define(options: DefineOptions): ACLRole { @@ -217,8 +217,14 @@ export class ACL extends EventEmitter { this.middlewares.push(fn); } - skip(resourceName: string, actionName: string, condition?: any) { - this.skipManager.skip(resourceName, actionName, condition); + allow(resourceName: string, actionNames: string[] | string, condition?: any) { + if (!Array.isArray(actionNames)) { + actionNames = [actionNames]; + } + + for (const actionName of actionNames) { + this.allowManager.allow(resourceName, actionName, condition); + } } parseJsonTemplate(json: any, ctx: any) { @@ -243,12 +249,6 @@ export class ACL extends EventEmitter { return params; }; - const ctxToObject = (ctx) => { - return { - state: JSON.parse(JSON.stringify(ctx.state)), - }; - }; - return async function ACLMiddleware(ctx, next) { const roleName = ctx.state.currentRole || 'anonymous'; const { resourceName, actionName } = ctx.action; diff --git a/packages/core/acl/src/skip-manager.ts b/packages/core/acl/src/allow-manager.ts similarity index 66% rename from packages/core/acl/src/skip-manager.ts rename to packages/core/acl/src/allow-manager.ts index cf6fccca6..de03b3959 100644 --- a/packages/core/acl/src/skip-manager.ts +++ b/packages/core/acl/src/allow-manager.ts @@ -1,26 +1,39 @@ import { ACL } from './acl'; -type ConditionFunc = (ctx: any) => boolean; +type ConditionFunc = (ctx: any) => Promise; -export class SkipManager { +export class AllowManager { protected skipActions = new Map>(); protected registeredCondition = new Map(); constructor(public acl: ACL) { - this.registerSkipCondition('logged-in', (ctx) => { + this.registerAllowCondition('loggedIn', (ctx) => { return ctx.state.currentUser; }); + + this.registerAllowCondition('allowConfigure', async (ctx) => { + const roleName = ctx.state.currentRole; + if (!roleName) { + return false; + } + + const roleInstance = await ctx.db.getRepository('roles').findOne({ + name: roleName, + }); + + return roleInstance?.get('allowConfigure'); + }); } - skip(resourceName: string, actionName: string, condition?: string | ConditionFunc) { + allow(resourceName: string, actionName: string, condition?: string | ConditionFunc) { const actionMap = this.skipActions.get(resourceName) || new Map(); actionMap.set(actionName, condition || true); this.skipActions.set(resourceName, actionMap); } - getSkippedConditions(resourceName: string, actionName: string): Array { + getAllowedConditions(resourceName: string, actionName: string): Array { const fetchActionSteps: string[] = ['*', resourceName]; const results = []; @@ -38,14 +51,14 @@ export class SkipManager { return results; } - registerSkipCondition(name: string, condition: ConditionFunc) { + registerAllowCondition(name: string, condition: ConditionFunc) { this.registeredCondition.set(name, condition); } aclMiddleware() { return async (ctx, next) => { const { resourceName, actionName } = ctx.action; - const skippedConditions = ctx.app.acl.skipManager.getSkippedConditions(resourceName, actionName); + const skippedConditions = ctx.app.acl.allowManager.getAllowedConditions(resourceName, actionName); let skip = false; for (const skippedCondition of skippedConditions) { @@ -53,7 +66,7 @@ export class SkipManager { let skipResult = false; if (typeof skippedCondition === 'function') { - skipResult = skippedCondition(ctx); + skipResult = await skippedCondition(ctx); } else if (skippedCondition) { skipResult = true; } diff --git a/packages/plugins/acl/src/__tests__/configuration.test.ts b/packages/plugins/acl/src/__tests__/configuration.test.ts new file mode 100644 index 000000000..d769e560e --- /dev/null +++ b/packages/plugins/acl/src/__tests__/configuration.test.ts @@ -0,0 +1,45 @@ +import { MockServer } from '@nocobase/test'; +import { Database } from '@nocobase/database'; +import { ACL } from '@nocobase/acl'; +import { UiSchemaRepository } from '@nocobase/plugin-ui-schema-storage'; +import { changeMockRole, prepareApp } from './prepare'; + +describe('configuration', () => { + let app: MockServer; + let db: Database; + let acl: ACL; + + let uiSchemaRepository: UiSchemaRepository; + + afterEach(async () => { + await app.destroy(); + }); + + beforeEach(async () => { + app = await prepareApp(); + db = app.db; + acl = app.acl; + + uiSchemaRepository = db.getRepository('uiSchemas'); + }); + + it('should list collections', async () => { + expect((await app.agent().resource('collections').create()).statusCode).toEqual(403); + expect((await app.agent().resource('collections').list()).statusCode).toEqual(200); + }); + + it('should allow when role has allowConfigure with true value', async () => { + await db.getRepository('roles').create({ + values: { + name: 'admin1', + title: 'admin allowConfigure', + allowConfigure: true, + }, + }); + + changeMockRole('admin1'); + + expect((await app.agent().resource('collections').create()).statusCode).toEqual(200); + expect((await app.agent().resource('collections').list()).statusCode).toEqual(200); + }); +}); diff --git a/packages/plugins/acl/src/collections/rolesResources.ts b/packages/plugins/acl/src/collections/rolesResources.ts index e3d82243a..8a59485ef 100644 --- a/packages/plugins/acl/src/collections/rolesResources.ts +++ b/packages/plugins/acl/src/collections/rolesResources.ts @@ -3,6 +3,12 @@ import { CollectionOptions } from '@nocobase/database'; export default { name: 'rolesResources', model: 'RoleResourceModel', + indexes: [ + { + unique: true, + fields: ['roleName', 'name'], + }, + ], fields: [ { type: 'belongsTo', diff --git a/packages/plugins/acl/src/model/RoleResourceActionModel.ts b/packages/plugins/acl/src/model/RoleResourceActionModel.ts index fdaf5a0ca..ef2f2ac9b 100644 --- a/packages/plugins/acl/src/model/RoleResourceActionModel.ts +++ b/packages/plugins/acl/src/model/RoleResourceActionModel.ts @@ -14,6 +14,7 @@ export class RoleResourceActionModel extends Model { const db: Database = this.constructor.database; const { resourceName, role, acl, associationFieldsActions, grantHelper } = options; + const actionName = this.get('name') as string; const fields = this.get('fields') as any; @@ -47,17 +48,19 @@ export class RoleResourceActionModel extends Model { const fieldActions: AssociationFieldAction = associationFieldsActions?.[fieldType]?.[availableAction]; + const fieldTarget = collectionField.get('target'); + if (fieldActions) { const associationActions = fieldActions.associationActions || []; associationActions.forEach((associationAction) => { - const actionName = `${resourceName}.${field}:${associationAction}`; + const actionName = `${resourceName}.${fieldTarget}:${associationAction}`; role.grantAction(actionName); }); const targetActions = fieldActions.targetActions || []; targetActions.forEach((targetAction) => { - const targetActionPath = `${field}:${targetAction}`; + const targetActionPath = `${fieldTarget}:${targetAction}`; grantHelper.resourceTargetActionMap.set(resourceName, [ ...(grantHelper.resourceTargetActionMap.get(resourceName) || []), diff --git a/packages/plugins/acl/src/model/RoleResourceModel.ts b/packages/plugins/acl/src/model/RoleResourceModel.ts index a75a5301d..8800349b2 100644 --- a/packages/plugins/acl/src/model/RoleResourceModel.ts +++ b/packages/plugins/acl/src/model/RoleResourceModel.ts @@ -35,6 +35,7 @@ export class RoleResourceModel extends Model { const roleName = this.get('roleName') as string; const role = acl.getRole(roleName); + // revoke resource of role await this.revoke({ role, resourceName, grantHelper }); // @ts-ignore diff --git a/packages/plugins/acl/src/server.ts b/packages/plugins/acl/src/server.ts index b9dfa80d1..85dcfe4d7 100644 --- a/packages/plugins/acl/src/server.ts +++ b/packages/plugins/acl/src/server.ts @@ -293,11 +293,13 @@ export class PluginACL extends Plugin { ], }); }); - this.app.acl.skip('roles.menuUiSchemas', 'set', 'logged-in'); - this.app.acl.skip('roles.menuUiSchemas', 'toggle', 'logged-in'); - this.app.acl.skip('roles.menuUiSchemas', 'list', 'logged-in'); - this.app.acl.skip('roles', 'check', 'logged-in'); - this.app.acl.skip('*', '*', (ctx) => { + + this.app.acl.allow('roles', 'check', 'loggedIn'); + this.app.acl.allow('roles', ['create', 'update', 'destroy'], 'allowConfigure'); + + this.app.acl.allow('roles.menuUiSchemas', ['set', 'toggle', 'list'], 'allowConfigure'); + + this.app.acl.allow('*', '*', (ctx) => { return ctx.state.currentRole === 'root'; }); diff --git a/packages/plugins/china-region/src/server.ts b/packages/plugins/china-region/src/server.ts index 7b98dfad6..7a95837c7 100644 --- a/packages/plugins/china-region/src/server.ts +++ b/packages/plugins/china-region/src/server.ts @@ -11,7 +11,7 @@ export class ChinaRegionPlugin extends Plugin { await this.db.import({ directory: resolve(__dirname, 'collections'), }); - this.app.acl.skip('chinaRegions', 'list', 'logged-in'); + this.app.acl.allow('chinaRegions', 'list', 'loggedIn'); } async importData() { diff --git a/packages/plugins/collection-manager/src/plugin.ts b/packages/plugins/collection-manager/src/plugin.ts index 8190ac444..6d1a950e6 100644 --- a/packages/plugins/collection-manager/src/plugin.ts +++ b/packages/plugins/collection-manager/src/plugin.ts @@ -235,7 +235,8 @@ export class CollectionManagerPlugin extends Plugin { await next(); }); - this.app.acl.skip('collections', 'list', 'logged-in'); + this.app.acl.allow('collections', 'list', 'loggedIn'); + this.app.acl.allow('collections', ['create', 'update', 'destroy'], 'allowConfigure'); } async load() { diff --git a/packages/plugins/file-manager/src/server.ts b/packages/plugins/file-manager/src/server.ts index e2ce2ea66..1bea76639 100644 --- a/packages/plugins/file-manager/src/server.ts +++ b/packages/plugins/file-manager/src/server.ts @@ -36,7 +36,7 @@ export default class PluginFileManager extends Plugin { await getStorageConfig(STORAGE_TYPE_LOCAL).middleware(this.app); } - this.app.acl.skip('attachments', 'upload', 'logged-in'); + this.app.acl.allow('attachments', 'upload', 'loggedIn'); } getName(): string { diff --git a/packages/plugins/ui-schema-storage/src/server.ts b/packages/plugins/ui-schema-storage/src/server.ts index f1c7339ec..25dff32ff 100644 --- a/packages/plugins/ui-schema-storage/src/server.ts +++ b/packages/plugins/ui-schema-storage/src/server.ts @@ -61,8 +61,8 @@ export class UiSchemaStoragePlugin extends Plugin { actions: uiSchemaActions, }); - this.app.acl.skip('uiSchemas', '*', 'logged-in'); - this.app.acl.skip('uiSchemaTemplates', '*', 'logged-in'); + this.app.acl.allow('uiSchemas', '*', 'loggedIn'); + this.app.acl.allow('uiSchemaTemplates', '*', 'loggedIn'); } async load() { diff --git a/packages/plugins/users/src/__tests__/fields.test.ts b/packages/plugins/users/src/__tests__/fields.test.ts index a2c66f59b..346ce50d7 100644 --- a/packages/plugins/users/src/__tests__/fields.test.ts +++ b/packages/plugins/users/src/__tests__/fields.test.ts @@ -11,6 +11,7 @@ describe('createdBy/updatedBy', () => { api = mockServer(); api.plugin(UsersPlugin, userPluginConfig); api.plugin(PluginACL); + await api.cleanDb(); await api.loadAndInstall(); db = api.db; }); diff --git a/packages/plugins/users/src/server.ts b/packages/plugins/users/src/server.ts index 7da44c3e6..54e0cfcd9 100644 --- a/packages/plugins/users/src/server.ts +++ b/packages/plugins/users/src/server.ts @@ -96,8 +96,8 @@ export default class UsersPlugin extends Plugin { const publicActions = ['check', 'signin', 'signup', 'lostpassword', 'resetpassword', 'getUserByResetToken']; const loggedInActions = ['signout', 'updateProfile', 'changePassword', 'setDefaultRole']; - publicActions.forEach((action) => this.app.acl.skip('users', action)); - loggedInActions.forEach((action) => this.app.acl.skip('users', action, 'logged-in')); + publicActions.forEach((action) => this.app.acl.allow('users', action)); + loggedInActions.forEach((action) => this.app.acl.allow('users', action, 'loggedIn')); } async load() {