Skip to content

Commit

Permalink
Fix GetGraph bug
Browse files Browse the repository at this point in the history
This fixes a model-inversion bug. Given:
```yaml
folder:
  relations:
    owner: user
  permissions:
    is_owner: owner | parent->is_owner

doc:
  relations:
    parent: folder
    viewer: user | user:* | group#member
  permissions:
    can_view: viewer | parent->owner
```

The `can_view` premission isn't inverted correctly.
A `group#member` can have the `can_view` permission, but only
through the first term (`viewer`) because `parent-owner` can only
be a `user`.
The problem is that the inversion logic attempts to create the inversion
of `parent->owner` on `group`
(i.e. `group#doc^can_view#member: doc_viewer#member | doc_owner#doc_parent`)
But `group` has no `doc_owner` relation because `group#member` isn't assignable
to`folder#owner`.
ronenh committed Sep 1, 2024
1 parent 3ca0ce1 commit 219ef28
Showing 3 changed files with 11 additions and 4 deletions.
6 changes: 4 additions & 2 deletions graph/check_test.yaml
Original file line number Diff line number Diff line change
@@ -63,10 +63,12 @@ types:
can_invite: parent->can_read - viewer

# viewer can be user or group but owner can only be user
union_type_subset: viewer | owner
union_type_subset_arrow: viewer | parent->owner
negation_type_subset: viewer - owner

# viewer can be user or group but owner can only be user
negation_type_subset_arrow: viewer - parent->owner
intersection_type_subset: viewer & owner
intersection_type_subset_arrow: viewer & parent->owner

cycle:
relations:
4 changes: 3 additions & 1 deletion graph/objects.go
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ import (
dsc "github.com/aserto-dev/go-directory/aserto/directory/common/v3"
dsr "github.com/aserto-dev/go-directory/aserto/directory/reader/v3"
"github.com/aserto-dev/go-directory/pkg/derr"
"github.com/rs/zerolog/log"
"github.com/samber/lo"
)

@@ -25,8 +26,9 @@ func NewObjectSearch(m *model.Model, req *dsr.GetGraphRequest, reader RelationRe
// validate the model but skip name validation. To avoid name collisions, the inverted model
// uses mangled names that are not valid identifiers.
if err := im.Validate(model.SkipNameValidation, model.AllowPermissionInArrowBase); err != nil {
log.Err(err).Interface("req", req).Msg("inverted model is invalid")
// TODO: we should persist the inverted model instead of computing it on the fly.
return nil, derr.ErrUnknown.Msg("internal error: unable to search for objects.")
return nil, derr.ErrUnknown.Msg("internal error: unable to search objects.")
}

iParams := invertGetGraphRequest(im, req)
5 changes: 4 additions & 1 deletion model/inverse.go
Original file line number Diff line number Diff line change
@@ -194,7 +194,10 @@ func (ti *termInverter) invertArrow() {
itip := ti.inv.irelSub(ti.objName, ti.term.Base)
baseRel := ti.obj.Relations[ti.term.Base]

typeRefs := set.NewSet(ti.perm.Types()...)
typeRefs := set.NewSet(ti.term.Types()...)
if typeRefs.IsEmpty() {
typeRefs.Append(ti.perm.Types()...)
}
typeRefs.Intersect(ti.typeSet).Each(func(rr RelationRef) bool {
iName := InverseRelation(ti.objName, ti.permName, rr.Relation)
iPerm := permissionOrNew(ti.inv.im.Objects[rr.Object], iName, ti.kind)

0 comments on commit 219ef28

Please sign in to comment.