From 1ca4e9620f5b0bb5abc5e27377519b6bd711279e Mon Sep 17 00:00:00 2001 From: Coki Date: Sun, 29 Sep 2024 19:38:37 +0800 Subject: [PATCH 1/4] feat: add validation for matchers in config parsing --- src/config.ts | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/config.ts b/src/config.ts index 674f1e7..c326965 100644 --- a/src/config.ts +++ b/src/config.ts @@ -164,9 +164,43 @@ export class Config implements ConfigInterface { if (equalIndex === -1) { throw new Error(`parse the content error : line ${lineNum}`); } - const key = line.substring(0, equalIndex); - const value = line.substring(equalIndex + 1); - this.addConfig(section, key.trim(), value.trim()); + const key = line.substring(0, equalIndex).trim(); + const value = line.substring(equalIndex + 1).trim(); + + if (section === 'matchers') { + this.validateMatcher(value, lineNum); + } + + this.addConfig(section, key, value); + } + + private validateMatcher(matcherStr: string, lineNumber: number): void { + const errors: string[] = []; + + const validProps = ['r.sub', 'r.obj', 'r.act', 'p.sub', 'p.obj', 'p.act', 'p.eft']; + const usedProps = matcherStr.match(/[rp]\.\w+/g) || []; + const invalidProps = usedProps.filter((prop) => !validProps.includes(prop)); + if (invalidProps.length > 0) { + errors.push(`Invalid properties: ${invalidProps.join(', ')}`); + } + + if (matcherStr.includes('..')) { + errors.push('Found extra dots'); + } + + if (matcherStr.trim().endsWith(',')) { + errors.push('Unnecessary comma'); + } + + const openBrackets = (matcherStr.match(/\(/g) || []).length; + const closeBrackets = (matcherStr.match(/\)/g) || []).length; + if (openBrackets !== closeBrackets) { + errors.push('Mismatched parentheses'); + } + + if (errors.length > 0) { + throw new Error(`${errors.join(', ')}`); + } } public getBool(key: string): boolean { From 0a58349e6217f029836f163c93e161bc3f7a0985 Mon Sep 17 00:00:00 2001 From: Coki Date: Sun, 29 Sep 2024 20:00:11 +0800 Subject: [PATCH 2/4] feat: add new properties to validProps in Config class --- src/config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.ts b/src/config.ts index c326965..9bae5c4 100644 --- a/src/config.ts +++ b/src/config.ts @@ -177,7 +177,7 @@ export class Config implements ConfigInterface { private validateMatcher(matcherStr: string, lineNumber: number): void { const errors: string[] = []; - const validProps = ['r.sub', 'r.obj', 'r.act', 'p.sub', 'p.obj', 'p.act', 'p.eft']; + const validProps = ['r.sub', 'r.obj', 'r.act', 'r.dom', 'p.sub', 'p.obj', 'p.act', 'p.dom', 'p.eft', 'p.sub_rule']; const usedProps = matcherStr.match(/[rp]\.\w+/g) || []; const invalidProps = usedProps.filter((prop) => !validProps.includes(prop)); if (invalidProps.length > 0) { From d6708c34735aaed27d571de852d788c0f2643034 Mon Sep 17 00:00:00 2001 From: Coki Date: Tue, 8 Oct 2024 14:24:07 +0800 Subject: [PATCH 3/4] refactor: move validation logic to ValidatorEnforcer class --- src/config.ts | 38 +++--------------- src/model/model.ts | 30 ++++---------- src/validatorEnforcer.ts | 85 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 56 deletions(-) create mode 100644 src/validatorEnforcer.ts diff --git a/src/config.ts b/src/config.ts index 9bae5c4..09ff347 100644 --- a/src/config.ts +++ b/src/config.ts @@ -13,6 +13,7 @@ // limitations under the License. import { FileSystem, mustGetDefaultFileSystem } from './persist'; +import { ValidatorEnforcer } from './validatorEnforcer'; // ConfigInterface defines the behavior of a Config implementation export interface ConfigInterface { @@ -140,7 +141,7 @@ export class Config implements ConfigInterface { } section = line.substring(1, line.length - 1); if (seenSections.has(section)) { - throw new Error(`Duplicated section: ${section} at line ${lineNumber}`); + ValidatorEnforcer.validateDuplicateSection(section, lineNumber); } seenSections.add(section); } else { @@ -162,47 +163,18 @@ export class Config implements ConfigInterface { private write(section: string, lineNum: number, line: string): void { const equalIndex = line.indexOf('='); if (equalIndex === -1) { - throw new Error(`parse the content error : line ${lineNum}`); + ValidatorEnforcer.validateContentParse(lineNum); } const key = line.substring(0, equalIndex).trim(); const value = line.substring(equalIndex + 1).trim(); if (section === 'matchers') { - this.validateMatcher(value, lineNum); + ValidatorEnforcer.validateMatcher(value); } this.addConfig(section, key, value); } - private validateMatcher(matcherStr: string, lineNumber: number): void { - const errors: string[] = []; - - const validProps = ['r.sub', 'r.obj', 'r.act', 'r.dom', 'p.sub', 'p.obj', 'p.act', 'p.dom', 'p.eft', 'p.sub_rule']; - const usedProps = matcherStr.match(/[rp]\.\w+/g) || []; - const invalidProps = usedProps.filter((prop) => !validProps.includes(prop)); - if (invalidProps.length > 0) { - errors.push(`Invalid properties: ${invalidProps.join(', ')}`); - } - - if (matcherStr.includes('..')) { - errors.push('Found extra dots'); - } - - if (matcherStr.trim().endsWith(',')) { - errors.push('Unnecessary comma'); - } - - const openBrackets = (matcherStr.match(/\(/g) || []).length; - const closeBrackets = (matcherStr.match(/\)/g) || []).length; - if (openBrackets !== closeBrackets) { - errors.push('Mismatched parentheses'); - } - - if (errors.length > 0) { - throw new Error(`${errors.join(', ')}`); - } - } - public getBool(key: string): boolean { return !!this.get(key); } @@ -226,7 +198,7 @@ export class Config implements ConfigInterface { public set(key: string, value: string): void { if (!key) { - throw new Error('key is empty'); + ValidatorEnforcer.validateEmptyKey(); } let section = ''; diff --git a/src/model/model.ts b/src/model/model.ts index 0ec9050..0c47dfd 100644 --- a/src/model/model.ts +++ b/src/model/model.ts @@ -20,6 +20,7 @@ import { getLogger, logPrint } from '../log'; import { DefaultRoleManager } from '../rbac'; import { EffectExpress, FieldIndex } from '../constants'; import { FileSystem } from '../persist/fileSystem'; +import { ValidatorEnforcer } from '../validatorEnforcer'; const defaultDomain = ''; const defaultSeparator = '::'; @@ -108,10 +109,7 @@ export class Model { value = value.replace(`$<${index}>`, n); }); - const invalidOperators = /(? { - if (!this.hasSection(n)) { - ms.push(sectionNameMap[n]); - } - }); - - if (ms.length > 0) { - throw new Error(`missing required sections: ${ms.join(',')}`); - } + ValidatorEnforcer.validateRequiredSections(this.model); } private hasSection(sec: string): boolean { @@ -317,13 +306,8 @@ export class Model { const priorityIndex = ast.tokens.indexOf(`${ptype}_priority`); if (priorityIndex !== -1) { - if (oldRule[priorityIndex] === newRule[priorityIndex]) { - ast.policy[index] = newRule; - } else { - // this.removePolicy(sec, ptype, oldRule); - // this.addPolicy(sec, ptype, newRule); - throw new Error('new rule should have the same priority with old rule.'); - } + ValidatorEnforcer.validatePolicyPriority(oldRule, newRule, priorityIndex); + ast.policy[index] = newRule; } else { ast.policy[index] = newRule; } @@ -594,14 +578,14 @@ export class Model { export function newModel(...text: string[]): Model { const m = new Model(); + ValidatorEnforcer.validateModelParameters(text.length); + if (text.length === 2) { if (text[0] !== '') { m.loadModelFromFile(text[0]); } } else if (text.length === 1) { m.loadModelFromText(text[0]); - } else if (text.length !== 0) { - throw new Error('Invalid parameters for model.'); } return m; diff --git a/src/validatorEnforcer.ts b/src/validatorEnforcer.ts new file mode 100644 index 0000000..bc85a7b --- /dev/null +++ b/src/validatorEnforcer.ts @@ -0,0 +1,85 @@ +import { sectionNameMap, requiredSections } from './model/model'; + +export class ValidatorEnforcer { + // Verify matcher + public static validateMatcher(matcherStr: string): void { + const errors: string[] = []; + + const validProps = ['r.sub', 'r.obj', 'r.act', 'r.dom', 'p.sub', 'p.obj', 'p.act', 'p.dom', 'p.eft', 'p.sub_rule']; + const usedProps = matcherStr.match(/[rp]\.\w+/g) || []; + const invalidProps = usedProps.filter((prop) => !validProps.includes(prop)); + if (invalidProps.length > 0) { + errors.push(`Invalid properties: ${invalidProps.join(', ')}`); + } + + if (matcherStr.includes('..')) { + errors.push('Found extra dots'); + } + + if (matcherStr.trim().endsWith(',')) { + errors.push('Unnecessary comma'); + } + + const openBrackets = (matcherStr.match(/\(/g) || []).length; + const closeBrackets = (matcherStr.match(/\)/g) || []).length; + if (openBrackets !== closeBrackets) { + errors.push('Mismatched parentheses'); + } + + const invalidOperators = /(? 0) { + throw new Error(`${errors.join(', ')}`); + } + } + + // Verify policy priority + public static validatePolicyPriority(oldRule: string[], newRule: string[], priorityIndex: number): void { + if (oldRule[priorityIndex] !== newRule[priorityIndex]) { + throw new Error('new rule should have the same priority with old rule.'); + } + } + + // Verify required sections + public static validateRequiredSections(model: Map>): void { + const missingSections = requiredSections.filter((section) => !model.has(section)); + + if (missingSections.length > 0) { + const missingNames = missingSections.map((s) => sectionNameMap[s]); + throw new Error(`missing required sections: ${missingNames.join(',')}`); + } + } + + // Verify duplicate section + public static validateDuplicateSection(section: string, lineNumber: number): void { + throw new Error(`Duplicated section: ${section} at line ${lineNumber}`); + } + + // Verify content parse + public static validateContentParse(lineNum: number): void { + throw new Error(`parse the content error : line ${lineNum}`); + } + + // Verify empty key + public static validateEmptyKey(): void { + throw new Error('key is empty'); + } + + // Verify operator in matcher + public static validateMatcherOperators(value: string): void { + const invalidOperators = /(? Date: Tue, 8 Oct 2024 16:14:59 +0800 Subject: [PATCH 4/4] test: add unit tests for ValidatorEnforcer --- test/validatorEnforcer.test.ts | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 test/validatorEnforcer.test.ts diff --git a/test/validatorEnforcer.test.ts b/test/validatorEnforcer.test.ts new file mode 100644 index 0000000..152e678 --- /dev/null +++ b/test/validatorEnforcer.test.ts @@ -0,0 +1,79 @@ +import { ValidatorEnforcer } from '../src/validatorEnforcer'; + +describe('ValidatorEnforcer', () => { + describe('validateMatcher', () => { + it('should not throw error for valid matcher', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('r.sub == p.sub && r.obj == p.obj && r.act == p.act'); + }).not.toThrow(); + }); + + it('should throw error for invalid properties', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('r.invalid == p.sub'); + }).toThrow('Invalid properties: r.invalid'); + }); + + it('should throw error for extra dots', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('r..sub == p.sub'); + }).toThrow('Found extra dots'); + }); + + it('should throw error for unnecessary comma', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('r.sub == p.sub,'); + }).toThrow('Unnecessary comma'); + }); + + it('should throw error for mismatched parentheses', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('(r.sub == p.sub'); + }).toThrow('Mismatched parentheses'); + }); + + it('should throw error for invalid operators', () => { + expect(() => { + ValidatorEnforcer.validateMatcher('r.sub & p.sub'); + }).toThrow('Invalid operator in matcher'); + }); + }); + + describe('validatePolicyPriority', () => { + it('should not throw error for same priority', () => { + expect(() => { + ValidatorEnforcer.validatePolicyPriority(['alice', 'data1', 'read', '1'], ['bob', 'data2', 'write', '1'], 3); + }).not.toThrow(); + }); + + it('should throw error for different priority', () => { + expect(() => { + ValidatorEnforcer.validatePolicyPriority(['alice', 'data1', 'read', '1'], ['bob', 'data2', 'write', '2'], 3); + }).toThrow('new rule should have the same priority with old rule.'); + }); + }); + + describe('validateRequiredSections', () => { + it('should not throw error when all required sections are present', () => { + const model = new Map([ + ['r', new Map()], + ['p', new Map()], + ['e', new Map()], + ['m', new Map()], + ]); + expect(() => { + ValidatorEnforcer.validateRequiredSections(model); + }).not.toThrow(); + }); + + it('should throw error when required sections are missing', () => { + const model = new Map([ + ['r', new Map()], + ['p', new Map()], + ]); + expect(() => { + ValidatorEnforcer.validateRequiredSections(model); + }).toThrow('missing required sections: policy_effect,matchers'); + }); + }); +});