Ensure all resources are returned for relation check when caveats are…

… specified Fixes #2026
authzed · Aug 15, 2024 · 0e4c99e · 0e4c99e
1 parent 881a32e
commit 0e4c99e
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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,14 +503,21 @@ 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{
 			&v1.DispatchCheckRequest{
 				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
@@ -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
@@ -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