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 {