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/x/graph"
+
 	"github.com/ory/herodot"
 
 	"github.com/ory/keto/internal/relationtuple"
@@ -35,6 +37,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 := graph.CheckAndAddVisited(ctx, sr.Subject)
+		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

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/ory/keto/internal/x"
+	"github.com/ory/keto/internal/x/graph"
 
 	"github.com/ory/keto/internal/relationtuple"
 )
@@ -21,7 +22,9 @@ type (
 )
 
 func NewEngine(d EngineDependencies) *Engine {
-	return &Engine{d: d}
+	return &Engine{
+		d: d,
+	}
 }
 
 func (e *Engine) BuildTree(ctx context.Context, subject relationtuple.Subject, restDepth int) (*Tree, error) {
@@ -30,6 +33,11 @@ func (e *Engine) BuildTree(ctx context.Context, subject relationtuple.Subject, r
 	}
 
 	if us, isUserSet := subject.(*relationtuple.SubjectSet); isUserSet {
+		ctx, wasAlreadyVisited := graph.CheckAndAddVisited(ctx, subject)
+		if wasAlreadyVisited {
+			return nil, nil
+		}
+
 		subTree := &Tree{
 			Type:    Union,
 			Subject: subject,

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/x/graph/graph_utils.go

@@ -0,0 +1,35 @@
+package graph
+
+import (
+	"context"
+
+	"github.com/ory/keto/internal/relationtuple"
+)
+
+type contextKey string
+
+const visitedMapKey = contextKey("visitedMap")
+
+func CheckAndAddVisited(ctx context.Context, current relationtuple.Subject) (context.Context, bool) {
+	visitedMap, ok := ctx.Value(visitedMapKey).(map[string]struct{})
+	if !ok {
+		// for the first time initialize the map
+		visitedMap = make(map[string]struct{})
+		visitedMap[current.String()] = struct{}{}
+		return context.WithValue(ctx, visitedMapKey, visitedMap), false
+	}
+
+	// check if current node was already visited
+	if _, ok := visitedMap[current.String()]; ok {
+		return ctx, true
+	}
+
+	// set current entry to visited
+	visitedMap[current.String()] = struct{}{}
+
+	return context.WithValue(
+		ctx,
+		visitedMapKey,
+		visitedMap,
+	), false
+}

internal/x/graph/graph_utils_test.go

@@ -0,0 +1,84 @@
+package graph
+
+import (
+	"context"
+	"testing"
+
+	"github.com/ory/keto/internal/relationtuple"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEngineUtilsProvider_CheckVisited(t *testing.T) {
+	t.Run("case=finds cycle", func(t *testing.T) {
+
+		linkedList := []relationtuple.SubjectSet{{
+			Namespace: "default",
+			Object:    "A",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "B",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "C",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "B",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "D",
+			Relation:  "connected",
+		}}
+
+		ctx := context.Background()
+		var isThereACycle bool
+		for i := range linkedList {
+			ctx, isThereACycle = CheckAndAddVisited(ctx, &linkedList[i])
+			if isThereACycle {
+				break
+			}
+		}
+
+		assert.Equal(t, isThereACycle, true)
+	})
+
+	t.Run("case=ignores if no cycle", func(t *testing.T) {
+
+		list := []relationtuple.SubjectSet{{
+			Namespace: "default",
+			Object:    "A",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "B",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "C",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "D",
+			Relation:  "connected",
+		}, {
+			Namespace: "default",
+			Object:    "E",
+			Relation:  "connected",
+		}}
+
+		ctx := context.Background()
+		var isThereACycle bool
+		for i := range list {
+			ctx, isThereACycle = CheckAndAddVisited(ctx, &list[i])
+			if isThereACycle {
+				break
+			}
+		}
+
+		assert.Equal(t, isThereACycle, false)
+	})
+}