From c7629a69bddaf464878deeb6f44ec7d4d61f5d54 Mon Sep 17 00:00:00 2001 From: Adrian Tam Date: Tue, 25 Oct 2022 11:21:52 -0400 Subject: [PATCH] fix(parse-dsl): fix issue with infinite loops triggered when parsing some models (#76) * fix: adding test for parseDSL with spaces * fix: handle a case of an infinite loop while parsing the grammar Co-authored-by: Raghd Hamzeh --- src/openfga.ne | 43 ++-- src/parse-dsl.ts | 15 +- tests/__snapshots__/index.test.ts.snap | 8 +- tests/dsl.test.ts | 317 ++++++++++++++++++++++++- tests/index.test.ts | 8 +- 5 files changed, 360 insertions(+), 31 deletions(-) diff --git a/src/openfga.ne b/src/openfga.ne index 022ceaf..d605096 100644 --- a/src/openfga.ne +++ b/src/openfga.ne @@ -1,11 +1,12 @@ @preprocessor typescript -types -> (_newline):* (type (relations (define):+):*):* {% +types -> (_newline):* (type (_relations (_multiline_comment define):+):*):* {% // @ts-ignore (data) => { // @ts-ignore const types = data[1].map((datum) => { - const relations = datum[1][0] ? datum[1][0][1].flat() : []; + // @ts-ignore + const relations = (datum[1] && datum[1][0] && datum[1][0][1].map(innerDatum => innerDatum[1])) || []; return { ...datum[0], relations } }) @@ -18,25 +19,29 @@ types -> (_newline):* (type (relations (define):+):*):* {% } %} -type -> _multiline_comment _type _naming (_newline):+ {% +type -> _multiline_comment _type _naming (_newline):+{% data => ({ comment: data[0], type: data[2] }) %} -relations -> _multiline_comment _relations {% - data => data[1] +relations -> _relations {% + data => data[0] %} -define -> (_newline):+ _multiline_comment define_initial (_relation_types):? _as (define_base | define_or | define_and | define_but_not) (_newline):* {% +define -> _newline define_initial (_spacing | _relation_types) _as _spacing (define_base | define_or | define_and | define_but_not) (_newline):* {% (data, _location, reject) => { - const relation = data[2]; - + const relation = data[1]; + // Not supported yet + const comment = ""; const def = data[5][0]; const definition = def.type ? def : { type: 'single', targets: [def] } - const allowedTypes = data[3] ? data[3][0] : [] + let allowedTypes = data[2] ? data[2][0] : []; + if (allowedTypes.length && typeof allowedTypes[0] !== "string") { + allowedTypes = []; + } - return { comment: data[1], allowedTypes, relation, definition }; + return { comment, allowedTypes, relation, definition }; } %} @@ -44,9 +49,9 @@ define_initial -> _define _naming {% data => data[1] %} -define_base -> _optional_space (_naming | from_phrase) _optional_space {% +define_base -> (_naming | from_phrase) {% data => { - const entry = data[1][0]; + const entry = data[0][0]; let target, rewrite, from if (typeof entry === "string") { if (entry === "self") { @@ -72,8 +77,8 @@ define_and -> define_base (_spacing _and _spacing define_base):+ {% // @ts-ignore data => ({ targets: [data[0], ...data[1].map((datum) => datum[3])], type: "intersection" }) %} -define_but_not -> define_base _but_not define_base {% - data => ({ base: data[0], diff: data[2], type: "exclusion" }) +define_but_not -> define_base _spacing _but_not _spacing define_base {% + data => ({ base: data[0], diff: data[4], type: "exclusion" }) %} from_phrase -> _naming _spacing _from _spacing _naming {% data => ({ target: data[0], from: data[4] }) @@ -82,15 +87,15 @@ from_phrase -> _naming _spacing _from _spacing _naming {% _multiline_comment -> (_comment):* {% data => data.flat(3).join('\n') %} -_comment -> " ":* "#" _spacing _naming (_spacing _word):* _newline {% +_comment -> (_newline):? _optional_space "#" _optional_space _word (_spacing _word):* (_newline):? {% data => data.flat(3).join('').trim().substring(1).trim() %} _word -> ([a-z] | [A-Z] | [0-9] | "_" | "-" | "," | "&" | "+" | "/" | "$" ):+ _optional_space {% data => data.flat(3).join('').trim() %} -_relation_types -> ":" _optional_space "[" _array_of_types "]" _spacing {% - data => data[3] +_relation_types -> _optional_space ":" _optional_space "[" _array_of_types "]" _spacing {% + data => data[4] %} _array_of_types -> ("$"):? ([a-zA-Z0-9_#\-,\s]):+ {% @@ -105,10 +110,10 @@ _but_not -> "but not" _self -> "self" _define -> " define" _spacing -_relations -> " relations" _optional_space +_relations -> " relations" (_newline):* _type -> "type" _spacing _no_relations -> "none" (_newline):* -_naming -> (("$"):? ( [a-z] | [A-Z] | [0-9] | "_" | "-" ):+) _optional_space {% +_naming -> (("$"):? ( [a-z] | [A-Z] | [0-9] | "_" | "-" ):+) {% data => data.flat(3).join('').trim() %} _optional_space -> " ":* diff --git a/src/parse-dsl.ts b/src/parse-dsl.ts index 3ca88f5..df48e8a 100644 --- a/src/parse-dsl.ts +++ b/src/parse-dsl.ts @@ -44,10 +44,19 @@ export interface ParserResult { types: TypeDefParserResult[]; } -export const parseDSL = (code: string): ParserResult => { +export const innerParseDSL = (code: string): ParserResult[] => { const parser = new Parser(Grammar.fromCompiled(grammar)); - parser.feed(code.trim() + "\n"); - return parser.results[0] || []; + const cleanedCode = + code + .split("\n") + .map((line) => line.trimEnd()) + .join("\n") + "\n"; + parser.feed(cleanedCode); + return parser.results; +}; + +export const parseDSL = (code: string): ParserResult => { + return innerParseDSL(code)[0] || []; }; // The TransformedType allows us to quickly access the various relations unique by diff --git a/tests/__snapshots__/index.test.ts.snap b/tests/__snapshots__/index.test.ts.snap index b2bc6c8..16ccfc0 100644 --- a/tests/__snapshots__/index.test.ts.snap +++ b/tests/__snapshots__/index.test.ts.snap @@ -28,7 +28,7 @@ exports[`apiSyntaxToFriendlySyntax should correctly read from \`or\` definition " `; -exports[`apiSyntaxToFriendlySyntax should read a complex definition 1`] = ` +exports[`apiSyntaxToFriendlySyntax should read a complex definition 4 1`] = ` "type folder relations define deleter as self @@ -212,7 +212,7 @@ exports[`friendlySyntaxToApiSyntax() should correctly read from \`or\` definitio } `; -exports[`friendlySyntaxToApiSyntax() should read a complex definition 1`] = ` +exports[`friendlySyntaxToApiSyntax() should read a complex definition 1 1`] = ` { "schema_version": "1.0", "type_definitions": [ @@ -327,7 +327,7 @@ exports[`friendlySyntaxToApiSyntax() should read a complex definition 1`] = ` } `; -exports[`friendlySyntaxToApiSyntax() should read a complex definition 2`] = ` +exports[`friendlySyntaxToApiSyntax() should read a complex definition 2 1`] = ` { "schema_version": "1.0", "type_definitions": [ @@ -415,7 +415,7 @@ exports[`friendlySyntaxToApiSyntax() should read a complex definition 2`] = ` } `; -exports[`friendlySyntaxToApiSyntax() should read a complex definition 3`] = ` +exports[`friendlySyntaxToApiSyntax() should read a complex definition 3 1`] = ` { "schema_version": "1.0", "type_definitions": [ diff --git a/tests/dsl.test.ts b/tests/dsl.test.ts index 38fc877..a7bc40f 100644 --- a/tests/dsl.test.ts +++ b/tests/dsl.test.ts @@ -1,5 +1,5 @@ import { checkDSL } from "../src"; -import { parseDSL } from "../src/parse-dsl"; +import { innerParseDSL, parseDSL } from "../src/parse-dsl"; describe("DSL", () => { describe("parseDSL()", () => { @@ -49,6 +49,321 @@ type app }); }); + describe("innerParseDSL()", () => { + it("should only return single result for simple valid model", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with non direct result", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with non direct result and spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with intersection", () => { + const result = innerParseDSL(`type team + relations + define member as self + define myfriend as self + define other as self or member or myfriend +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with intersection and spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as self or member or myfriend +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with union", () => { + const result = innerParseDSL(`type team + relations + define member as self + define myfriend as self + define other as self and member and myfriend +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with union and spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define myfriend as self + define other as self and member and myfriend +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with but not", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as self but not member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with but not and spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define other as self but not member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with tuple to userset", () => { + const result = innerParseDSL(`type team + relations + define viewer as self + define parent as self + define can_view as viewer from parent +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with tuple to userset and spaces", () => { + const result = innerParseDSL(`type team + relations + define viewer as self + define parent as self + define can_view as viewer from parent +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with but not + tuple to userset", () => { + const result = innerParseDSL(`type team + relations + define member as self + define parent as self + define viewer as self + define other as viewer from parent but not member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with but not + tuple to userset and spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + define parent as self + define viewer as self + define other as viewer from parent but not member +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with multiple types", () => { + const result = innerParseDSL(`type team + relations + define viewer as self +type group + relations + define parent as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with multiple types and empty lines", () => { + const result = innerParseDSL(`type team + relations + define viewer as self + + +type group + relations + define parent as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple valid model with multiple types and empty lines + spaces", () => { + const result = innerParseDSL(`type team + relations + define viewer as self + + +type group + relations + define parent as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple 1.1 valid model", () => { + const result = innerParseDSL(`type user +type team + relations + define member: [user] as self + define other: [user] as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for simple 1.1 valid model with spaces", () => { + const result = innerParseDSL(`type user +type team + relations + define member: [user] as self + define other: [user] as self +`); + expect(result.length).toEqual(1); + }); + + it.skip("should only return single result for simple 1.1 valid model with comment", () => { + const result = innerParseDSL(`type user +type team + relations + define member: [user] as self + # Comment for other + define other: [user] as self +`); + expect(result.length).toEqual(1); + }); + + it.skip("should only return single result for simple 1.1 valid model with comment and spaces at the end", () => { + const result = innerParseDSL(`type user +type team + relations + define member: [user] as self + # Comment for other + define other: [user] as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for complex valid model", () => { + const result = innerParseDSL(`type team + relations + define member as self + +type repo + relations + define admin as self or repo_admin from owner + define maintainer as self or admin + define owner as self + define reader as self or triager or repo_reader from owner + define triager as self or writer + define writer as self or maintainer or repo_writer from owner + +type org + relations + define billing_manager as self or owner + define member as self or owner + define owner as self + define repo_admin as self + define repo_reader as self + define repo_writer as self + +type app + relations + define app_manager as self or owner from owner + define owner as self +`); + expect(result.length).toEqual(1); + }); + + it("should only return single result for complex model with spaces", () => { + const result = innerParseDSL(`type team + relations + define member as self + +type repo + relations + + define admin as self or repo_admin from owner + define maintainer as self or admin + define owner as self + define reader as self or triager or repo_reader from owner + define triager as self or writer + define writer as self or maintainer or repo_writer from owner + +type org + relations + define billing_manager as self or owner + define member as self or owner + define owner as self + define repo_admin as self + define repo_reader as self + define repo_writer as self + +type app + relations + define app_manager as self or owner from owner + define owner as self +`); + expect(result.length).toEqual(1); + }); + + it("should parse DSL in reasonable time", () => { + const time1 = new Date(); + // Add in addition `define R as X from Y but not Z` to increase the time + const result = innerParseDSL(`type T1 + relations + define A as A1 from A2 but not A3 + define B as B1 from B2 but not B3 + define C as C1 from C2 but not C3 + define D as D1 from D2 but not D3 +type T2 + relations + define A as A1 from A2 but not A3 + define B as B1 from B2 but not B3 + define C as C1 from C2 but not C3 + define D as D1 from D2 but not D3 +type T3 + relations + define A as A1 from A2 but not A3 + define B as B1 from B2 but not B3 + define C as C1 from C2 but not C3 + define D as D1 from D2 but not D3 +type T4 + relations + define A as A1 from A2 but not A3 + define B as B1 from B2 but not B3 + define C as C1 from C2 but not C3 + define D as D1 from D2 but not D3 +type T5 + relations + define A as A1 from A2 but not A3 +`); + const time2 = new Date(); + expect(result.length).toEqual(1); + expect(time2.getTime() - time1.getTime()).toBeLessThan(1000); + }); + }); + describe("checkDSL()", () => { it("should correctly parse a simple sample", () => { const markers = checkDSL(`type group diff --git a/tests/index.test.ts b/tests/index.test.ts index 8d30e64..e0d2c2a 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -163,7 +163,7 @@ describe("friendlySyntaxToApiSyntax()", () => { expect(result).toMatchSnapshot(); }); - it("should read a complex definition", () => { + it("should read a complex definition 1", () => { const result = friendlySyntaxToApiSyntax(`type folder relations define deleter as self @@ -184,7 +184,7 @@ type doc expect(result).toMatchSnapshot(); }); - it("should read a complex definition", () => { + it("should read a complex definition 2", () => { const result = friendlySyntaxToApiSyntax(`type website relations define admin as self or owner @@ -198,7 +198,7 @@ type doc expect(result).toMatchSnapshot(); }); - it("should read a complex definition", () => { + it("should read a complex definition 3", () => { const result = friendlySyntaxToApiSyntax(`type website relations define admin as self or randy @@ -314,7 +314,7 @@ describe("apiSyntaxToFriendlySyntax", () => { expect(result).toMatchSnapshot(); }); - it("should read a complex definition", () => { + it("should read a complex definition 4", () => { const result = apiSyntaxToFriendlySyntax({ type_definitions: [ {