From a51362f17502c7bbd9c2c58148be35812486a804 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 31 Jul 2024 15:07:45 -0400 Subject: [PATCH] Handle functioned arrows in warnings system --- pkg/development/warningdefs.go | 64 ++++++++++++++++----------- pkg/development/warnings.go | 76 +++++++++++++++++++++++++++++++- pkg/development/warnings_test.go | 21 +++++++++ 3 files changed, 133 insertions(+), 28 deletions(-) diff --git a/pkg/development/warningdefs.go b/pkg/development/warningdefs.go index 544456fcba..cf5c9dae00 100644 --- a/pkg/development/warningdefs.go +++ b/pkg/development/warningdefs.go @@ -51,7 +51,7 @@ var lintPermissionReferencingItself = computedUsersetCheck{ ) (*devinterface.DeveloperWarning, error) { parentRelation := ctx.Value(relationKey).(*corev1.Relation) permName := parentRelation.Name - if computedUserset.Relation == permName { + if computedUserset.GetRelation() == permName { return warningForPosition( "permission-references-itself", fmt.Sprintf("Permission %q references itself, which will cause an error to be raised due to infinite recursion", permName), @@ -68,13 +68,13 @@ var lintArrowReferencingUnreachable = ttuCheck{ "arrow-references-unreachable-relation", func( ctx context.Context, - ttu *corev1.TupleToUserset, + ttu ttu, sourcePosition *corev1.SourcePosition, ts *typesystem.TypeSystem, ) (*devinterface.DeveloperWarning, error) { parentRelation := ctx.Value(relationKey).(*corev1.Relation) - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) + referencedRelation, ok := ts.GetRelation(ttu.GetTupleset().GetRelation()) if !ok { return nil, nil } @@ -91,24 +91,28 @@ var lintArrowReferencingUnreachable = ttuCheck{ return nil, err } - _, ok := nts.GetRelation(ttu.ComputedUserset.Relation) + _, ok := nts.GetRelation(ttu.GetComputedUserset().GetRelation()) if ok { wasFound = true } } if !wasFound { + arrowString, err := ttu.GetArrowString() + if err != nil { + return nil, err + } + return warningForPosition( "arrow-references-unreachable-relation", fmt.Sprintf( - "Arrow `%s->%s` under permission %q references relation/permission %q that does not exist on any subject types of relation %q", - ttu.Tupleset.Relation, - ttu.ComputedUserset.Relation, + "Arrow `%s` under permission %q references relation/permission %q that does not exist on any subject types of relation %q", + arrowString, parentRelation.Name, - ttu.ComputedUserset.Relation, - ttu.Tupleset.Relation, + ttu.GetComputedUserset().GetRelation(), + ttu.GetTupleset().GetRelation(), ), - fmt.Sprintf("%s->%s", ttu.Tupleset.Relation, ttu.ComputedUserset.Relation), + arrowString, sourcePosition, ), nil } @@ -121,13 +125,13 @@ var lintArrowOverSubRelation = ttuCheck{ "arrow-walks-subject-relation", func( ctx context.Context, - ttu *corev1.TupleToUserset, + ttu ttu, sourcePosition *corev1.SourcePosition, ts *typesystem.TypeSystem, ) (*devinterface.DeveloperWarning, error) { parentRelation := ctx.Value(relationKey).(*corev1.Relation) - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) + referencedRelation, ok := ts.GetRelation(ttu.GetTupleset().GetRelation()) if !ok { return nil, nil } @@ -137,20 +141,24 @@ var lintArrowOverSubRelation = ttuCheck{ return nil, err } + arrowString, err := ttu.GetArrowString() + if err != nil { + return nil, err + } + for _, subjectType := range allowedSubjectTypes { - if subjectType.Relation != tuple.Ellipsis { + if subjectType.GetRelation() != tuple.Ellipsis { return warningForPosition( "arrow-walks-subject-relation", fmt.Sprintf( - "Arrow `%s->%s` under permission %q references relation %q that has relation %q on subject %q: *the subject relation will be ignored for the arrow*", - ttu.Tupleset.Relation, - ttu.ComputedUserset.Relation, + "Arrow `%s` under permission %q references relation %q that has relation %q on subject %q: *the subject relation will be ignored for the arrow*", + arrowString, parentRelation.Name, - ttu.Tupleset.Relation, - subjectType.Relation, + ttu.GetTupleset().GetRelation(), + subjectType.GetRelation(), subjectType.Namespace, ), - fmt.Sprintf("%s->%s", ttu.Tupleset.Relation, ttu.ComputedUserset.Relation), + arrowString, sourcePosition, ), nil } @@ -164,13 +172,13 @@ var lintArrowReferencingRelation = ttuCheck{ "arrow-references-relation", func( ctx context.Context, - ttu *corev1.TupleToUserset, + ttu ttu, sourcePosition *corev1.SourcePosition, ts *typesystem.TypeSystem, ) (*devinterface.DeveloperWarning, error) { parentRelation := ctx.Value(relationKey).(*corev1.Relation) - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) + referencedRelation, ok := ts.GetRelation(ttu.GetTupleset().GetRelation()) if !ok { return nil, nil } @@ -182,13 +190,18 @@ var lintArrowReferencingRelation = ttuCheck{ return nil, err } + arrowString, err := ttu.GetArrowString() + if err != nil { + return nil, err + } + for _, subjectType := range allowedSubjectTypes { nts, err := ts.TypeSystemForNamespace(ctx, subjectType.Namespace) if err != nil { return nil, err } - targetRelation, ok := nts.GetRelation(ttu.ComputedUserset.Relation) + targetRelation, ok := nts.GetRelation(ttu.GetComputedUserset().GetRelation()) if !ok { continue } @@ -197,14 +210,13 @@ var lintArrowReferencingRelation = ttuCheck{ return warningForPosition( "arrow-references-relation", fmt.Sprintf( - "Arrow `%s->%s` under permission %q references relation %q on definition %q; it is recommended to point to a permission", - ttu.Tupleset.Relation, - ttu.ComputedUserset.Relation, + "Arrow `%s` under permission %q references relation %q on definition %q; it is recommended to point to a permission", + arrowString, parentRelation.Name, targetRelation.Name, subjectType.Namespace, ), - fmt.Sprintf("%s->%s", ttu.Tupleset.Relation, ttu.ComputedUserset.Relation), + arrowString, sourcePosition, ), nil } diff --git a/pkg/development/warnings.go b/pkg/development/warnings.go index fff0b2ecad..4fe5f606f7 100644 --- a/pkg/development/warnings.go +++ b/pkg/development/warnings.go @@ -2,6 +2,7 @@ package development import ( "context" + "fmt" "github.com/authzed/spicedb/pkg/namespace" corev1 "github.com/authzed/spicedb/pkg/proto/core/v1" @@ -120,10 +121,20 @@ func shouldSkipCheck(metadata *corev1.Metadata, name string) bool { return false } +type tupleset interface { + GetRelation() string +} + +type ttu interface { + GetTupleset() tupleset + GetComputedUserset() *corev1.ComputedUserset + GetArrowString() (string, error) +} + type ( relationChecker func(ctx context.Context, relation *corev1.Relation, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error) computedUsersetChecker func(ctx context.Context, computedUserset *corev1.ComputedUserset, sourcePosition *corev1.SourcePosition, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error) - ttuChecker func(ctx context.Context, ttu *corev1.TupleToUserset, sourcePosition *corev1.SourcePosition, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error) + ttuChecker func(ctx context.Context, ttu ttu, sourcePosition *corev1.SourcePosition, vts *typesystem.TypeSystem) (*devinterface.DeveloperWarning, error) ) type relationCheck struct { @@ -198,13 +209,29 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child warnings = append(warnings, found...) + case *corev1.SetOperation_Child_FunctionedTupleToUserset: + for _, check := range checks.ttuChecks { + if shouldSkipCheck(relation.Metadata, check.name) { + continue + } + + checkerWarning, err := check.fn(ctx, wrappedFunctionedTTU{t.FunctionedTupleToUserset}, op.SourcePosition, ts) + if err != nil { + return nil, err + } + + if checkerWarning != nil { + warnings = append(warnings, checkerWarning) + } + } + case *corev1.SetOperation_Child_TupleToUserset: for _, check := range checks.ttuChecks { if shouldSkipCheck(relation.Metadata, check.name) { continue } - checkerWarning, err := check.fn(ctx, t.TupleToUserset, op.SourcePosition, ts) + checkerWarning, err := check.fn(ctx, wrappedTTU{t.TupleToUserset}, op.SourcePosition, ts) if err != nil { return nil, err } @@ -224,3 +251,48 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child return warnings, nil } + +type wrappedFunctionedTTU struct { + *corev1.FunctionedTupleToUserset +} + +func (wfttu wrappedFunctionedTTU) GetTupleset() tupleset { + return wfttu.FunctionedTupleToUserset.GetTupleset() +} + +func (wfttu wrappedFunctionedTTU) GetComputedUserset() *corev1.ComputedUserset { + return wfttu.FunctionedTupleToUserset.GetComputedUserset() +} + +func (wfttu wrappedFunctionedTTU) GetArrowString() (string, error) { + var functionName string + switch wfttu.Function { + case corev1.FunctionedTupleToUserset_FUNCTION_ANY: + functionName = "any" + + case corev1.FunctionedTupleToUserset_FUNCTION_ALL: + functionName = "all" + + default: + return "", spiceerrors.MustBugf("unknown function type %T", wfttu.Function) + } + + return fmt.Sprintf("%s.%s(%s)", wfttu.GetTupleset().GetRelation(), functionName, wfttu.GetComputedUserset().GetRelation()), nil +} + +type wrappedTTU struct { + *corev1.TupleToUserset +} + +func (wtu wrappedTTU) GetTupleset() tupleset { + return wtu.TupleToUserset.GetTupleset() +} + +func (wtu wrappedTTU) GetComputedUserset() *corev1.ComputedUserset { + return wtu.TupleToUserset.GetComputedUserset() +} + +func (wtu wrappedTTU) GetArrowString() (string, error) { + arrowString := fmt.Sprintf("%s->%s", wtu.GetTupleset().GetRelation(), wtu.GetComputedUserset().GetRelation()) + return arrowString, nil +} diff --git a/pkg/development/warnings_test.go b/pkg/development/warnings_test.go index 915e013b43..7385d6ba5e 100644 --- a/pkg/development/warnings_test.go +++ b/pkg/development/warnings_test.go @@ -116,6 +116,27 @@ func TestWarnings(t *testing.T) { SourceCode: "parent_group->member", }, }, + { + name: "all arrow referencing subject relation", + schema: `definition group { + relation direct_member: user + permission member = direct_member + } + + definition user {} + + definition document { + relation parent_group: group#member + permission view = parent_group.all(member) + } + `, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Arrow `parent_group.all(member)` under permission \"view\" references relation \"parent_group\" that has relation \"member\" on subject \"group\": *the subject relation will be ignored for the arrow* (arrow-walks-subject-relation)", + Line: 10, + Column: 23, + SourceCode: "parent_group.all(member)", + }, + }, { name: "relation referencing its parent definition in its name", schema: `definition user {}