From b07431e21bb5edd549ddcffbb9b7f970e80af800 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 19 May 2023 17:57:24 -0400 Subject: [PATCH] Change namespace+relation validation to use a single DB lookup Should reduce DB roundtrips and goroutine usage Fixes #1333 --- internal/namespace/util.go | 63 ++++++++++++++ internal/namespace/util_test.go | 122 +++++++++++++++++++++++++++- internal/services/v1/permissions.go | 103 +++++++++-------------- 3 files changed, 220 insertions(+), 68 deletions(-) diff --git a/internal/namespace/util.go b/internal/namespace/util.go index 3870399088..eb41299889 100644 --- a/internal/namespace/util.go +++ b/internal/namespace/util.go @@ -35,6 +35,69 @@ func ReadNamespaceAndRelation( return nil, nil, NewRelationNotFoundErr(namespace, relation) } +// TypeAndRelationToCheck is a single check of a namespace+relation pair. +type TypeAndRelationToCheck struct { + // NamespaceName is the namespace name to ensure exists. + NamespaceName string + + // RelationName is the relation name to ensure exists under the namespace. + RelationName string + + // AllowEllipsis, if true, allows for the ellipsis as the RelationName. + AllowEllipsis bool +} + +// CheckNamespaceAndRelations ensures that the given namespace+relation checks all succeed. If any fail, returns an error. +// +// Returns ErrNamespaceNotFound if the namespace cannot be found. +// Returns ErrRelationNotFound if the relation was not found in the namespace. +// Returns the direct downstream error for all other unknown error. +func CheckNamespaceAndRelations(ctx context.Context, checks []TypeAndRelationToCheck, ds datastore.Reader) error { + nsNames := util.NewSet[string]() + for _, toCheck := range checks { + nsNames.Add(toCheck.NamespaceName) + } + + if nsNames.IsEmpty() { + return nil + } + + namespaces, err := ds.LookupNamespacesWithNames(ctx, nsNames.AsSlice()) + if err != nil { + return err + } + + mappedNamespaces := make(map[string]*core.NamespaceDefinition, len(namespaces)) + for _, namespace := range namespaces { + mappedNamespaces[namespace.Definition.Name] = namespace.Definition + } + + for _, toCheck := range checks { + nsDef, ok := mappedNamespaces[toCheck.NamespaceName] + if !ok { + return NewNamespaceNotFoundErr(toCheck.NamespaceName) + } + + if toCheck.AllowEllipsis && toCheck.RelationName == datastore.Ellipsis { + continue + } + + foundRelation := false + for _, rel := range nsDef.Relation { + if rel.Name == toCheck.RelationName { + foundRelation = true + break + } + } + + if !foundRelation { + return NewRelationNotFoundErr(toCheck.NamespaceName, toCheck.RelationName) + } + } + + return nil +} + // CheckNamespaceAndRelation checks that the specified namespace and relation exist in the // datastore. // diff --git a/internal/namespace/util_test.go b/internal/namespace/util_test.go index ba10999a67..7865860244 100644 --- a/internal/namespace/util_test.go +++ b/internal/namespace/util_test.go @@ -1,14 +1,17 @@ -package namespace +package namespace_test import ( + "context" "sort" "testing" "github.com/stretchr/testify/require" - core "github.com/authzed/spicedb/pkg/proto/core/v1" - + "github.com/authzed/spicedb/internal/datastore/memdb" + "github.com/authzed/spicedb/internal/namespace" + "github.com/authzed/spicedb/internal/testfixtures" ns "github.com/authzed/spicedb/pkg/namespace" + core "github.com/authzed/spicedb/pkg/proto/core/v1" ) func TestListReferencedNamespaces(t *testing.T) { @@ -59,7 +62,7 @@ func TestListReferencedNamespaces(t *testing.T) { t.Run(tc.name, func(t *testing.T) { require := require.New(t) - found := ListReferencedNamespaces(tc.toCheck) + found := namespace.ListReferencedNamespaces(tc.toCheck) sort.Strings(found) sort.Strings(tc.expectedNames) @@ -67,3 +70,114 @@ func TestListReferencedNamespaces(t *testing.T) { }) } } + +func TestCheckNamespaceAndRelations(t *testing.T) { + tcs := []struct { + name string + schema string + checks []namespace.TypeAndRelationToCheck + expectedError string + }{ + { + "missing namespace", + `definition user {}`, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: "user", + RelationName: "...", + AllowEllipsis: true, + }, + { + NamespaceName: "resource", + RelationName: "viewer", + AllowEllipsis: false, + }, + }, + "object definition `resource` not found", + }, + { + "missing relation", + `definition resource {}`, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: "resource", + RelationName: "viewer", + AllowEllipsis: false, + }, + }, + "relation/permission `viewer` not found", + }, + { + "valid", + `definition user {} + + definition resource { + relation viewer: user + } + `, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: "user", + RelationName: "...", + AllowEllipsis: true, + }, + { + NamespaceName: "resource", + RelationName: "viewer", + AllowEllipsis: false, + }, + }, + "", + }, + { + "ellipsis not allowed", + `definition user {}`, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: "user", + RelationName: "...", + AllowEllipsis: false, + }, + }, + "relation/permission `...` not found", + }, + { + "another missing namespace", + `definition user {}`, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: "resource", + RelationName: "viewer", + AllowEllipsis: false, + }, + { + NamespaceName: "user", + RelationName: "...", + AllowEllipsis: true, + }, + }, + "object definition `resource` not found", + }, + } + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + req := require.New(t) + rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC) + req.NoError(err) + + ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, tc.schema, nil, req) + + rev, err := ds.HeadRevision(context.Background()) + require.NoError(t, err) + + reader := ds.SnapshotReader(rev) + + err = namespace.CheckNamespaceAndRelations(context.Background(), tc.checks, reader) + if tc.expectedError == "" { + require.Nil(t, err) + } else { + require.ErrorContains(t, err, tc.expectedError) + } + }) + } +} diff --git a/internal/services/v1/permissions.go b/internal/services/v1/permissions.go index 2266dfee37..8714392176 100644 --- a/internal/services/v1/permissions.go +++ b/internal/services/v1/permissions.go @@ -10,7 +10,6 @@ import ( "github.com/authzed/authzed-go/pkg/responsemeta" v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" "github.com/jzelinskie/stringz" - "golang.org/x/sync/errgroup" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" @@ -45,27 +44,19 @@ func (ps *permissionServer) CheckPermission(ctx context.Context, req *v1.CheckPe return nil, shared.RewriteError(ctx, err) } - // Perform our preflight checks in parallel - errG, checksCtx := errgroup.WithContext(ctx) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - checksCtx, - req.Resource.ObjectType, - req.Permission, - false, - ds, - ) - }) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - checksCtx, - req.Subject.Object.ObjectType, - normalizeSubjectRelation(req.Subject), - true, - ds, - ) - }) - if err := errG.Wait(); err != nil { + if err := namespace.CheckNamespaceAndRelations(ctx, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: req.Resource.ObjectType, + RelationName: req.Permission, + AllowEllipsis: false, + }, + { + NamespaceName: req.Subject.Object.ObjectType, + RelationName: normalizeSubjectRelation(req.Subject), + AllowEllipsis: true, + }, + }, ds); err != nil { return nil, shared.RewriteError(ctx, err) } @@ -332,27 +323,19 @@ func (ps *permissionServer) LookupResources(req *v1.LookupResourcesRequest, resp ds := datastoremw.MustFromContext(ctx).SnapshotReader(atRevision) - // Perform our preflight checks in parallel - errG, checksCtx := errgroup.WithContext(ctx) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - checksCtx, - req.Subject.Object.ObjectType, - normalizeSubjectRelation(req.Subject), - true, - ds, - ) - }) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - ctx, - req.ResourceObjectType, - req.Permission, - false, - ds, - ) - }) - if err := errG.Wait(); err != nil { + if err := namespace.CheckNamespaceAndRelations(ctx, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: req.ResourceObjectType, + RelationName: req.Permission, + AllowEllipsis: false, + }, + { + NamespaceName: req.Subject.Object.ObjectType, + RelationName: normalizeSubjectRelation(req.Subject), + AllowEllipsis: true, + }, + }, ds); err != nil { return shared.RewriteError(ctx, err) } @@ -416,27 +399,19 @@ func (ps *permissionServer) LookupSubjects(req *v1.LookupSubjectsRequest, resp v return shared.RewriteError(ctx, err) } - // Perform our preflight checks in parallel - errG, checksCtx := errgroup.WithContext(ctx) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - checksCtx, - req.Resource.ObjectType, - req.Permission, - false, - ds, - ) - }) - errG.Go(func() error { - return namespace.CheckNamespaceAndRelation( - ctx, - req.SubjectObjectType, - stringz.DefaultEmpty(req.OptionalSubjectRelation, tuple.Ellipsis), - true, - ds, - ) - }) - if err := errG.Wait(); err != nil { + if err := namespace.CheckNamespaceAndRelations(ctx, + []namespace.TypeAndRelationToCheck{ + { + NamespaceName: req.Resource.ObjectType, + RelationName: req.Permission, + AllowEllipsis: false, + }, + { + NamespaceName: req.SubjectObjectType, + RelationName: stringz.DefaultEmpty(req.OptionalSubjectRelation, tuple.Ellipsis), + AllowEllipsis: true, + }, + }, ds); err != nil { return shared.RewriteError(ctx, err) }