From 0e4c99e403bcee92e47692dfc73696bc872244a4 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 15 Aug 2024 18:09:20 -0400 Subject: [PATCH] Ensure all resources are returned for relation check when caveats are specified Fixes #2026 --- internal/dispatch/graph/check_test.go | 64 +++++++++++++++++++ .../dispatch/graph/lookupsubjects_test.go | 1 + internal/graph/check.go | 13 +++- .../caveatmultiplebranchessamerel.yaml | 23 +++++++ pkg/tuple/tuple.go | 2 +- 5 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 internal/services/integrationtesting/testconfigs/caveatmultiplebranchessamerel.yaml diff --git a/internal/dispatch/graph/check_test.go b/internal/dispatch/graph/check_test.go index 4acb33e5bf..811b5ebc63 100644 --- a/internal/dispatch/graph/check_test.go +++ b/internal/dispatch/graph/check_test.go @@ -1203,6 +1203,70 @@ func TestCheckPermissionOverSchema(t *testing.T) { v1.ResourceCheckResult_NOT_MEMBER, nil, }, + { + "caveated over multiple branches", + ` + caveat somecaveat(somevalue int) { + somevalue == 42 + } + + definition user {} + + definition role { + relation member: user + } + + definition resource { + relation viewer: role#member with somecaveat + permission view = viewer + } + `, + []*core.RelationTuple{ + tuple.MustParse(`role:firstrole#member@user:tom[somecaveat:{"somevalue":40}]`), + tuple.MustParse(`role:secondrole#member@user:tom[somecaveat:{"somevalue":42}]`), + tuple.MustParse(`resource:doc1#viewer@role:firstrole#member`), + tuple.MustParse(`resource:doc1#viewer@role:secondrole#member`), + }, + ONR("resource", "doc1", "view"), + ONR("user", "tom", "..."), + v1.ResourceCheckResult_CAVEATED_MEMBER, + caveatOr( + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(40)}), + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(42)}), + ), + }, + { + "caveated over multiple branches reversed", + ` + caveat somecaveat(somevalue int) { + somevalue == 42 + } + + definition user {} + + definition role { + relation member: user + } + + definition resource { + relation viewer: role#member with somecaveat + permission view = viewer + } + `, + []*core.RelationTuple{ + tuple.MustParse(`role:secondrole#member@user:tom[somecaveat:{"somevalue":42}]`), + tuple.MustParse(`role:firstrole#member@user:tom[somecaveat:{"somevalue":40}]`), + tuple.MustParse(`resource:doc1#viewer@role:secondrole#member`), + tuple.MustParse(`resource:doc1#viewer@role:firstrole#member`), + }, + ONR("resource", "doc1", "view"), + ONR("user", "tom", "..."), + v1.ResourceCheckResult_CAVEATED_MEMBER, + caveatOr( + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(42)}), + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(40)}), + ), + }, } for _, tc := range testCases { diff --git a/internal/dispatch/graph/lookupsubjects_test.go b/internal/dispatch/graph/lookupsubjects_test.go index debd35f92f..7ec1e9c3da 100644 --- a/internal/dispatch/graph/lookupsubjects_test.go +++ b/internal/dispatch/graph/lookupsubjects_test.go @@ -28,6 +28,7 @@ var ( caveatAndCtx = caveats.MustCaveatExprForTestingWithContext caveatAnd = caveats.And caveatInvert = caveats.Invert + caveatOr = caveats.Or ) func TestSimpleLookupSubjects(t *testing.T) { diff --git a/internal/graph/check.go b/internal/graph/check.go index c4a011929d..8dd1f3f840 100644 --- a/internal/graph/check.go +++ b/internal/graph/check.go @@ -466,6 +466,7 @@ func (cc *ConcurrentChecker) checkDirect(ctx context.Context, crc currentRequest // Find the subjects over which to dispatch. subjectsToDispatch := tuple.NewONRByTypeSet() relationshipsBySubjectONR := mapz.NewMultiMap[string, *core.RelationTuple]() + hasCaveats := false for tpl := it.Next(); tpl != nil; tpl = it.Next() { if it.Err() != nil { @@ -479,6 +480,9 @@ func (cc *ConcurrentChecker) checkDirect(ctx context.Context, crc currentRequest subjectsToDispatch.Add(tpl.Subject) relationshipsBySubjectONR.Add(tuple.StringONR(tpl.Subject), tpl) + if tpl.Caveat != nil && tpl.Caveat.CaveatName != "" { + hasCaveats = true + } } it.Close() @@ -499,6 +503,13 @@ func (cc *ConcurrentChecker) checkDirect(ctx context.Context, crc currentRequest dispatchChunkCountHistogram.Observe(chunkCount) }) + // If there are caveats on the incoming relationships, then we must require all results to be + // found, as we need to ensure that all caveats are used for building the final expression. + resultsSetting := crc.resultsSetting + if hasCaveats { + resultsSetting = v1.DispatchCheckRequest_REQUIRE_ALL_RESULTS + } + // Dispatch and map to the associated resource ID(s). result := union(ctx, crc, toDispatch, func(ctx context.Context, crc currentRequestContext, dd directDispatch) CheckResult { childResult := cc.dispatch(ctx, crc, ValidatedCheckRequest{ @@ -506,7 +517,7 @@ func (cc *ConcurrentChecker) checkDirect(ctx context.Context, crc currentRequest ResourceRelation: dd.resourceType, ResourceIds: dd.resourceIds, Subject: crc.parentReq.Subject, - ResultsSetting: crc.resultsSetting, + ResultsSetting: resultsSetting, Metadata: decrementDepth(crc.parentReq.Metadata), Debug: crc.parentReq.Debug, diff --git a/internal/services/integrationtesting/testconfigs/caveatmultiplebranchessamerel.yaml b/internal/services/integrationtesting/testconfigs/caveatmultiplebranchessamerel.yaml new file mode 100644 index 0000000000..74bd3912d9 --- /dev/null +++ b/internal/services/integrationtesting/testconfigs/caveatmultiplebranchessamerel.yaml @@ -0,0 +1,23 @@ +schema: >- + definition user {} + + caveat write_limit(limit uint, count uint) { + count < limit + } + + definition role { + relation member: user + } + + definition database { + relation writer: role#member with write_limit + permission write = writer + } +relationships: |- + database:listings#writer@role:default#member[write_limit:{"limit":2}] + database:listings#writer@role:premium#member[write_limit:{"limit":4}] + role:default#member@user:bob + role:premium#member@user:bob +assertions: + assertTrue: + - 'database:listings#write@user:bob with {"count":3}' diff --git a/pkg/tuple/tuple.go b/pkg/tuple/tuple.go index 6929f6c8ea..96b94f40f0 100644 --- a/pkg/tuple/tuple.go +++ b/pkg/tuple/tuple.go @@ -175,7 +175,7 @@ func MustParse(tpl string) *core.RelationTuple { if parsed := Parse(tpl); parsed != nil { return parsed } - panic("failed to parse tuple") + panic("failed to parse tuple: " + tpl) } // Parse unmarshals the string form of a Tuple and returns nil if there is a