From 4b7ed0fefffb7c39ffda3e92dcd130dc704cab1a Mon Sep 17 00:00:00 2001 From: Anurag Bandyopadhyay Date: Fri, 18 Sep 2026 21:43:18 +0530 Subject: [PATCH] feat(pkg/go): add graph-based validation entry points ValidateDSLWithGraph and ValidateJSONWithGraph resolve relation entrypoints from the weighted authorization-model graph rather than by walking the rewrite tree. They reuse every other validation phase and the cascade gate, differing only in how entrypoint reachability is answered. Where the graph builds, both paths report identical entrypoint findings (message, code, position). A model the builder refuses yields a single graph-model-unbuildable finding, a Go-side code with no pkg/js or pkg/java counterpart. --- pkg/go/validation/findings.go | 5 + pkg/go/validation/graph_validation.go | 211 ++++++ .../graph_validation_corpus_test.go | 255 +++++++ pkg/go/validation/graph_validation_test.go | 681 ++++++++++++++++++ pkg/go/validation/messages.go | 12 + pkg/go/validation/validate.go | 42 +- 6 files changed, 1203 insertions(+), 3 deletions(-) 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/pkg/go/validation/findings.go b/pkg/go/validation/findings.go index 8515d51a..509a8a1a 100644 --- a/pkg/go/validation/findings.go +++ b/pkg/go/validation/findings.go @@ -45,6 +45,11 @@ const ( InvalidSchemaVersion Kind = "invalid-schema-version" ) +// GraphModelUnbuildable is reported when the weighted-graph builder refuses a +// model. Unlike the codes above it has no pkg/js or pkg/java counterpart: the +// weighted graph is a Go-side construct, so this code is not corpus-pinned. +const GraphModelUnbuildable Kind = "graph-model-unbuildable" + // Range is a start and end position in the source text, used for both the line // and the column a finding is at. // diff --git a/pkg/go/validation/graph_validation.go b/pkg/go/validation/graph_validation.go new file mode 100644 index 00000000..7365995b --- /dev/null +++ b/pkg/go/validation/graph_validation.go @@ -0,0 +1,211 @@ +package validation + +import ( + "slices" + "strings" + + "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 finding naming none of them. Every rule therefore runs +// only on a model that built, and a refused model produces the single finding +// modelUnbuildable builds. +func validateWithGraph(idx *index, src source) error { + if idx == nil || idx.model == nil { + return nil + } + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(idx.model) + if err != nil { + return joinFindings(modelUnbuildable(err)) + } + + scope := &graphScope{weighted: weighted, idx: idx, src: src} + + var fs []*Finding + for _, rule := range graphRules { + fs = append(fs, rule.check(scope)...) + } + + return joinFindings(fs...) +} + +// 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 Kind the rule's findings carry. + id string + check func(*graphScope) []*Finding +} + +// 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 indexed model behind it, +// and the source text a finding's position is stamped from. +type graphScope struct { + weighted *graph.WeightedAuthorizationModelGraph + idx *index + src source +} + +// 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) []*Finding { + var fs []*Finding + + 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 + } + + finding := noEntryPoint(relation, objectType) + if scope.reachesOnlyRewrites(nodeID) { + finding = noEntryPointLoop(relation, objectType) + } + + // Stamp position and provenance through the same helpers the rewrite-tree + // phases use, so a graph finding and a traversal finding for the same + // relation land in the same place. + file, module := relationMeta(scope.idx.typeDef(objectType), relation) + line := scope.src.relationLine(relation, scope.src.typeLine(objectType)) + + finding.at(scope.src, line) + finding.File, finding.Metadata.Module = file, module + fs = append(fs, finding) + } + + return fs +} + +// 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 +} + +// 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 reports 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..ed034236 --- /dev/null +++ b/pkg/go/validation/graph_validation_corpus_test.go @@ -0,0 +1,255 @@ +package validation + +import ( + "fmt" + "slices" + "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 := len(findingsOf(ValidateDSL(entry.Model, entry.Case.DSL))) > 0 + + _, 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 the graph +// entry-point phase, 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 entry-point +// phase 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, + findingsOf(ValidateDSLWithGraph(entry.Model, entry.Case.DSL))) + + if result.Status != corpusPass { + diverged = append(diverged, entry.Case.Name) + } + } + + slices.Sort(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 := findingsOf(ValidateDSLWithGraph(entry.Model, entry.Case.DSL)) + + require.Lenf(t, findings, 1, "case %q reports more than the unbuildable finding", entry.Case.Name) + assert.Equalf(t, GraphModelUnbuildable, findings[0].Metadata.Kind, + "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, validateEntryPoints) + fromGraph := entrypointFindings(entry.Model, entry.Case.DSL, validateWithGraph) + + 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, entryPoints entryPointPhase) []string { + findings := findingsOf(validateWith(model, newSource(dsl), entryPoints)) + + described := []string{} + + for _, finding := range findings { + if finding.Metadata.Kind != RelationNoEntrypoint { + continue + } + + described = append(described, fmt.Sprintf("%s %s %q", + finding.Metadata.Symbol, + describePosition(finding.Line, finding.Column), + finding.Message)) + } + + slices.Sort(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..04716805 --- /dev/null +++ b/pkg/go/validation/graph_validation_test.go @@ -0,0 +1,681 @@ +package validation + +import ( + "fmt" + "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" +) + +// graphFindings runs the public graph entry point over the model parsed from +// dsl and recovers the findings. +func graphFindings(t *testing.T, dsl string) []*Finding { + t.Helper() + + return findingsOf(ValidateDSLWithGraph(mustParse(t, dsl), dsl)) +} + +// describeFindings renders findings as code, symbol, position and message, which +// is everything a caller can see. +func describeFindings(findings []*Finding) []string { + described := []string{} + + for _, finding := range findings { + described = append(described, fmt.Sprintf("[%s] %s%s %q", + finding.Metadata.Kind, finding.Metadata.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))) + }) + } +} + +// TestGraphEntrypointFindingsMatchTheTraversal checks that the graph path reports +// the same finding as the tree walk for the same model, field for field. +// +// Without this the rule could satisfy the corpus on message text alone while +// handing callers a finding whose code, symbol or position the traversal set and +// it left different. +func TestGraphEntrypointFindingsMatchTheTraversal(t *testing.T) { + t.Parallel() + + dsl := `model + schema 1.1 +type user +type document + relations + define viewer: viewer +` + + fromGraph := graphFindings(t, dsl) + require.Len(t, fromGraph, 1) + + finding := fromGraph[0] + assert.Equal(t, RelationNoEntrypoint, finding.Metadata.Kind) + + // The same model down the traversal path, field for field. Anything the graph + // path leaves different is a contract a caller loses by switching. + fromTraversal := findingsOf(ValidateDSL(mustParse(t, dsl), dsl)) + require.Len(t, fromTraversal, 1) + + assert.Equal(t, describeFindings(fromTraversal), describeFindings(fromGraph)) + assert.Equal(t, fromTraversal[0].Metadata, finding.Metadata) + assert.Equal(t, fromTraversal[0].Line, finding.Line) + assert.Equal(t, fromTraversal[0].Column, finding.Column) +} + +// TestModelUnbuildableFinding covers the one code this path adds. A model the +// builder refuses yields a single positionless finding, since Build returns no +// graph to enumerate relations from. +func TestModelUnbuildableFinding(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + dsl string + wantMessage string + }{ + "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", + }, + "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", + }, + } + + 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] + assert.Equal(t, GraphModelUnbuildable, finding.Metadata.Kind) + assert.Equal(t, test.wantMessage, finding.Message) + + // 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) + }) + } +} + +// TestGraphValidationIsNotWiredIntoTheDefaultPipeline pins the switch. The graph +// path is not the source of truth yet, so the default entry points must report +// the traversal's answer and never the graph-only code. +func TestGraphValidationIsNotWiredIntoTheDefaultPipeline(t *testing.T) { + t.Parallel() + + // A model the graph refuses but the traversal reports on relation by relation: + // the two paths give different findings, so a default that leaked the graph + // answer would show here. + dsl := `model + schema 1.1 +type user +type document + relations + define admin: [user] + define viewer: admin and editor + define editor: viewer +` + + byDefault := findingsOf(ValidateDSL(mustParse(t, dsl), dsl)) + require.NotEmpty(t, byDefault) + + for _, finding := range byDefault { + assert.NotEqual(t, GraphModelUnbuildable, finding.Metadata.Kind, + "the default path reported a graph finding, so it is wired to the graph phase") + } +} + +// TestValidateJSONWithGraphHandlesNoSourceText 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 TestValidateJSONWithGraphHandlesNoSourceText(t *testing.T) { + t.Parallel() + + model := mustParse(t, `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`) + + findings := findingsOf(ValidateJSONWithGraph(model)) + require.Len(t, findings, 1) + + assert.Equal(t, RelationNoEntrypoint, findings[0].Metadata.Kind) + 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, reached directly since the +// pipeline returns before its phases on a nil model. +func TestValidateWithGraphOnNilModel(t *testing.T) { + t.Parallel() + + require.NoError(t, validateWithGraph(nil, source{})) + require.NoError(t, validateWithGraph(newIndex(nil), source{})) +} + +// 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 := mustParse(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 := mustParse(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() + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(mustParse(t, test.dsl)) + 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} + 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. +func TestReachesOnlyRewritesTerminatesOnACycle(t *testing.T) { + t.Parallel() + + weighted, err := graph.NewWeightedAuthorizationModelGraphBuilder().Build(mustParse(t, `model + schema 1.1 +type user +type document + relations + define viewer: viewer +`)) + 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} + 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 +` + + findings := graphFindings(t, dsl) + require.NotEmpty(t, findings) + + for _, finding := range findings { + assert.NotEqual(t, GraphModelUnbuildable, finding.Metadata.Kind, + "the refusal was reported on top of the reference error that explains it") + } +} diff --git a/pkg/go/validation/messages.go b/pkg/go/validation/messages.go index 69345161..473ae45f 100644 --- a/pkg/go/validation/messages.go +++ b/pkg/go/validation/messages.go @@ -110,6 +110,18 @@ func noEntryPoint(relation, typeName string) *Finding { } } +// modelUnbuildable reports a model the weighted-graph builder refuses. A +// whole-model failure has no line, so the finding carries no position, and the +// builder's own error is folded into the message. It is the single finding the +// graph path emits for a refused model, standing in for the per-relation +// findings the rewrite-tree traversal would report. +func modelUnbuildable(cause error) *Finding { + return &Finding{ + Message: fmt.Sprintf("the model cannot be built into a weighted graph: %s", cause), + Metadata: Metadata{Kind: GraphModelUnbuildable}, + } +} + // invalidRelationOnTupleset reports a tuple-to-userset whose computed relation // does not exist on the type the tupleset relation is assignable to. The // tupleset relation is parent; the finding is about relationName on typeDef. diff --git a/pkg/go/validation/validate.go b/pkg/go/validation/validate.go index c8bc5d48..dabd8dab 100644 --- a/pkg/go/validation/validate.go +++ b/pkg/go/validation/validate.go @@ -28,7 +28,43 @@ func ValidateJSON(model *openfgav1.AuthorizationModel) error { return validate(model, source{}) } -// validate runs the validation phases in the reference implementation's order. +// ValidateDSLWithGraph runs the same validations as ValidateDSL but resolves +// entry-point reachability from the weighted authorization-model graph rather +// than by walking the rewrite tree. Everywhere the graph builds, it reports the +// same relation-no-entry-point findings as ValidateDSL: same message, code and +// position. A model the graph builder refuses yields a single positionless +// finding coded GraphModelUnbuildable, which stands in for the per-relation +// findings the tree walk would report and has no pkg/js or pkg/java counterpart. +// +// ValidateDSL remains the default; this path is opt-in until the graph is the +// source of truth for reachability. +func ValidateDSLWithGraph(model *openfgav1.AuthorizationModel, dsl string) error { + return validateWith(model, newSource(dsl), validateWithGraph) +} + +// ValidateJSONWithGraph is ValidateDSLWithGraph for a model that reached the +// caller as JSON, so with no DSL source text behind it. Findings carry a nil +// Line and Column; the messages and metadata match what ValidateDSLWithGraph +// reports for the same model. The name matches ValidateJSON. +func ValidateJSONWithGraph(model *openfgav1.AuthorizationModel) error { + return validateWith(model, source{}, validateWithGraph) +} + +// validate runs every validation phase over model, resolving entry points by +// walking the rewrite tree. +func validate(model *openfgav1.AuthorizationModel, src source) error { + return validateWith(model, src, validateEntryPoints) +} + +// entryPointPhase reports relations that can never be satisfied. The default +// validateEntryPoints walks the rewrite tree; validateWithGraph reads the same +// question off the weighted graph. Passing it to validateWith as a parameter +// lets the two share every other phase, the phase order and the cascade gate, +// and differ only in how they answer that one question. +type entryPointPhase func(idx *index, src source) error + +// validateWith runs the validation phases in the reference implementation's +// order, resolving entry points through the given phase. // // Schema, name and reference validation always run. The later structural // phases are gated on nothing having been found yet: a model with bad @@ -37,7 +73,7 @@ func ValidateJSON(model *openfgav1.AuthorizationModel) error { // reference's modelValidation, which skips the later passes once any error has // been recorded. Multi-file and condition checks are independent of the cascade // and always run, matching the reference's handling of conditions. -func validate(model *openfgav1.AuthorizationModel, src source) error { +func validateWith(model *openfgav1.AuthorizationModel, src source, entryPoints entryPointPhase) error { if model == nil { return nil } @@ -60,7 +96,7 @@ func validate(model *openfgav1.AuthorizationModel, src source) error { } if len(errs) == 0 { - add(validateEntryPoints(idx, src)) + add(entryPoints(idx, src)) add(validateTupleToUsersets(idx, src)) add(validateComplexOperations(idx, src)) add(validateWildcards(idx, src))