Skip to content

Commit fe8dd9f

Browse files
miparnisarimiparnisari
committed
fix: Checks involving relations with caveats can result in no permission when permission is expected
1 parent d5479cd commit fe8dd9f

File tree

5 files changed

+172
-2
lines changed

5 files changed

+172
-2
lines changed

‎internal/developmentmembership/membership.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package developmentmembership
33
import (
44
"fmt"
55

6+
"github.com/authzed/spicedb/internal/datasets"
67
core "github.com/authzed/spicedb/pkg/proto/core/v1"
78
"github.com/authzed/spicedb/pkg/spiceerrors"
89
"github.com/authzed/spicedb/pkg/tuple"
@@ -83,7 +84,7 @@ func populateFoundSubjects(rootONR tuple.ObjectAndRelation, treeNode *core.Relat
8384

8485
case core.SetOperationUserset_INTERSECTION:
8586
if len(typed.IntermediateNode.ChildNodes) == 0 {
86-
return nil, fmt.Errorf("found intersection with no children")
87+
return &TrackingSubjectSet{setByType: make(map[tuple.RelationReference]datasets.BaseSubjectSet[FoundSubject])}, nil
8788
}
8889

8990
firstChildSet, err := populateFoundSubjects(rootONR, typed.IntermediateNode.ChildNodes[0])

‎internal/dispatch/graph/check_test.go

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1320,6 +1320,96 @@ func TestCheckPermissionOverSchema(t *testing.T) {
13201320
caveatAndCtx("somecaveat", map[string]any{"somevalue": int64(40)}),
13211321
),
13221322
},
1323+
{
1324+
name: "caveated_with_arrows",
1325+
schema: `
1326+
definition user {}
1327+
1328+
definition office {
1329+
relation parent: office
1330+
relation manager: user
1331+
permission read = manager + parent->read
1332+
}
1333+
1334+
definition group {
1335+
relation parent: office
1336+
permission read = parent->read
1337+
}
1338+
1339+
definition document {
1340+
relation owner: group with equals
1341+
permission read = owner->read
1342+
}
1343+
1344+
caveat equals(actual string, required string) {
1345+
actual == required
1346+
}
1347+
`,
1348+
relationships: []tuple.Relationship{
1349+
tuple.MustParse(`office:headoffice#manager@user:maria`),
1350+
tuple.MustParse(`office:branch1#parent@office:headoffice`),
1351+
tuple.MustParse(`group:admins#parent@office:branch1`),
1352+
tuple.MustParse(`group:managers#parent@office:headoffice`),
1353+
tuple.MustParse(`document:budget#owner@group:admins[equals:{"required":"admin"}]`),
1354+
tuple.MustParse(`document:budget#owner@group:managers[equals:{"required":"manager"}]`),
1355+
},
1356+
resource: ONR("document", "budget", "read"),
1357+
subject: ONR("user", "maria", "..."),
1358+
expectedPermissionship: v1.ResourceCheckResult_CAVEATED_MEMBER,
1359+
expectedCaveat: caveatOr(
1360+
caveatAndCtx("equals", map[string]any{"required": "admin"}),
1361+
caveatAndCtx("equals", map[string]any{"required": "manager"}),
1362+
),
1363+
alternativeExpectedCaveat: caveatOr(
1364+
caveatAndCtx("equals", map[string]any{"required": "manager"}),
1365+
caveatAndCtx("equals", map[string]any{"required": "admin"}),
1366+
),
1367+
},
1368+
{
1369+
name: "caveated_nested_with_intersection_arrows",
1370+
schema: `
1371+
definition user {}
1372+
1373+
definition office {
1374+
relation parent: office
1375+
relation manager: user
1376+
permission read = manager + parent.all(read)
1377+
}
1378+
1379+
definition group {
1380+
relation parent: office
1381+
permission read = parent.all(read)
1382+
}
1383+
1384+
definition document {
1385+
relation owner: group with equals
1386+
permission read = owner.all(read)
1387+
}
1388+
1389+
caveat equals(actual string, required string) {
1390+
actual == required
1391+
}
1392+
`,
1393+
relationships: []tuple.Relationship{
1394+
tuple.MustParse(`office:headoffice#manager@user:maria`),
1395+
tuple.MustParse(`office:branch1#parent@office:headoffice`),
1396+
tuple.MustParse(`group:admins#parent@office:branch1`),
1397+
tuple.MustParse(`group:managers#parent@office:headoffice`),
1398+
tuple.MustParse(`document:budget#owner@group:admins[equals:{"required":"admin"}]`),
1399+
tuple.MustParse(`document:budget#owner@group:managers[equals:{"required":"manager"}]`),
1400+
},
1401+
resource: ONR("document", "budget", "read"),
1402+
subject: ONR("user", "maria", "..."),
1403+
expectedPermissionship: v1.ResourceCheckResult_CAVEATED_MEMBER,
1404+
expectedCaveat: caveatAnd(
1405+
caveatAndCtx("equals", map[string]any{"required": "admin"}),
1406+
caveatAndCtx("equals", map[string]any{"required": "manager"}),
1407+
),
1408+
alternativeExpectedCaveat: caveatAnd(
1409+
caveatAndCtx("equals", map[string]any{"required": "manager"}),
1410+
caveatAndCtx("equals", map[string]any{"required": "admin"}),
1411+
),
1412+
},
13231413
}
13241414

