Detect cycles in check functions (#57)

* Add cycle detection to check/check_relation/check_permission * Use protobuf helpers from go-directory/pkg/pb
aserto-dev · Nov 13, 2023 · b6c3305 · b6c3305
1 parent 0f76237
commit b6c3305
Show file tree

Hide file tree

Showing 14 changed files with 47 additions and 104 deletions.
diff --git a/go.mod b/go.mod
@@ -9,7 +9,7 @@ require (
 	github.com/Masterminds/semver v1.5.0
 	github.com/aserto-dev/azm v0.0.15
 	github.com/aserto-dev/errors v0.0.6
-	github.com/aserto-dev/go-directory v0.30.3
+	github.com/aserto-dev/go-directory v0.30.4
 	github.com/bufbuild/protovalidate-go v0.4.0
 	github.com/gonvenience/ytbx v1.4.4
 	github.com/google/uuid v1.4.0

diff --git a/go.sum b/go.sum
@@ -12,8 +12,8 @@ github.com/aserto-dev/azm v0.0.15 h1:GO222vhAfcEeJJBTuWJBHNm1lIMA6qxkFjiVHN5pPeo
 github.com/aserto-dev/azm v0.0.15/go.mod h1:oNNvPYysttO/otHryQDm6u71jAt/Qhs9izhrde7HlD0=
 github.com/aserto-dev/errors v0.0.6 h1:iH5fkJwBGFPbcdS4B8mwvNdwODlhDEXXPduZtjLh6vo=
 github.com/aserto-dev/errors v0.0.6/go.mod h1:kenI1gamsemaR2wS+M2un0kXIJ9exTrmeRT/fCFwlWc=
-github.com/aserto-dev/go-directory v0.30.3 h1:agzqBvC1biaJUBzkJYCqUiKnmNXRQ67JVlxlLFsxu2c=
-github.com/aserto-dev/go-directory v0.30.3/go.mod h1:jHn6ckERuci6nWz9vCzAbYoF2UnbOwEzl++sA1L3nyc=
+github.com/aserto-dev/go-directory v0.30.4 h1:MifQgaA2ino54mWcmJAbJZSmUPNqat0V/u60EpFd9Vs=
+github.com/aserto-dev/go-directory v0.30.4/go.mod h1:jHn6ckERuci6nWz9vCzAbYoF2UnbOwEzl++sA1L3nyc=
 github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA=
 github.com/bufbuild/protovalidate-go v0.4.0 h1:ModSkCLEW07fiyGtdtMXKY+Gz3oPFKSfiaSCgL+FtpU=
 github.com/bufbuild/protovalidate-go v0.4.0/go.mod h1:QqeUPLVYEKQc+/rkoUXFqXW03zPBfrEfIbX+zmA0VxA=

diff --git a/pkg/bdb/generic.go b/pkg/bdb/generic.go
@@ -5,7 +5,7 @@ import (
 	"context"
 	"encoding/json"
 
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	bolt "go.etcd.io/bbolt"
 	"google.golang.org/protobuf/proto"
 )

diff --git a/pkg/bdb/migrate/mig001/migrate.go b/pkg/bdb/migrate/mig001/migrate.go
@@ -5,10 +5,10 @@ import (
 	"time"
 
 	"github.com/Masterminds/semver"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb/metadata"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb/migrate/mig"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 	"github.com/rs/zerolog"
 	bolt "go.etcd.io/bbolt"
 	"google.golang.org/protobuf/types/known/timestamppb"

diff --git a/pkg/bdb/migrate/mig002/migrate.go b/pkg/bdb/migrate/mig002/migrate.go
@@ -6,10 +6,10 @@ import (
 	"os"
 
 	dsc2 "github.com/aserto-dev/go-directory/aserto/directory/common/v2"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb/migrate/mig"
 	"github.com/aserto-dev/go-edge-ds/pkg/ds"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 	"github.com/rs/zerolog"
 
 	"github.com/Masterminds/semver"

diff --git a/pkg/directory/v3/model.go b/pkg/directory/v3/model.go
@@ -12,9 +12,9 @@ import (
 	dsm3 "github.com/aserto-dev/go-directory/aserto/directory/model/v3"
 	"github.com/aserto-dev/go-directory/pkg/derr"
 	"github.com/aserto-dev/go-directory/pkg/gateway/model/v3"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
 	"github.com/aserto-dev/go-edge-ds/pkg/ds"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 
 	"github.com/bufbuild/protovalidate-go"
 	"github.com/pkg/errors"

diff --git a/pkg/directory/v3/reader.go b/pkg/directory/v3/reader.go
@@ -303,7 +303,7 @@ func (s *Reader) CheckPermission(ctx context.Context, req *dsr3.CheckPermissionR
 	return resp, err
 }
 
-// CheckRelation, check if subject is permitted to access resource (object).
+// CheckRelation, check if subject has the specified relation to a resource (object).
 func (s *Reader) CheckRelation(ctx context.Context, req *dsr3.CheckRelationRequest) (*dsr3.CheckRelationResponse, error) {
 	resp := &dsr3.CheckRelationResponse{}
 

diff --git a/pkg/ds/check.go b/pkg/ds/check.go
@@ -5,8 +5,8 @@ import (
 
 	"github.com/aserto-dev/azm/cache"
 	"github.com/aserto-dev/azm/model"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 
 	dsc3 "github.com/aserto-dev/go-directory/aserto/directory/common/v3"
 	dsr3 "github.com/aserto-dev/go-directory/aserto/directory/reader/v3"
@@ -99,11 +99,18 @@ func (i *check) newChecker(ctx context.Context, tx *bolt.Tx, path []string, mc *
 		filter:  relations,
 		trace:   [][]*dsc3.Relation{},
 		mc:      mc,
+		visited: map[ot]bool{},
 	}
 
 	return checker, nil
 }
 
+// value type to be used as a key in a map.
+type ot struct {
+	ObjectType string
+	ObjectID   string
+}
+
 type checker struct {
 	ctx     context.Context
 	tx      *bolt.Tx
@@ -113,6 +120,7 @@ type checker struct {
 	filter  []model.RelationName
 	trace   [][]*dsc3.Relation
 	mc      *cache.Cache
+	visited map[ot]bool
 }
 
 func (c *checker) check(root *dsc3.ObjectIdentifier) (bool, error) {
@@ -123,14 +131,16 @@ func (c *checker) check(root *dsc3.ObjectIdentifier) (bool, error) {
 		return false, err
 	}
 
+	c.visited[ot{ObjectType: root.ObjectType, ObjectID: root.ObjectId}] = true
+
 	for _, r := range relations {
 		if c.isMatch(r) {
 			return true, nil
 		}
 	}
 
 	for _, r := range relations {
-		if lo.Contains(c.filter, model.RelationName(r.Relation)) {
+		if c.isCandidate(r) {
 			match, err := c.check(Relation(r).Subject())
 			if err != nil {
 				return false, err
@@ -146,8 +156,11 @@ func (c *checker) check(root *dsc3.ObjectIdentifier) (bool, error) {
 }
 
 func (c *checker) isMatch(relation *dsc3.Relation) bool {
-	if lo.Contains(c.filter, model.RelationName(relation.Relation)) && pb.Contains[*dsc3.ObjectIdentifier](c.userSet, Relation(relation).Subject()) {
-		return true
-	}
-	return false
+	return lo.Contains(c.filter, model.RelationName(relation.Relation)) &&
+		pb.Contains(c.userSet, Relation(relation).Subject())
+}
+
+func (c *checker) isCandidate(r *dsc3.Relation) bool {
+	return lo.Contains(c.filter, model.RelationName(r.Relation)) &&
+		!c.visited[ot{ObjectType: r.SubjectType, ObjectID: r.SubjectId}]
 }
diff --git a/pkg/ds/check_permission.go b/pkg/ds/check_permission.go
@@ -7,8 +7,8 @@ import (
 	"github.com/aserto-dev/azm/model"
 	dsc3 "github.com/aserto-dev/go-directory/aserto/directory/common/v3"
 	dsr3 "github.com/aserto-dev/go-directory/aserto/directory/reader/v3"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 
 	"github.com/samber/lo"
 	bolt "go.etcd.io/bbolt"
@@ -87,6 +87,7 @@ func (i *checkPermission) newChecker(ctx context.Context, tx *bolt.Tx, path []st
 		userSet: userSet,
 		filter:  relations,
 		trace:   [][]*dsc3.Relation{},
+		visited: map[ot]bool{},
 	}, nil
 }
 
@@ -98,6 +99,7 @@ type permissionChecker struct {
 	userSet []*dsc3.ObjectIdentifier
 	filter  []model.RelationName
 	trace   [][]*dsc3.Relation
+	visited map[ot]bool
 }
 
 func (c *permissionChecker) check(root *dsc3.ObjectIdentifier) (bool, error) {
@@ -108,14 +110,16 @@ func (c *permissionChecker) check(root *dsc3.ObjectIdentifier) (bool, error) {
 		return false, err
 	}
 
+	c.visited[ot{root.ObjectType, root.ObjectId}] = true
+
 	for _, r := range relations {
 		if c.isMatch(r) {
 			return true, nil
 		}
 	}
 
 	for _, r := range relations {
-		if lo.Contains(c.filter, model.RelationName(r.Relation)) || r.Relation == "parent" {
+		if c.isCandidate(r) {
 			match, err := c.check(Relation(r).Subject())
 			if err != nil {
 				return false, err
@@ -136,3 +140,8 @@ func (c *permissionChecker) isMatch(relation *dsc3.Relation) bool {
 	}
 	return false
 }
+
+func (c *permissionChecker) isCandidate(r *dsc3.Relation) bool {
+	return (lo.Contains(c.filter, model.RelationName(r.Relation)) || r.Relation == "parent") &&
+		!c.visited[ot{r.SubjectType, r.SubjectId}]
+}
diff --git a/pkg/ds/check_relation.go b/pkg/ds/check_relation.go
@@ -7,8 +7,8 @@ import (
 	"github.com/aserto-dev/azm/model"
 	dsc3 "github.com/aserto-dev/go-directory/aserto/directory/common/v3"
 	dsr3 "github.com/aserto-dev/go-directory/aserto/directory/reader/v3"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 	"github.com/aserto-dev/go-edge-ds/pkg/bdb"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
 
 	"github.com/samber/lo"
 	bolt "go.etcd.io/bbolt"
@@ -87,6 +87,7 @@ func (i *checkRelation) newChecker(ctx context.Context, tx *bolt.Tx, path []stri
 		userSet: userSet,
 		filter:  relations,
 		trace:   [][]*dsc3.Relation{},
+		visited: map[ot]bool{},
 	}, nil
 }
 
@@ -98,6 +99,7 @@ type relationChecker struct {
 	userSet []*dsc3.ObjectIdentifier
 	filter  []model.RelationName
 	trace   [][]*dsc3.Relation
+	visited map[ot]bool
 }
 
 func (c *relationChecker) check(root *dsc3.ObjectIdentifier) (bool, error) {
@@ -115,7 +117,7 @@ func (c *relationChecker) check(root *dsc3.ObjectIdentifier) (bool, error) {
 	}
 
 	for _, r := range relations {
-		if lo.Contains(c.filter, model.RelationName(r.Relation)) {
+		if c.isCandidate(r) {
 			match, err := c.check(Relation(r).Subject())
 			if err != nil {
 				return false, err
@@ -136,3 +138,7 @@ func (c *relationChecker) isMatch(relation *dsc3.Relation) bool {
 	}
 	return false
 }
+
+func (c *relationChecker) isCandidate(r *dsc3.Relation) bool {
+	return lo.Contains(c.filter, model.RelationName(r.Relation)) && !c.visited[ot{r.SubjectType, r.SubjectId}]
+}
diff --git a/pkg/ds/object.go b/pkg/ds/object.go
@@ -9,7 +9,7 @@ import (
 	"github.com/aserto-dev/azm/model"
 	dsc3 "github.com/aserto-dev/go-directory/aserto/directory/common/v3"
 	"github.com/aserto-dev/go-directory/pkg/derr"
-	"github.com/aserto-dev/go-edge-ds/pkg/pb"
+	"github.com/aserto-dev/go-directory/pkg/pb"
 
 	"github.com/mitchellh/hashstructure/v2"
 )

diff --git a/pkg/pb/generic.go b/pkg/pb/generic.go
diff --git a/pkg/pb/pb.go b/pkg/pb/pb.go
diff --git a/pkg/pb/struct.go b/pkg/pb/struct.go