fix: update IterSubjects for wildcards and Alias iterators for confomance (#2864)

Author: barakmich

internal/datastore/memdb/readonly.go

@@ -505,15 +505,12 @@ func newSubjectSortedIterator(now time.Time, it memdb.ResultIterator, limit *uin
 			return nil, err
 		}
 
-		if rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) {
-			continue
-		}
-
 		if skipCaveats && rt.OptionalCaveat != nil {
 			continue
 		}
 
-		if skipExpiration && rt.OptionalExpiration != nil {
+		// Only check expiration if skipExpiration is false
+		if !skipExpiration && rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) {
 			continue
 		}
 
@@ -578,11 +575,8 @@ func newMemdbTupleIterator(now time.Time, it memdb.ResultIterator, limit *uint64
 				continue
 			}
 
-			if skipExpiration && rt.OptionalExpiration != nil {
-				continue
-			}
-
-			if rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) {
+			// Only check expiration if skipExpiration is false
+			if !skipExpiration && rt.OptionalExpiration != nil && rt.OptionalExpiration.Before(now) {
 				continue
 			}
 

pkg/query/alias.go

@@ -2,6 +2,9 @@ package query
 
 import (
 	"github.com/google/uuid"
+
+	"github.com/authzed/spicedb/pkg/datastore"
+	"github.com/authzed/spicedb/pkg/datastore/options"
 )
 
 // Alias is an iterator that rewrites the Resource's Relation field of all paths
@@ -24,60 +27,46 @@ func NewAlias(relation string, subIt Iterator) *Alias {
 	}
 }
 
