From 20855de75812bcbc975efebe7f76abf47c0f3edb 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 | 133 ++++++++++++++++-- .../dispatch/graph/lookupsubjects_test.go | 1 + internal/graph/check.go | 13 +- .../caveatmultiplebranchessamerel.yaml | 23 +++ pkg/tuple/tuple.go | 2 +- 5 files changed, 161 insertions(+), 11 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..eab41591f7 100644 --- a/internal/dispatch/graph/check_test.go +++ b/internal/dispatch/graph/check_test.go @@ -288,13 +288,14 @@ func TestCheckMetadata(t *testing.T) { func TestCheckPermissionOverSchema(t *testing.T) { testCases := []struct { - name string - schema string - relationships []*core.RelationTuple - resource *core.ObjectAndRelation - subject *core.ObjectAndRelation - expectedPermissionship v1.ResourceCheckResult_Membership - expectedCaveat *core.CaveatExpression + name string + schema string + relationships []*core.RelationTuple + resource *core.ObjectAndRelation + subject *core.ObjectAndRelation + expectedPermissionship v1.ResourceCheckResult_Membership + expectedCaveat *core.CaveatExpression + alternativeExpectedCaveat *core.CaveatExpression }{ { "basic union", @@ -312,6 +313,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic intersection", @@ -330,6 +332,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic exclusion", @@ -347,6 +350,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic union, multiple branches", @@ -365,6 +369,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic union no permission", @@ -380,6 +385,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "basic intersection no permission", @@ -397,6 +403,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "basic exclusion no permission", @@ -415,6 +422,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "exclusion with multiple branches", @@ -441,6 +449,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "intersection with multiple branches", @@ -467,6 +476,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "exclusion with multiple branches no permission", @@ -494,6 +504,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "intersection with multiple branches no permission", @@ -519,6 +530,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "basic arrow", @@ -541,6 +553,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic any arrow", @@ -563,6 +576,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic all arrow negative", @@ -585,6 +599,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "basic all arrow positive", @@ -608,6 +623,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic all arrow positive with different types", @@ -635,6 +651,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "basic all arrow negative over different types", @@ -663,6 +680,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "basic all arrow positive over different types", @@ -692,6 +710,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "all arrow for single org", @@ -713,6 +732,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "all arrow for no orgs", @@ -733,6 +753,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "view_by_all negative", @@ -766,6 +787,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "fred", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "view_by_any positive", @@ -801,6 +823,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "fred", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "view_by_any positive directly", @@ -836,6 +859,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "rachel", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "caveated intersection arrow", @@ -862,6 +886,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", nil), + nil, }, { "intersection arrow with caveated member", @@ -888,6 +913,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", nil), + nil, }, { "caveated intersection arrow with caveated member", @@ -914,6 +940,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", nil), + nil, }, { "caveated intersection arrow with caveated member, different context", @@ -947,6 +974,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { caveatAndCtx("anothercaveat", map[string]any{"someparam": int64(43)}), caveatAndCtx("somecaveat", map[string]any{"someparam": int64(42)}), ), + nil, }, { "caveated intersection arrow with multiple caveated branches", @@ -978,8 +1006,8 @@ func TestCheckPermissionOverSchema(t *testing.T) { caveatAndCtx("somecaveat", map[string]any{"someparam": int64(41)}), caveatAndCtx("somecaveat", map[string]any{"someparam": int64(42)}), ), + nil, }, - { "caveated intersection arrow with multiple caveated members", `definition user {} @@ -1010,6 +1038,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { caveatAndCtx("somecaveat", map[string]any{"someparam": int64(41)}), caveatAndCtx("somecaveat", map[string]any{"someparam": int64(42)}), ), + nil, }, { "caveated intersection arrow with one caveated branch", @@ -1038,6 +1067,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", map[string]any{"someparam": int64(42)}), + nil, }, { "caveated intersection arrow with one caveated member", @@ -1066,6 +1096,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", map[string]any{"someparam": int64(42)}), + nil, }, { "caveated intersection arrow multiple paths to the same subject", @@ -1093,6 +1124,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_CAVEATED_MEMBER, caveatAndCtx("somecaveat", nil), + nil, }, { "recursive all arrow positive result", @@ -1129,6 +1161,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "fred", "..."), v1.ResourceCheckResult_MEMBER, nil, + nil, }, { "recursive all arrow negative result", @@ -1165,6 +1198,7 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "tom", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + nil, }, { "recursive all arrow negative result due to recursion missing a folder", @@ -1202,6 +1236,79 @@ func TestCheckPermissionOverSchema(t *testing.T) { ONR("user", "fred", "..."), v1.ResourceCheckResult_NOT_MEMBER, nil, + 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)}), + ), + caveatOr( + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(42)}), + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(40)}), + ), + }, + { + "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(40)}), + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(42)}), + ), + caveatOr( + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(42)}), + caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(40)}), + ), }, } @@ -1239,10 +1346,18 @@ func TestCheckPermissionOverSchema(t *testing.T) { require.Equal(tc.expectedPermissionship, membership) - if tc.expectedCaveat != nil { + if tc.expectedCaveat != nil && tc.alternativeExpectedCaveat == nil { require.NotEmpty(resp.ResultsByResourceId[tc.resource.ObjectId].Expression) testutil.RequireProtoEqual(t, tc.expectedCaveat, resp.ResultsByResourceId[tc.resource.ObjectId].Expression, "mismatch in caveat") } + + if tc.expectedCaveat != nil && tc.alternativeExpectedCaveat != nil { + require.NotEmpty(resp.ResultsByResourceId[tc.resource.ObjectId].Expression) + + if testutil.AreProtoEqual(tc.expectedCaveat, resp.ResultsByResourceId[tc.resource.ObjectId].Expression, "mismatch in caveat") != nil { + testutil.RequireProtoEqual(t, tc.alternativeExpectedCaveat, resp.ResultsByResourceId[tc.resource.ObjectId].Expression, "mismatch in caveat") + } + } }) } } 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