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))