Skip to content

Commit

Permalink
Review feedback and additional tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@josephschorr
josephschorr committed Jul 23, 2024
1 parent 5147ba1 commit d889495
Show file tree
Hide file tree
Showing 14 changed files with 386 additions and 93 deletions.
22 changes: 13 additions & 9 deletions internal/caveats/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,12 @@ func (lc loadedCaveats) Get(caveatDefName string) (*core.CaveatDefinition, *cave
return caveat, justDeserialized, nil
}

func isFalseResult(resuilt ExpressionResult) bool {
return !resuilt.Value() && !resuilt.IsPartial()
func isFalseResult(result ExpressionResult) bool {
return !result.Value() && !result.IsPartial()
}

func isTrueResult(resuilt ExpressionResult) bool {
return resuilt.Value() && !resuilt.IsPartial()
func isTrueResult(result ExpressionResult) bool {
return result.Value() && !result.IsPartial()
}

func runExpressionWithCaveats(
Expand Down Expand Up @@ -222,16 +222,16 @@ func runExpressionWithCaveats(

var currentResult ExpressionResult = syntheticResult{
value: false,
contextValues: map[string]any{},
contextValues: nil,
exprString: "",
missingContextParams: []string{},
missingContextParams: nil,
}
if cop.Op == core.CaveatOperation_AND {
currentResult = syntheticResult{
value: true,
contextValues: map[string]any{},
contextValues: nil,
exprString: "",
missingContextParams: []string{},
missingContextParams: nil,
}
}

Expand Down Expand Up @@ -324,7 +324,7 @@ func runExpressionWithCaveats(

missingContextParams = params
} else if existing.Value() {
return found, nil
return existing, nil
}

if found.IsPartial() {
Expand Down Expand Up @@ -430,6 +430,10 @@ func combineMaps(first map[string]any, second map[string]any) map[string]any {
first = make(map[string]any, len(second))
}

if second == nil {
return first
}

cloned := maps.Clone(first)
maps.Copy(cloned, second)
return cloned
Expand Down
201 changes: 176 additions & 25 deletions internal/dispatch/graph/check_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,7 @@ func TestCheckWithHints(t *testing.T) {
relationships []*core.RelationTuple
resource *core.ObjectAndRelation
subject *core.ObjectAndRelation
hint *v1.CheckHint
hints []*v1.CheckHint
expectedPermissionship bool
}{
{
Expand Down Expand Up @@ -1410,13 +1410,13 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
true,
},
{
"no relationships with non check hint",
"no relationships with unrelated check hint",
`definition user {}
definition document {
Expand All @@ -1427,13 +1427,13 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForComputedUserset("document", "anotherdoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "anotherdoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
false,
},
{
"no relationships with non check hint due to subject",
"no relationships with unrelated check hint due to subject",
`definition user {}
definition document {
Expand All @@ -1444,9 +1444,9 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "anotheruser", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "anotheruser", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
false,
},
{
Expand All @@ -1464,9 +1464,9 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForArrow("document", "somedoc", "org", "member", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForArrow("document", "somedoc", "org", "member", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
true,
},
{
Expand All @@ -1484,9 +1484,9 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForArrow("document", "somedoc", "anotherrel", "member", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForArrow("document", "somedoc", "anotherrel", "member", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
false,
},
{
Expand All @@ -1504,9 +1504,9 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForArrow("document", "somedoc", "org", "membersssss", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForArrow("document", "somedoc", "org", "membersssss", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
false,
},
{
Expand All @@ -1521,9 +1521,9 @@ func TestCheckWithHints(t *testing.T) {
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
false,
},
{
Expand All @@ -1540,11 +1540,167 @@ func TestCheckWithHints(t *testing.T) {
},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
})},
true,
},
{
"check hint of wrong type over part of an intersection with other branch in rels",
`definition user {}
definition document {
relation editor: user
relation viewer: user
permission view = viewer & editor
}`,
[]*core.RelationTuple{
tuple.MustParse("document:somedoc#editor@user:tom"),
},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{hints.CheckHintForArrow("document", "somedoc", "viewer", "something", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
})},
false,
},
{
"check hint over part of an exclusion with other branch not in rels",
`definition user {}
definition document {
relation banned: user
relation viewer: user
permission view = viewer - banned
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
})},
true,
},
{
"check hint for wildcard",
`definition user {}
definition document {
relation viewer: user:*
permission view = viewer
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
})},
true,
},
{
"check hint for wildcard, negative",
`definition user {}
definition document {
relation viewer: user:*
permission view = viewer
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_NOT_MEMBER,
})},
false,
},
{
"check hint for each branch of an intersection",
`definition user {}
definition document {
relation viewer: user
relation editor: user
permission view = viewer & editor
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
hints.CheckHintForComputedUserset("document", "somedoc", "editor", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
},
true,
},
{
"check hint for each branch of an exclusion",
`definition user {}
definition document {
relation viewer: user
relation banned: user
permission view = viewer - banned
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
hints.CheckHintForComputedUserset("document", "somedoc", "banned", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
},
false,
},
{
"check hint for each branch of an exclusion, allowed",
`definition user {}
definition document {
relation viewer: user
relation banned: user
permission view = viewer - banned
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_MEMBER,
}),
hints.CheckHintForComputedUserset("document", "somedoc", "banned", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_NOT_MEMBER,
}),
},
true,
},
{
"check hint for each branch of an exclusion, not member on either branch",
`definition user {}
definition document {
relation viewer: user
relation banned: user
permission view = viewer - banned
}`,
[]*core.RelationTuple{},
ONR("document", "somedoc", "view"),
ONR("user", "tom", graph.Ellipsis),
[]*v1.CheckHint{
hints.CheckHintForComputedUserset("document", "somedoc", "viewer", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_NOT_MEMBER,
}),
hints.CheckHintForComputedUserset("document", "somedoc", "banned", ONR("user", "tom", graph.Ellipsis), &v1.ResourceCheckResult{
Membership: v1.ResourceCheckResult_NOT_MEMBER,
}),
},
false,
},
}

for _, tc := range testCases {
Expand All @@ -1562,11 +1718,6 @@ func TestCheckWithHints(t *testing.T) {
ctx := datastoremw.ContextWithHandle(context.Background())
require.NoError(datastoremw.SetInContext(ctx, ds))

hints := []*v1.CheckHint{tc.hint}
if tc.hint == nil {
hints = nil
}

resp, err := dispatcher.DispatchCheck(ctx, &v1.DispatchCheckRequest{
ResourceRelation: RR(tc.resource.Namespace, tc.resource.Relation),
ResourceIds: []string{tc.resource.ObjectId},
Expand All @@ -1576,7 +1727,7 @@ func TestCheckWithHints(t *testing.T) {
AtRevision: revision.String(),
DepthRemaining: 50,
},
CheckHints: hints,
CheckHints: tc.hints,
})
require.NoError(err)

Expand Down
2 changes: 1 addition & 1 deletion internal/dispatch/graph/graph.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ func (ld *localDispatcher) DispatchLookupResources2(
req *v1.DispatchLookupResources2Request,
stream dispatch.LookupResources2Stream,
) error {
ctx, span := tracer.Start(stream.Context(), "DispatchLookupResources", trace.WithAttributes(
ctx, span := tracer.Start(stream.Context(), "DispatchLookupResources2", trace.WithAttributes(
attribute.String("resource-type", tuple.StringRR(req.ResourceRelation)),
attribute.String("subject", tuple.StringONR(req.TerminalSubject)),
))
Expand Down
Loading

0 comments on commit d889495

Please sign in to comment.