fix: adding functionality to handle cycles in expand and check engine

internal/check/engine.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"errors"
 
+	"github.com/ory/keto/internal/utils"
+
 	"github.com/ory/herodot"
 
 	"github.com/ory/keto/internal/relationtuple"
@@ -16,6 +18,7 @@ type (
 	}
 	Engine struct {
 		d EngineDependencies
+		u utils.EngineUtils
 	}
 	EngineDependencies interface {
 		relationtuple.ManagerProvider
@@ -25,6 +28,7 @@ type (
 func NewEngine(d EngineDependencies) *Engine {
 	return &Engine{
 		d: d,
+		u: &utils.EngineUtilsProvider{},
 	}
 }
 
@@ -35,6 +39,11 @@ func (e *Engine) subjectIsAllowed(ctx context.Context, requested *relationtuple.
 	// TODO replace by more performant algorithm: https://github.com/ory/keto/issues/483
 
 	for _, sr := range rels {
+		ctx, wasAlreadyVisited := e.u.CheckVisited(ctx, sr.String())
+		if wasAlreadyVisited {
+			continue
+		}
+
 		// we only have to check Subject here as we know that sr was reached from requested.ObjectID, requested.Relation through 0...n indirections
 		if requested.Subject.Equals(sr.Subject) {
 			// found the requested relation

internal/check/engine_test.go

@@ -438,4 +438,57 @@ func TestEngine(t *testing.T) {
 			assert.True(t, allowed, req.String())
 		}
 	})
+
+	t.Run("case=circular tuples", func(t *testing.T) {
+		sendlingerTor, odeonsplatz, centralStation, connected, namesp := "Sendlinger Tor", "Odeonsplatz", "Central Station", "connected", "munich transport"
+
+		reg := newDepsProvider(t, []*namespace.Namespace{{Name: namesp}})
+
+		require.NoError(t, reg.RelationTupleManager().WriteRelationTuples(context.Background(), []*relationtuple.InternalRelationTuple{
+			{
+				Namespace: namesp,
+				Object:    sendlingerTor,
+				Relation:  connected,
+				Subject: &relationtuple.SubjectSet{
+					Namespace: namesp,
+					Object:    odeonsplatz,
+					Relation:  connected,
+				},
+			},
+			{
+				Namespace: namesp,
+				Object:    odeonsplatz,
+				Relation:  connected,
+				Subject: &relationtuple.SubjectSet{
+					Namespace: namesp,
+					Object:    centralStation,
+					Relation:  connected,
+				},
+			},
+			{
+				Namespace: namesp,
+				Object:    centralStation,
+				Relation:  connected,
+				Subject: &relationtuple.SubjectSet{
+					Namespace: namesp,
+					Object:    sendlingerTor,
+					Relation:  connected,
+				},
+			},
+		}...))
+
+		e := check.NewEngine(reg)
+
+		stations := []string{sendlingerTor, odeonsplatz, centralStation}
+		res, err := e.SubjectIsAllowed(context.Background(), &relationtuple.InternalRelationTuple{
+			Namespace: namesp,
+			Object:    stations[0],
+			Relation:  connected,
+			Subject: &relationtuple.SubjectID{
+				ID: stations[2],
+			},
+		})
+		require.NoError(t, err)
+		assert.False(t, res)
+	})
 }

internal/expand/engine.go

@@ -3,6 +3,8 @@ package expand
 import (
 	"context"
 
+	"github.com/ory/keto/internal/utils"
+
 	"github.com/ory/keto/internal/x"
 
 	"github.com/ory/keto/internal/relationtuple"
@@ -14,21 +16,30 @@ type (
 	}
 	Engine struct {
 		d EngineDependencies
+		u utils.EngineUtils
 	}
 	EngineProvider interface {
 		ExpandEngine() *Engine
 	}
 )
 
 func NewEngine(d EngineDependencies) *Engine {
-	return &Engine{d: d}
+	return &Engine{
+		d: d,
+		u: &utils.EngineUtilsProvider{},
+	}
 }
 
 func (e *Engine) BuildTree(ctx context.Context, subject relationtuple.Subject, restDepth int) (*Tree, error) {
 	if restDepth <= 0 {
 		return nil, nil
 	}
 
+	ctx, wasAlreadyVisited := e.u.CheckVisited(ctx, subject.String())
+	if wasAlreadyVisited {
+		return nil, nil
+	}
+
 	if us, isUserSet := subject.(*relationtuple.SubjectSet); isUserSet {
 		subTree := &Tree{
 			Type:    Union,

internal/expand/engine_test.go

@@ -287,4 +287,77 @@ func TestEngine(t *testing.T) {
 		require.NoError(t, err)
 		assert.Equal(t, expectedTree, tree)
 	})
+
+	t.Run("case=circular tuples", func(t *testing.T) {
+		sendlingerTor, odeonsplatz, centralStation, connected, namesp := "Sendlinger Tor", "Odeonsplatz", "Central Station", "connected", "munich transport"
+
+		sendlingerTorSS, odeonsplatzSS, centralStationSS := &relationtuple.SubjectSet{
+			Namespace: namesp,
+			Object:    sendlingerTor,
+			Relation:  connected,
+		}, &relationtuple.SubjectSet{
+			Namespace: namesp,
+			Object:    odeonsplatz,
+			Relation:  connected,
+		}, &relationtuple.SubjectSet{
+			Namespace: namesp,
+			Object:    centralStation,
+			Relation:  connected,
+		}
+
+		reg, e := newTestEngine(t, []*namespace.Namespace{{Name: namesp}})
+
+		expectedTree := &expand.Tree{
+			Type:    expand.Union,
+			Subject: sendlingerTorSS,
+			Children: []*expand.Tree{
+				{
+					Type:    expand.Union,
+					Subject: odeonsplatzSS,
+					Children: []*expand.Tree{
+						{
+							Type:    expand.Union,
+							Subject: centralStationSS,
+							Children: []*expand.Tree{
+								{
+									Type:     expand.Leaf,
+									Subject:  sendlingerTorSS,
+									Children: nil,
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+
+		require.NoError(t, reg.RelationTupleManager().WriteRelationTuples(context.Background(), []*relationtuple.InternalRelationTuple{
+			{
+				Namespace: namesp,
+				Object:    sendlingerTor,
+				Relation:  connected,
+				Subject:   odeonsplatzSS,
+			},
+			{
+				Namespace: namesp,
+				Object:    odeonsplatz,
+				Relation:  connected,
+				Subject:   centralStationSS,
+			},
+			{
+				Namespace: namesp,
+				Object:    centralStation,
+				Relation:  connected,
+				Subject:   sendlingerTorSS,
+			},
+		}...))
+
+		tree, err := e.BuildTree(context.Background(), &relationtuple.SubjectSet{
+			Namespace: namesp,
+			Object:    sendlingerTor,
+			Relation:  connected,
+		}, 100)
+		require.NoError(t, err)
+		assert.Equal(t, expectedTree, tree)
+	})
 }

internal/utils/graph_utils.go

@@ -0,0 +1,41 @@
+package utils
+
+import (
+	"context"
+)
+
+type (
+	EngineUtils interface {
+		CheckVisited(ctx context.Context, current string) (context.Context, bool)
+	}
+	EngineUtilsProvider struct {
+		EngineUtils
+	}
+	contextKey string
+)
+
+const visitedMapKey = contextKey("visitedMap")
+
+func (*EngineUtilsProvider) CheckVisited(ctx context.Context, current string) (context.Context, bool) {
+	visitedMap, ok := ctx.Value(visitedMapKey).(map[string]bool)
+	if !ok {
+		// for the first time initialize the map
+		visitedMap = make(map[string]bool)
+		visitedMap[current] = true
+		return context.WithValue(ctx, visitedMapKey, visitedMap), false
+	}
+
+	// check if current node was already visited
+	if visitedMap[current] {
+		return ctx, true
+	}
+
+	// set current entry to visited
+	visitedMap[current] = true
+
+	return context.WithValue(
+		ctx,
+		visitedMapKey,
+		visitedMap,
+	), false
+}

internal/utils/graph_utils_test.go

@@ -0,0 +1,44 @@
+package utils
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEngineUtilsProvider_CheckVisited(t *testing.T) {
+	t.Run("case=finds cycle", func(t *testing.T) {
+		ep := &EngineUtilsProvider{}
+
+		linkedList := []string{"A", "B", "C", "B", "D"}
+
+		ctx := context.Background()
+		var isThereACycle bool
+		for _, n := range linkedList {
+			ctx, isThereACycle = ep.CheckVisited(ctx, n)
+			if isThereACycle {
+				break
+			}
+		}
+
+		assert.Equal(t, isThereACycle, true)
+	})
+
+	t.Run("case=ignores if no cycle", func(t *testing.T) {
+		ep := &EngineUtilsProvider{}
+
+		list := []string{"A", "B", "C", "D"}
+
+		ctx := context.Background()
+		var isThereACycle bool
+		for _, n := range list {
+			ctx, isThereACycle = ep.CheckVisited(ctx, n)
+			if isThereACycle {
+				break
+			}
+		}
+
+		assert.Equal(t, isThereACycle, false)
+	})
+}