13251415
for _, tc := range testCases {
@@ -1356,7 +1446,7 @@ func TestCheckPermissionOverSchema(t *testing.T) {
13561446
membership = r.Membership
13571447
}
13581448

1359-
require.Equal(tc.expectedPermissionship, membership)
1449+
require.Equal(tc.expectedPermissionship, membership, fmt.Sprintf("expected permissionship %s, got %s", tc.expectedPermissionship, membership))
13601450

13611451
if tc.expectedCaveat != nil && tc.alternativeExpectedCaveat == nil {
13621452
require.NotEmpty(resp.ResultsByResourceId[tc.resource.ObjectID].Expression)

‎internal/graph/check.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -988,6 +988,9 @@ func checkTupleToUserset[T relation](
988988
toDispatch,
989989
func(ctx context.Context, crc currentRequestContext, dd checkDispatchChunk) CheckResult {
990990
resourceType := dd.resourceType
991+
if dd.hasIncomingCaveats {
992+
crc.resultsSetting = v1.DispatchCheckRequest_REQUIRE_ALL_RESULTS
993+
}
991994
childResult := cc.checkComputedUserset(ctx, crc, ttu.GetComputedUserset(), &resourceType, dd.resourceIds)
992995
if childResult.Err != nil {
993996
return childResult

‎internal/services/integrationtesting/testconfigs/multibranchcaveatwitharrows.yaml

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
schema: |+
3+
definition user {}
4+
5+
definition office {
6+
relation parent: office
7+
relation manager: user
8+
permission read = manager + parent->read
9+
}
10+
11+
definition group {
12+
relation parent: office
13+
permission read = parent->read
14+
}
15+
16+
definition document {
17+
relation owner: group with equals
18+
permission read = owner->read
19+
}
20+
21+
caveat equals(actual string, required string) {
22+
actual == required
23+
}
24+
25+
relationships: |
26+
office:headoffice#manager@user:maria
27+
office:branch1#parent@office:headoffice
28+
group:admins#parent@office:branch1
29+
group:managers#parent@office:headoffice
30+
document:budget#owner@group:admins[equals:{"required":"admin"}]
31+
document:budget#owner@group:managers[equals:{"required":"manager"}]
32+
assertions:
33+
assertTrue:
34+
- 'document:budget#read@user:maria with {"actual" : "admin"}'
35+
- 'document:budget#read@user:maria with {"actual" : "manager"}'
36+
assertFalse:
37+
- 'document:budget#read@user:maria with {"actual" : "unknown"}'

‎internal/services/integrationtesting/testconfigs/multibranchcaveatwithintersectionarrows.yaml

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
schema: |+
3+
definition user {}
4+
5+
definition office {
6+
relation parent: office
7+
relation manager: user
8+
permission read = manager + parent.all(read)
9+
}
10+
11+
definition group {
12+
relation parent: office
13+
permission read = parent.all(read)
14+
}
15+
16+
definition document {
17+
relation owner: group with equals
18+
permission read = owner.all(read)
19+
}
20+
21+
caveat equals(actual string, required string) {
22+
actual == required
23+
}
24+
25+
relationships: |
26+
office:headoffice#manager@user:maria
27+
office:branch1#manager@user:maria
28+
office:branch1#parent@office:headoffice
29+
group:admins#parent@office:branch1
30+
group:managers#parent@office:headoffice
31+
document:budget#owner@group:admins[equals:{"required":"admin"}]
32+
document:budget#owner@group:managers[equals:{"required":"admin"}]
33+
assertions:
34+
assertTrue:
35+
- 'document:budget#read@user:maria with {"actual" : "admin"}'
36+
assertFalse:
37+
- 'document:budget#read@user:maria with {"actual" : "unknown"}'
38+
- 'document:unknown#read@user:maria with {"actual" : "admin"}'
39+
- 'document:budget#read@user:unknown with {"actual" : "admin"}'

0 commit comments

Comments
 (0)