diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 558d31b3f9..db63c7de95 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -100,7 +100,7 @@ func TestDocumentWarningDiagnostics(t *testing.T) { require.Equal(t, "full", resp.Kind) require.Len(t, resp.Items, 1) require.Equal(t, lsp.DiagnosticSeverity(lsp.Warning), resp.Items[0].Severity) - require.Equal(t, `Permission "view_resource" references parent type "resource" in its name; it is recommended to drop the suffix`, resp.Items[0].Message) + require.Equal(t, `Permission "view_resource" references parent type "resource" in its name; it is recommended to drop the suffix (relation-name-references-parent)`, resp.Items[0].Message) require.Equal(t, lsp.Range{ Start: lsp.Position{Line: 5, Character: 3}, End: lsp.Position{Line: 5, Character: 3}, diff --git a/pkg/development/warningdefs.go b/pkg/development/warningdefs.go index d1b56673b0..17307e3ade 100644 --- a/pkg/development/warningdefs.go +++ b/pkg/development/warningdefs.go @@ -11,178 +11,199 @@ import ( "github.com/authzed/spicedb/pkg/typesystem" ) -var lintRelationReferencesParentType = func( - ctx context.Context, - relation *corev1.Relation, - ts *typesystem.TypeSystem, -) (*devinterface.DeveloperWarning, error) { - parentDef := ts.Namespace() - if strings.HasSuffix(relation.Name, parentDef.Name) { - if ts.IsPermission(relation.Name) { +var lintRelationReferencesParentType = relationCheck{ + "relation-name-references-parent", + func( + ctx context.Context, + relation *corev1.Relation, + ts *typesystem.TypeSystem, + ) (*devinterface.DeveloperWarning, error) { + parentDef := ts.Namespace() + if strings.HasSuffix(relation.Name, parentDef.Name) { + if ts.IsPermission(relation.Name) { + return warningForMetadata( + "relation-name-references-parent", + fmt.Sprintf("Permission %q references parent type %q in its name; it is recommended to drop the suffix", relation.Name, parentDef.Name), + relation, + ), nil + } + return warningForMetadata( - fmt.Sprintf("Permission %q references parent type %q in its name; it is recommended to drop the suffix", relation.Name, parentDef.Name), + "relation-name-references-parent", + fmt.Sprintf("Relation %q references parent type %q in its name; it is recommended to drop the suffix", relation.Name, parentDef.Name), relation, ), nil } - return warningForMetadata( - fmt.Sprintf("Relation %q references parent type %q in its name; it is recommended to drop the suffix", relation.Name, parentDef.Name), - relation, - ), nil - } - - return nil, nil -} - -var lintPermissionReferencingItself = func( - ctx context.Context, - computedUserset *corev1.ComputedUserset, - sourcePosition *corev1.SourcePosition, - ts *typesystem.TypeSystem, -) (*devinterface.DeveloperWarning, error) { - parentRelation := ctx.Value(relationKey).(*corev1.Relation) - permName := parentRelation.Name - if computedUserset.Relation == permName { - return warningForPosition( - fmt.Sprintf("Permission %q references itself, which will cause an error to be raised due to infinite recursion", permName), - sourcePosition, - ), nil - } - - return nil, nil + return nil, nil + }, } -var lintArrowReferencingUnreachable = func( - ctx context.Context, - ttu *corev1.TupleToUserset, - sourcePosition *corev1.SourcePosition, - ts *typesystem.TypeSystem, -) (*devinterface.DeveloperWarning, error) { - parentRelation := ctx.Value(relationKey).(*corev1.Relation) +var lintPermissionReferencingItself = computedUsersetCheck{ + "permission-references-itself", + func( + ctx context.Context, + computedUserset *corev1.ComputedUserset, + sourcePosition *corev1.SourcePosition, + ts *typesystem.TypeSystem, + ) (*devinterface.DeveloperWarning, error) { + parentRelation := ctx.Value(relationKey).(*corev1.Relation) + permName := parentRelation.Name + if computedUserset.Relation == 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), + sourcePosition, + ), nil + } - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) - if !ok { return nil, nil - } + }, +} - allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) - if err != nil { - return nil, err - } +var lintArrowReferencingUnreachable = ttuCheck{ + "arrow-references-unreachable-relation", + func( + ctx context.Context, + ttu *corev1.TupleToUserset, + sourcePosition *corev1.SourcePosition, + ts *typesystem.TypeSystem, + ) (*devinterface.DeveloperWarning, error) { + parentRelation := ctx.Value(relationKey).(*corev1.Relation) + + referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) + if !ok { + return nil, nil + } - wasFound := false - for _, subjectType := range allowedSubjectTypes { - nts, err := ts.TypeSystemForNamespace(ctx, subjectType.Namespace) + allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) if err != nil { return nil, err } - _, ok := nts.GetRelation(ttu.ComputedUserset.Relation) - if ok { - wasFound = true + wasFound := false + for _, subjectType := range allowedSubjectTypes { + nts, err := ts.TypeSystemForNamespace(ctx, subjectType.Namespace) + if err != nil { + return nil, err + } + + _, ok := nts.GetRelation(ttu.ComputedUserset.Relation) + if ok { + wasFound = true + } } - } - - if !wasFound { - return warningForPosition( - 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, - parentRelation.Name, - ttu.ComputedUserset.Relation, - ttu.Tupleset.Relation, - ), - sourcePosition, - ), nil - } - - return nil, nil -} - -var lintArrowOverSubRelation = func( - ctx context.Context, - ttu *corev1.TupleToUserset, - sourcePosition *corev1.SourcePosition, - ts *typesystem.TypeSystem, -) (*devinterface.DeveloperWarning, error) { - parentRelation := ctx.Value(relationKey).(*corev1.Relation) - - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) - if !ok { - return nil, nil - } - allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) - if err != nil { - return nil, err - } - - for _, subjectType := range allowedSubjectTypes { - if subjectType.Relation != tuple.Ellipsis { + if !wasFound { return warningForPosition( + "arrow-references-unreachable-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*", + "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, parentRelation.Name, + ttu.ComputedUserset.Relation, ttu.Tupleset.Relation, - subjectType.Relation, - subjectType.Namespace, ), sourcePosition, ), nil } - } - - return nil, nil -} - -var lintArrowReferencingRelation = func( - ctx context.Context, - ttu *corev1.TupleToUserset, - sourcePosition *corev1.SourcePosition, - ts *typesystem.TypeSystem, -) (*devinterface.DeveloperWarning, error) { - parentRelation := ctx.Value(relationKey).(*corev1.Relation) - referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) - if !ok { return nil, nil - } + }, +} - // For each subject type of the referenced relation, check if the referenced permission - // is, in fact, a relation. - allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) - if err != nil { - return nil, err - } +var lintArrowOverSubRelation = ttuCheck{ + "arrow-walks-subject-relation", + func( + ctx context.Context, + ttu *corev1.TupleToUserset, + sourcePosition *corev1.SourcePosition, + ts *typesystem.TypeSystem, + ) (*devinterface.DeveloperWarning, error) { + parentRelation := ctx.Value(relationKey).(*corev1.Relation) + + referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) + if !ok { + return nil, nil + } - for _, subjectType := range allowedSubjectTypes { - nts, err := ts.TypeSystemForNamespace(ctx, subjectType.Namespace) + allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) if err != nil { return nil, err } - targetRelation, ok := nts.GetRelation(ttu.ComputedUserset.Relation) + for _, subjectType := range allowedSubjectTypes { + if subjectType.Relation != 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, + parentRelation.Name, + ttu.Tupleset.Relation, + subjectType.Relation, + subjectType.Namespace, + ), + sourcePosition, + ), nil + } + } + + return nil, nil + }, +} + +var lintArrowReferencingRelation = ttuCheck{ + "arrow-references-relation", + func( + ctx context.Context, + ttu *corev1.TupleToUserset, + sourcePosition *corev1.SourcePosition, + ts *typesystem.TypeSystem, + ) (*devinterface.DeveloperWarning, error) { + parentRelation := ctx.Value(relationKey).(*corev1.Relation) + + referencedRelation, ok := ts.GetRelation(ttu.Tupleset.Relation) if !ok { - continue + return nil, nil } - if !nts.IsPermission(targetRelation.Name) { - return warningForPosition( - 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, - parentRelation.Name, - targetRelation.Name, - subjectType.Namespace, - ), - sourcePosition, - ), nil + // For each subject type of the referenced relation, check if the referenced permission + // is, in fact, a relation. + allowedSubjectTypes, err := ts.AllowedSubjectRelations(referencedRelation.Name) + if err != nil { + return nil, err } - } - return nil, nil + for _, subjectType := range allowedSubjectTypes { + nts, err := ts.TypeSystemForNamespace(ctx, subjectType.Namespace) + if err != nil { + return nil, err + } + + targetRelation, ok := nts.GetRelation(ttu.ComputedUserset.Relation) + if !ok { + continue + } + + if !nts.IsPermission(targetRelation.Name) { + 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, + parentRelation.Name, + targetRelation.Name, + subjectType.Namespace, + ), + sourcePosition, + ), nil + } + } + + return nil, nil + }, } diff --git a/pkg/development/warnings.go b/pkg/development/warnings.go index 8dc30c8f46..4c40657640 100644 --- a/pkg/development/warnings.go +++ b/pkg/development/warnings.go @@ -10,25 +10,25 @@ import ( "github.com/authzed/spicedb/pkg/typesystem" ) -var allChecks = checkers{ - relationCheckers: []relationChecker{ +var allChecks = checks{ + relationChecks: []relationCheck{ lintRelationReferencesParentType, }, - computedUsersetCheckers: []computedUsersetChecker{ + computedUsersetChecks: []computedUsersetCheck{ lintPermissionReferencingItself, }, - ttuCheckers: []ttuChecker{ + ttuChecks: []ttuCheck{ lintArrowReferencingRelation, lintArrowReferencingUnreachable, lintArrowOverSubRelation, }, } -func warningForMetadata(message string, metadata namespace.WithSourcePosition) *devinterface.DeveloperWarning { - return warningForPosition(message, metadata.GetSourcePosition()) +func warningForMetadata(warningName string, message string, metadata namespace.WithSourcePosition) *devinterface.DeveloperWarning { + return warningForPosition(warningName, message, metadata.GetSourcePosition()) } -func warningForPosition(message string, sourcePosition *corev1.SourcePosition) *devinterface.DeveloperWarning { +func warningForPosition(warningName string, message string, sourcePosition *corev1.SourcePosition) *devinterface.DeveloperWarning { if sourcePosition == nil { return &devinterface.DeveloperWarning{ Message: message, @@ -39,7 +39,7 @@ func warningForPosition(message string, sourcePosition *corev1.SourcePosition) * columnNumber := sourcePosition.ZeroIndexedColumnPosition + 1 return &devinterface.DeveloperWarning{ - Message: message, + Message: message + " (" + warningName + ")", Line: uint32(lineNumber), Column: uint32(columnNumber), } @@ -74,8 +74,13 @@ func addDefinitionWarnings(ctx context.Context, def *corev1.NamespaceDefinition, warnings := []*devinterface.DeveloperWarning{} for _, rel := range def.Relation { ctx = context.WithValue(ctx, relationKey, rel) - for _, checker := range allChecks.relationCheckers { - checkerWarning, err := checker(ctx, rel, ts) + + for _, check := range allChecks.relationChecks { + if shouldSkipCheck(rel.Metadata, check.name) { + continue + } + + checkerWarning, err := check.fn(ctx, rel, ts) if err != nil { return nil, err } @@ -86,7 +91,7 @@ func addDefinitionWarnings(ctx context.Context, def *corev1.NamespaceDefinition, } if ts.IsPermission(rel.Name) { - found, err := walkUsersetRewrite(ctx, rel.UsersetRewrite, allChecks, ts) + found, err := walkUsersetRewrite(ctx, rel.UsersetRewrite, rel, allChecks, ts) if err != nil { return nil, err } @@ -98,39 +103,69 @@ func addDefinitionWarnings(ctx context.Context, def *corev1.NamespaceDefinition, return warnings, nil } +func shouldSkipCheck(metadata *corev1.Metadata, name string) bool { + if metadata == nil { + return false + } + + comments := namespace.GetComments(metadata) + for _, comment := range comments { + if comment == "// spicedb-ignore-warning: "+name { + return true + } + } + + return false +} + 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) ) -type checkers struct { - relationCheckers []relationChecker - computedUsersetCheckers []computedUsersetChecker - ttuCheckers []ttuChecker +type relationCheck struct { + name string + fn relationChecker } -func walkUsersetRewrite(ctx context.Context, rewrite *corev1.UsersetRewrite, checkers checkers, ts *typesystem.TypeSystem) ([]*devinterface.DeveloperWarning, error) { +type computedUsersetCheck struct { + name string + fn computedUsersetChecker +} + +type ttuCheck struct { + name string + fn ttuChecker +} + +type checks struct { + relationChecks []relationCheck + computedUsersetChecks []computedUsersetCheck + ttuChecks []ttuCheck +} + +func walkUsersetRewrite(ctx context.Context, rewrite *corev1.UsersetRewrite, relation *corev1.Relation, checks checks, ts *typesystem.TypeSystem) ([]*devinterface.DeveloperWarning, error) { if rewrite == nil { return nil, nil } switch t := (rewrite.RewriteOperation).(type) { case *corev1.UsersetRewrite_Union: - return walkUsersetOperations(ctx, t.Union.Child, checkers, ts) + return walkUsersetOperations(ctx, t.Union.Child, relation, checks, ts) case *corev1.UsersetRewrite_Intersection: - return walkUsersetOperations(ctx, t.Intersection.Child, checkers, ts) + return walkUsersetOperations(ctx, t.Intersection.Child, relation, checks, ts) case *corev1.UsersetRewrite_Exclusion: - return walkUsersetOperations(ctx, t.Exclusion.Child, checkers, ts) + return walkUsersetOperations(ctx, t.Exclusion.Child, relation, checks, ts) default: return nil, spiceerrors.MustBugf("unexpected rewrite operation type %T", t) } } -func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child, checkers checkers, ts *typesystem.TypeSystem) ([]*devinterface.DeveloperWarning, error) { +func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child, relation *corev1.Relation, checks checks, ts *typesystem.TypeSystem) ([]*devinterface.DeveloperWarning, error) { warnings := []*devinterface.DeveloperWarning{} for _, op := range ops { switch t := op.ChildType.(type) { @@ -138,8 +173,12 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child continue case *corev1.SetOperation_Child_ComputedUserset: - for _, checker := range checkers.computedUsersetCheckers { - checkerWarning, err := checker(ctx, t.ComputedUserset, op.SourcePosition, ts) + for _, check := range checks.computedUsersetChecks { + if shouldSkipCheck(relation.Metadata, check.name) { + continue + } + + checkerWarning, err := check.fn(ctx, t.ComputedUserset, op.SourcePosition, ts) if err != nil { return nil, err } @@ -150,7 +189,7 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child } case *corev1.SetOperation_Child_UsersetRewrite: - found, err := walkUsersetRewrite(ctx, t.UsersetRewrite, checkers, ts) + found, err := walkUsersetRewrite(ctx, t.UsersetRewrite, relation, checks, ts) if err != nil { return nil, err } @@ -158,8 +197,12 @@ func walkUsersetOperations(ctx context.Context, ops []*corev1.SetOperation_Child warnings = append(warnings, found...) case *corev1.SetOperation_Child_TupleToUserset: - for _, checker := range checkers.ttuCheckers { - checkerWarning, err := checker(ctx, t.TupleToUserset, op.SourcePosition, ts) + for _, check := range checks.ttuChecks { + if shouldSkipCheck(relation.Metadata, check.name) { + continue + } + + checkerWarning, err := check.fn(ctx, t.TupleToUserset, op.SourcePosition, ts) if err != nil { return nil, err } diff --git a/pkg/development/warnings_test.go b/pkg/development/warnings_test.go index 1a7d9c88bd..5e5c245204 100644 --- a/pkg/development/warnings_test.go +++ b/pkg/development/warnings_test.go @@ -36,7 +36,7 @@ func TestWarnings(t *testing.T) { permission view = view }`, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion", + Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion (permission-references-itself)", Line: 2, Column: 23, }, @@ -49,7 +49,7 @@ func TestWarnings(t *testing.T) { permission view = viewer + (editor & view) }`, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion", + Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion (permission-references-itself)", Line: 4, Column: 42, }, @@ -68,7 +68,7 @@ func TestWarnings(t *testing.T) { } `, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Arrow `group->member` under permission \"view\" references relation \"member\" on definition \"group\"; it is recommended to point to a permission", + Message: "Arrow `group->member` under permission \"view\" references relation \"member\" on definition \"group\"; it is recommended to point to a permission (arrow-references-relation)", Line: 9, Column: 23, }, @@ -86,7 +86,7 @@ func TestWarnings(t *testing.T) { } `, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Arrow `group->member` under permission \"view\" references relation/permission \"member\" that does not exist on any subject types of relation \"group\"", + Message: "Arrow `group->member` under permission \"view\" references relation/permission \"member\" that does not exist on any subject types of relation \"group\" (arrow-references-unreachable-relation)", Line: 8, Column: 23, }, @@ -106,7 +106,7 @@ func TestWarnings(t *testing.T) { } `, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Arrow `parent_group->member` under permission \"view\" references relation \"parent_group\" that has relation \"member\" on subject \"group\": *the subject relation will be ignored for the arrow*", + Message: "Arrow `parent_group->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, }, @@ -120,11 +120,60 @@ func TestWarnings(t *testing.T) { permission view_document = viewer }`, expectedWarning: &developerv1.DeveloperWarning{ - Message: "Permission \"view_document\" references parent type \"document\" in its name; it is recommended to drop the suffix", + Message: "Permission \"view_document\" references parent type \"document\" in its name; it is recommended to drop the suffix (relation-name-references-parent)", Line: 5, Column: 5, }, }, + { + name: "relation referencing its parent definition in its name but warning disabled", + schema: `definition user {} + + definition document { + relation viewer: user + + // spicedb-ignore-warning: relation-name-references-parent + permission view_document = viewer + }`, + expectedWarning: nil, + }, + { + name: "permission referencing itself but warning disabled", + schema: `definition test { + // spicedb-ignore-warning: permission-references-itself + permission view = view + }`, + expectedWarning: nil, + }, + { + name: "arrow referencing relation but warning disabled", + schema: `definition group { + relation member: user + } + + definition user {} + + definition document { + relation group: group + + // spicedb-ignore-warning: arrow-references-relation + permission view = group->member + } + `, + expectedWarning: nil, + }, + { + name: "permission referencing itself with wrong warning disabled", + schema: `definition test { + // spicedb-ignore-warning: arrow-references-relation + permission view = view + }`, + expectedWarning: &developerv1.DeveloperWarning{ + Message: "Permission \"view\" references itself, which will cause an error to be raised due to infinite recursion (permission-references-itself)", + Line: 3, + Column: 23, + }, + }, } for _, tc := range tcs {