diff --git a/docs/validation/model/README.md b/docs/validation/model/README.md index 4f187507..0f743691 100644 --- a/docs/validation/model/README.md +++ b/docs/validation/model/README.md @@ -34,7 +34,7 @@ OpenFGA model validation ensures that authorization models are syntactically cor | `invalid-type` | Semantic | Invalid type in relation definition | [invalid-type.md](./invalid-type.md) | | `relation-no-entry-point` | Semantic | Relation has no entry point for assignment | [relation-no-entry-point.md](./relation-no-entry-point.md) | | `cyclic-error` | Semantic | Circular dependency in relations | [cyclic-error.md](./cyclic-error.md) | -| `cyclic-relation` | Semantic | Circular relation dependency detected | [cyclic-relation.md](./cyclic-relation.md) | +| `cyclic-relation` | Semantic | Relation takes part in a cycle that cannot be resolved | [cyclic-relation.md](./cyclic-relation.md) | | `invalid-relation-on-tupleset` | Structure | Invalid relation in tuple-to-userset | [invalid-relation-on-tupleset.md](./invalid-relation-on-tupleset.md) | | `tupleuserset-not-direct` | Structure | Tuple-to-userset must have direct assignment | [tupleuserset-not-direct.md](./tupleuserset-not-direct.md) | | `invalid-wildcard-error` | Wildcard | Invalid wildcard usage in relation | [invalid-wildcard-error.md](./invalid-wildcard-error.md) | @@ -46,16 +46,17 @@ OpenFGA model validation ensures that authorization models are syntactically cor | `multiple-modules-in-file` | Multi-file | Multiple modules detected in single file | [multiple-modules-in-file.md](./multiple-modules-in-file.md) | | `invalid-schema` | Schema | Unrecognised schema version | [invalid-schema.md](./invalid-schema.md) | | `invalid-syntax` | Syntax | Invalid DSL syntax | [invalid-syntax.md](./invalid-syntax.md) | -| `graph-model-unbuildable` | Semantic | Model cannot be built into a weighted graph | [graph-model-unbuildable.md](./graph-model-unbuildable.md) | +| `graph-model-unbuildable` | Semantic | Model cannot be built into a weighted graph, and no per-relation check accounts for it | [graph-model-unbuildable.md](./graph-model-unbuildable.md) | -Five of the codes above are declared but never emitted, so no validation output -carries them: `invalid-schema-version`, `self-error`, `invalid-syntax`, `cyclic-error` -and `cyclic-relation`. An unrecognised schema version reports `invalid-schema`, and a -cycle with no entrypoint reports `relation-no-entry-point`. Their pages are kept -because each is a published URL. +Four of the codes above are declared but never emitted, so no validation output +carries them: `invalid-schema-version`, `self-error`, `invalid-syntax` and +`cyclic-error`. An unrecognised schema version reports `invalid-schema`, and a cycle +with no entrypoint reports `relation-no-entry-point`. Their pages are kept because each +is a published URL. -`graph-model-unbuildable` is emitted only when graph-backed validation is enabled, -which is not the default. Every other code above is reported whatever the options. +`cyclic-relation` and `graph-model-unbuildable` are emitted only when graph-backed +validation is enabled, which is not the default. Every other code above is reported +whatever the options. ## Usage diff --git a/docs/validation/model/cyclic-relation.md b/docs/validation/model/cyclic-relation.md index 2bcdd4e0..c69d83c1 100644 --- a/docs/validation/model/cyclic-relation.md +++ b/docs/validation/model/cyclic-relation.md @@ -6,22 +6,48 @@ ## Summary -A circular dependency has been detected in relation definitions, creating an infinite loop that prevents proper authorization evaluation. +A relation takes part in a cycle the resolver cannot work through. The relation itself may +be perfectly satisfiable; it is reported for the cycle it belongs to. ## Description -This error occurs when relations reference each other in a circular pattern, creating an infinite loop during authorization evaluation. OpenFGA must be able to resolve all relation dependencies to a finite set of directly assigned users or computed values. +Not every cycle between relations is a problem. `define member: [user, group#member]` is +the nested-group pattern every deployment has, and it terminates because each step around +the loop reads a tuple, so the set of groups to look at shrinks until it is empty. -Circular dependencies can occur: -- **Direct cycles:** `A → B → A` -- **Indirect cycles:** `A → B → C → A` -- **Self-referential cycles:** `A → A` +Two shapes do not terminate, and this error reports the relations taking part in either. -Unlike [`relation-no-entry-point`](./relation-no-entry-point.md), this error specifically focuses on detecting cycles in the relation dependency graph, even when entry points exist. +**The cycle reads no tuple.** Every step is a rewrite, so going round the loop consumes +nothing and gets no closer to an answer: + +``` +define a: [user] or b +define b: [user] or a +``` + +**A step of the cycle is an operand of an `and` or a `but not`.** The cycle does read a +tuple, so it terminates, but the resolver cannot subtract or intersect a set it is still in +the middle of computing: + +``` +define member: [user, group#member] but not blocked +define blocked: [user, group#member] +``` + +Unlike [`relation-no-entry-point`](./relation-no-entry-point.md), this error is not about a +relation that can never be satisfied. In both examples above every relation holds a plain +`[user]`, so every one of them has a way in. That is exactly why a separate code exists: +the entrypoint check has nothing to say about these models, and telling someone to give +`a` an entrypoint it already has would send them looking for the wrong thing. + +`errors.Is` on a finding with this code matches `errors.ErrRelationInUnresolvableCycle`. It +does not match `errors.ErrNoEntrypoints`. + +It is raised only when graph-backed validation is enabled, which is not the default. ## Example -The following model would trigger this error: +The following model reports this error: ``` model @@ -33,18 +59,37 @@ type document relations define viewer: [user] or editor define editor: admin - define admin: viewer # Creates cycle: viewer → editor → admin → viewer + define admin: viewer ``` -**Error Location:** The cycle involves multiple relations forming a circular dependency. +**Error Message:** ``​`viewer` on `document` takes part in a cycle that cannot be resolved: no relation in it reads a tuple, so resolving it never terminates.`` + +One finding is reported per relation in the cycle, each with the line and column of its +`define`, so all three of `viewer`, `editor` and `admin` are reported here. -**Error Message:** `Cyclic relation dependency detected involving relations: viewer, editor, admin` +A cycle under an exclusion reports the other reason: + +``` +model + schema 1.1 + +type user + +type group + relations + define member: [user, group#member] but not blocked + define blocked: [user, group#member] +``` + +**Error Message:** ``​`member` on `group` takes part in a cycle that cannot be resolved: a relation in it is an operand of an `and` or a `but not`.`` ## Resolution -Break the circular dependency by removing or restructuring one of the relation references: +Break the cycle, or take it out of the operator. -### Option 1: Remove problematic reference +### Option 1: give the cycle a rewrite-free step + +For a cycle that reads no tuple, the fix is to stop one relation depending on another: ``` model @@ -55,11 +100,13 @@ type user type document relations define viewer: [user] or editor - define editor: [user] # Remove reference to admin + define editor: [user] define admin: [user] or editor ``` -### Option 2: Restructure hierarchy +### Option 2: restructure into a hierarchy + +Dependencies that all flow one way cannot close a loop: ``` model @@ -70,57 +117,79 @@ type user type document relations define viewer: [user] - define editor: [user] or viewer # Editor includes viewer - define admin: [user] or editor # Admin includes editor (and transitively viewer) + define editor: [user] or viewer + define admin: [user] or editor ``` -### Steps to fix: +### Option 3: move the recursion out of the operand -1. **Identify the cycle:** - - Review the error message to see which relations form the cycle - - Map out the dependency chain +For a cycle under an `and` or a `but not`, the recursion is what has to leave the operator. +Making the other side non-recursive is not enough: `member: [user, group#member] but not +blocked` is still reported even when `blocked` is a plain `[user]`, because `member`'s own +recursion is the operand. Give the recursion its own relation and apply the operator to +that: -2. **Analyze intended authorization hierarchy:** - - Determine the correct permission hierarchy - - Identify which direction relationships should flow +``` +model + schema 1.1 -3. **Break the cycle:** - - Remove one problematic reference - - Restructure to create a proper hierarchy - - Ensure the authorization logic still meets requirements +type user -4. **Validate the solution:** - - Check that all necessary permissions are still achievable - - Verify no new cycles are introduced +type group + relations + define member: [user, group#member] + define blocked: [user] + define visible: member but not blocked +``` -## Common Authorization Patterns +The same applies to an intersection: -### ✅ Valid hierarchical structure: -``` -define viewer: [user] -define editor: [user] or viewer -define admin: [user] or editor -define owner: [user] or admin ``` +model + schema 1.1 -### ❌ Invalid circular structure: -``` -define viewer: editor -define editor: admin -define admin: viewer # Creates cycle +type user + +type group + relations + define admin: [user] + define member: [user, group#member] + define approved: member and admin ``` +### Steps to fix: + +1. **Read which reason it gives:** the clause after the colon says whether the cycle reads + no tuple or sits under an operator. They call for different fixes. + +2. **Use the positions:** every relation in the cycle is reported with its own line, so the + findings together are the cycle. + +3. **Check the recursion, not just the other operand:** for the operator case, a relation + that refers to itself through a userset or a tupleset is a cycle on its own, and + enclosing it in an `and` or a `but not` is what makes it unresolvable. + +4. **Re-validate:** breaking one cycle can leave another, and a relation can sit in more + than one. + ## Related Errors -- [`relation-no-entry-point`](./relation-no-entry-point.md) - When cycles prevent any entry points -- [`cyclic-error`](./cyclic-error.md) - General cyclic dependency error -- [`undefined-relation`](./undefined-relation.md) - When relations in cycle don't exist +- [`relation-no-entry-point`](./relation-no-entry-point.md) - a relation nothing can + satisfy, as against one that is satisfiable but caught in a cycle +- [`graph-model-unbuildable`](./graph-model-unbuildable.md) - a refused build that no + per-relation check could account for +- [`cyclic-error`](./cyclic-error.md) - declared but not raised; a cycle with no entry + point surfaces as `relation-no-entry-point` ## Implementation Notes -This validation is enforced consistently across: -- Go implementation: `pkg/go/validation/cycle_detection.go` -- JavaScript implementation: `pkg/js/validator/validate-dsl.ts` -- Java implementation: Java semantic validation package +This code is specific to the Go implementation's graph-backed validation path. The +JavaScript and Java validators walk the rewrite tree, which answers has-an-entry-point for +both shapes above and reports nothing, so they have no equivalent. + +- Go implementation: `pkg/go/validation/cycle_shape.go` -The cycle detection uses depth-first search with visited node tracking to identify circular dependencies in the relation graph. +The weighted graph refuses both shapes, with `ErrModelCycle` and `ErrTupleCycle`, and names +no relation in either. The check reads the model to find the relations, and the graph stays +the authority on whether a model is resolvable: it may only report relations in a model the +builder refuses. diff --git a/docs/validation/model/graph-model-unbuildable.md b/docs/validation/model/graph-model-unbuildable.md index 47160e6c..6260b10a 100644 --- a/docs/validation/model/graph-model-unbuildable.md +++ b/docs/validation/model/graph-model-unbuildable.md @@ -6,31 +6,41 @@ ## Summary -The model could not be built into a weighted authorization model graph, so the checks -that read that graph had nothing to read. The message carries the reason the graph -builder gave. +The model could not be built into a weighted authorization model graph, and nothing else +had a finding for it either. This is the last resort finding: it names the class of +problem and no relation, so it appears only when no other check could say something more +useful. ## Description Entrypoint and cycle validation can be answered two ways: by walking the model's rewrite tree, or by building the same weighted graph the server builds and reading it. The second route needs a graph, and the builder refuses to produce one for some -models. This code reports that refusal. +models. + +A refused build is not reported as this code straight away. Three things are tried +first, and this code is what is left when all three come up empty: + +1. The rewrite-tree walk runs, which reports + [`relation-no-entry-point`](./relation-no-entry-point.md) per relation with a line + and column. Most refused models are answered here. +2. If the walk finds nothing, relations taking part in a cycle the resolver cannot work + through are reported as [`cyclic-relation`](./cyclic-relation.md), again per relation + with a position. +3. Only if neither has anything to say is the refusal itself reported, as this code. It is raised only when graph-backed validation is enabled, which is not the default. -With the default options the models below report -[`relation-no-entry-point`](./relation-no-entry-point.md) instead, one finding per -affected relation, each carrying a line and column. -The builder refuses a model for reasons of its own, and the message ends with the one -it gave: +The message ends with the sentinel the graph builder returned, one of `model cycle`, +`tuple cycle`, `tuple cycle: operands AND or BUT NOT cannot be involved in a cycle`, or +`invalid model`. -- `model cycle`, when relations refer to each other in a loop that admits no - assignment -- `tuple cycle: ...`, when a cycle cannot be resolved, most often because it runs - through an intersection or an exclusion -- `invalid model: ...`, when the graph cannot be constructed from the definitions - given, with the specific reason following the colon +That is the sentinel's own text and nothing more. The builder's own message often goes +on to name a type or a relation, and that part is deliberately left out: the builder +stops at the first problem it meets and picks which one that is by ranging over a map, +so on a model with several independent problems it names one of them and names a +different one on the next run. Every such message is true, and none is stable enough to +put in a finding. The builder's full text stays reachable by unwrapping. A finding with this code wraps two errors. `errors.Is` matches `errors.ErrModelNotBuildable` for the refusal itself, and it also matches whichever @@ -47,13 +57,42 @@ if errors.As(err, &findings) { if errors.Is(f, graph.ErrTupleCycle) { // and this is why } + // f.Unwrap() reaches the builder's own message, first problem and all } } ``` ## Example -The following model would trigger this error: +The following model reports this error: + +``` +model + schema 1.1 + +type user + +type folder + relations + define viewer: [user] + +type team + +type document + relations + define parent: [folder, team] + define viewer: viewer from parent +``` + +**Error Message:** `the model cannot be built into a weighted graph: invalid model` + +`parent` accepts a `folder` or a `team`, and `viewer from parent` needs a `viewer` +relation on both. `folder` has one and `team` does not. Every relation here has a way in, +so the rewrite-tree walk reports nothing, and there is no cycle, so nothing names a +relation. The refusal is all there is to report. + +Cycles do not reach this code. Both of the following are refused by the builder, and both +are reported per relation with a position instead: ``` model @@ -68,12 +107,8 @@ type document define editor: viewer ``` -**Error Message:** `the model cannot be built into a weighted graph: model cycle` - -`viewer` requires `editor`, `editor` is `viewer`, and neither can be entered. The -builder stops there. - -A cycle that runs through an exclusion is refused with a different reason: +reports `relation-no-entry-point` for `viewer` and `editor`, because neither can be +entered. ``` model @@ -81,23 +116,24 @@ model type user -type folder +type document relations - define parent: [folder] - define viewer: [user] but not banned - define banned: viewer from parent + define a: [user] or b + define b: [user] or a ``` -**Error Message:** `the model cannot be built into a weighted graph: tuple cycle: operands AND or BUT NOT cannot be involved in a cycle` +reports `cyclic-relation` for `a` and `b`. Both hold a plain `[user]`, so both can be +entered and the walk is silent, but going round the cycle reads no tuple, so resolving +either one never terminates. ## Resolution -The model is invalid, and the fix is the same fix the reason calls for. Read the -message after the colon and treat it as the finding. +The model is invalid and the sentinel names the class of problem. Unwrap the finding for +the builder's own message if you want the first specific instance it hit, remembering it +is one of possibly several. -For a cycle, break the loop, so that one relation in it stops depending on the rest. -Giving a relation in the loop a way to be entered is not enough on its own, because the -loop still resolves to itself: +For `invalid model` on a tupleset, give the computed relation to every type the tupleset +accepts: ``` model @@ -105,14 +141,21 @@ model type user +type folder + relations + define viewer: [user] + +type team + relations + define viewer: [user] + type document relations - define admin: [user] - define viewer: admin and editor - define editor: [user] + define parent: [folder, team] + define viewer: viewer from parent ``` -For a cycle through `and` or `but not`, take the cyclic relation out of the operand: +or narrow the tupleset to the types that have it: ``` model @@ -121,37 +164,39 @@ model type user type folder + relations + define viewer: [user] + +type team + +type document relations define parent: [folder] - define banned: [user] - define viewer: [user] but not banned + define viewer: viewer from parent ``` ### Steps to fix: -1. **Read the reason:** the text after the colon is the graph builder's own error, and - it names the class of problem. +1. **Read the sentinel:** the text after the last colon names the class of problem. -2. **Find the relations involved:** this finding is model-level and carries no - position, because the builder stops at the first problem and returns no graph to - locate it in. Running validation with the default options reports the same model as - `relation-no-entry-point`, once per affected relation, with a line and column for - each. +2. **Unwrap for the detail:** the builder's own message names the first problem it met. + Treat it as one instance rather than the whole list, and re-validate after fixing it. -3. **Fix the model, not the finding:** every reason the builder gives is a model that - cannot answer a check. There is no configuration that makes one of these models - valid. +3. **Expect no position:** this finding is model-level. The builder stops at the first + problem and returns no graph to locate it in, and anything that could have been + located has already been reported as `relation-no-entry-point` or `cyclic-relation`. -4. **Re-validate:** a model the builder accepts still gets the graph-backed - entrypoint checks run over it, so a clean build is not on its own a clean model. +4. **Re-validate:** a model the builder accepts still gets the graph-backed entrypoint + checks run over it, so a clean build is not on its own a clean model. ## Related Errors -- [`relation-no-entry-point`](./relation-no-entry-point.md) - what the default - validation path reports for these models, per relation and with positions -- [`cyclic-error`](./cyclic-error.md) - circular dependency between relations -- [`invalid-relation-type`](./invalid-relation-type.md) - a relation that is not valid - for the type it is used with +- [`cyclic-relation`](./cyclic-relation.md) - a relation in a cycle the resolver cannot + work through, reported per relation with a position +- [`relation-no-entry-point`](./relation-no-entry-point.md) - what most refused models + report instead, per relation and with positions +- [`invalid-relation-on-tupleset`](./invalid-relation-on-tupleset.md) - a tupleset whose + computed relation is missing everywhere, rather than on some types only ## Implementation Notes diff --git a/pkg/go/errors/sentinels.go b/pkg/go/errors/sentinels.go index 85fb70db..d4f49b58 100644 --- a/pkg/go/errors/sentinels.go +++ b/pkg/go/errors/sentinels.go @@ -93,6 +93,14 @@ var ( // errors.Is reaches either. ErrModelNotBuildable = errors.New("model cannot be built into a weighted graph") + // ErrRelationInUnresolvableCycle is reported when a relation takes part in a + // cycle that cannot be resolved, either because no step in it reads a tuple or + // because a step is an operand of an intersection or an exclusion. + // + // It is distinct from ErrNoEntrypoints: a relation reported here can be + // satisfiable on its own and is reported for the company it keeps. + ErrRelationInUnresolvableCycle = errors.New("relation takes part in a cycle that cannot be resolved") + // ErrUnknownModelErrorKind is returned when a ModelErrorKind has no wire // name, either marshalling a value this package does not declare or reading // a name it does not recognise. It is not a validation finding. diff --git a/pkg/go/validation/criticality_test.go b/pkg/go/validation/criticality_test.go index 81713c1b..91dd7026 100644 --- a/pkg/go/validation/criticality_test.go +++ b/pkg/go/validation/criticality_test.go @@ -74,12 +74,11 @@ func TestCriticalityOfEveryEmittedCode(t *testing.T) { InvalidSchema: true, MultipleModulesInFile: true, GraphModelUnbuildable: true, + CyclicRelation: true, } - // Nothing raises these two, so they are held at not-critical rather than listed - // above. + // Nothing raises this one, so it is held at not-critical rather than listed above. neverRaised := map[ValidationErrorType]struct{}{ - CyclicRelation: {}, InvalidSchemaVersion: {}, } diff --git a/pkg/go/validation/cycle_shape.go b/pkg/go/validation/cycle_shape.go new file mode 100644 index 00000000..ee708d59 --- /dev/null +++ b/pkg/go/validation/cycle_shape.go @@ -0,0 +1,340 @@ +package validation + +import ( + "fmt" + "maps" + "slices" + + openfgav1 "github.com/openfga/api/proto/openfga/v1" +) + +// Cycles among relations are not all the same, and the rewrite-tree walk in +// cycle_detection.go only reports the ones that leave a relation with no way in. Two other +// shapes make a model unresolvable while every relation in them stays satisfiable, so that +// walk answers has-an-entry-point for all of them and reports nothing: +// +// define a: [user] or b +// define b: [user] or a +// +// Both hold a plain [user], so both have an entry point. The cycle between them reads no +// tuple, so resolving either one never gets closer to an answer. +// +// define member: [user, group#member] but not blocked +// define blocked: [user, group#member] +// +// Both hold a plain [user] again. Here the cycle does read a tuple, which is what makes +// `member: [user, group#member]` on its own a legal nesting, but a step of it is an +// operand of the exclusion, and the resolver cannot subtract a set it is still computing. +// +// The weighted graph refuses both, with ErrModelCycle and ErrTupleCycle, and names no +// relation in either. What follows finds the same two shapes over the model and names the +// relations, so a caller gets a position rather than a sentence about the whole model. +// +// The graph stays the authority on whether a model is resolvable. +// TestCycleShapesAgreeWithTheBuilder holds this to it: it may only report relations in +// models the builder refuses. + +// cycleStep is one relation depending on another. +type cycleStep struct { + to string + + // readsTuple marks a step that consumes a tuple, which is what lets a cycle + // terminate. A direct `[type#relation]` restriction and a tuple-to-userset both read + // one; rewriting `define x: y` does not. + readsTuple bool + + // constrained marks a step written as an operand of an intersection or an exclusion. + constrained bool +} + +// cycleShape names why a cycle cannot be resolved. +type cycleShape int + +// The values are ordered by how badly the cycle fails, least first. A relation can sit in +// several cycles and the worst decides what it is reported for, so the order is what makes +// that answer independent of which cycle the search reaches first. +const ( + cycleResolvable cycleShape = iota + // The graph's ErrTupleCycle for AND and BUT NOT: the cycle terminates, but a step + // of it is an operand of an intersection or an exclusion. + cycleUnderConstraint + // The graph's ErrModelCycle: going round the cycle consumes nothing, so it never + // terminates. It ranks above the constraint because it fails whether or not an + // operator encloses it. + cycleReadsNoTuple +) + +// worstCycleShape is the top of that order. A search that reaches it can stop, because +// nothing it has left to walk can change the answer. +const worstCycleShape = cycleReadsNoTuple + +// cycleStepBudget caps the steps one relation's search may walk. Enumerating simple cycles +// is exponential in the worst case and a model is not required to be small. +// +// Exhausting it gives up rather than guesses, so the relation goes unreported and the +// refusal falls through to the positionless finding the graph's own error carries. A caller +// loses the position, not the answer. TestCycleSearchStaysWithinItsBudget pins both that +// the shared corpus stays far below the cap and that a model over it still degrades that +// way. +const cycleStepBudget = 200_000 + +// checkRelationCycleShapes reports every relation that takes part in a cycle the resolver +// cannot work through. +// +// A relation is reported for the cycle it is in rather than for anything wrong with the +// relation, so a satisfiable relation is reported when the cycle it belongs to is not +// resolvable. That is the case the builder refuses the whole model over. +func checkRelationCycleShapes(collector *ErrorCollector, validator *SemanticValidator, lines []string) { + if validator == nil || validator.model == nil { + return + } + + steps := relationDependencySteps(validator) + + for _, typeDef := range validator.model.GetTypeDefinitions() { + relations := typeDef.GetRelations() + if len(relations) == 0 { + continue + } + + typeName := typeDef.GetType() + typeLineIndex := GetTypeLineNumber(typeName, lines, nil) + + for _, relationName := range slices.Sorted(maps.Keys(relations)) { + shape, _ := worstCycleThrough(steps, typeName+"#"+relationName, cycleStepBudget) + if shape == cycleResolvable { + continue + } + + collector.RaiseCyclicRelation(relationName, typeName, describeCycleShape(shape), + relationMeta(typeDef, relationName), + GetRelationLineNumber(relationName, lines, typeLineIndex)) + } + } +} + +// describeCycleShape is the clause the finding ends with, and it is the only place the two +// shapes are worded. +func describeCycleShape(shape cycleShape) string { + if shape == cycleReadsNoTuple { + return "no relation in it reads a tuple, so resolving it never terminates" + } + + return "a relation in it is an operand of an `and` or a `but not`" +} + +// relationDependencySteps builds, for every relation in the model, the relations it depends +// on and how. +// +// Only the steps between relations are recorded. Terminal types and wildcards end a path +// rather than continuing one, so they cannot be part of a cycle and are left out. +func relationDependencySteps(validator *SemanticValidator) map[string][]cycleStep { + steps := map[string][]cycleStep{} + + for _, typeDef := range validator.model.GetTypeDefinitions() { + typeName := typeDef.GetType() + + // Not sorted, unlike the pass in checkRelationCycleShapes. Each relation writes + // one key of its own and the steps within a key come out in rewrite-tree order, + // so the map this builds is the same map whatever order it is filled in. What + // findings come out in is decided where they are raised. + for relationName, rewrite := range typeDef.GetRelations() { + from := typeName + "#" + relationName + collectCycleSteps(validator, typeName, relationName, rewrite, false, func(step cycleStep) { + steps[from] = append(steps[from], step) + }) + } + } + + return steps +} + +// collectCycleSteps walks one relation's rewrite and hands every relation it reaches to +// emit. +// +// Whether a step is constrained is carried down rather than recomputed: a union's operands +// are as constrained as the union itself, while an intersection's and an exclusion's +// operands are constrained whatever encloses them, so nesting a union inside an exclusion +// keeps the exclusion's answer. +func collectCycleSteps(validator *SemanticValidator, typeName, relationName string, + rewrite *openfgav1.Userset, constrained bool, emit func(cycleStep)) { + if rewrite == nil { + return + } + + switch rewrite.GetUserset().(type) { + case *openfgav1.Userset_This: + for _, tr := range directlyRelatedTypes(validator, typeName, relationName) { + // A concrete type or a wildcard is where a path ends. Only a userset + // restriction continues to another relation. + if tr.GetRelation() == "" || tr.GetWildcard() != nil { + continue + } + + emit(cycleStep{to: tr.GetType() + "#" + tr.GetRelation(), readsTuple: true, constrained: constrained}) + } + + case *openfgav1.Userset_ComputedUserset: + if computed := rewrite.GetComputedUserset().GetRelation(); computed != "" { + emit(cycleStep{to: typeName + "#" + computed, readsTuple: false, constrained: constrained}) + } + + case *openfgav1.Userset_TupleToUserset: + ttu := rewrite.GetTupleToUserset() + + tupleset := ttu.GetTupleset().GetRelation() + computed := ttu.GetComputedUserset().GetRelation() + + if tupleset == "" || computed == "" { + return + } + + for _, tr := range directlyRelatedTypes(validator, typeName, tupleset) { + if tr.GetType() == "" { + continue + } + + emit(cycleStep{to: tr.GetType() + "#" + computed, readsTuple: true, constrained: constrained}) + } + + case *openfgav1.Userset_Union: + for _, child := range rewrite.GetUnion().GetChild() { + collectCycleSteps(validator, typeName, relationName, child, constrained, emit) + } + + case *openfgav1.Userset_Intersection: + for _, child := range rewrite.GetIntersection().GetChild() { + collectCycleSteps(validator, typeName, relationName, child, true, emit) + } + + case *openfgav1.Userset_Difference: + difference := rewrite.GetDifference() + collectCycleSteps(validator, typeName, relationName, difference.GetBase(), true, emit) + collectCycleSteps(validator, typeName, relationName, difference.GetSubtract(), true, emit) + } +} + +// directlyRelatedTypes returns the type restrictions declared for a relation. +func directlyRelatedTypes(validator *SemanticValidator, typeName, + relationName string) []*openfgav1.RelationReference { + typeDef := validator.GetTypeDefinition(typeName) + if typeDef == nil { + return nil + } + + metadata, ok := typeDef.GetMetadata().GetRelations()[relationName] + if !ok { + return nil + } + + return metadata.GetDirectlyRelatedUserTypes() +} + +// worstCycleThrough returns the least resolvable cycle that passes through start, and the +// steps it walked to decide. +// +// A relation can sit in several cycles at once, a resolvable one and an unresolvable one, +// and a legal cycle alongside an illegal one does not make the model legal. So every cycle +// back to start is classified and the worst one wins, which is also why the search cannot +// stop at the first cycle it finds. It stops only once the answer can no longer change, at +// the top of the order or out of budget. +// +// The budget is a parameter rather than the constant so a test can pin that the answer for +// a real model does not depend on it. +func worstCycleThrough(steps map[string][]cycleStep, start string, budget int) (cycleShape, int) { + worst := cycleResolvable + walked := 0 + + // onPath holds the steps taken from start to the relation being expanded, so a step + // back to start closes a cycle whose properties are the disjunction over the path. + var onPath []cycleStep + + inPath := map[string]bool{start: true} + + var walk func(from string) + + walk = func(from string) { + for _, step := range steps[from] { + // Checked per step rather than on entry so a decided search unwinds + // here instead of walking every remaining sibling first. + if worst == worstCycleShape || walked >= budget { + return + } + + walked++ + + if step.to == start { + if shape := classifyCycle(append(onPath, step)); shape > worst { + worst = shape + } + + continue + } + + // A relation already on this path leads only to cycles that do not + // pass through start, which belong to that relation's own search. + if inPath[step.to] { + continue + } + + inPath[step.to] = true + onPath = append(onPath, step) + + walk(step.to) + + onPath = onPath[:len(onPath)-1] + delete(inPath, step.to) + } + } + + walk(start) + + // Reaching the top of the order settles it whatever the budget did, because no cycle + // left unwalked could be worse. + if worst == worstCycleShape { + return worst, walked + } + + // Otherwise a search cut short has not seen every cycle, so what it holds is a lower + // bound. Reporting it would give one relation of a cycle a finding and the next one + // nothing, depending on where each search ran out. + if walked >= budget { + return cycleResolvable, walked + } + + return worst, walked +} + +// classifyCycle decides why, if at all, a closed cycle cannot be resolved. +// +// Reading no tuple is checked first because it is the more basic failure: such a cycle +// does not terminate whether or not an operator encloses it. +func classifyCycle(cycle []cycleStep) cycleShape { + readsTuple, constrained := false, false + + for _, step := range cycle { + readsTuple = readsTuple || step.readsTuple + constrained = constrained || step.constrained + } + + switch { + case !readsTuple: + return cycleReadsNoTuple + case constrained: + return cycleUnderConstraint + default: + return cycleResolvable + } +} + +// describeCycleSteps renders a relation's outgoing steps. It exists for test failure +// output, where a wrong answer is unreadable without them. +func describeCycleSteps(steps map[string][]cycleStep, from string) string { + rendered := "" + + for _, step := range steps[from] { + rendered += fmt.Sprintf(" -> %s(readsTuple=%v constrained=%v)", step.to, step.readsTuple, step.constrained) + } + + return rendered +} diff --git a/pkg/go/validation/cycle_shape_test.go b/pkg/go/validation/cycle_shape_test.go new file mode 100644 index 00000000..465f63cb --- /dev/null +++ b/pkg/go/validation/cycle_shape_test.go @@ -0,0 +1,804 @@ +package validation + +import ( + "fmt" + "strings" + "testing" + + openfgav1 "github.com/openfga/api/proto/openfga/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + fgaerrors "github.com/openfga/language/pkg/go/errors" + "github.com/openfga/language/pkg/go/graph" +) + +// TestCheckRelationCycleShapes covers the shapes the rewrite-tree walk has nothing to say +// about, and the legal cycles that must not be caught alongside them. +// +// The messages are asserted in full. A finding that named the right relation and worded the +// reason for a different shape would be worse than no finding, because a reader would go +// looking for the wrong thing. +func TestCheckRelationCycleShapes(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + dsl string + want []string + }{ + // Both relations hold a plain [user], so both have an entry point and the walk is + // silent. The cycle between them reads no tuple. + "rewrite cycle that reads no tuple": { + dsl: `model + schema 1.1 +type user +type document + relations + define a: [user] or b + define b: [user] or a +`, + want: []string{ + "[cyclic-relation] a line 5-5 column 11-12 " + + "\"`a` on `document` takes part in a cycle that cannot be resolved: " + + "no relation in it reads a tuple, so resolving it never terminates.\"", + "[cyclic-relation] b line 6-6 column 11-12 " + + "\"`b` on `document` takes part in a cycle that cannot be resolved: " + + "no relation in it reads a tuple, so resolving it never terminates.\"", + }, + }, + // The cycle does read a tuple, which is what makes the nesting on its own legal, but + // a step of it is an operand of the exclusion. + "tuple cycle through an exclusion": { + dsl: `model + schema 1.1 +type user +type group + relations + define member: [user, group#member] but not blocked + define blocked: [user, group#member] +`, + want: []string{ + "[cyclic-relation] blocked line 6-6 column 11-18 " + + "\"`blocked` on `group` takes part in a cycle that cannot be resolved: " + + "a relation in it is an operand of an `and` or a `but not`.\"", + "[cyclic-relation] member line 5-5 column 11-17 " + + "\"`member` on `group` takes part in a cycle that cannot be resolved: " + + "a relation in it is an operand of an `and` or a `but not`.\"", + }, + }, + // admin is an operand of the same intersection but is in no cycle, so it is not + // reported. The finding is for taking part in the cycle, not for the operator. + "tuple cycle through an intersection names only the relations in it": { + dsl: `model + schema 1.1 +type user +type group + relations + define admin: [user] + define member: [user, group#member] and admin +`, + want: []string{ + "[cyclic-relation] member line 6-6 column 11-17 " + + "\"`member` on `group` takes part in a cycle that cannot be resolved: " + + "a relation in it is an operand of an `and` or a `but not`.\"", + }, + }, + // The cycle reads a tuple at every step and nothing constrains it, which is the + // nested-group model every deployment has. + "userset nesting is not reported": { + dsl: `model + schema 1.1 +type user +type group + relations + define member: [user, group#member] +`, + want: []string{}, + }, + // Recursion through a tupleset reads a tuple too. + "tupleset recursion is not reported": { + dsl: `model + schema 1.1 +type user +type folder + relations + define parent: [folder] + define viewer: [user] or viewer from parent +`, + want: []string{}, + }, + // An exclusion with no cycle running through it is ordinary. + "exclusion outside a cycle is not reported": { + dsl: `model + schema 1.1 +type user +type document + relations + define blocked: [user] + define viewer: [user] but not blocked +`, + want: []string{}, + }, + // A resolvable cycle alongside an unresolvable one does not make the model legal, so + // member is reported for the constrained cycle it is also in. + "a legal cycle does not excuse an illegal one": { + dsl: `model + schema 1.1 +type user +type group + relations + define member: [user, group#member] but not blocked + define blocked: [user, group#member] + define owner: [user, group#owner] +`, + want: []string{ + "[cyclic-relation] blocked line 6-6 column 11-18 " + + "\"`blocked` on `group` takes part in a cycle that cannot be resolved: " + + "a relation in it is an operand of an `and` or a `but not`.\"", + "[cyclic-relation] member line 5-5 column 11-17 " + + "\"`member` on `group` takes part in a cycle that cannot be resolved: " + + "a relation in it is an operand of an `and` or a `but not`.\"", + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.ElementsMatch(t, test.want, describeFindings(graphFindings(t, test.dsl))) + }) + } +} + +// TestCycleShapeFindingsCarryTheCallerContract checks the parts of a finding a consumer +// branches on rather than reads, and that the sentinel is not the entrypoint one. +// +// A relation reported here is satisfiable on its own, so a caller that treated the two as +// interchangeable would tell a user to give the relation an entrypoint it already has. +func TestCycleShapeFindingsCarryTheCallerContract(t *testing.T) { + t.Parallel() + + findings := graphFindings(t, `model + schema 1.1 +type user +type document + relations + define a: [user] or b + define b: [user] or a +`) + require.Len(t, findings, 2) + + for _, finding := range findings { + require.NotNil(t, finding.Metadata) + + assert.Equal(t, CyclicRelation, finding.Metadata.ErrorType) + require.ErrorIs(t, finding, fgaerrors.ErrRelationInUnresolvableCycle) + require.NotErrorIs(t, finding, fgaerrors.ErrNoEntrypoints, + "the relation has an entrypoint, and a caller filtering on one must not see the other") + require.NotErrorIs(t, finding, fgaerrors.ErrModelNotBuildable, + "a positioned finding must not also read as the positionless refusal") + + assert.Equal(t, fgaerrors.SeverityError, finding.Severity) + assert.Equal(t, fgaerrors.ErrorKindRelation, finding.Category) + assert.True(t, isCriticalErrorType(finding.Metadata.ErrorType)) + + // The whole point of computing this outside the graph is somewhere to put it. + require.NotNil(t, finding.Line) + require.NotNil(t, finding.Column) + } +} + +// TestCycleShapesAgreeWithTheBuilder holds the check to the graph, which stays the +// authority on whether a model is resolvable. +// +// The check reads the model rather than the graph, so it is a second implementation of a +// question the builder already answers, and the failure that costs a user most is a +// relation reported in a model the builder accepts. Every case in the shared corpus is run +// through both. +func TestCycleShapesAgreeWithTheBuilder(t *testing.T) { + t.Parallel() + + var accepted, acceptedAndReported, refused, refusedAndReported int + + for _, entry := range corpusModels(t) { + collector := NewErrorCollector(nil) + checkRelationCycleShapes(collector, NewSemanticValidator(entry.Model), + strings.Split(entry.Case.DSL, "\n")) + + reported := collector.CountAll() > 0 + + if _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(entry.Model); buildErr == nil { + accepted++ + + assert.Falsef(t, reported, + "the builder accepts %q, so no relation in it takes part in an unresolvable cycle:\n%s", + entry.Case.Name, strings.Join(describeFindings(collector.AllFindings()), "\n")) + + if reported { + acceptedAndReported++ + } + + continue + } + + refused++ + + if reported { + refusedAndReported++ + } + } + + // Without these the assertion above passes on a check that reports nothing at all. + assert.Positive(t, accepted, "no corpus model built, so agreement was never tested") + assert.Positive(t, refusedAndReported, + "the check reported nothing anywhere, so agreeing with the builder cost it nothing") + + t.Logf("builder accepted %d (reported on %d), refused %d (reported on %d)", + accepted, acceptedAndReported, refused, refusedAndReported) +} + +// TestCycleSearchStaysWithinItsBudget pins the two things the cap has to be true of: it is +// nowhere near binding on a real model, and a model that does exhaust it gives up rather +// than reports half an answer. +func TestCycleSearchStaysWithinItsBudget(t *testing.T) { + t.Parallel() + + t.Run("no corpus model comes close to the cap", func(t *testing.T) { + t.Parallel() + + worstWalked, worstCase := 0, "" + + for _, entry := range corpusModels(t) { + steps := relationDependencySteps(NewSemanticValidator(entry.Model)) + + for _, typeDef := range entry.Model.GetTypeDefinitions() { + for relationName := range typeDef.GetRelations() { + start := typeDef.GetType() + "#" + relationName + + shape, walked := worstCycleThrough(steps, start, cycleStepBudget) + if walked > worstWalked { + worstWalked, worstCase = walked, entry.Case.Name+" "+start + } + + // The answer for a real model cannot depend on the cap, or + // tightening it later would silently change what is reported. + tighter, _ := worstCycleThrough(steps, start, 1_000) + assert.Equalf(t, shape, tighter, + "%s in %q answers differently at a tighter budget", start, entry.Case.Name) + } + } + } + + assert.Positive(t, worstWalked, "no relation walked a step, so the search was never exercised") + assert.Lessf(t, worstWalked, cycleStepBudget/100, + "the worst corpus relation walked %d steps, which is within two orders of the cap (%s)", + worstWalked, worstCase) + + t.Logf("worst corpus relation walked %d of %d steps (%s)", worstWalked, cycleStepBudget, worstCase) + }) + + t.Run("the top of the order stops the search at once", func(t *testing.T) { + t.Parallel() + + // Twelve relations rewriting each other is factorially many paths, and no path + // reads a tuple, so the first cycle closed is already the worst there is. + steps := relationDependencySteps(NewSemanticValidator(modelFromDSL(t, denselyRewritingModel(12)))) + + shape, walked := worstCycleThrough(steps, "document#r0", cycleStepBudget) + + assert.Equal(t, cycleReadsNoTuple, shape) + assert.Lessf(t, walked, 10, "the search kept walking after the answer could no longer change") + }) + + t.Run("a search that settles below the top reports what it found", func(t *testing.T) { + t.Parallel() + + // Constrained tuple cycles are not the top of the order, so every path is walked + // on the chance that a worse cycle is further in. At eight relations that finishes + // inside the cap. + dsl := denselyConstrainedModel(8) + model := modelFromDSL(t, dsl) + + require.ErrorIs(t, buildRefusal(t, model), graph.ErrTupleCycle) + + shape, walked := worstCycleThrough(relationDependencySteps(NewSemanticValidator(model)), + "document#r0", cycleStepBudget) + + assert.Equal(t, cycleUnderConstraint, shape) + require.Lessf(t, walked, cycleStepBudget, + "the search hit the cap, so settling inside it is not what this case tests") + + findings := findingsFrom(ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: true})).GetErrors() + assert.Len(t, findings, 8, "one per relation in the cycle, admin excepted") + + for _, finding := range findings { + require.NotNil(t, finding.Metadata) + assert.Equal(t, CyclicRelation, finding.Metadata.ErrorType) + assert.NotNil(t, finding.Line) + } + }) + + t.Run("a search that exhausts the cap gives up rather than half answers", func(t *testing.T) { + t.Parallel() + + // One more relation is around eight times the paths, which is over the cap. + dsl := denselyConstrainedModel(9) + model := modelFromDSL(t, dsl) + + require.ErrorIs(t, buildRefusal(t, model), graph.ErrTupleCycle) + + shape, walked := worstCycleThrough(relationDependencySteps(NewSemanticValidator(model)), + "document#r0", cycleStepBudget) + + assert.Equal(t, cycleResolvable, shape, + "a search cut short must not report the shape it had reached, which is a lower bound") + require.Equal(t, cycleStepBudget, walked, "the cap did not bind, so giving up is not being tested") + + // What a caller loses is the position, not the answer. The refusal still reaches + // them through the backstop. + findings := findingsFrom(ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: true})).GetErrors() + require.Len(t, findings, 1) + require.NotNil(t, findings[0].Metadata) + + assert.Equal(t, GraphModelUnbuildable, findings[0].Metadata.ErrorType) + require.ErrorIs(t, findings[0], graph.ErrTupleCycle) + }) +} + +// buildRefusal returns the error the builder refused a model with, failing if it accepted +// it. A test that means to exercise a refusal is testing nothing once the model builds. +func buildRefusal(t *testing.T, model *openfgav1.AuthorizationModel) error { + t.Helper() + + _, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.Error(t, err, "the builder accepted the model, so there is no refusal to fall back from") + + return err +} + +// denselyRewritingModel builds relations that all rewrite each other and nothing else, so +// the paths through any one of them grow factorially and no path reads a tuple. +func denselyRewritingModel(relationCount int) string { + return denseModel(relationCount, "r%d", " or ", "%s") +} + +// denselyConstrainedModel builds the same density out of userset restrictions under an +// intersection, so every path reads a tuple and every step is constrained. That is the +// combination no cycle in it can be the top of the order for. +func denselyConstrainedModel(relationCount int) string { + return denseModel(relationCount, "document#r%d", ", ", "[user, %s] and admin") +} + +// denseModel writes relationCount relations, each referring to every other. Each reference +// is target formatted with the relation's index, the references are joined with separator, +// and the joined list is substituted into rewrite. +func denseModel(relationCount int, target, separator, rewrite string) string { + var dsl strings.Builder + + dsl.WriteString("model\n schema 1.1\ntype user\ntype document\n relations\n define admin: [user]\n") + + for i := range relationCount { + references := make([]string, 0, relationCount-1) + + for j := range relationCount { + if j != i { + references = append(references, fmt.Sprintf(target, j)) + } + } + + dsl.WriteString(fmt.Sprintf(" define r%d: "+rewrite+"\n", i, strings.Join(references, separator))) + } + + return dsl.String() +} + +// TestClassifyCycle covers the classifier on its own, over closed cycles rather than +// models, so the precedence between the two shapes is pinned where it is decided. +func TestClassifyCycle(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + cycle []cycleStep + want cycleShape + }{ + "reads a tuple and nothing constrains it": { + cycle: []cycleStep{{to: "group#member", readsTuple: true}}, + want: cycleResolvable, + }, + "reads no tuple": { + cycle: []cycleStep{{to: "document#b"}, {to: "document#a"}}, + want: cycleReadsNoTuple, + }, + "reads a tuple under a constraint": { + cycle: []cycleStep{{to: "group#blocked", constrained: true}, {to: "group#member", readsTuple: true}}, + want: cycleUnderConstraint, + }, + // One step reading a tuple is enough for the cycle to terminate, so the constraint + // is what is left to report. + "one step of several reads a tuple": { + cycle: []cycleStep{{to: "a"}, {to: "b", readsTuple: true, constrained: true}, {to: "c"}}, + want: cycleUnderConstraint, + }, + // Reading no tuple outranks the constraint, and it has to outrank it here as well + // as in the search, or a relation in both shapes would be worded by whichever the + // search reached first. + "reads no tuple under a constraint": { + cycle: []cycleStep{{to: "a", constrained: true}, {to: "b"}}, + want: cycleReadsNoTuple, + }, + // Not a cycle any model produces, and the answer still has to be one of the three + // rather than a zero value that happens to read as resolvable. + "no steps": { + cycle: nil, + want: cycleReadsNoTuple, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, test.want, classifyCycle(test.cycle)) + }) + } +} + +// TestWorstCycleShapeIsTheTopOfTheOrder pins the constant against the order it names. It is +// what the search stops on, so a shape added below it would quietly stop the search early. +func TestWorstCycleShapeIsTheTopOfTheOrder(t *testing.T) { + t.Parallel() + + for _, shape := range []cycleShape{cycleResolvable, cycleUnderConstraint, cycleReadsNoTuple} { + assert.LessOrEqual(t, shape, worstCycleShape) + } +} + +// TestRelationDependencySteps covers what the steps map records, which is what every answer +// above is computed from. +// +// A missing step is a cycle never found and a spurious one is a cycle that is not there, so +// each case is a claim about one kind of rewrite rather than about a whole model. +func TestRelationDependencySteps(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + dsl string + from string + want []cycleStep + other map[string][]cycleStep + }{ + // A concrete type and a wildcard end a path, so neither is a step. Only the userset + // restriction continues to another relation, and reading its tuple is what lets the + // cycle terminate. + "direct assignment records only the userset restrictions": { + dsl: `model + schema 1.1 +type user +type group + relations + define member: [user, user:*, group#member] +`, + from: "group#member", + want: []cycleStep{{to: "group#member", readsTuple: true}}, + }, + // A rewrite reads nothing, which is the whole distinction the shapes turn on. + "computed userset reads no tuple": { + dsl: `model + schema 1.1 +type user +type document + relations + define editor: [user] + define viewer: editor +`, + from: "document#viewer", + want: []cycleStep{{to: "document#editor"}}, + }, + // A tupleset expands to one step per assignable type of the tupleset relation, and + // the computed relation is looked for on each. Recording only the first would miss + // a cycle that runs through the second. + "tupleset expands across every assignable type": { + dsl: `model + schema 1.1 +type user +type folder + relations + define viewer: [user] +type team + relations + define viewer: [user] +type document + relations + define parent: [folder, team] + define viewer: viewer from parent +`, + from: "document#viewer", + want: []cycleStep{ + {to: "folder#viewer", readsTuple: true}, + {to: "team#viewer", readsTuple: true}, + }, + }, + // A union constrains nothing, so its operands are as constrained as the union is. + "union operands are unconstrained": { + dsl: `model + schema 1.1 +type user +type document + relations + define a: [user] + define b: [user] + define viewer: a or b +`, + from: "document#viewer", + want: []cycleStep{{to: "document#a"}, {to: "document#b"}}, + }, + "intersection operands are constrained": { + dsl: `model + schema 1.1 +type user +type document + relations + define a: [user] + define b: [user] + define viewer: a and b +`, + from: "document#viewer", + want: []cycleStep{{to: "document#a", constrained: true}, {to: "document#b", constrained: true}}, + }, + // Both sides of an exclusion are constrained. The subtrahend obviously is, and the + // base is too, because the resolver cannot finish it without the other. + "both operands of an exclusion are constrained": { + dsl: `model + schema 1.1 +type user +type document + relations + define a: [user] + define b: [user] + define viewer: a but not b +`, + from: "document#viewer", + want: []cycleStep{{to: "document#a", constrained: true}, {to: "document#b", constrained: true}}, + }, + // Nesting a union inside an exclusion keeps the exclusion's answer, so an operand + // two levels down is still constrained. + "a union nested in an exclusion stays constrained": { + dsl: `model + schema 1.1 +type user +type document + relations + define a: [user] + define b: [user] + define c: [user] + define viewer: (a or b) but not c +`, + from: "document#viewer", + want: []cycleStep{ + {to: "document#a", constrained: true}, + {to: "document#b", constrained: true}, + {to: "document#c", constrained: true}, + }, + }, + // A relation with nowhere to go has no steps rather than an empty one, so it cannot + // close a cycle. + "a terminal relation records no steps": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: [user] +`, + from: "document#viewer", + want: nil, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + steps := relationDependencySteps(NewSemanticValidator(modelFromDSL(t, test.dsl))) + + assert.ElementsMatchf(t, test.want, steps[test.from], "steps for %s:%s", + test.from, describeCycleSteps(steps, test.from)) + }) + } +} + +// TestCycleShapesRunOnlyBehindARefusedBuild pins where the check sits. It is a second +// answer to a question the graph already answers, so on a model the graph accepts the graph +// rules are what run. +func TestCycleShapesRunOnlyBehindARefusedBuild(t *testing.T) { + t.Parallel() + + // Legal, and every relation in it is in a cycle the check has an opinion about only + // because the check is never asked. + dsl := `model + schema 1.1 +type user +type group + relations + define member: [user, group#member] +` + model := modelFromDSL(t, dsl) + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, buildErr, "the build has to succeed for this to test the branch it means to") + + assert.Empty(t, describeFindings(graphFindings(t, dsl))) +} + +// TestCycleShapesDoNotDisplaceTheWalk keeps the two fallbacks in order. The walk names the +// same relations for a model that is both cyclic and unsatisfiable, and it names them with +// the wording the corpus pins, so running the cycle check as well would double every +// finding. +func TestCycleShapesDoNotDisplaceTheWalk(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define admin: [user] + define viewer: admin and editor + define editor: viewer +` + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(modelFromDSL(t, dsl)) + require.Error(t, buildErr, "the model has to be one the builder refuses") + + collector := NewErrorCollector(nil) + checkRelationCycleShapes(collector, NewSemanticValidator(modelFromDSL(t, dsl)), + strings.Split(dsl, "\n")) + require.NotEmpty(t, collector.AllFindings(), + "the cycle check has to have something to say here, or the ordering is untested") + + for _, finding := range graphFindings(t, dsl) { + require.NotNil(t, finding.Metadata) + assert.Equal(t, RelationNoEntrypoint, finding.Metadata.ErrorType, + "the walk found this relation first, so the cycle check must not report it again") + } +} + +// TestCheckRelationCycleShapesOnNilModel checks the guard, which is reached the same way +// validateWithGraph's is. +func TestCheckRelationCycleShapesOnNilModel(t *testing.T) { + t.Parallel() + + collector := NewErrorCollector(nil) + + checkRelationCycleShapes(collector, nil, nil) + assert.Equal(t, 0, collector.CountAll(), "no validator, so nothing to read") + + checkRelationCycleShapes(collector, NewSemanticValidator(nil), nil) + assert.Equal(t, 0, collector.CountAll(), "a validator over a nil model has no relations") +} + +// TestCycleShapeFindingsAreOrderedDeterministically pins the order findings come out in. +// +// The relations of a type live in a map, so without sorting them the same model would report +// the same cycle in a different order per run. That is not a wrong answer but it is an +// unusable one: a corpus comparison goes flaky and an editor redraws its diagnostics in a +// different order every keystroke. +func TestCycleShapeFindingsAreOrderedDeterministically(t *testing.T) { + t.Parallel() + + // Relation names deliberately out of declaration order, so sorting them is visible + // rather than incidentally matching the source. + dsl := `model + schema 1.1 +type user +type document + relations + define zebra: [user] or alpha + define alpha: [user] or mango + define mango: [user] or zebra +` + + first := describeFindings(graphFindings(t, dsl)) + require.Len(t, first, 3, "all three relations are in the cycle") + + for range 20 { + assert.Equal(t, first, describeFindings(graphFindings(t, dsl))) + } + + symbols := make([]string, 0, len(first)) + + for _, finding := range graphFindings(t, dsl) { + require.NotNil(t, finding.Metadata) + + symbols = append(symbols, finding.Metadata.Symbol) + } + + assert.Equal(t, []string{"alpha", "mango", "zebra"}, symbols, + "findings within a type come out in relation-name order") +} + +// TestCycleShapeFindingCarriesTheDeclaringFileAndModule covers the provenance an editor +// needs to put the diagnostic in the right document. +// +// A modular model reaches validation already flattened, with the file and module each +// relation came from left on the model's metadata, so a finding that dropped them would be +// unplaceable in the only case where placing it is hard. The relation-level source info is +// read where it is set and the type's is the fallback, so both are exercised here. +func TestCycleShapeFindingCarriesTheDeclaringFileAndModule(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define a: [user] or b + define b: [user] or a +` + model := modelFromDSL(t, dsl) + + for _, typeDef := range model.GetTypeDefinitions() { + if typeDef.GetType() != "document" { + continue + } + + require.NotNil(t, typeDef.GetMetadata()) + + typeDef.Metadata.Module = "core" + typeDef.Metadata.SourceInfo = &openfgav1.SourceInfo{File: "core.fga"} + + // Only a carries its own source info. b falls back to the type's. + relationMetadata, ok := typeDef.GetMetadata().GetRelations()["a"] + require.True(t, ok, "relation a has no metadata, so the direct read is untested") + + relationMetadata.Module = "documents" + relationMetadata.SourceInfo = &openfgav1.SourceInfo{File: "documents.fga"} + } + + byRelation := map[string]*ValidationError{} + + for _, finding := range findingsFrom(ValidateDSL(model, dsl, + &EngineOptions{UseGraphValidation: true})).GetErrors() { + require.NotNil(t, finding.Metadata) + require.Equal(t, CyclicRelation, finding.Metadata.ErrorType) + + byRelation[finding.Metadata.Symbol] = finding + } + + require.Contains(t, byRelation, "a") + require.Contains(t, byRelation, "b") + + assert.Equal(t, "documents.fga", byRelation["a"].File, "a declares its own file") + assert.Equal(t, "documents", byRelation["a"].Metadata.Module) + + assert.Equal(t, "core.fga", byRelation["b"].File, "b has none, so the type's file stands in") + assert.Equal(t, "core", byRelation["b"].Metadata.Module) +} + +// TestCycleShapeFindingHasNoPositionWithoutSourceText covers the JSON entry point. The +// relation is still named; only the position is missing, and it is nil rather than line +// zero. +func TestCycleShapeFindingHasNoPositionWithoutSourceText(t *testing.T) { + t.Parallel() + + model := modelFromDSL(t, `model + schema 1.1 +type user +type document + relations + define a: [user] or b + define b: [user] or a +`) + + findings := findingsFrom(ValidateJSON(model, &EngineOptions{UseGraphValidation: true})).GetErrors() + require.Len(t, findings, 2) + + for _, finding := range findings { + require.NotNil(t, finding.Metadata) + assert.Equal(t, CyclicRelation, finding.Metadata.ErrorType) + assert.NotEmpty(t, finding.Metadata.Symbol) + assert.Nil(t, finding.Line) + assert.Nil(t, finding.Column) + } +} diff --git a/pkg/go/validation/error_collector.go b/pkg/go/validation/error_collector.go index 0652e3db..32af5aa4 100644 --- a/pkg/go/validation/error_collector.go +++ b/pkg/go/validation/error_collector.go @@ -416,6 +416,21 @@ func (c *ErrorCollector) RaiseNoEntryPoint(symbol, typeName string, meta *Meta, }) } +// RaiseCyclicRelation raises an error for a relation that takes part in a cycle the +// resolver cannot work through. +// +// The through argument names what makes the cycle unresolvable, and it is the whole +// difference between this and RaiseNoEntryPoint: the relation may well be satisfiable, and +// what is wrong is the cycle it sits in. +func (c *ErrorCollector) RaiseCyclicRelation(symbol, typeName, through string, meta *Meta, lineIndex *int) { + message := fmt.Sprintf("`%s` on `%s` takes part in a cycle that cannot be resolved: %s.", + symbol, typeName, through) + c.addScopedError(message, CyclicRelation, symbol, lineIndex, meta, nil, scope{ + objectType: typeName, + relation: symbol, + }) +} + // RaiseInvalidRelationOnTupleset raises an error for invalid relation on tupleset. func (c *ErrorCollector) RaiseInvalidRelationOnTupleset(symbol, typeName, typeDef, relationName, offendingRelation, parent string, lineIndex *int, meta *Meta) { @@ -593,14 +608,20 @@ func (c *ErrorCollector) RaiseEmptyDifference(relationName, typeName, operation // RaiseModelUnbuildable raises an error for a model the weighted graph refuses to build. // // The finding carries no position, and one refused model raises one finding however many -// problems it has. Both follow from the builder returning on the first problem it meets -// and returning no graph with it: there is nothing left to walk for the rest, and the -// error it returns names a count rather than the relations responsible. +// problems it has. Both follow from the builder returning on the first problem it meets and +// returning no graph with it: there is nothing left to walk for the rest. +// +// The reason is worded by the caller from the sentinel the refusal carries, not taken from +// the builder's message. The builder picks which problem to report by ranging over a map, so +// a model with several independent problems gets a message naming whichever one came out +// first, and that is not the same one on the next run. Every statement it makes is true, but +// none of them is stable enough to word a finding from. // -// The builder's error is chained under ErrModelNotBuildable, so errors.Is matches the -// build being refused and the specific reason alike. -func (c *ErrorCollector) RaiseModelUnbuildable(cause error) { - message := fmt.Sprintf("the model cannot be built into a weighted graph: %s", cause) +// The builder's error is chained under ErrModelNotBuildable, so errors.Is matches the build +// being refused and the specific reason alike, and a caller that wants the builder's own text +// can still reach it through the chain. +func (c *ErrorCollector) RaiseModelUnbuildable(reason string, cause error) { + message := fmt.Sprintf("the model cannot be built into a weighted graph: %s", reason) chained := fmt.Errorf("%w: %w", fgaerrors.ErrModelNotBuildable, cause) c.addScopedError(message, GraphModelUnbuildable, "", nil, nil, nil, scope{cause: chained}) } diff --git a/pkg/go/validation/error_info.go b/pkg/go/validation/error_info.go index a73fc597..3871d587 100644 --- a/pkg/go/validation/error_info.go +++ b/pkg/go/validation/error_info.go @@ -174,6 +174,12 @@ var errorInfoByType = map[ValidationErrorType]errorInfo{ Cause: fgaerrors.ErrModelNotBuildable, Critical: true, }, + CyclicRelation: { + Severity: fgaerrors.SeverityError, + Category: fgaerrors.ErrorKindRelation, + Cause: fgaerrors.ErrRelationInUnresolvableCycle, + Critical: true, + }, } // unemittedErrorTypes are declared ValidationErrorType values that no validation @@ -182,9 +188,9 @@ var errorInfoByType = map[ValidationErrorType]errorInfo{ // They are kept rather than deleted because each has a published documentation // page, and because SelfError and InvalidSyntax are equally unemitted in // pkg/js/errors.ts. A cycle with no entrypoint surfaces as RelationNoEntrypoint, -// leaving CyclicError and CyclicRelation nothing to report. InvalidSchemaVersion is -// unreachable because RaiseInvalidSchemaVersion emits InvalidSchema, which is what -// the shared corpus expects. +// leaving CyclicError nothing to report. InvalidSchemaVersion is unreachable +// because RaiseInvalidSchemaVersion emits InvalidSchema, which is what the shared +// corpus expects. // // None get an errorInfoByType entry, so lookupErrorInfo treats them as blocking // with no cause. Anything that starts emitting one must add it to the table in the @@ -193,7 +199,6 @@ var unemittedErrorTypes = map[ValidationErrorType]struct{}{ SelfError: {}, InvalidSyntax: {}, CyclicError: {}, - CyclicRelation: {}, InvalidSchemaVersion: {}, } diff --git a/pkg/go/validation/graph_validation.go b/pkg/go/validation/graph_validation.go index 9a82284e..ca5660db 100644 --- a/pkg/go/validation/graph_validation.go +++ b/pkg/go/validation/graph_validation.go @@ -1,6 +1,7 @@ package validation import ( + "errors" "slices" "strings" @@ -16,19 +17,39 @@ import ( // reimplementing the walk, so what validation reports and what anything else reading that // graph concludes cannot drift apart the way two implementations of one question would. // -// The graph reports in two ways and only one of them can carry a position. Build either -// returns a graph or refuses the model, and on refusal it returns no graph, so there is -// nothing left to enumerate: a model with three broken relations yields one error naming -// none of them. Every rule therefore runs only on a model that built, and a refused model -// produces the single finding RaiseModelUnbuildable writes. -func validateWithGraph(collector *ErrorCollector, model *openfgav1.AuthorizationModel, lines []string) { - if model == nil { +// Only a graph Build accepted is read. Weight assignment marks a node visited before it walks +// that node's edges and stops at the first node it cannot weight, so in a graph it refused the +// relations left without weights are the ones the walk had not reached yet as much as the ones +// nothing can satisfy. An accepted graph has no such ambiguity: every node was weighted, and a +// relation node with no weights reaches no terminal type, which is what the rewrite-tree walk +// calls an impossible relation. +// +// A refused model is answered by the rewrite-tree walk, which resolves the same relations as +// this rule does for every model in the shared corpus the graph accepts. Where the walk finds +// nothing the refusal is reported as itself: the model holds a pattern the walk does not look +// for, and the refusal names that pattern without naming a relation. +func validateWithGraph(collector *ErrorCollector, semantic *SemanticValidator, lines []string) { + if semantic == nil || semantic.model == nil { return } + model := semantic.model + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) if err != nil { - collector.RaiseModelUnbuildable(err) + validateCyclesAndEntryPoints(collector, semantic, lines) + + // Only where the walk found nothing, because the relations it names are the same + // relations an unresolvable cycle runs through and one finding per relation is + // enough. Where it found nothing the refusal is about a cycle shape it does not + // look for, which is what this names. + if !collector.HasErrors() { + checkRelationCycleShapes(collector, semantic, lines) + } + + if !collector.HasErrors() { + collector.RaiseModelUnbuildable(describeRefusal(err), err) + } return } @@ -39,6 +60,45 @@ func validateWithGraph(collector *ErrorCollector, model *openfgav1.Authorization } } +// graphRefusalReasons words a build refusal from the sentinel it carries. +// +// The wording is each sentinel's own text, which is a fixed string, rather than the message +// the builder returned. The builder reports the first problem it meets and chooses that +// problem by ranging over a map, so the message names one of possibly several broken +// relations and names a different one on the next run. Reading the sentinel instead gives the +// same finding every run for the same model, which is what lets a consumer cache it, diff it, +// or compare it against a corpus. +// +// Ordered most specific first, because the constrained tuple cycle wraps the plain one and +// its text says more. TestEveryGraphSentinelHasAWording keeps this exhaustive. +var graphRefusalReasons = []struct { + sentinel error + reason string +}{ + {sentinel: graph.ErrContrainstTupleCycle, reason: graph.ErrContrainstTupleCycle.Error()}, + {sentinel: graph.ErrTupleCycle, reason: graph.ErrTupleCycle.Error()}, + {sentinel: graph.ErrModelCycle, reason: graph.ErrModelCycle.Error()}, + {sentinel: graph.ErrInvalidModel, reason: graph.ErrInvalidModel.Error()}, +} + +// unrecognisedRefusal is the wording for a refusal carrying no sentinel this package knows. +// +// It says nothing about the model on purpose. The alternative is falling back to the +// builder's message, which is the text this exists to keep out of a finding, and a sentinel +// missing from the table is a gap to close rather than a case to paper over. +const unrecognisedRefusal = "a reason this version does not recognise" + +// describeRefusal returns the wording for the sentinel a refusal carries. +func describeRefusal(err error) string { + for _, refusal := range graphRefusalReasons { + if errors.Is(err, refusal.sentinel) { + return refusal.reason + } + } + + return unrecognisedRefusal +} + // graphRule is one check over a built graph. Every rule collects all of its hits rather // than returning on the first, which is the whole reason these run outside pkg/go/graph. type graphRule struct { @@ -98,7 +158,7 @@ func checkRelationEntrypoints(scope *graphScope) { meta := scope.metaFor(objectType, relation) lineIndex := scope.relationLine(objectType, relation) - if scope.reachesOnlyRewrites(nodeID) { + if scope.cyclesThroughRewrites(nodeID) { scope.collector.RaiseNoEntryPointLoop(relation, objectType, meta, lineIndex) continue @@ -108,17 +168,19 @@ func checkRelationEntrypoints(scope *graphScope) { } } -// reachesOnlyRewrites reports whether every edge reachable from nodeID rewrites another -// relation, with no direct assignment or tupleset anywhere. +// cyclesThroughRewrites reports whether nodeID reaches itself following only the edges that +// rewrite one relation into another. // -// That separates the two things the traversal words differently. `define viewer: viewer` -// closes on itself through a computed rewrite and nothing else, which it calls a potential -// loop. `define viewer: viewer from parent` reaches a tupleset it can never satisfy, which -// it calls a missing entrypoint. -func (s *graphScope) reachesOnlyRewrites(nodeID string) bool { - visited := map[string]bool{nodeID: true} +// That is what separates the two wordings the traversal uses. A relation that comes back to +// itself through rewrites is a potential loop; one that cannot be satisfied for any other +// reason has no entrypoint. Edges that are not rewrites are skipped rather than treated as +// disqualifying, because a relation can hold both a rewrite that loops and a direct +// assignment that does not, and the loop is still what makes it impossible. That mirrors how +// the rewrite-tree walk propagates a loop out of an operator if any one child loops, while +// reading only reachability off its direct and tupleset children. +func (s *graphScope) cyclesThroughRewrites(nodeID string) bool { + visited := map[string]bool{} queue := []string{nodeID} - sawEdge := false for len(queue) > 0 { current := queue[0] @@ -126,15 +188,15 @@ func (s *graphScope) reachesOnlyRewrites(nodeID string) bool { edges, _ := s.weighted.GetEdgesFromNodeID(current) for _, edge := range edges { - sawEdge = true - - if edge.GetEdgeType() != graph.ComputedEdge && edge.GetEdgeType() != graph.RewriteEdge { - // A direct, tupleset or grouping edge means something outside this - // relation feeds it, so the problem is a missing entrypoint. - return false + if !rewritesAnotherRelation(edge.GetEdgeType()) { + continue } next := edge.GetTo().GetUniqueLabel() + if next == nodeID { + return true + } + if !visited[next] { visited[next] = true queue = append(queue, next) @@ -142,7 +204,13 @@ func (s *graphScope) reachesOnlyRewrites(nodeID string) bool { } } - return sawEdge + return false +} + +// rewritesAnotherRelation reports whether an edge kind stands for one relation being +// rewritten as another, as against a tuple being read or a set of edges being grouped. +func rewritesAnotherRelation(kind graph.EdgeType) bool { + return kind == graph.ComputedEdge || kind == graph.RewriteEdge } // relationLine resolves the source line a relation is declared on, through the same diff --git a/pkg/go/validation/graph_validation_corpus_test.go b/pkg/go/validation/graph_validation_corpus_test.go index 93f3f6f0..fea00ced 100644 --- a/pkg/go/validation/graph_validation_corpus_test.go +++ b/pkg/go/validation/graph_validation_corpus_test.go @@ -91,43 +91,19 @@ func TestGraphAgreesWithTraversalOnWhichModelsAreInvalid(t *testing.T) { assert.Emptyf(t, graphOnlyNames, "the graph refuses a model validation calls valid, so the two paths disagree on validity") - assert.Len(t, cases, 84, "non-skipped corpus cases") - assert.Equal(t, 33, bothFlag, "cases both paths reject") + assert.Len(t, cases, 89, "non-skipped corpus cases") + assert.Equal(t, 38, bothFlag, "cases both paths reject") assert.Equal(t, 47, validationOnly, "cases validation rejects and the graph builds") assert.Equal(t, 0, graphOnly, "cases the graph rejects and validation accepts") assert.Equal(t, 4, bothClean, "cases both paths accept") } -// graphDivergentCorpusCases are the corpus cases that do not pass under -// UseGraphValidation, and every one of them diverges the same way: the case expects one -// relation-no-entry-point per unreachable relation, and the graph path reports a single -// graph-model-unbuildable for the model. -// -// The cause is the builder, which stops at the first problem and returns no graph with -// its error, so there is nothing left to enumerate relations from. Twenty-three -// positioned findings across these eleven cases become eleven positionless ones. -// -// Every other case whose build is refused is stopped by the cascade gate before the -// entrypoint phase runs, so its refusal is never reported and the case still passes. -var graphDivergentCorpusCases = []string{ - "cycle 1 should fail", - "cycle 2 should fail", - "cyclic loop", - "exclusion base not allow to reference itself in TTU", - "intersection child not allow to reference itself in TTU", - "no entry point exclusion that relates to itself", - "no entry point intersection that relates to itself", - "no_entrypoint_1 should fail", - "no_entrypoint_2 should fail", - "no_entrypoint_3a should fail", - "no_entrypoint_3b should fail", -} - // TestCorpusUnderGraphValidation runs the whole corpus with the graph path selected and -// pins the divergence to exactly the cases above, by name. +// requires every case to pass, which is what makes the option safe to offer. // -// Naming them is the point. A count alone would stay green if one case started failing -// as another started passing, which is the shape a rule-parity regression takes. +// Failures are named rather than counted. A count alone would stay green if one case +// started failing as another started passing, which is the shape a rule-parity regression +// takes. func TestCorpusUnderGraphValidation(t *testing.T) { t.Parallel() @@ -144,53 +120,65 @@ func TestCorpusUnderGraphValidation(t *testing.T) { sort.Strings(diverged) - assert.Equal(t, graphDivergentCorpusCases, diverged, - "the set of corpus cases the graph path does not satisfy has changed") + assert.Empty(t, diverged, "corpus cases the graph path does not satisfy") } -// TestGraphDivergenceIsOnlyTheUnbuildableSubstitution checks what the divergence above -// consists of, so the test names a behaviour rather than a list. +// TestRefusedCorpusModelsReportWhatTheWalkReports covers the cases the graph is never read +// for, which is 38 of the 89. +// +// Weight assignment stops at the first node it cannot weight, and it marks a node visited +// before walking that node's edges, so a graph it refused holds relations left unweighted +// because the walk had not reached them yet. Nothing distinguishes those from the relations +// nothing can satisfy, so a refused build is answered by the rewrite-tree walk and the +// findings have to be identical to what the traversal path gives. // -// For each divergent case: the case expects nothing but entrypoint findings, and the -// graph path reports exactly one finding, the unbuildable one. Anything else, a second -// finding or a different code, is a different problem than fail-fast Build. -func TestGraphDivergenceIsOnlyTheUnbuildableSubstitution(t *testing.T) { +// The count is asserted so the comparison cannot pass by covering nothing. +func TestRefusedCorpusModelsReportWhatTheWalkReports(t *testing.T) { t.Parallel() - divergent := make(map[string]struct{}, len(graphDivergentCorpusCases)) - for _, name := range graphDivergentCorpusCases { - divergent[name] = struct{}{} - } - - var expectedEntrypointFindings, checked int + var refused int for _, entry := range corpusModels(t) { - if _, ok := divergent[entry.Case.Name]; !ok { + if _, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(entry.Model); err == nil { continue } - checked++ + refused++ - for _, expected := range entry.Case.ExpectedErrors { - require.Equalf(t, string(RelationNoEntrypoint), expected.Metadata.ErrorType, - "case %q expects a code other than the entrypoint one, so it is not this divergence", - entry.Case.Name) + fromWalk := findingsFrom(ValidateDSL(entry.Model, entry.Case.DSL, DefaultEngineOptions())).GetErrors() + viaGraph := findingsFrom( + ValidateDSL(entry.Model, entry.Case.DSL, &EngineOptions{UseGraphValidation: true})).GetErrors() - expectedEntrypointFindings++ - } + assert.Equalf(t, describeFindings(fromWalk), describeFindings(viaGraph), + "the graph path reports something other than the walk for refused case %q", entry.Case.Name) + } - findings := findingsFrom( - ValidateDSL(entry.Model, entry.Case.DSL, &EngineOptions{UseGraphValidation: true})).GetErrors() + assert.Equal(t, 38, refused, "corpus cases the builder refuses") +} + +// TestNoCorpusModelIsRefusedWithNothingToSay pins the gap this path leaves. +// +// A model the builder refuses and the walk has no finding for is reported as the refusal +// itself, which names a pattern but no relation and carries no position. No corpus case is +// in that state; the models that are have unit tests instead. A case arriving here means +// the corpus grew a model whose only finding a consumer cannot put on a line. +func TestNoCorpusModelIsRefusedWithNothingToSay(t *testing.T) { + t.Parallel() - require.Lenf(t, findings, 1, "case %q reports more than the unbuildable finding", entry.Case.Name) - require.NotNil(t, findings[0].Metadata) - assert.Equalf(t, GraphModelUnbuildable, findings[0].Metadata.ErrorType, - "case %q diverges by reporting something other than the unbuildable finding", entry.Case.Name) + var positionless []string + + for _, entry := range corpusModels(t) { + for _, finding := range findingsFrom( + ValidateDSL(entry.Model, entry.Case.DSL, &EngineOptions{UseGraphValidation: true})).GetErrors() { + if finding.Metadata != nil && finding.Metadata.ErrorType == GraphModelUnbuildable { + positionless = append(positionless, fmt.Sprintf("%s: %s", entry.Case.Name, finding.Message)) + } + } } - assert.Len(t, graphDivergentCorpusCases, checked, "every named case was found in the corpus") - assert.Equal(t, 23, expectedEntrypointFindings, - "positioned entrypoint findings the graph path gives up, one per unreachable relation") + sort.Strings(positionless) + + assert.Empty(t, positionless, "corpus cases reporting a refusal with no relation and no position") } // TestEntrypointRuleMatchesTraversalWhereTheGraphBuilds is the parity guarantee behind diff --git a/pkg/go/validation/graph_validation_test.go b/pkg/go/validation/graph_validation_test.go index c91e79c3..7fb96487 100644 --- a/pkg/go/validation/graph_validation_test.go +++ b/pkg/go/validation/graph_validation_test.go @@ -1,6 +1,7 @@ package validation import ( + "errors" "fmt" "testing" @@ -268,7 +269,39 @@ type document assert.Equal(t, fromTraversal[0].Metadata, finding.Metadata) } -// TestRaiseModelUnbuildableReachesBothSentinels covers the one code this path adds. +// modelRefusedWithNothingToSay is the model the positionless backstop exists for: the +// builder refuses it, the rewrite-tree walk finds nothing, and the cycle-shape check finds +// nothing either. +// +// The walk requires the computed relation on at least one assignable type of the tupleset; +// the builder requires it on every one. Here folder has viewer and team does not. No cycle +// is involved, so naming a relation would take a rule this branch does not have, and the +// refusal is all there is to report. +var modelRefusedWithNothingToSay = struct { + dsl string + wantMessage string + wantCause error +}{ + dsl: `model + schema 1.1 +type user +type folder + relations + define viewer: [user] +type team +type document + relations + define parent: [folder, team] + define viewer: viewer from parent +`, + // The sentinel's own text. The builder's message goes on to name the type and relation + // it stopped at, which is a detail this deliberately drops: it is the first of possibly + // several and which one it is changes between runs. + wantMessage: "the model cannot be built into a weighted graph: invalid model", + wantCause: graph.ErrInvalidModel, +} + +// TestRaiseModelUnbuildableReachesBothSentinels covers the code the backstop raises. // // A caller has two questions about a refused build, whether it was refused and why, and // the finding has to answer both through errors.Is. Chaining is the only reason the raise @@ -276,61 +309,171 @@ type document func TestRaiseModelUnbuildableReachesBothSentinels(t *testing.T) { t.Parallel() - tests := map[string]struct { - dsl string - wantMessage string - wantCause error - }{ - "cycle through an intersection": { - dsl: `model - schema 1.1 -type user -type document - relations - define admin: [user] - define viewer: admin and editor - define editor: viewer -`, - wantMessage: "the model cannot be built into a weighted graph: model cycle", - wantCause: graph.ErrModelCycle, - }, - "cycle through an exclusion": { - dsl: `model - schema 1.1 -type user -type folder - relations - define parent: [folder] - define viewer: [user] but not banned - define banned: viewer from parent -`, - wantMessage: "the model cannot be built into a weighted graph: tuple cycle: " + - "operands AND or BUT NOT cannot be involved in a cycle", - wantCause: graph.ErrTupleCycle, - }, + dsl := modelRefusedWithNothingToSay.dsl + + require.Empty(t, findingsFrom(ValidateDSL(modelFromDSL(t, dsl), dsl, + DefaultEngineOptions())).GetErrors(), + "the walk has to be silent here or the refusal is not what is being tested") + + findings := graphFindings(t, dsl) + require.Len(t, findings, 1, "nothing else found anything, so the refusal is the only finding") + + finding := findings[0] + require.NotNil(t, finding.Metadata) + + assert.Equal(t, GraphModelUnbuildable, finding.Metadata.ErrorType) + assert.Equal(t, modelRefusedWithNothingToSay.wantMessage, finding.Message) + + require.ErrorIs(t, finding, fgaerrors.ErrModelNotBuildable, "the refusal itself") + require.ErrorIs(t, finding, modelRefusedWithNothingToSay.wantCause, "the reason the builder gave") + + // The builder answers a refusal with an error and no graph, so there is nothing to + // resolve a position against and no relation named. + assert.Nil(t, finding.Line) + assert.Nil(t, finding.Column) +} + +// TestRaiseModelUnbuildableChainsEveryGraphSentinel covers the raise site over every +// sentinel the builder can refuse with, rather than only the one that still reaches the +// backstop end to end. +// +// The cycle-shape check answers the other two before the backstop does, so without this +// the chaining would only ever be exercised for ErrInvalidModel and a caller matching on +// either cycle sentinel would have no test behind it. +func TestRaiseModelUnbuildableChainsEveryGraphSentinel(t *testing.T) { + t.Parallel() + + for _, sentinel := range []error{graph.ErrModelCycle, graph.ErrTupleCycle, graph.ErrInvalidModel} { + t.Run(sentinel.Error(), func(t *testing.T) { + t.Parallel() + + cause := fmt.Errorf("%w: from the builder", sentinel) + + collector := NewErrorCollector(nil) + collector.RaiseModelUnbuildable(describeRefusal(cause), cause) + + findings := collector.AllFindings() + require.Len(t, findings, 1) + + require.ErrorIs(t, findings[0], fgaerrors.ErrModelNotBuildable, "the refusal itself") + require.ErrorIs(t, findings[0], sentinel, "the reason the builder gave") + + assert.NotContains(t, findings[0].Message, "from the builder", + "the builder's own text reached the message, which is what varies between runs") + }) } +} - for name, test := range tests { +// TestEveryGraphSentinelHasAWording keeps graphRefusalReasons exhaustive over what the +// builder can refuse with. +// +// A sentinel missing from the table falls back to wording that says nothing about the model, +// which is a worse finding than the one it replaced. Adding a sentinel to pkg/go/graph should +// fail here rather than quietly degrade a message. +func TestEveryGraphSentinelHasAWording(t *testing.T) { + t.Parallel() + + // Every error value pkg/go/graph declares, read off its source rather than inferred, so + // this list going stale is a compile failure. + declared := map[string]error{ + "ErrModelCycle": graph.ErrModelCycle, + "ErrInvalidModel": graph.ErrInvalidModel, + "ErrTupleCycle": graph.ErrTupleCycle, + "ErrContrainstTupleCycle": graph.ErrContrainstTupleCycle, + } + + for name, sentinel := range declared { t.Run(name, func(t *testing.T) { t.Parallel() - findings := graphFindings(t, test.dsl) - require.Len(t, findings, 1, "a refused build yields one finding, there being no graph to enumerate") + reason := describeRefusal(fmt.Errorf("%w: detail the builder added", sentinel)) - finding := findings[0] - require.NotNil(t, finding.Metadata) + assert.NotEqual(t, unrecognisedRefusal, reason, "no wording for %s", name) + assert.NotContains(t, reason, "detail the builder added") - assert.Equal(t, GraphModelUnbuildable, finding.Metadata.ErrorType) - assert.Equal(t, test.wantMessage, finding.Message) + // The sentinel's own text, not that of something it wraps. Checking only + // that the fallback was avoided would pass on ErrContrainstTupleCycle + // resolving to the plain tuple cycle it wraps, which says less. + assert.Equalf(t, sentinel.Error(), reason, + "%s resolved to the wording of a less specific sentinel", name) + }) + } - require.ErrorIs(t, finding, fgaerrors.ErrModelNotBuildable, "the refusal itself") - require.ErrorIs(t, finding, test.wantCause, "the reason the builder gave") + assert.Equal(t, unrecognisedRefusal, describeRefusal(errors.New("something else entirely")), + "an error carrying no graph sentinel has to fall through rather than match one") +} - // The builder stops at the first problem and returns no graph with its error, - // so there is nothing to resolve a position against. - assert.Nil(t, finding.Line) - assert.Nil(t, finding.Column) - }) +// TestRefusalWordingIsTheSentinelNotTheBuilderMessage pins the substitution itself. +// +// The builder's message for this model names the type and relation it stopped at. That text +// is what varies between runs on a model with several problems, so it has to be absent from +// the finding and reachable through the chain. +func TestRefusalWordingIsTheSentinelNotTheBuilderMessage(t *testing.T) { + t.Parallel() + + dsl := modelRefusedWithNothingToSay.dsl + model := modelFromDSL(t, dsl) + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.Error(t, buildErr) + require.Contains(t, buildErr.Error(), "team type does not have defined viewer relation", + "the builder stopped naming a relation, so there is a detail to drop") + + findings := graphFindings(t, dsl) + require.Len(t, findings, 1) + + assert.NotContains(t, findings[0].Message, "team type does not have defined viewer relation", + "the builder's first-problem detail reached the finding") + assert.Contains(t, findings[0].Message, graph.ErrInvalidModel.Error()) + + // Dropped from the message, not lost. A caller that wants it unwraps. + require.ErrorIs(t, findings[0], graph.ErrInvalidModel) + assert.Contains(t, findings[0].Unwrap().Error(), "team type does not have defined viewer relation", + "the builder's text has to stay reachable through the chain") +} + +// TestGraphPathOutputIsStableAcrossRuns is the guarantee all of the sorting in this package +// exists for, asserted end to end rather than per pass. +// +// Whatever the builder's map iteration does, the same model has to produce the same findings +// in the same order every run, or a corpus comparison is flaky and an editor redraws +// diagnostics in a different order on every keystroke. +func TestGraphPathOutputIsStableAcrossRuns(t *testing.T) { + t.Parallel() + + // A model with two independent problems, which is the shape the builder picks between. + // Both tuplesets are missing the computed relation on one of their assignable types, so + // the rewrite-tree walk is silent and the refusal is what gets reported. + dsl := `model + schema 1.1 +type user +type folder + relations + define viewer: [user] + define editor: [user] +type team +type squad +type document + relations + define parent: [folder, team] + define viewer: viewer from parent +type file + relations + define owner: [folder, squad] + define editor: editor from owner +` + model := modelFromDSL(t, dsl) + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.Error(t, buildErr, "the model has to be one the builder refuses") + + first := describeFindings(findingsFrom( + ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: true})).GetErrors()) + require.NotEmpty(t, first) + + for range 50 { + assert.Equal(t, first, describeFindings(findingsFrom( + ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: true})).GetErrors())) } } @@ -340,7 +483,28 @@ type folder func TestUnbuildableFindingDoesNotClaimTheWrongSentinel(t *testing.T) { t.Parallel() - findings := graphFindings(t, `model + findings := graphFindings(t, modelRefusedWithNothingToSay.dsl) + require.Len(t, findings, 1) + + require.ErrorIs(t, findings[0], graph.ErrInvalidModel) + require.NotErrorIs(t, findings[0], graph.ErrModelCycle) + require.NotErrorIs(t, findings[0], graph.ErrTupleCycle) + require.NotErrorIs(t, findings[0], fgaerrors.ErrNoEntrypoints, + "a refused build is not an entrypoint finding, and a caller filtering on one must not see the other") + require.NotErrorIs(t, findings[0], fgaerrors.ErrRelationInUnresolvableCycle, + "the refusal is not a cycle finding, and the two share this branch") +} + +// TestRefusedModelKeepsThePerRelationFindingsTheWalkHas pins what a refusal costs a +// caller, which is a position and a relation name rather than a finding. +// +// The relations here are impossible as well as caught in a cycle, so the walk names them +// and the refusal adds nothing. Reporting the refusal instead would replace two findings a +// consumer can put on a line with one it cannot. +func TestRefusedModelKeepsThePerRelationFindingsTheWalkHas(t *testing.T) { + t.Parallel() + + dsl := `model schema 1.1 type user type document @@ -348,14 +512,20 @@ type document define admin: [user] define viewer: admin and editor define editor: viewer -`) - require.Len(t, findings, 1) +` - require.ErrorIs(t, findings[0], graph.ErrModelCycle) - require.NotErrorIs(t, findings[0], graph.ErrTupleCycle) - require.NotErrorIs(t, findings[0], graph.ErrInvalidModel) - require.NotErrorIs(t, findings[0], fgaerrors.ErrNoEntrypoints, - "a refused build is not an entrypoint finding, and a caller filtering on one must not see the other") + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(modelFromDSL(t, dsl)) + require.Error(t, buildErr, "the model has to be one the builder refuses") + + assert.Equal(t, describeFindings(findingsFrom(ValidateDSL(modelFromDSL(t, dsl), dsl, + DefaultEngineOptions())).GetErrors()), describeFindings(graphFindings(t, dsl)), + "a refused build reports what the walk reports, down to the position") + + for _, finding := range graphFindings(t, dsl) { + require.NotNil(t, finding.Metadata) + assert.Equal(t, RelationNoEntrypoint, finding.Metadata.ErrorType) + assert.NotNil(t, finding.Line, "a consumer needs somewhere to put this") + } } // TestGraphValidationIsOffByDefault pins the switch. The graph path is not the source of @@ -441,7 +611,8 @@ type document } // TestValidateWithGraphOnNilModel checks the guard. RunAllValidations returns before the -// phases on a nil model, so this reaches the function directly. +// phases on a nil model, so this reaches the function directly, both with no validator at +// all and with one built over a nil model, which is the shape the engine would hold. func TestValidateWithGraphOnNilModel(t *testing.T) { t.Parallel() @@ -449,6 +620,10 @@ func TestValidateWithGraphOnNilModel(t *testing.T) { validateWithGraph(collector, nil, nil) assert.Equal(t, 0, collector.CountAll(), "a nil model has nothing to build and nothing to report") + + validateWithGraph(collector, NewSemanticValidator(nil), nil) + + assert.Equal(t, 0, collector.CountAll(), "a validator over a nil model has nothing to report either") } // TestGraphRuleRegistryIsWellFormed keeps the registry usable as the place rules are @@ -626,9 +801,9 @@ type document } } -// TestReachesOnlyRewritesPicksTheMessageVariant covers the discriminator on its own, so a +// TestCyclesThroughRewritesPicksTheMessageVariant covers the discriminator on its own, so a // change to it is a failure here rather than a message that reads oddly in the corpus. -func TestReachesOnlyRewritesPicksTheMessageVariant(t *testing.T) { +func TestCyclesThroughRewritesPicksTheMessageVariant(t *testing.T) { t.Parallel() tests := map[string]struct { @@ -636,7 +811,7 @@ func TestReachesOnlyRewritesPicksTheMessageVariant(t *testing.T) { nodeID string wantOnly bool }{ - "self rewrite reaches only rewrites": { + "self rewrite cycles through rewrites": { dsl: `model schema 1.1 type user @@ -698,19 +873,19 @@ type document require.Truef(t, ok, "node %q is not in the graph, so this case tests nothing", test.nodeID) scope := &graphScope{weighted: weighted, model: model} - assert.Equal(t, test.wantOnly, scope.reachesOnlyRewrites(test.nodeID)) + assert.Equal(t, test.wantOnly, scope.cyclesThroughRewrites(test.nodeID)) }) } } -// TestReachesOnlyRewritesTerminatesOnACycle checks the visited set. A relation that +// TestCyclesThroughRewritesTerminatesOnACycle checks the visited set. A relation that // rewrites itself has an edge back to the node the walk started at, so without the visited // set the queue would never empty. // // A self-rewrite is the only cycle this can reach. The walk stops at the first edge that // is not a rewrite, so a longer cycle would have to be a chain of rewrites, and the // builder refuses every one of those with a model cycle before a rule sees it. -func TestReachesOnlyRewritesTerminatesOnACycle(t *testing.T) { +func TestCyclesThroughRewritesTerminatesOnACycle(t *testing.T) { t.Parallel() model := modelFromDSL(t, `model @@ -739,7 +914,7 @@ type document require.True(t, closesOnItself, "the node does not reach itself, so this case tests nothing") scope := &graphScope{weighted: weighted, model: model} - assert.True(t, scope.reachesOnlyRewrites("document#viewer")) + assert.True(t, scope.cyclesThroughRewrites("document#viewer")) } // TestGraphValidationRunsBehindTheCascadeGate pins where the phase sits. A model with an @@ -796,15 +971,7 @@ type document func TestUnbuildableFindingIsRecoverableThroughTheEntryPoints(t *testing.T) { t.Parallel() - dsl := `model - schema 1.1 -type user -type document - relations - define admin: [user] - define viewer: admin and editor - define editor: viewer -` + dsl := modelRefusedWithNothingToSay.dsl err := ValidateDSL(modelFromDSL(t, dsl), dsl, &EngineOptions{UseGraphValidation: true}) require.Error(t, err) @@ -814,5 +981,5 @@ type document require.Len(t, collection.GetErrors(), 1) require.ErrorIs(t, err, fgaerrors.ErrModelNotBuildable, "the collection carries the sentinel through") - require.ErrorIs(t, err, graph.ErrModelCycle) + require.ErrorIs(t, err, graph.ErrInvalidModel) } diff --git a/pkg/go/validation/validation_engine.go b/pkg/go/validation/validation_engine.go index 8ed1f801..3e3430c4 100644 --- a/pkg/go/validation/validation_engine.go +++ b/pkg/go/validation/validation_engine.go @@ -117,7 +117,7 @@ func (ve *ValidationEngine) RunAllValidations(options *EngineOptions) *Validatio // One of these resolves entrypoints and cycles, never both. See // EngineOptions.UseGraphValidation. if options.UseGraphValidation { - validateWithGraph(ve.collector, ve.model, ve.lines) + validateWithGraph(ve.collector, ve.semantic, ve.lines) } else { validateCyclesAndEntryPoints(ve.collector, ve.semantic, ve.lines) } diff --git a/pkg/java/src/main/java/dev/openfga/language/validation/ModelValidator.java b/pkg/java/src/main/java/dev/openfga/language/validation/ModelValidator.java index 65412af3..62ffa85f 100644 --- a/pkg/java/src/main/java/dev/openfga/language/validation/ModelValidator.java +++ b/pkg/java/src/main/java/dev/openfga/language/validation/ModelValidator.java @@ -183,7 +183,11 @@ private void modelValidation() { var currentRelations = typeMap.get(typeName).getRelations(); var typeDefMetadata = typeDef.getMetadata(); var typeDefRelationsMetadata = getNullSafe(typeMap.get(typeName).getMetadata(), Metadata::getRelations); - for (var relationName : typeDef.getRelations().keySet()) { + // Sorted, because the relations are held in a HashMap and iterating it + // yields them in hash bucket order. Two relations of one type that both + // lack an entry point would otherwise be reported in an order that has + // nothing to do with the model, and pkg/go reports them sorted. + for (var relationName : new TreeSet<>(typeDef.getRelations().keySet())) { var result = EntryPointOrLoop.compute( typeMap, typeName, relationName, currentRelations.get(relationName), new HashMap<>()); diff --git a/tests/data/dsl-semantic-validation-cases.yaml b/tests/data/dsl-semantic-validation-cases.yaml index d7a53a42..0aee73da 100644 --- a/tests/data/dsl-semantic-validation-cases.yaml +++ b/tests/data/dsl-semantic-validation-cases.yaml @@ -621,6 +621,128 @@ end: 18 metadata: errorType: relation-no-entry-point +- name: no entry point alongside relations that have one + dsl: | + model + schema 1.1 + type user + type document + relations + define parent: [document] + define broken: [user] and broken from parent + define ok1: [user] + define ok2: [user] + expected_errors: + - msg: "`broken` is an impossible relation for `document` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 17 + metadata: + symbol: "broken" + errorType: relation-no-entry-point +- name: no entry point in exclusion alongside a relation that has one + dsl: | + model + schema 1.1 + type user + type document + relations + define parent: [document] + define broken: [user] but not broken from parent + define ok1: [user] + expected_errors: + - msg: "`broken` is an impossible relation for `document` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 17 + metadata: + symbol: "broken" + errorType: relation-no-entry-point +- name: no entry point reported for every relation that lacks one + dsl: | + model + schema 1.1 + type user + type document + relations + define parent: [document] + define brokenA: [user] and brokenA from parent + define brokenB: [user] and brokenB from parent + define ok1: [user] + expected_errors: + - msg: "`brokenA` is an impossible relation for `document` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 18 + metadata: + symbol: "brokenA" + errorType: relation-no-entry-point + - msg: "`brokenB` is an impossible relation for `document` (no entrypoint)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 18 + metadata: + symbol: "brokenB" + errorType: relation-no-entry-point +- name: no entry point on one type does not affect another type + dsl: | + model + schema 1.1 + type user + type folder + relations + define fparent: [folder] + define fbroken: [user] and fbroken from fparent + type document + relations + define parent: [document] + define ok1: [user] + expected_errors: + - msg: "`fbroken` is an impossible relation for `folder` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 18 + metadata: + symbol: "fbroken" + errorType: relation-no-entry-point +- name: rewrite chain keeps its entry point beside a relation that has none + dsl: | + model + schema 1.1 + type user + type document + relations + define parent: [document] + define c3: [user] + define c2: c3 + define c1: c2 + define broken: c1 and broken from parent + expected_errors: + - msg: "`broken` is an impossible relation for `document` (no entrypoint)." + line: + start: 9 + end: 9 + column: + start: 11 + end: 17 + metadata: + symbol: "broken" + errorType: relation-no-entry-point - name: intersection child not allow to reference itself in TTU dsl: | model