From 1e1f5204e3e9c9eb3b7531196cef69c41ab5b904 Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Wed, 19 Aug 2026 20:55:03 +0530 Subject: [PATCH] feat(pkg/go): validate entrypoints from the weighted graph Adds pkg/go/validation/graph_validation.go, which builds the weighted graph through the exported builder and reports unreachable relations by reading it rather than by walking the rewrite tree. A relation node carries one weight per terminal type it can reach, so reaching none is what the traversal calls an impossible relation. EngineOptions.UseGraphValidation selects it, and the two paths are an if/else rather than two phases. They answer the same question by different means, so running both would report one problem twice and disagree wherever they diverge. It is off by default because the graph path is not the source of truth yet. Over the 84 non-skipped cases of the shared semantic corpus, validation and the graph reject 33 of the same models, validation rejects 47 the graph builds, and the graph rejects none that validation accepts. Of the 51 the graph builds, the new rule and the traversal name an identical set of entrypoint findings on all 51, compared as whole findings including symbol, line, column and message. The rule fires on 7, so that comparison is not of empty lists. Under the option, 73 of 84 corpus cases pass. The 11 that do not diverge one way: Build stops at the first problem and returns no graph with its error, so there is nothing left to enumerate relations from, and a model with three broken relations yields one finding naming none of them. Those cases report the new graph-model-unbuildable instead, which carries no position. It chains the error the builder returned under ErrModelNotBuildable, so errors.Is reaches both the refusal and the reason. Nothing under pkg/go/graph changes. --- docs/validation/model/README.md | 4 + .../model/graph-model-unbuildable.md | 165 ++++ pkg/go/errors/sentinels.go | 6 + pkg/go/validation/criticality_test.go | 1 + pkg/go/validation/error_collector.go | 29 +- pkg/go/validation/error_info.go | 12 + pkg/go/validation/errors.go | 3 + pkg/go/validation/graph_validation.go | 227 +++++ .../graph_validation_corpus_test.go | 253 ++++++ pkg/go/validation/graph_validation_test.go | 818 ++++++++++++++++++ pkg/go/validation/validation_engine.go | 21 +- 11 files changed, 1537 insertions(+), 2 deletions(-) create mode 100644 docs/validation/model/graph-model-unbuildable.md create mode 100644 pkg/go/validation/graph_validation.go create mode 100644 pkg/go/validation/graph_validation_corpus_test.go create mode 100644 pkg/go/validation/graph_validation_test.go diff --git a/docs/validation/model/README.md b/docs/validation/model/README.md index 6843609c..4f187507 100644 --- a/docs/validation/model/README.md +++ b/docs/validation/model/README.md @@ -46,6 +46,7 @@ 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) | 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` @@ -53,6 +54,9 @@ and `cyclic-relation`. An unrecognised schema version reports `invalid-schema`, 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. + ## Usage Each error documentation includes: diff --git a/docs/validation/model/graph-model-unbuildable.md b/docs/validation/model/graph-model-unbuildable.md new file mode 100644 index 00000000..47160e6c --- /dev/null +++ b/docs/validation/model/graph-model-unbuildable.md @@ -0,0 +1,165 @@ +# Graph Model Unbuildable + +**Error Code:** `graph-model-unbuildable` + +**Category:** Semantic Validation + +## 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. + +## 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. + +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: + +- `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 + +A finding with this code wraps two errors. `errors.Is` matches +`errors.ErrModelNotBuildable` for the refusal itself, and it also matches whichever +sentinel the graph package returned, so a caller can branch on the specific reason +without parsing the message: + +```go +var findings *validation.ValidationErrors +if errors.As(err, &findings) { + for _, f := range findings.GetErrors() { + if errors.Is(f, fgaerrors.ErrModelNotBuildable) { + // the build was refused + } + if errors.Is(f, graph.ErrTupleCycle) { + // and this is why + } + } +} +``` + +## Example + +The following model would trigger this error: + +``` +model + schema 1.1 + +type user + +type document + relations + define admin: [user] + define viewer: admin and editor + 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: + +``` +model + schema 1.1 + +type user + +type folder + relations + define parent: [folder] + define viewer: [user] but not banned + define banned: viewer from parent +``` + +**Error Message:** `the model cannot be built into a weighted graph: tuple cycle: operands AND or BUT NOT cannot be involved in a cycle` + +## 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. + +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: + +``` +model + schema 1.1 + +type user + +type document + relations + define admin: [user] + define viewer: admin and editor + define editor: [user] +``` + +For a cycle through `and` or `but not`, take the cyclic relation out of the operand: + +``` +model + schema 1.1 + +type user + +type folder + relations + define parent: [folder] + define banned: [user] + define viewer: [user] but not banned +``` + +### 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. + +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. + +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. + +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 + +## Implementation Notes + +This code is specific to the Go implementation's graph-backed validation path. The +JavaScript and Java validators walk the rewrite tree and have no equivalent. + +- Go implementation: `pkg/go/validation/graph_validation.go` +- Graph builder: `pkg/go/graph/weighted_graph_builder.go` + +Validation calls the exported builder rather than reimplementing it, so a model refused +here is refused for anything else that builds the same graph. diff --git a/pkg/go/errors/sentinels.go b/pkg/go/errors/sentinels.go index 73851944..85fb70db 100644 --- a/pkg/go/errors/sentinels.go +++ b/pkg/go/errors/sentinels.go @@ -87,6 +87,12 @@ var ( // module. ErrMultipleModulesInFile = errors.New("file contains multiple modules") + // ErrModelNotBuildable is reported when a model cannot be built into a + // weighted graph. It says only that the build was refused; the reason is the + // error the builder returned, which a finding carries underneath this one, so + // errors.Is reaches either. + ErrModelNotBuildable = errors.New("model cannot be built into a weighted graph") + // 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 5ece9e2c..81713c1b 100644 --- a/pkg/go/validation/criticality_test.go +++ b/pkg/go/validation/criticality_test.go @@ -73,6 +73,7 @@ func TestCriticalityOfEveryEmittedCode(t *testing.T) { DuplicatedError: true, InvalidSchema: true, MultipleModulesInFile: true, + GraphModelUnbuildable: true, } // Nothing raises these two, so they are held at not-critical rather than listed diff --git a/pkg/go/validation/error_collector.go b/pkg/go/validation/error_collector.go index 70a48ea9..0652e3db 100644 --- a/pkg/go/validation/error_collector.go +++ b/pkg/go/validation/error_collector.go @@ -123,6 +123,11 @@ type scope struct { // with different scopes: a duplicate type and a duplicate type restriction share // one code without being the same kind of finding. category fgaerrors.ModelErrorKind + + // cause overrides the table's sentinel when set, for a finding that carries an + // error it did not raise itself. It is wrapped in the scoped type as any sentinel + // would be, so errors.Is still reaches whatever the original error wrapped. + cause error } // addError is a helper to add an error to the collection. @@ -166,10 +171,15 @@ func (c *ErrorCollector) addScopedError(message string, errorType ValidationErro category = errorScope.category } + sentinel := entry.Cause + if errorScope.cause != nil { + sentinel = errorScope.cause + } + // The cause carries the scope and the metadata is derived from it, so the JSON // and the errors.As payload cannot disagree. offendingType is metadata only, so // it comes straight off the scope. - cause := newScopedCause(category, errorScope, entry.Cause) + cause := newScopedCause(category, errorScope, sentinel) objectType, relation, condition := causeScope(cause) metadata := &ErrorMetadata{ @@ -577,3 +587,20 @@ func (c *ErrorCollector) RaiseEmptyDifference(relationName, typeName, operation relation: relationName, }) } + +// Graph validation error methods + +// 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. +// +// 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) + 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 a27bb25d..a73fc597 100644 --- a/pkg/go/validation/error_info.go +++ b/pkg/go/validation/error_info.go @@ -163,6 +163,17 @@ var errorInfoByType = map[ValidationErrorType]errorInfo{ Cause: fgaerrors.ErrMultipleModulesInFile, Critical: true, }, + + // Weighted graph. The raise site chains the error the builder returned underneath + // this sentinel, so a caller can match on the build being refused without knowing + // which of the graph's own sentinels fired. Critical, because a model the graph + // refuses has no graph, so nothing further can be reported about it. + GraphModelUnbuildable: { + Severity: fgaerrors.SeverityError, + Category: fgaerrors.ErrorKindInvalidModel, + Cause: fgaerrors.ErrModelNotBuildable, + Critical: true, + }, } // unemittedErrorTypes are declared ValidationErrorType values that no validation @@ -219,6 +230,7 @@ var allErrorTypes = []ValidationErrorType{ MultipleModulesInFile, CyclicRelation, InvalidSchemaVersion, + GraphModelUnbuildable, } // isCriticalErrorType reports whether a code invalidates the model as a whole. diff --git a/pkg/go/validation/errors.go b/pkg/go/validation/errors.go index 08387792..3ea1ff6c 100644 --- a/pkg/go/validation/errors.go +++ b/pkg/go/validation/errors.go @@ -44,6 +44,9 @@ const ( MultipleModulesInFile ValidationErrorType = "multiple-modules-in-file" CyclicRelation ValidationErrorType = "cyclic-relation" InvalidSchemaVersion ValidationErrorType = "invalid-schema-version" + + // Weighted graph errors. + GraphModelUnbuildable ValidationErrorType = "graph-model-unbuildable" ) // Range is a start and end position in the source text, used for both the line and diff --git a/pkg/go/validation/graph_validation.go b/pkg/go/validation/graph_validation.go new file mode 100644 index 00000000..9a82284e --- /dev/null +++ b/pkg/go/validation/graph_validation.go @@ -0,0 +1,227 @@ +package validation + +import ( + "slices" + "strings" + + openfgav1 "github.com/openfga/api/proto/openfga/v1" + + "github.com/openfga/language/pkg/go/graph" +) + +// validateWithGraph resolves entrypoints and cycles from the weighted graph rather than +// by walking the rewrite tree. +// +// It builds through the exported WeightedAuthorizationModelGraphBuilder rather than +// 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 { + return + } + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + if err != nil { + collector.RaiseModelUnbuildable(err) + + return + } + + scope := &graphScope{weighted: weighted, model: model, collector: collector, lines: lines} + for _, rule := range graphRules { + rule.check(scope) + } +} + +// 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 { + // id names the rule in tests and in failure output. It is not a wire value; what + // reaches a caller is the ValidationErrorType the rule raises. + id string + check func(*graphScope) +} + +// graphRules is the registry every rule joins. A new rule is one entry here plus one +// function, so rules written in parallel meet only on this line. +var graphRules = []graphRule{ + {id: "entrypoints", check: checkRelationEntrypoints}, +} + +// graphScope is what a rule gets: the built graph, the model behind it, and the means to +// report a finding against a relation. +type graphScope struct { + weighted *graph.WeightedAuthorizationModelGraph + model *openfgav1.AuthorizationModel + collector *ErrorCollector + lines []string +} + +// checkRelationEntrypoints reports relations that can never be satisfied. +// +// A relation node carries one weight per terminal type it can reach. Reaching none means +// no tuple can ever satisfy the relation, which is what the rewrite-tree traversal calls +// an impossible relation. Recursion alone does not empty the weights: a relation with a +// base case keeps its terminal types and takes weight Infinite, so +// `[user] or viewer from parent` is untouched here while `viewer from parent` alone is +// reported. +// +// Only SpecificTypeAndRelation nodes are considered. A type node has no weights either, +// because a type is what weights are counted to rather than from. +func checkRelationEntrypoints(scope *graphScope) { + for _, nodeID := range sortedNodeIDs(scope.weighted) { + node, ok := scope.weighted.GetNodeByID(nodeID) + if !ok || node.GetNodeType() != graph.SpecificTypeAndRelation { + continue + } + + if len(node.GetWeights()) > 0 { + continue + } + + definition, ok := relationForNode(scope.weighted, node) + if !ok { + continue + } + + objectType, relation, ok := splitRelationLabel(definition) + if !ok { + continue + } + + meta := scope.metaFor(objectType, relation) + lineIndex := scope.relationLine(objectType, relation) + + if scope.reachesOnlyRewrites(nodeID) { + scope.collector.RaiseNoEntryPointLoop(relation, objectType, meta, lineIndex) + + continue + } + + scope.collector.RaiseNoEntryPoint(relation, objectType, meta, lineIndex) + } +} + +// reachesOnlyRewrites reports whether every edge reachable from nodeID rewrites another +// relation, with no direct assignment or tupleset anywhere. +// +// 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} + queue := []string{nodeID} + sawEdge := false + + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + + 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 + } + + next := edge.GetTo().GetUniqueLabel() + if !visited[next] { + visited[next] = true + queue = append(queue, next) + } + } + } + + return sawEdge +} + +// relationLine resolves the source line a relation is declared on, through the same +// helpers the other phases use, so a graph finding and a traversal finding for the same +// relation land in the same place. +func (s *graphScope) relationLine(objectType, relation string) *int { + if len(s.lines) == 0 { + return nil + } + + typeLine := GetTypeLineNumber(objectType, s.lines, nil) + + return GetRelationLineNumber(relation, s.lines, typeLine) +} + +// metaFor resolves the file and module a relation was declared in. The graph knows +// neither: a modular model reaches Build already flattened, with its provenance left on +// the model's metadata. +func (s *graphScope) metaFor(objectType, relation string) *Meta { + for _, typeDef := range s.model.GetTypeDefinitions() { + if typeDef.GetType() != objectType { + continue + } + + return relationMeta(typeDef, relation) + } + + return nil +} + +// splitRelationLabel splits a relation definition into its object type and relation, +// e.g. "document#viewer". +func splitRelationLabel(definition string) (objectType, relation string, ok bool) { + objectType, relation, found := strings.Cut(definition, "#") + if !found || objectType == "" || relation == "" { + return "", "", false + } + + return objectType, relation, true +} + +// relationForNode resolves the relation a node belongs to. +// +// A relation node is its own answer. An operator or logical node is not: its label holds +// the relation but also an operator and an index, and the grouping labels are built +// differently again. Rather than parse those forms, this reads the relation off an +// outgoing edge, which records the relation the edge was written for. Operator and logical +// nodes exist to group edges, so they always have one. +func relationForNode(weighted *graph.WeightedAuthorizationModelGraph, + node *graph.WeightedAuthorizationModelNode) (string, bool) { + switch node.GetNodeType() { + case graph.SpecificType, graph.SpecificTypeWildcard: + // A terminal node belongs to no relation. + return "", false + case graph.SpecificTypeAndRelation: + return node.GetUniqueLabel(), true + } + + edges, _ := weighted.GetEdgesFromNodeID(node.GetUniqueLabel()) + for _, edge := range edges { + if definition := edge.GetRelationDefinition(); definition != "" { + return definition, true + } + } + + return "", false +} + +// sortedNodeIDs returns the graph's node IDs in a stable order, so the same model raises +// findings in the same order. The graph stores nodes in a map, which Go iterates randomly. +func sortedNodeIDs(weighted *graph.WeightedAuthorizationModelGraph) []string { + nodes := weighted.GetNodes() + ids := make([]string, 0, len(nodes)) + + for id := range nodes { + ids = append(ids, id) + } + + slices.Sort(ids) + + return ids +} diff --git a/pkg/go/validation/graph_validation_corpus_test.go b/pkg/go/validation/graph_validation_corpus_test.go new file mode 100644 index 00000000..93f3f6f0 --- /dev/null +++ b/pkg/go/validation/graph_validation_corpus_test.go @@ -0,0 +1,253 @@ +package validation + +import ( + "fmt" + "sort" + "testing" + + openfgav1 "github.com/openfga/api/proto/openfga/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/openfga/language/pkg/go/graph" + "github.com/openfga/language/pkg/go/transformer" +) + +// corpusModels returns the non-skipped cases of the shared semantic corpus with their +// models already parsed. +// +// It fails on a case that does not parse rather than dropping it. The corpus is the +// contract between the implementations, and a case silently excluded from a coverage +// count is worse than one that fails. +func corpusModels(t *testing.T) []struct { + Case YAMLTestCase + Model *openfgav1.AuthorizationModel +} { + t.Helper() + + suite, err := NewYAMLTestRunner(corpusDir).LoadTestSuite("dsl-semantic-validation-cases.yaml") + require.NoError(t, err) + require.NotEmpty(t, suite.TestCases, "corpus loaded no cases") + + var cases []struct { + Case YAMLTestCase + Model *openfgav1.AuthorizationModel + } + + for _, testCase := range suite.TestCases { + if testCase.Skip { + continue + } + + model, err := transformer.TransformDSLToProto(testCase.DSL) + require.NoErrorf(t, err, "corpus case %q does not parse", testCase.Name) + + cases = append(cases, struct { + Case YAMLTestCase + Model *openfgav1.AuthorizationModel + }{Case: testCase, Model: model}) + } + + return cases +} + +// TestGraphAgreesWithTraversalOnWhichModelsAreInvalid pins how the two paths divide the +// corpus, and the claim that matters is graphOnly being zero: over every case available +// there is no model the graph rejects that validation calls valid. That is what makes +// the graph safe to put behind the same entry points. +// +// The other counts are here so a change to either path shows up as a number rather than +// as a corpus case quietly moving between buckets. +func TestGraphAgreesWithTraversalOnWhichModelsAreInvalid(t *testing.T) { + t.Parallel() + + var bothFlag, validationOnly, graphOnly, bothClean int + + var graphOnlyNames []string + + cases := corpusModels(t) + + for _, entry := range cases { + invalidByTraversal := findingsFrom( + ValidateDSL(entry.Model, entry.Case.DSL, DefaultEngineOptions())).HasErrors() + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(entry.Model) + refusedByGraph := buildErr != nil + + switch { + case invalidByTraversal && refusedByGraph: + bothFlag++ + case invalidByTraversal: + validationOnly++ + case refusedByGraph: + graphOnly++ + + graphOnlyNames = append(graphOnlyNames, fmt.Sprintf("%s: %v", entry.Case.Name, buildErr)) + default: + bothClean++ + } + } + + 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.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. +// +// 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. +func TestCorpusUnderGraphValidation(t *testing.T) { + t.Parallel() + + var diverged []string + + for _, entry := range corpusModels(t) { + result := compareWithCorpus(entry.Case.ExpectedErrors, findingsFrom( + ValidateDSL(entry.Model, entry.Case.DSL, &EngineOptions{UseGraphValidation: true}))) + + if result.Status != corpusPass { + diverged = append(diverged, entry.Case.Name) + } + } + + sort.Strings(diverged) + + assert.Equal(t, graphDivergentCorpusCases, diverged, + "the set of corpus cases the graph path does not satisfy has changed") +} + +// TestGraphDivergenceIsOnlyTheUnbuildableSubstitution checks what the divergence above +// consists of, so the test names a behaviour rather than a list. +// +// 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) { + t.Parallel() + + divergent := make(map[string]struct{}, len(graphDivergentCorpusCases)) + for _, name := range graphDivergentCorpusCases { + divergent[name] = struct{}{} + } + + var expectedEntrypointFindings, checked int + + for _, entry := range corpusModels(t) { + if _, ok := divergent[entry.Case.Name]; !ok { + continue + } + + checked++ + + 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) + + expectedEntrypointFindings++ + } + + findings := findingsFrom( + ValidateDSL(entry.Model, entry.Case.DSL, &EngineOptions{UseGraphValidation: true})).GetErrors() + + 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) + } + + 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") +} + +// TestEntrypointRuleMatchesTraversalWhereTheGraphBuilds is the parity guarantee behind +// the switch: wherever there is a graph to read, reading it names the same unreachable +// relations as walking the rewrite tree, with the same message and the same position. +// +// It compares whole findings rather than counts, so a rule that found the right +// relations and resolved them to the wrong line fails here. +func TestEntrypointRuleMatchesTraversalWhereTheGraphBuilds(t *testing.T) { + t.Parallel() + + var built, firedOn int + + for _, entry := range corpusModels(t) { + if _, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(entry.Model); err != nil { + continue + } + + built++ + + fromTraversal := entrypointFindings(entry.Model, entry.Case.DSL, false) + fromGraph := entrypointFindings(entry.Model, entry.Case.DSL, true) + + if len(fromGraph) > 0 { + firedOn++ + } + + assert.Equalf(t, fromTraversal, fromGraph, + "the two paths report different entrypoint findings for %q", entry.Case.Name) + } + + assert.Equal(t, 51, built, "corpus cases the graph builds") + assert.Equal(t, 7, firedOn, + "cases the graph rule reports an unreachable relation for; zero here would make the "+ + "comparison above a comparison of empty lists") +} + +// entrypointFindings returns the entrypoint findings for a model as sorted strings +// carrying the symbol, the position and the message, which is everything the corpus +// compares. +func entrypointFindings(model *openfgav1.AuthorizationModel, dsl string, useGraph bool) []string { + findings := findingsFrom(ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: useGraph})).GetErrors() + + described := []string{} + + for _, finding := range findings { + if finding.Metadata == nil || finding.Metadata.ErrorType != RelationNoEntrypoint { + continue + } + + described = append(described, fmt.Sprintf("%s %s %q", + finding.Metadata.Symbol, + describePosition(finding.Line, finding.Column), + finding.Message)) + } + + sort.Strings(described) + + return described +} diff --git a/pkg/go/validation/graph_validation_test.go b/pkg/go/validation/graph_validation_test.go new file mode 100644 index 00000000..c91e79c3 --- /dev/null +++ b/pkg/go/validation/graph_validation_test.go @@ -0,0 +1,818 @@ +package validation + +import ( + "fmt" + "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" +) + +// graphFindings runs validation with the graph path selected and returns the findings. +func graphFindings(t *testing.T, dsl string) []*ValidationError { + t.Helper() + + return findingsFrom(ValidateDSL(modelFromDSL(t, dsl), dsl, + &EngineOptions{UseGraphValidation: true})).GetErrors() +} + +// describeFindings renders findings as code, symbol, position and message, which is +// everything a caller can see. +func describeFindings(findings []*ValidationError) []string { + described := []string{} + + for _, finding := range findings { + errorType := ValidationErrorType("") + symbol := "" + + if finding.Metadata != nil { + errorType = finding.Metadata.ErrorType + symbol = finding.Metadata.Symbol + } + + described = append(described, fmt.Sprintf("[%s] %s%s %q", + errorType, symbol, describePosition(finding.Line, finding.Column), finding.Message)) + } + + return described +} + +// TestCheckRelationEntrypoints covers the rule that reads unreachable relations off the +// graph, one case per thing the rule has to get right. +// +// The messages are asserted in full, not by code alone. They are what the shared corpus +// compares across implementations, so a rule that found the right relation and worded it +// differently is a divergence. +func TestCheckRelationEntrypoints(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + dsl string + want []string + }{ + // A relation that rewrites only itself closes through a computed edge and reaches + // nothing, which is the loop wording. + "relation that rewrites itself": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`, + want: []string{ + "[relation-no-entry-point] viewer line 5-5 column 11-17 " + + "\"`viewer` is an impossible relation for `document` (potential loop).\"", + }, + }, + // Every unreachable relation is reported, not just the first the walk reached. + "two relations that rewrite themselves": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: viewer + define reader: reader +`, + want: []string{ + "[relation-no-entry-point] reader line 6-6 column 11-17 " + + "\"`reader` is an impossible relation for `document` (potential loop).\"", + "[relation-no-entry-point] viewer line 5-5 column 11-17 " + + "\"`viewer` is an impossible relation for `document` (potential loop).\"", + }, + }, + // Both wordings out of one model, which is the discriminator doing its work rather + // than one variant happening to be right for every case. + "a loop and a missing entrypoint in one model": { + dsl: `model + schema 1.1 +type user +type folder + relations + define parent: [folder] + define viewer: viewer + define reader: reader from parent +`, + want: []string{ + "[relation-no-entry-point] viewer line 6-6 column 11-17 " + + "\"`viewer` is an impossible relation for `folder` (potential loop).\"", + "[relation-no-entry-point] reader line 7-7 column 11-17 " + + "\"`reader` is an impossible relation for `folder` (no entrypoint).\"", + }, + }, + // A tupleset that reaches itself across two types, which is the shape the corpus + // fires on rather than the single-relation one above. + "tupleset that reaches itself across two types": { + dsl: `model + schema 1.1 +type user +type team + relations + define parent: [group] + define viewer: viewer from parent +type group + relations + define parent: [team] + define viewer: viewer from parent +`, + want: []string{ + "[relation-no-entry-point] viewer line 6-6 column 11-17 " + + "\"`viewer` is an impossible relation for `team` (no entrypoint).\"", + "[relation-no-entry-point] viewer line 10-10 column 11-17 " + + "\"`viewer` is an impossible relation for `group` (no entrypoint).\"", + }, + }, + // One reachable branch of a union is enough, so the unreachable one is not a + // finding on its own. + "union with one satisfiable branch is not reported": { + dsl: `model + schema 1.1 +type user +type folder + relations + define parent: [folder] + define viewer: viewer from parent or reader from parent + define reader: [user] +`, + want: []string{}, + }, + // A tupleset that can never be satisfied reaches a TTU edge, so it is a missing + // entrypoint rather than a loop. + "tupleset with no reachable terminal type": { + dsl: `model + schema 1.1 +type user +type folder + relations + define parent: [folder] + define viewer: viewer from parent +`, + want: []string{ + "[relation-no-entry-point] viewer line 6-6 column 11-17 " + + "\"`viewer` is an impossible relation for `folder` (no entrypoint).\"", + }, + }, + // Recursion with a base case keeps its terminal types and takes weight Infinite, + // so an empty weights map is not merely "this relation recurses". + "recursion with a base case 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{}, + }, + "direct assignment is not reported": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: [user] +`, + want: []string{}, + }, + // A wildcard is a terminal type, so it is an entrypoint like any other. + "wildcard is an entrypoint": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: [user:*] +`, + want: []string{}, + }, + // A relation reachable only through a userset still has a terminal type behind it. + "userset restriction is an entrypoint": { + dsl: `model + schema 1.1 +type user +type group + relations + define member: [user] +type document + relations + define viewer: [group#member] +`, + want: []string{}, + }, + // The rule looks at relation nodes only. An operator node has no weights of its + // own to speak of and must not be reported as a relation. + "operator over reachable children is not reported": { + dsl: `model + schema 1.1 +type user +type document + relations + define admin: [user] + define editor: [user] + define viewer: admin or editor +`, + want: []string{}, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.ElementsMatch(t, test.want, describeFindings(graphFindings(t, test.dsl))) + }) + } +} + +// TestGraphEntrypointFindingsCarryTheSameContractAsTheTraversal checks the parts of a +// finding a caller branches on, rather than reads: the code, the sentinel and the +// criticality. +// +// Without this the rule could satisfy the corpus on message text alone while handing +// callers a finding they cannot classify. +func TestGraphEntrypointFindingsCarryTheSameContractAsTheTraversal(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define viewer: viewer +` + + findings := graphFindings(t, dsl) + require.Len(t, findings, 1) + + finding := findings[0] + require.NotNil(t, finding.Metadata) + + assert.Equal(t, RelationNoEntrypoint, finding.Metadata.ErrorType) + require.ErrorIs(t, finding, fgaerrors.ErrNoEntrypoints) + assert.Equal(t, fgaerrors.SeverityError, finding.Severity) + assert.True(t, isCriticalErrorType(finding.Metadata.ErrorType)) + + // The same model down the traversal path, field for field. Anything the graph path + // leaves unset that the traversal sets is a contract the caller loses by switching. + fromTraversal := findingsFrom(ValidateDSL(modelFromDSL(t, dsl), dsl, DefaultEngineOptions())).GetErrors() + require.Len(t, fromTraversal, 1) + assert.Equal(t, describeFindings(fromTraversal), describeFindings(findings)) + assert.Equal(t, fromTraversal[0].Severity, finding.Severity) + assert.Equal(t, fromTraversal[0].Category, finding.Category) + assert.Equal(t, fromTraversal[0].Metadata, finding.Metadata) +} + +// TestRaiseModelUnbuildableReachesBothSentinels covers the one code this path adds. +// +// 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 +// site overrides the table's cause. +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, + }, + } + + for name, test := range tests { + 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") + + finding := findings[0] + require.NotNil(t, finding.Metadata) + + assert.Equal(t, GraphModelUnbuildable, finding.Metadata.ErrorType) + assert.Equal(t, test.wantMessage, finding.Message) + + require.ErrorIs(t, finding, fgaerrors.ErrModelNotBuildable, "the refusal itself") + require.ErrorIs(t, finding, test.wantCause, "the reason the builder gave") + + // 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) + }) + } +} + +// TestUnbuildableFindingDoesNotClaimTheWrongSentinel checks the chain is specific. A +// finding that matched every graph sentinel would satisfy the test above and tell a +// caller nothing. +func TestUnbuildableFindingDoesNotClaimTheWrongSentinel(t *testing.T) { + t.Parallel() + + findings := graphFindings(t, `model + schema 1.1 +type user +type document + relations + 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") +} + +// TestGraphValidationIsOffByDefault pins the switch. The graph path is not the source of +// truth yet, so a model that reports differently under it must report the traversal's +// answer when nothing asked for the graph. +func TestGraphValidationIsOffByDefault(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 +` + model := modelFromDSL(t, dsl) + + byDefault := findingsFrom(ValidateDSL(model, dsl, DefaultEngineOptions())).GetErrors() + explicitlyOff := findingsFrom(ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: false})).GetErrors() + nilOptions := findingsFrom(ValidateDSL(model, dsl, nil)).GetErrors() + + assert.Equal(t, describeFindings(byDefault), describeFindings(explicitlyOff)) + assert.Equal(t, describeFindings(byDefault), describeFindings(nilOptions)) + + for _, finding := range byDefault { + require.NotNil(t, finding.Metadata) + assert.NotEqual(t, GraphModelUnbuildable, finding.Metadata.ErrorType, + "the default path reported a graph finding, so the switch is not doing the switching") + } +} + +// TestGraphAndTraversalNeverBothRun checks the exclusion structurally rather than by +// counting findings, which a rule with exact parity would satisfy either way. +// +// A model whose relations are unreachable and whose build succeeds reports the same +// findings down both paths, so if the engine ran both it would report each one twice. +func TestGraphAndTraversalNeverBothRun(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define viewer: viewer + define reader: reader +` + model := modelFromDSL(t, dsl) + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, err, "the build has to succeed, or the graph path reports one finding for another reason") + require.NotNil(t, weighted) + + // Both paths find these two, so a union would be four. + require.Len(t, findingsFrom(ValidateDSL(model, dsl, DefaultEngineOptions())).GetErrors(), 2) + assert.Len(t, findingsFrom(ValidateDSL(model, dsl, &EngineOptions{UseGraphValidation: true})).GetErrors(), 2) +} + +// TestValidateWithGraphHandlesNoSourceText covers the JSON entry point, where there are +// no lines to resolve a position against. The finding is still raised, with its position +// left nil rather than resolved to line zero. +func TestValidateWithGraphHandlesNoSourceText(t *testing.T) { + t.Parallel() + + model := modelFromDSL(t, `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`) + + findings := findingsFrom(ValidateJSON(model, &EngineOptions{UseGraphValidation: true})).GetErrors() + require.Len(t, findings, 1) + require.NotNil(t, findings[0].Metadata) + + assert.Equal(t, RelationNoEntrypoint, findings[0].Metadata.ErrorType) + assert.Equal(t, "`viewer` is an impossible relation for `document` (potential loop).", findings[0].Message) + assert.Nil(t, findings[0].Line) + assert.Nil(t, findings[0].Column) +} + +// TestValidateWithGraphOnNilModel checks the guard. RunAllValidations returns before the +// phases on a nil model, so this reaches the function directly. +func TestValidateWithGraphOnNilModel(t *testing.T) { + t.Parallel() + + collector := NewErrorCollector(nil) + validateWithGraph(collector, nil, nil) + + assert.Equal(t, 0, collector.CountAll(), "a nil model has nothing to build and nothing to report") +} + +// TestGraphRuleRegistryIsWellFormed keeps the registry usable as the place rules are +// added. Two rules under one id, or an entry with no function, would make a failure +// unattributable. +func TestGraphRuleRegistryIsWellFormed(t *testing.T) { + t.Parallel() + + require.NotEmpty(t, graphRules) + + seen := make(map[string]struct{}, len(graphRules)) + + for _, rule := range graphRules { + assert.NotEmpty(t, rule.id, "a rule with no id cannot be named in failure output") + assert.NotNil(t, rule.check, "a rule with no check silently passes") + + _, duplicate := seen[rule.id] + assert.Falsef(t, duplicate, "two rules share the id %q", rule.id) + seen[rule.id] = struct{}{} + } +} + +// TestSplitRelationLabel covers the label parsing, including the forms it has to refuse. +// A malformed label reaching the raise site would report a finding against a relation +// named "". +func TestSplitRelationLabel(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + label string + wantObjectType string + wantRelation string + wantOK bool + }{ + "type and relation": {label: "document#viewer", wantObjectType: "document", wantRelation: "viewer", wantOK: true}, + "module qualified type": {label: "core.document#viewer", wantObjectType: "core.document", wantRelation: "viewer", wantOK: true}, + "type only": {label: "document"}, + "no relation": {label: "document#"}, + "no type": {label: "#viewer"}, + "empty": {label: ""}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + objectType, relation, ok := splitRelationLabel(test.label) + + assert.Equal(t, test.wantOK, ok) + assert.Equal(t, test.wantObjectType, objectType) + assert.Equal(t, test.wantRelation, relation) + }) + } +} + +// TestRelationForNodeResolvesEveryNode covers node attribution, which is what lets a rule +// report against a relation without the graph exposing one. +// +// Every node except a terminal type has to resolve, and it has to resolve to a relation +// node that is in the graph. Checking only that the label splits on a "#" is not enough: +// an operator label contains one too, so returning the node's own label unchanged would +// split cleanly and name a relation that does not exist. +func TestRelationForNodeResolvesEveryNode(t *testing.T) { + t.Parallel() + + model := modelFromDSL(t, `model + schema 1.1 +type user +type group + relations + define member: [user, group#member] +type folder + relations + define parent: [folder] + define viewer: [user] or viewer from parent +type document + relations + define parent: [folder] + define admin: [user] + define editor: [user, group#member] or admin + define viewer: (editor or viewer from parent) but not blocked + define blocked: [user] +`) + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, err) + + var terminals, relations, grouped int + + for _, nodeID := range sortedNodeIDs(weighted) { + node, ok := weighted.GetNodeByID(nodeID) + require.True(t, ok) + + definition, ok := relationForNode(weighted, node) + + if node.GetNodeType() == graph.SpecificType || node.GetNodeType() == graph.SpecificTypeWildcard { + assert.Falsef(t, ok, "terminal node %q resolved to relation %q", nodeID, definition) + + terminals++ + + continue + } + + require.Truef(t, ok, "node %q resolved to no relation", nodeID) + + // The resolved label has to name a relation the graph holds, and one whose + // declared type and relation the model can be searched for. + resolvedNode, inGraph := weighted.GetNodeByID(definition) + require.Truef(t, inGraph, "node %q resolved to %q, which is not a node in the graph", nodeID, definition) + assert.Equalf(t, graph.SpecificTypeAndRelation, resolvedNode.GetNodeType(), + "node %q resolved to %q, which is not a relation node", nodeID, definition) + + objectType, relation, split := splitRelationLabel(definition) + require.Truef(t, split, "node %q resolved to %q, which is not a relation label", nodeID, definition) + assert.NotNilf(t, relationMetaFor(model, objectType, relation), + "node %q resolved to %q, which the model does not declare", nodeID, definition) + + if node.GetNodeType() == graph.SpecificTypeAndRelation { + assert.Equalf(t, nodeID, definition, "relation node %q resolved to another relation", nodeID) + + relations++ + } else { + grouped++ + } + } + + assert.Positive(t, terminals, "no terminal nodes, so the refusal above was never exercised") + assert.Positive(t, relations, "no relation nodes, so resolving to self was never exercised") + assert.Positive(t, grouped, + "no operator or logical nodes, so reading the relation off an outgoing edge was never exercised") +} + +// relationMetaFor reports whether the model declares a relation, by locating it the way +// the rule's own position lookup does. +func relationMetaFor(model *openfgav1.AuthorizationModel, objectType, relation string) *openfgav1.Userset { + for _, typeDef := range model.GetTypeDefinitions() { + if typeDef.GetType() != objectType { + continue + } + + if userset, ok := typeDef.GetRelations()[relation]; ok { + return userset + } + } + + return nil +} + +// TestSortedNodeIDsIsStable checks the ordering the rule iterates in. The graph stores +// nodes in a map, so without this a model with two unreachable relations would report +// them in a different order per run and the corpus comparison would be flaky rather than +// wrong. +func TestSortedNodeIDsIsStable(t *testing.T) { + t.Parallel() + + model := modelFromDSL(t, `model + schema 1.1 +type user +type document + relations + define admin: [user] + define editor: [user] + define viewer: admin or editor +`) + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, err) + + first := sortedNodeIDs(weighted) + require.NotEmpty(t, first) + assert.IsIncreasing(t, first) + + for range 20 { + assert.Equal(t, first, sortedNodeIDs(weighted)) + } +} + +// TestReachesOnlyRewritesPicksTheMessageVariant 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) { + t.Parallel() + + tests := map[string]struct { + dsl string + nodeID string + wantOnly bool + }{ + "self rewrite reaches only rewrites": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`, + nodeID: "document#viewer", + wantOnly: true, + }, + "tupleset does not": { + dsl: `model + schema 1.1 +type user +type folder + relations + define parent: [folder] + define viewer: viewer from parent +`, + nodeID: "folder#viewer", + wantOnly: false, + }, + "direct assignment does not": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: [user] +`, + nodeID: "document#viewer", + wantOnly: false, + }, + // A terminal node has no outgoing edges at all, which is not the same as having + // only rewrites and must not read as a loop. + "a node with no outgoing edges does not": { + dsl: `model + schema 1.1 +type user +type document + relations + define viewer: [user] +`, + nodeID: "user", + wantOnly: false, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + model := modelFromDSL(t, test.dsl) + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, err) + + _, ok := weighted.GetNodeByID(test.nodeID) + 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)) + }) + } +} + +// TestReachesOnlyRewritesTerminatesOnACycle 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) { + t.Parallel() + + model := modelFromDSL(t, `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`) + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.NoError(t, err) + + edges, ok := weighted.GetEdgesFromNodeID("document#viewer") + require.True(t, ok) + require.NotEmpty(t, edges, "the node has no outgoing edge, so there is no cycle to terminate on") + + var closesOnItself bool + + for _, edge := range edges { + if edge.GetTo().GetUniqueLabel() == "document#viewer" { + closesOnItself = true + } + } + + 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")) +} + +// TestGraphValidationRunsBehindTheCascadeGate pins where the phase sits. A model with an +// undefined reference is reported as that, and the build refusal it would also produce is +// not piled on top. +func TestGraphValidationRunsBehindTheCascadeGate(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define viewer: editor +` + model := modelFromDSL(t, dsl) + + _, buildErr := graph.NewWeightedAuthorizationModelGraphBuilder().Build(model) + require.Error(t, buildErr, "this model has to be one the builder refuses for the test to mean anything") + + findings := graphFindings(t, dsl) + require.NotEmpty(t, findings) + + for _, finding := range findings { + require.NotNil(t, finding.Metadata) + assert.NotEqual(t, GraphModelUnbuildable, finding.Metadata.ErrorType, + "the refusal was reported on top of the reference error that explains it") + } +} + +// TestGraphValidationHonoursSkipSemanticValidation checks the switch does not smuggle the +// phase past the skip it belongs to. +func TestGraphValidationHonoursSkipSemanticValidation(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define viewer: viewer +` + + err := ValidateDSL(modelFromDSL(t, dsl), dsl, &EngineOptions{ + UseGraphValidation: true, + SkipSemanticValidation: true, + }) + + assert.NoError(t, err, "the semantic phase was skipped, so the graph rule must not have run") +} + +// TestUnbuildableFindingIsRecoverableThroughTheEntryPoints checks the finding survives the +// route a consumer actually uses, errors.As on what ValidateDSL returned. +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 +` + + err := ValidateDSL(modelFromDSL(t, dsl), dsl, &EngineOptions{UseGraphValidation: true}) + require.Error(t, err) + + var collection *ValidationErrors + require.ErrorAs(t, err, &collection) + require.Len(t, collection.GetErrors(), 1) + + require.ErrorIs(t, err, fgaerrors.ErrModelNotBuildable, "the collection carries the sentinel through") + require.ErrorIs(t, err, graph.ErrModelCycle) +} diff --git a/pkg/go/validation/validation_engine.go b/pkg/go/validation/validation_engine.go index 2743138c..8ed1f801 100644 --- a/pkg/go/validation/validation_engine.go +++ b/pkg/go/validation/validation_engine.go @@ -26,6 +26,18 @@ type EngineOptions struct { SkipWildcardValidation bool SkipMultiFileValidation bool SkipConditionValidation bool + + // UseGraphValidation resolves entrypoints and cycles from the weighted graph + // instead of by walking the rewrite tree. The two are alternatives, never layers: + // they answer the same question by different means, so running both would report + // one problem twice and disagree wherever they diverge. + // + // It is not a Skip field because the graph path is not the source of truth yet, + // which is also why it is off by default. Two things it does less well today: a + // model the graph refuses to build yields one finding rather than one per relation, + // and that finding carries no line or column. Both come from the builder returning + // on the first problem and returning no graph with it. + UseGraphValidation bool } func DefaultEngineOptions() *EngineOptions { @@ -102,7 +114,14 @@ func (ve *ValidationEngine) RunAllValidations(options *EngineOptions) *Validatio if !ve.collector.HasErrors() { if !options.SkipSemanticValidation { - validateCyclesAndEntryPoints(ve.collector, ve.semantic, ve.lines) + // One of these resolves entrypoints and cycles, never both. See + // EngineOptions.UseGraphValidation. + if options.UseGraphValidation { + validateWithGraph(ve.collector, ve.model, ve.lines) + } else { + validateCyclesAndEntryPoints(ve.collector, ve.semantic, ve.lines) + } + validateTupleToUsersetRequirements(ve.collector, ve.semantic, ve.lines) } if !options.SkipComplexOperationValidation {