Skip to content

Commit 684f857

Browse files
josephschorrjosephschorr
committed
Fix exclusion operation in Check dispatch to *always* request the full set of results for the first branch
1 parent c50701a commit 684f857

File tree

4 files changed

+327
-1
lines changed

4 files changed

+327
-1
lines changed

‎internal/dispatch/graph/check_test.go

+261
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,267 @@ func addFrame(trace *v1.CheckDebugTrace, foundFrames *mapz.Set[string]) {
298298
}
299299
}
300300

301+
func TestCheckPermissionOverSchema(t *testing.T) {
302+
testCases := []struct {
303+
name string
304+
schema string
305+
relationships []*core.RelationTuple
306+
resource *core.ObjectAndRelation
307+
subject *core.ObjectAndRelation
308+
expectedPermissionship v1.ResourceCheckResult_Membership
309+
}{
310+
{
311+
"basic union",
312+
`definition user {}
313+
314+
definition document {
315+
relation editor: user
316+
relation viewer: user
317+
permission view = viewer + editor
318+
}`,
319+
[]*core.RelationTuple{
320+
tuple.MustParse("document:first#viewer@user:tom"),
321+
},
322+
ONR("document", "first", "view"),
323+
ONR("user", "tom", "..."),
324+
v1.ResourceCheckResult_MEMBER,
325+
},
326+
{
327+
"basic intersection",
328+
`definition user {}
329+
330+
definition document {
331+
relation editor: user
332+
relation viewer: user
333+
permission view = viewer & editor
334+
}`,
335+
[]*core.RelationTuple{
336+
tuple.MustParse("document:first#viewer@user:tom"),
337+
tuple.MustParse("document:first#editor@user:tom"),
338+
},
339+
ONR("document", "first", "view"),
340+
ONR("user", "tom", "..."),
341+
v1.ResourceCheckResult_MEMBER,
342+
},
343+
{
344+
"basic exclusion",
345+
`definition user {}
346+
347+
definition document {
348+
relation editor: user
349+
relation viewer: user
350+
permission view = viewer - editor
351+
}`,
352+
[]*core.RelationTuple{
353+
tuple.MustParse("document:first#viewer@user:tom"),
354+
},
355+
ONR("document", "first", "view"),
356+
ONR("user", "tom", "..."),
357+
v1.ResourceCheckResult_MEMBER,
358+
},
359+
{
360+
"basic union, multiple branches",
361+
`definition user {}
362+
363+
definition document {
364+
relation editor: user
365+
relation viewer: user
366+
permission view = viewer + editor
367+
}`,
368+
[]*core.RelationTuple{
369+
tuple.MustParse("document:first#viewer@user:tom"),
370+
tuple.MustParse("document:first#editor@user:tom"),
371+
},
372+
ONR("document", "first", "view"),
373+
ONR("user", "tom", "..."),
374+
v1.ResourceCheckResult_MEMBER,
375+
},
376+
{
377+
"basic union no permission",
378+
`definition user {}
379+
380+
definition document {
381+
relation editor: user
382+
relation viewer: user
383+
permission view = viewer + editor
384+
}`,
385+
[]*core.RelationTuple{},
386+
ONR("document", "first", "view"),
387+
ONR("user", "tom", "..."),
388+
v1.ResourceCheckResult_NOT_MEMBER,
389+
},
390+
{
391+
"basic intersection no permission",
392+
`definition user {}
393+
394+
definition document {
395+
relation editor: user
396+
relation viewer: user
397+
permission view = viewer & editor
398+
}`,
399+
[]*core.RelationTuple{
400+
tuple.MustParse("document:first#viewer@user:tom"),
401+
},
402+
ONR("document", "first", "view"),
403+
ONR("user", "tom", "..."),
404+
v1.ResourceCheckResult_NOT_MEMBER,
405+
},
406+
{
407+
"basic exclusion no permission",
408+
`definition user {}
409+
410+
definition document {
411+
relation banned: user
412+
relation viewer: user
413+
permission view = viewer - banned
414+
}`,
415+
[]*core.RelationTuple{
416+
tuple.MustParse("document:first#viewer@user:tom"),
417+
tuple.MustParse("document:first#banned@user:tom"),
418+
},
419+
ONR("document", "first", "view"),
420+
ONR("user", "tom", "..."),
421+
v1.ResourceCheckResult_NOT_MEMBER,
422+
},
423+
{
424+
"exclusion with multiple branches",
425+
`definition user {}
426+
427+
definition group {
428+
relation member: user
429+
relation banned: user
430+
permission view = member - banned
431+
}
432+
433+
definition document {
434+
relation group: group
435+
permission view = group->view
436+
}`,
437+
[]*core.RelationTuple{
438+
tuple.MustParse("document:first#group@group:first"),
439+
tuple.MustParse("document:first#group@group:second"),
440+
tuple.MustParse("group:first#member@user:tom"),
441+
tuple.MustParse("group:first#banned@user:tom"),
442+
tuple.MustParse("group:second#member@user:tom"),
443+
},
444+
ONR("document", "first", "view"),
445+
ONR("user", "tom", "..."),
446+
v1.ResourceCheckResult_MEMBER,
447+
},
448+
{
449+
"intersection with multiple branches",
450+
`definition user {}
451+
452+
definition group {
453+
relation member: user
454+
relation other: user
455+
permission view = member & other
456+
}
457+
458+
definition document {
459+
relation group: group
460+
permission view = group->view
461+
}`,
462+
[]*core.RelationTuple{
463+
tuple.MustParse("document:first#group@group:first"),
464+
tuple.MustParse("document:first#group@group:second"),
465+
tuple.MustParse("group:first#member@user:tom"),
466+
tuple.MustParse("group:first#other@user:tom"),
467+
tuple.MustParse("group:second#member@user:tom"),
468+
},
469+
ONR("document", "first", "view"),
470+
ONR("user", "tom", "..."),
471+
v1.ResourceCheckResult_MEMBER,
472+
},
473+
{
474+
"exclusion with multiple branches no permission",
475+
`definition user {}
476+
477+
definition group {
478+
relation member: user
479+
relation banned: user
480+
permission view = member - banned
481+
}
482+
483+
definition document {
484+
relation group: group
485+
permission view = group->view
486+
}`,
487+
[]*core.RelationTuple{
488+
tuple.MustParse("document:first#group@group:first"),
489+
tuple.MustParse("document:first#group@group:second"),
490+
tuple.MustParse("group:first#member@user:tom"),
491+
tuple.MustParse("group:first#banned@user:tom"),
492+
tuple.MustParse("group:second#member@user:tom"),
493+
tuple.MustParse("group:second#banned@user:tom"),
494+
},
495+
ONR("document", "first", "view"),
496+
ONR("user", "tom", "..."),
497+
v1.ResourceCheckResult_NOT_MEMBER,
498+
},
499+
{
500+
"intersection with multiple branches no permission",
501+
`definition user {}
502+
503+
definition group {
504+
relation member: user
505+
relation other: user
506+
permission view = member & other
507+
}
508+
509+
definition document {
510+
relation group: group
511+
permission view = group->view
512+
}`,
513+
[]*core.RelationTuple{
514+
tuple.MustParse("document:first#group@group:first"),
515+
tuple.MustParse("document:first#group@group:second"),
516+
tuple.MustParse("group:first#member@user:tom"),
517+
tuple.MustParse("group:second#member@user:tom"),
518+
},
519+
ONR("document", "first", "view"),
520+
ONR("user", "tom", "..."),
521+
v1.ResourceCheckResult_NOT_MEMBER,
522+
},
523+
}
524+
525+
for _, tc := range testCases {
526+
tc := tc
527+
t.Run(tc.name, func(t *testing.T) {
528+
require := require.New(t)
529+
530+
dispatcher := NewLocalOnlyDispatcher(10)
531+
532+
ds, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC)
533+
require.NoError(err)
534+
535+
ds, revision := testfixtures.DatastoreFromSchemaAndTestRelationships(ds, tc.schema, tc.relationships, require)
536+
537+
ctx := datastoremw.ContextWithHandle(context.Background())
538+
require.NoError(datastoremw.SetInContext(ctx, ds))
539+
540+
resp, err := dispatcher.DispatchCheck(ctx, &v1.DispatchCheckRequest{
541+
ResourceRelation: RR(tc.resource.Namespace, tc.resource.Relation),
542+
ResourceIds: []string{tc.resource.ObjectId},
543+
Subject: tc.subject,
544+
Metadata: &v1.ResolverMeta{
545+
AtRevision: revision.String(),
546+
DepthRemaining: 50,
547+
},
548+
ResultsSetting: v1.DispatchCheckRequest_ALLOW_SINGLE_RESULT,
549+
})
550+
require.NoError(err)
551+
552+
membership := v1.ResourceCheckResult_NOT_MEMBER
553+
if r, ok := resp.ResultsByResourceId[tc.resource.ObjectId]; ok {
554+
membership = r.Membership
555+
}
556+
557+
require.Equal(tc.expectedPermissionship, membership)
558+
})
559+
}
560+
}
561+
301562
func TestCheckDebugging(t *testing.T) {
302563
type expectedFrame struct {
303564
resourceType *core.RelationReference

‎internal/graph/check.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -770,7 +770,12 @@ func difference[T any](
770770
othersChan := make(chan CheckResult, len(children)-1)
771771

772772
go func() {
773-
result := handler(childCtx, crc, children[0])
773+
result := handler(childCtx, currentRequestContext{
774+
parentReq: crc.parentReq,
775+
filteredResourceIDs: crc.filteredResourceIDs,
776+
resultsSetting: v1.DispatchCheckRequest_REQUIRE_ALL_RESULTS,
777+
maxDispatchCount: crc.maxDispatchCount,
778+
}, children[0])
774779
baseChan <- result
775780
}()
776781

‎internal/graph/membershipset_test.go

+28
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,34 @@ func TestMembershipSetSubtract(t *testing.T) {
705705
false,
706706
false,
707707
},
708+
{
709+
"non overlapping",
710+
map[string]*core.CaveatExpression{
711+
"resource1": nil,
712+
"resource2": nil,
713+
},
714+
map[string]*core.CaveatExpression{
715+
"resource2": nil,
716+
},
717+
map[string]*core.CaveatExpression{
718+
"resource1": nil,
719+
},
720+
true,
721+
false,
722+
},
723+
{
724+
"non overlapping reversed",
725+
map[string]*core.CaveatExpression{
726+
"resource2": nil,
727+
},
728+
map[string]*core.CaveatExpression{
729+
"resource1": nil,
730+
"resource2": nil,
731+
},
732+
map[string]*core.CaveatExpression{},
733+
false,
734+
true,
735+
},
708736
}
709737

710738
for _, tc := range tcs {

‎internal/services/integrationtesting/arrowovermultiexclusion.yaml

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
schema: |+
3+
definition user {}
4+
5+
definition group {
6+
relation direct_member: user
7+
relation excluded: user:*
8+
permission view = direct_member - excluded
9+
}
10+
11+
definition resource {
12+
relation group: group
13+
permission view = group->view
14+
}
15+
16+
relationships: >-
17+
resource:one#group@group:group1
18+
19+
resource:one#group@group:group2
20+
21+
group:group1#direct_member@user:fred
22+
23+
group:group1#excluded@user:*
24+
25+
group:group2#direct_member@user:fred
26+
assertions:
27+
assertTrue:
28+
- "group:group1#direct_member@user:fred"
29+
- "group:group2#direct_member@user:fred"
30+
- "group:group2#view@user:fred"
31+
assertFalse:
32+
- "group:group1#view@user:fred"

0 commit comments

Comments
 (0)