feat: add detection for more model errors

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,11 +163,16 @@ 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);
-    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') {
+      ValidatorEnforcer.validateMatcher(value);
+    }
+
+    this.addConfig(section, key, value);
   }
 
   public getBool(key: string): boolean {
@@ -192,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 = '';

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 = /(?<![&|])&(?!&)|(?<![&|])\|(?!\|)|&{3,}|\|{3,}/g;
-      if (invalidOperators.test(value)) {
-        throw new Error(`Invalid operator in matcher`);
-      }
+      ValidatorEnforcer.validateMatcherOperators(value);
 
       ast.value = value;
     } else {
@@ -164,16 +162,7 @@ export class Model {
       this.loadSection(cfg, s);
     }
 
-    const ms: string[] = [];
-    requiredSections.forEach((n) => {
-      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;

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 = /(?<![&|])&(?!&)|(?![&|])\|(?!\|)|&{3,}|\|{3,}/g;
+    if (invalidOperators.test(matcherStr)) {
+      errors.push('Invalid operator in matcher');
+    }
+
+    if (errors.length > 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<string, Map<string, any>>): 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 = /(?<![&|])&(?!&)|(?![&|])\|(?!\|)|&{3,}|\|{3,}/g;
+    if (invalidOperators.test(value)) {
+      throw new Error(`Invalid operator in matcher`);
+    }
+  }
+
+  // Verify model parameters
+  public static validateModelParameters(textLength: number): void {
+    if (textLength !== 0 && textLength !== 1 && textLength !== 2) {
+      throw new Error('Invalid parameters for model');
+    }
+  }
+}

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');
+    });
+  });
+});