-func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRelation) (PathSeq, error) {
-	// First, check for self-edge: if the object with internal relation matches the subject
-	for _, resource := range resources {
-		resourceWithAlias := resource.WithRelation(a.relation)
-		if resourceWithAlias.ObjectID == subject.ObjectID &&
-			resourceWithAlias.ObjectType == subject.ObjectType &&
-			resourceWithAlias.Relation == subject.Relation {
-			// Return the self-edge path first
-			selfPath := Path{
-				Resource: GetObject(resourceWithAlias),
-				Relation: resourceWithAlias.Relation,
-				Subject:  subject,
-				Metadata: make(map[string]any),
-			}
-
-			// Also get relations from sub-iterator
-			subSeq, err := ctx.Check(a.subIt, resources, subject)
-			if err != nil {
-				return nil, err
-			}
-
-			// Create combined sequence with self-edge and rewritten paths
-			combined := func(yield func(Path, error) bool) {
-				// Yield the self-edge first
-				if !yield(selfPath, nil) {
+// maybePrependSelfEdge checks if a self-edge should be added and combines it with the given sequence.
+// A self-edge is added when the resource with the alias relation matches the subject.
+func (a *Alias) maybePrependSelfEdge(resource Object, subSeq PathSeq, shouldAddSelfEdge bool) PathSeq {
+	if !shouldAddSelfEdge {
+		// No self-edge, just rewrite paths from sub-iterator
+		return func(yield func(Path, error) bool) {
+			for path, err := range subSeq {
+				if err != nil {
+					yield(Path{}, err)
 					return
 				}
 
-				// Then yield rewritten paths from sub-iterator
-				for path, err := range subSeq {
-					if err != nil {
-						yield(Path{}, err)
-						return
-					}
-
-					path.Relation = a.relation
-					if !yield(path, nil) {
-						return
-					}
+				path.Relation = a.relation
+				if !yield(path, nil) {
+					return
 				}
 			}
-
-			// Wrap with deduplication to handle duplicate paths after rewriting
-			return DeduplicatePathSeq(combined), nil
 		}
 	}
 
-	// No self-edge detected, just rewrite paths from sub-iterator
-	subSeq, err := ctx.Check(a.subIt, resources, subject)
-	if err != nil {
-		return nil, err
+	// Create a self-edge path
+	selfPath := Path{
+		Resource: resource,
+		Relation: a.relation,
+		Subject: ObjectAndRelation{
+			ObjectType: resource.ObjectType,
+			ObjectID:   resource.ObjectID,
+			Relation:   a.relation,
+		},
+		Metadata: make(map[string]any),
 	}
 
-	rewritten := func(yield func(Path, error) bool) {
+	// Combine self-edge with paths from sub-iterator
+	combined := func(yield func(Path, error) bool) {
+		// Yield the self-edge first
+		if !yield(selfPath, nil) {
+			return
+		}
+
+		// Then yield rewritten paths from sub-iterator
 		for path, err := range subSeq {
 			if err != nil {
 				yield(Path{}, err)
@@ -91,84 +80,111 @@ func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRel
 		}
 	}
 
-	// Wrap with deduplication to handle duplicate paths after rewriting
-	return DeduplicatePathSeq(rewritten), nil
+	// Wrap with deduplication to handle duplicate paths
+	return DeduplicatePathSeq(combined)
 }
 
-func (a *Alias) IterSubjectsImpl(ctx *Context, resource Object, filterSubjectType ObjectType) (PathSeq, error) {
-	subSeq, err := ctx.IterSubjects(a.subIt, resource, filterSubjectType)
+func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRelation) (PathSeq, error) {
+	// Get relations from sub-iterator
+	subSeq, err := ctx.Check(a.subIt, resources, subject)
 	if err != nil {
 		return nil, err
 	}
 
-	return func(yield func(Path, error) bool) {
-		for path, err := range subSeq {
-			if err != nil {
-				yield(Path{}, err)
-				return
-			}
-
-			path.Relation = a.relation
-			if !yield(path, nil) {
-				return
-			}
+	// Check for self-edge: if any resource with the alias relation matches the subject
+	for _, resource := range resources {
+		resourceWithAlias := resource.WithRelation(a.relation)
+		if resourceWithAlias.ObjectID == subject.ObjectID &&
+			resourceWithAlias.ObjectType == subject.ObjectType &&
+			resourceWithAlias.Relation == subject.Relation {
+			return a.maybePrependSelfEdge(GetObject(resourceWithAlias), subSeq, true), nil
 		}
-	}, nil
+	}
+
+	// No self-edge detected, just rewrite paths from sub-iterator
+	return DeduplicatePathSeq(a.maybePrependSelfEdge(Object{}, subSeq, false)), nil
 }
 
-func (a *Alias) IterResourcesImpl(ctx *Context, subject ObjectAndRelation, filterResourceType ObjectType) (PathSeq, error) {
-	subSeq, err := ctx.IterResources(a.subIt, subject, filterResourceType)
+func (a *Alias) IterSubjectsImpl(ctx *Context, resource Object, filterSubjectType ObjectType) (PathSeq, error) {
+	subSeq, err := ctx.IterSubjects(a.subIt, resource, filterSubjectType)
 	if err != nil {
 		return nil, err
 	}
 
-	// If the relation on the alias iterator is the same
-	// as the subject relation on the subject, we have
-	// a self edge and want to yield it accordingly.
-	if a.relation == subject.Relation {
-		selfPath := Path{
-			Resource: GetObject(subject),
-			Relation: a.relation,
-			Subject:  subject,
-		}
+	// Check if we should add a self-edge based on identity semantics.
+	// The dispatcher Check includes an identity check (see filterForFoundMemberResource
+	// in internal/graph/check.go): if the resource (with relation) matches the subject
+	// exactly, it returns MEMBER. This only applies if the resource actually appears
+	// as a subject in the data and the filter allows it.
+	shouldAddSelfEdge := a.shouldIncludeSelfEdge(ctx, resource, filterSubjectType)
 
-		combined := func(yield func(Path, error) bool) {
-			// Yield the self path first
-			if !yield(selfPath, nil) {
-				return
-			}
+	return a.maybePrependSelfEdge(resource, subSeq, shouldAddSelfEdge), nil
+}
 
-			for subPath, err := range subSeq {
-				if err != nil {
-					yield(Path{}, err)
-					return
-				}
+// shouldIncludeSelfEdge checks if a self-edge should be included for the given resource.
+// This matches the dispatcher's identity check behavior: if resource#relation appears as
+// a subject anywhere in the datastore (expired or not), and the filter allows it, we
+// include a self-edge in the results.
+func (a *Alias) shouldIncludeSelfEdge(ctx *Context, resource Object, filterSubjectType ObjectType) bool {
+	// First check: does the filter allow this resource type as a subject?
+	typeMatches := filterSubjectType.Type == "" || filterSubjectType.Type == resource.ObjectType
+	relationMatches := filterSubjectType.Subrelation == "" || filterSubjectType.Subrelation == a.relation
+	if !typeMatches || !relationMatches || ctx.Reader == nil {
+		return false
+	}
 
-				// Then yield remaining paths with the rewrite
-				subPath.Relation = a.relation
-				if !yield(subPath, nil) {
-					return
-				}
-			}
-		}
+	// Second check: does the resource actually appear as a subject in the data?
+	// We check for ANY relationships (expired or not) because the dispatcher's
+	// identity check applies regardless of expiration.
+	exists, err := a.resourceExistsAsSubject(ctx, resource)
+	if err != nil {
+		// On error, conservatively return false rather than failing the entire operation
+		return false
+	}
+	return exists
+}
 
-		return DeduplicatePathSeq(combined), nil
+// resourceExistsAsSubject queries the datastore to check if the given resource appears
+// as a subject in any relationship, including expired relationships.
+func (a *Alias) resourceExistsAsSubject(ctx *Context, resource Object) (bool, error) {
+	filter := datastore.RelationshipsFilter{
+		OptionalSubjectsSelectors: []datastore.SubjectsSelector{{
+			OptionalSubjectType: resource.ObjectType,
+			OptionalSubjectIds:  []string{resource.ObjectID},
+			RelationFilter:      datastore.SubjectRelationFilter{}.WithNonEllipsisRelation(a.relation),
+		}},
+		OptionalExpirationOption: datastore.ExpirationFilterOptionNone,
 	}
 
-	// else we don't have a subpath and do the normal logic
-	return func(yield func(Path, error) bool) {
-		for path, err := range subSeq {
-			if err != nil {
-				yield(Path{}, err)
-				return
-			}
+	iter, err := ctx.Reader.QueryRelationships(ctx, filter,
+		options.WithLimit(options.LimitOne),
+		options.WithSkipExpiration(true)) // Include expired relationships
+	if err != nil {
+		return false, err
+	}
 
-			path.Relation = a.relation
-			if !yield(path, nil) {
-				return
-			}
+	// Check if any relationship exists
+	for _, err := range iter {
+		if err != nil {
+			return false, err
 		}
-	}, nil
+		return true, nil
+	}
+
+	return false, nil
+}
+
+func (a *Alias) IterResourcesImpl(ctx *Context, subject ObjectAndRelation, filterResourceType ObjectType) (PathSeq, error) {
+	subSeq, err := ctx.IterResources(a.subIt, subject, filterResourceType)
+	if err != nil {
+		return nil, err
+	}
+
+	// If the relation on the alias iterator is the same as the subject relation,
+	// we have a self-edge and should yield it
+	shouldAddSelfEdge := a.relation == subject.Relation
+
+	return a.maybePrependSelfEdge(GetObject(subject), subSeq, shouldAddSelfEdge), nil
 }
 
 func (a *Alias) Clone() Iterator {

pkg/query/datastore.go

@@ -231,13 +231,18 @@ func (r *RelationIterator) iterSubjectsWildcardImpl(ctx *Context, resource Objec
 		return EmptyPathSeq(), nil
 	}
 
-	// Wildcard exists, so enumerate all concrete subjects for this resource.
-	// Note: This may return some of the same subjects as the non-wildcard branch
-	// (when both wildcard and concrete relationships exist), but the Union will
-	// deduplicate them.
+	// Wildcard exists, so enumerate all concrete subjects of the appropriate type.
+	// A wildcard (e.g., user:*) means "all subjects of that type", so we need to enumerate
+	// all defined subjects of that type in the datastore. This may return some of the same
+	// subjects as the non-wildcard branch (when both wildcard and concrete relationships exist),
+	// but the Union will deduplicate them.
+	//
+	// Note: We query for all subjects of the appropriate type, not just those with a relationship
+	// to this specific resource. This matches the semantics of wildcards, which grant access to
+	// ALL subjects of the type, regardless of whether they have other relationships.
 	allSubjectsFilter := datastore.RelationshipsFilter{
-		OptionalResourceType: r.base.DefinitionName(),
-		OptionalResourceIds:  []string{resource.ObjectID},
+		// Note: We intentionally omit OptionalResourceType and OptionalResourceIds to find
+		// all subjects of the appropriate type across all resources
 		OptionalSubjectsSelectors: []datastore.SubjectsSelector{
 			{
 				OptionalSubjectType: r.base.Type(),

pkg/query/recursive.go

@@ -4,8 +4,6 @@ import (
 	"fmt"
 
 	"github.com/google/uuid"
-
-	"github.com/authzed/spicedb/pkg/tuple"
 )
 
 const defaultMaxRecursionDepth = 50
@@ -394,15 +392,15 @@ func (r *RecursiveIterator) isRecursiveSubject(subject ObjectAndRelation) bool {
 		return false
 	}
 
-	// Must match the relation or be ellipsis/empty
-	// Empty relation means the subject reference doesn't specify a relation
-	// Ellipsis means "any relation on this object"
-	if subject.Relation != r.relationName &&
-		subject.Relation != "" &&
-		subject.Relation != tuple.Ellipsis {
-		return false
-	}
-
+	// For IterSubjects, we should recursively expand any subject of the same type,
+	// regardless of its specific relation. This is because:
+	// 1. Permissions can include other relations (e.g., member = direct_member + contributor + manager)
+	// 2. When querying engineering#direct_member, we may find applications#member as a subject,
+	//    and we need to expand it to find transitive subjects
+	// 3. The specific relation on the subject determines what we query on that subject,
+	//    but doesn't determine whether it should be expanded
+	//
+	// Note: Empty relation means no relation specified, ellipsis means "any relation"
 	return true
 }