From a1ca45f027a916c4cda1d5ee7b729e22e4ada9c8 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Mon, 6 May 2024 15:18:52 -0600 Subject: [PATCH 01/22] feat(list-users): ListUsers with Excluded Users --- assets/tests/consolidated_1_1_tests.yaml | 4 +- internal/test/listusers/listusers.go | 24 +- pkg/server/commands/listusers/list_users.go | 12 +- .../commands/listusers/list_users_rpc.go | 247 +++++++++++++--- .../commands/listusers/list_users_rpc_test.go | 272 +++++++++++++++--- pkg/server/list_users.go | 3 +- pkg/server/list_users_test.go | 57 +++- pkg/tuple/tuple.go | 41 +++ pkg/tuple/tuple_test.go | 78 +++++ tests/listusers/listusers.go | 4 +- 10 files changed, 639 insertions(+), 103 deletions(-) diff --git a/assets/tests/consolidated_1_1_tests.yaml b/assets/tests/consolidated_1_1_tests.yaml index adce082d70..ef5c52ad51 100644 --- a/assets/tests/consolidated_1_1_tests.yaml +++ b/assets/tests/consolidated_1_1_tests.yaml @@ -6517,7 +6517,9 @@ tests: object: document:1 relation: can_view expectation: - - user:* # TODO but user:bob doesn't have access + - user:* + expectedExcludedUsers: + - user:bob - request: filters: - user diff --git a/internal/test/listusers/listusers.go b/internal/test/listusers/listusers.go index e35e4de373..19f47cbfae 100644 --- a/internal/test/listusers/listusers.go +++ b/internal/test/listusers/listusers.go @@ -11,11 +11,12 @@ import ( ) type Assertion struct { - Request *TestListUsersRequest - ContextualTuples []*openfgav1.TupleKey `json:"contextualTuples"` - Context *structpb.Struct - Expectation []string - ErrorCode int `json:"errorCode"` // If ErrorCode is non-zero then we expect that the ListUsers call failed. + Request *TestListUsersRequest + ContextualTuples []*openfgav1.TupleKey `json:"contextualTuples"` + Context *structpb.Struct + Expectation []string + ExpectedExcludedUsers []string `json:"expectedExcludedUsers"` + ErrorCode int `json:"errorCode"` // If ErrorCode is non-zero then we expect that the ListUsers call failed. } type TestListUsersRequest struct { @@ -28,14 +29,23 @@ func (t *TestListUsersRequest) ToString() string { return fmt.Sprintf("object=%s, relation=%s, filters=%v", t.Object, t.Relation, strings.Join(t.Filters, ", ")) } -func FromProtoResponse(r *openfgav1.ListUsersResponse) []string { +func FromUsersProto(r []*openfgav1.User) []string { var users []string - for _, user := range r.GetUsers() { + for _, user := range r { users = append(users, tuple.UserProtoToString(user)) } return users } +func FromObjectOrUsersetProto(u []*openfgav1.ObjectOrUserset) []string { + var users []string + for _, user := range u { + users = append(users, tuple.FromObjectOrUsersetProto(user)) + } + + return users +} + func (t *TestListUsersRequest) ToProtoRequest() *openfgav1.ListUsersRequest { var protoFilters []*openfgav1.UserTypeFilter diff --git a/pkg/server/commands/listusers/list_users.go b/pkg/server/commands/listusers/list_users.go index 3228c68b72..6bf3466922 100644 --- a/pkg/server/commands/listusers/list_users.go +++ b/pkg/server/commands/listusers/list_users.go @@ -95,8 +95,9 @@ func (r *internalListUsersRequest) GetContext() *structpb.Struct { } type listUsersResponse struct { - Users []*openfgav1.User - Metadata listUsersResponseMetadata + Users []*openfgav1.User + ExcludedUsers []*openfgav1.ObjectOrUserset + Metadata listUsersResponseMetadata } type listUsersResponseMetadata struct { @@ -110,6 +111,13 @@ func (r *listUsersResponse) GetUsers() []*openfgav1.User { return r.Users } +func (r *listUsersResponse) GetExcludedUsers() []*openfgav1.ObjectOrUserset { + if r == nil { + return []*openfgav1.ObjectOrUserset{} + } + return r.ExcludedUsers +} + func (r *listUsersResponse) GetMetadata() listUsersResponseMetadata { if r == nil { return listUsersResponseMetadata{} diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 85af7e0fec..7255742455 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "maps" "sync" "sync/atomic" "time" @@ -47,6 +48,12 @@ type expandResponse struct { err error } +type foundUser struct { + user *openfgav1.User + excludedUsers []*openfgav1.User + noRelationship bool +} + type ListUsersQueryOption func(l *listUsersQuery) func WithListUsersQueryLogger(l logger.Logger) ListUsersQueryOption { @@ -126,11 +133,12 @@ func (l *listUsersQuery) ListUsers( defer span.End() cancellableCtx, cancelCtx := context.WithCancel(ctx) - defer cancelCtx() if l.deadline != 0 { cancellableCtx, cancelCtx = context.WithTimeout(cancellableCtx, l.deadline) defer cancelCtx() } + defer cancelCtx() + l.ds = storagewrappers.NewCombinedTupleReader( storagewrappers.NewBoundedConcurrencyTupleReader(l.ds, l.maxConcurrentReads), req.GetContextualTuples(), @@ -164,11 +172,17 @@ func (l *listUsersQuery) ListUsers( foundUsersCh := l.buildResultsChannel() expandErrCh := make(chan error, 1) - foundUsersUnique := make(map[tuple.UserString]struct{}, 1000) + foundUsersUnique := make(map[tuple.UserString]foundUser, 1000) + excludedUsersUnique := make(map[tuple.UserString]struct{}, 1000) + doneWithFoundUsersCh := make(chan struct{}, 1) go func() { - for foundObject := range foundUsersCh { - foundUsersUnique[tuple.UserProtoToString(foundObject)] = struct{}{} + for foundUser := range foundUsersCh { + foundUsersUnique[tuple.UserProtoToString(foundUser.user)] = foundUser + for _, exception := range foundUser.excludedUsers { + excludedUsersUnique[tuple.UserProtoToString(exception)] = struct{}{} + } + if l.maxResults > 0 { if uint32(len(foundUsersUnique)) >= l.maxResults { span.SetAttributes(attribute.Bool("max_results_found", true)) @@ -205,13 +219,30 @@ func (l *listUsersQuery) ListUsers( cancelCtx() foundUsers := make([]*openfgav1.User, 0, len(foundUsersUnique)) - for foundUser := range foundUsersUnique { - foundUsers = append(foundUsers, tuple.StringToUserProto(foundUser)) + for foundUserKey, foundUser := range foundUsersUnique { + _, userIsExcluded := excludedUsersUnique[foundUserKey] + if userIsExcluded { + continue + } + + if !foundUser.noRelationship { + foundUsers = append(foundUsers, tuple.StringToUserProto(foundUserKey)) + } + } + + var excludedUsers []*openfgav1.ObjectOrUserset + if len(foundUsers) > 0 { + excludedUsers = make([]*openfgav1.ObjectOrUserset, 0, len(excludedUsersUnique)) + for foundExcludedUser := range excludedUsersUnique { + excludedUsers = append(excludedUsers, tuple.StringToObjectOrUserset(foundExcludedUser)) + } } + span.SetAttributes(attribute.Int("result_count", len(foundUsers))) return &listUsersResponse{ - Users: foundUsers, + Users: foundUsers, + ExcludedUsers: excludedUsers, Metadata: listUsersResponseMetadata{ DatastoreQueryCount: datastoreQueryCount.Load(), }, @@ -237,8 +268,9 @@ func doesHavePossibleEdges(typesys *typesystem.TypeSystem, req *openfgav1.ListUs func (l *listUsersQuery) expand( ctx context.Context, req *internalListUsersRequest, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { + ctx, span := tracer.Start(ctx, "expand") defer span.End() span.SetAttributes(attribute.Int("depth", int(req.depth))) @@ -262,16 +294,17 @@ func (l *listUsersQuery) expand( for _, userFilter := range req.GetUserFilters() { if reqObjectType == userFilter.GetType() && reqRelation == userFilter.GetRelation() { - user := &openfgav1.User{ - User: &openfgav1.User_Userset{ - Userset: &openfgav1.UsersetUser{ - Type: reqObjectType, - Id: reqObjectID, - Relation: reqRelation, + trySendResult(ctx, foundUser{ + user: &openfgav1.User{ + User: &openfgav1.User_Userset{ + Userset: &openfgav1.UsersetUser{ + Type: reqObjectType, + Id: reqObjectID, + Relation: reqRelation, + }, }, }, - } - trySendResult(ctx, user, foundUsersChan) + }, foundUsersChan) } } @@ -304,7 +337,7 @@ func (l *listUsersQuery) expandRewrite( ctx context.Context, req *internalListUsersRequest, rewrite *openfgav1.Userset, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { ctx, span := tracer.Start(ctx, "expandRewrite") defer span.End() @@ -351,7 +384,7 @@ func (l *listUsersQuery) expandRewrite( func (l *listUsersQuery) expandDirect( ctx context.Context, req *internalListUsersRequest, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { ctx, span := tracer.Start(ctx, "expandDirect") defer span.End() @@ -427,7 +460,10 @@ LoopOnIterator: for _, f := range req.GetUserFilters() { if f.GetType() == userObjectType { user := tuple.StringToUserProto(tuple.BuildObject(userObjectType, userObjectID)) - trySendResult(ctx, user, foundUsersChan) + + trySendResult(ctx, foundUser{ + user: user, + }, foundUsersChan) } } continue @@ -459,7 +495,7 @@ func (l *listUsersQuery) expandIntersection( ctx context.Context, req *internalListUsersRequest, rewrite *openfgav1.Userset_Intersection, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { ctx, span := tracer.Start(ctx, "expandIntersection") defer span.End() @@ -468,11 +504,11 @@ func (l *listUsersQuery) expandIntersection( pool.WithMaxGoroutines(int(l.resolveNodeBreadthLimit)) childOperands := rewrite.Intersection.GetChild() - intersectionFoundUsersChans := make([]chan *openfgav1.User, len(childOperands)) + intersectionFoundUsersChans := make([]chan foundUser, len(childOperands)) for i, rewrite := range childOperands { i := i rewrite := rewrite - intersectionFoundUsersChans[i] = make(chan *openfgav1.User, 1) + intersectionFoundUsersChans[i] = make(chan foundUser, 1) pool.Go(func(ctx context.Context) error { resp := l.expandRewrite(ctx, req, rewrite, intersectionFoundUsersChans[i]) return resp.err @@ -499,11 +535,15 @@ func (l *listUsersQuery) expandIntersection( wildcardKey := tuple.TypedPublicWildcard(req.GetUserFilters()[0].GetType()) foundUsersCountMap := make(map[string]uint32, 0) for _, foundUsersChan := range intersectionFoundUsersChans { - go func(foundUsersChan chan *openfgav1.User) { + go func(foundUsersChan chan foundUser) { defer wg.Done() foundUsersMap := make(map[string]uint32, 0) for foundUser := range foundUsersChan { - key := tuple.UserProtoToString(foundUser) + if foundUser.noRelationship { + continue + } + + key := tuple.UserProtoToString(foundUser.user) foundUsersMap[key]++ } @@ -533,7 +573,10 @@ func (l *listUsersQuery) expandIntersection( // If this summed value equals the number of operands, the user satisfies // the intersection expression and can be sent on `foundUsersChan` if (count + wildcardCount.Load()) == uint32(len(childOperands)) { - trySendResult(ctx, tuple.StringToUserProto(key), foundUsersChan) + fu := foundUser{ + user: tuple.StringToUserProto(key), + } + trySendResult(ctx, fu, foundUsersChan) } } @@ -546,12 +589,12 @@ func (l *listUsersQuery) expandExclusion( ctx context.Context, req *internalListUsersRequest, rewrite *openfgav1.Userset_Difference, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { ctx, span := tracer.Start(ctx, "expandExclusion") defer span.End() - baseFoundUsersCh := make(chan *openfgav1.User, 1) - subtractFoundUsersCh := make(chan *openfgav1.User, 1) + baseFoundUsersCh := make(chan foundUser, 1) + subtractFoundUsersCh := make(chan foundUser, 1) var baseError error go func() { @@ -569,15 +612,22 @@ func (l *listUsersQuery) expandExclusion( close(subtractFoundUsersCh) }() - baseFoundUsersMap := make(map[string]struct{}, 0) + baseFoundUsersMap := make(map[string]foundUser, 0) for fu := range baseFoundUsersCh { - key := tuple.UserProtoToString(fu) - baseFoundUsersMap[key] = struct{}{} + key := tuple.UserProtoToString(fu.user) + baseFoundUsersMap[key] = fu } - subtractFoundUsersMap := make(map[string]struct{}, len(baseFoundUsersMap)) + + subtractFoundUsersMap := make(map[string]foundUser, len(baseFoundUsersMap)) + negatedSubtractionMap := make(map[string]struct{}) + for fu := range subtractFoundUsersCh { - key := tuple.UserProtoToString(fu) - subtractFoundUsersMap[key] = struct{}{} + for _, s := range fu.excludedUsers { + negatedSubtractionMap[tuple.UserProtoToString(s)] = struct{}{} + } + + key := tuple.UserProtoToString(fu.user) + subtractFoundUsersMap[key] = fu } if subtractHasCycle { @@ -592,14 +642,116 @@ func (l *listUsersQuery) expandExclusion( } wildcardKey := tuple.TypedPublicWildcard(req.GetUserFilters()[0].GetType()) - _, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] - for key := range baseFoundUsersMap { - if _, isSubtracted := subtractFoundUsersMap[key]; !isSubtracted && !subtractWildcardExists { - // Iterate over base users because at minimum they need to pass - // but then they are further compared to the subtracted users map. - // If users exist in both maps, they are excluded. Only users that exist - // solely in the base map will be returned. - trySendResult(ctx, tuple.StringToUserProto(key), foundUsersChan) + + _, baseWildcardExists := baseFoundUsersMap[wildcardKey] + subtractWildcardUser, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] + + if baseWildcardExists { + if !subtractWildcardExists || subtractWildcardUser.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(wildcardKey), + }, foundUsersChan) + } + + residualSubtractedUserMap := maps.Clone(subtractFoundUsersMap) + + for userKey := range baseFoundUsersMap { + _, userIsSubtracted := subtractFoundUsersMap[userKey] + _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] + + if !userIsSubtracted && !wildcardSubtracted { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + }, foundUsersChan) + + delete(residualSubtractedUserMap, userKey) + } + } + + for userKey, fu := range residualSubtractedUserMap { + excludedUsers := map[string]struct{}{} + if tuple.IsTypedWildcard(userKey) { + for _, excludedUser := range fu.excludedUsers { + excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} + + trySendResult(ctx, foundUser{ + user: excludedUser, + noRelationship: false, + }, foundUsersChan) + } + + if !fu.noRelationship { + continue + } + } + + if _, userIsExcluded := excludedUsers[userKey]; !userIsExcluded { + if fu.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: false, + }, foundUsersChan) + } else { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: true, + excludedUsers: []*openfgav1.User{ + tuple.StringToUserProto(userKey), + }, + }, foundUsersChan) + } + } + } + } else if subtractWildcardExists { + for userKey := range baseFoundUsersMap { + if !tuple.IsTypedWildcard(userKey) { + subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] + if (userIsSubtracted && subtractedUser.noRelationship) || subtractWildcardUser.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: false, + }, foundUsersChan) + + continue + } + + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: true, + }, foundUsersChan) + + } + } + } else { + for key, fu := range baseFoundUsersMap { + subtractedUser, userIsSubtracted := subtractFoundUsersMap[key] + + if userIsSubtracted { + if subtractedUser.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(key), + noRelationship: false, + }, foundUsersChan) + + continue + } else { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(key), + noRelationship: true, + }, foundUsersChan) + + continue + } + } else { + if !fu.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(key), + noRelationship: false, + }, foundUsersChan) + } + + continue + } } } @@ -616,7 +768,7 @@ func (l *listUsersQuery) expandTTU( ctx context.Context, req *internalListUsersRequest, rewrite *openfgav1.Userset_TupleToUserset, - foundUsersChan chan<- *openfgav1.User, + foundUsersChan chan<- foundUser, ) expandResponse { ctx, span := tracer.Start(ctx, "expandTTU") defer span.End() @@ -717,16 +869,17 @@ func enteredCycle(req *internalListUsersRequest) bool { return false } -func (l *listUsersQuery) buildResultsChannel() chan *openfgav1.User { - foundUsersCh := make(chan *openfgav1.User, serverconfig.DefaultListUsersMaxResults) +func (l *listUsersQuery) buildResultsChannel() chan foundUser { + foundUsersCh := make(chan foundUser, serverconfig.DefaultListUsersMaxResults) maxResults := l.maxResults if maxResults > 0 { - foundUsersCh = make(chan *openfgav1.User, maxResults) + foundUsersCh = make(chan foundUser, maxResults) } + return foundUsersCh } -func trySendResult(ctx context.Context, user *openfgav1.User, foundUsersCh chan<- *openfgav1.User) { +func trySendResult(ctx context.Context, user foundUser, foundUsersCh chan<- foundUser) { select { case <-ctx.Done(): return diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index c7723aa0d7..097861ba6b 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -31,11 +31,46 @@ type ListUsersTests []struct { model string tuples []*openfgav1.TupleKey expectedUsers []string + butNot []string expectedErrorMsg string } const maximumRecursiveDepth = 25 +func TestBlah(t *testing.T) { + tests := ListUsersTests{ + { + name: "blah_1", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{ + Type: "document", + Id: "1", + }, + Relation: "can_view", + UserFilters: []*openfgav1.UserTypeFilter{ + {Type: "user"}, + }, + }, + model: ` + model + schema 1.1 + type user + type document + relations + define restricted: [user] + define viewer: [user:*] but not restricted + define can_view: viewer`, + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "restricted", "user:bob"), + }, + expectedUsers: []string{"user:*"}, + butNot: []string{"user:bob"}, + }, + } + tests.runListUsersTestCases(t) +} + func TestListUsersDirectRelationship(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) @@ -504,12 +539,12 @@ func TestListUsersUsersets(t *testing.T) { model: `model schema 1.1 type user - + type org relations define member: [user] define admin: [org#member] - + type repo relations define owner: [org] @@ -536,7 +571,7 @@ func TestListUsersUsersets(t *testing.T) { model: `model schema 1.1 type user - + type org relations define member: [user] @@ -627,14 +662,14 @@ func TestListUsersTTU(t *testing.T) { model: `model schema 1.1 type user - + type folder relations define owner: [user] define editor: [user] or owner define viewer: [user] or owner or editor define unrelated_not_computed: [user] - + type document relations define parent: [folder] @@ -665,8 +700,8 @@ func TestListUsersTTU(t *testing.T) { }, model: `model schema 1.1 - type user - type folder + type user + type folder relations define parent: [folder] define viewer: [user] or viewer from parent`, @@ -1029,7 +1064,7 @@ func TestListUsersConditions(t *testing.T) { condition isEqualToFive(param1: int) { param1 == 5 } - + condition isEqualToTen(param2: int) { param2 == 10 }`, @@ -1063,7 +1098,7 @@ func TestListUsersConditions(t *testing.T) { condition isEqualToFive(param1: int) { param1 == 5 } - + condition isEqualToTen(param2: int) { param2 == 10 }`, @@ -1465,13 +1500,13 @@ func TestListUsersExclusion(t *testing.T) { }, model: `model schema 1.1 - + type org relations define blocked: [user] - - type user - + + type user + type document relations define parent: [org] @@ -1502,19 +1537,19 @@ func TestListUsersExclusion(t *testing.T) { }, model: `model schema 1.1 - + type user - + type org relations define blocked: [user] - + type folder relations define blocked: blocked from org define org: [org] define viewer: [user] - + type document relations define parent: [folder] @@ -1545,9 +1580,9 @@ func TestListUsersExclusion(t *testing.T) { }, model: `model schema 1.1 - + type user - + type group relations define member: [user, group#member] but not blocked @@ -1603,9 +1638,9 @@ func TestListUsersExclusion(t *testing.T) { }, model: `model schema 1.1 - + type user - + type document relations define blocked: [user, document#viewer] @@ -1628,9 +1663,9 @@ func TestListUsersExclusionWildcards(t *testing.T) { model := `model schema 1.1 - + type user - + type document relations define blocked: [user:*,user] @@ -1711,6 +1746,7 @@ func TestListUsersExclusionWildcards(t *testing.T) { tuple.NewTupleKey("document:1", "blocked", "user:maria"), }, expectedUsers: []string{"user:*"}, + butNot: []string{"user:maria"}, }, { name: "exclusion_and_wildcards_5", @@ -1742,15 +1778,24 @@ func TestListUsersExclusionWildcards(t *testing.T) { }, }, }, - model: model, + model: `model + schema 1.1 + + type user + + type document + relations + define blocked: [user:*,user] + define viewer: [user:*,user] but not blocked`, tuples: []*openfgav1.TupleKey{ - tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "viewer", "user:*"), // base wildcard tuple.NewTupleKey("document:1", "viewer", "user:maria"), tuple.NewTupleKey("document:1", "viewer", "user:jon"), tuple.NewTupleKey("document:1", "blocked", "user:jon"), tuple.NewTupleKey("document:1", "blocked", "user:will"), }, expectedUsers: []string{"user:*", "user:maria"}, + butNot: []string{"user:jon", "user:will"}, }, } tests.runListUsersTestCases(t) @@ -1919,7 +1964,7 @@ func TestListUsersEdgePruning(t *testing.T) { type user type document - relations + relations define viewer: [user]`, tuples: []*openfgav1.TupleKey{ tuple.NewTupleKey("document:1", "viewer", "user:maria"), @@ -2279,13 +2324,13 @@ func TestListUsersWildcardsAndIntersection(t *testing.T) { }, model: `model schema 1.1 - + type user - + type group relations define member: [user:*, user] - + type document relations define group: [group] @@ -2321,7 +2366,7 @@ func TestListUsersCycleDetection(t *testing.T) { l := NewListUsersQuery(mockDatastore, WithResolveNodeLimit(maximumRecursiveDepth)) channelDone := make(chan struct{}) - channelWithResults := make(chan *openfgav1.User) + channelWithResults := make(chan foundUser) channelWithError := make(chan error, 1) model := testutils.MustTransformDSLToProtoWithID(` model @@ -2378,6 +2423,153 @@ func TestListUsersCycleDetection(t *testing.T) { }) } +func TestListUsersChainedNegation(t *testing.T) { + t.Cleanup(func() { + goleak.VerifyNone(t) + }) + + model := `model + schema 1.1 + type user + type document + relations + define viewer: [user, user:*] but not blocked + define blocked: [user, user:*] but not unblocked + define unblocked: [user, user:*]` + + tests := ListUsersTests{ + { + name: "chained_negation_1", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_2", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_3", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_4", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:*"}, + }, + { + name: "chained_negation_5", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:*"}, + }, + { + name: "chained_negation_6", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_7", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_8", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:maria"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_9", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_10", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:*", "user:jon"}, + butNot: []string{"user:maria"}, + }, + { + name: "chained_negation_11", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + }, + expectedUsers: []string{"user:jon"}, + }, + { + name: "chained_negation_12", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "blocked", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:*", "user:jon"}, + }, + { + name: "chained_negation_13", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "viewer", "user:jon"), + tuple.NewTupleKey("document:1", "viewer", "user:maria"), + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:*", "user:jon", "user:maria"}, + }, + } + + for i := range tests { + tests[i].model = model + tests[i].req = &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{{Type: "user"}}, + } + } + + tests.runListUsersTestCases(t) +} + func TestListUsersDepthExceeded(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) @@ -2507,7 +2699,7 @@ func TestListUsersStorageErrors(t *testing.T) { model schema 1.1 type user - + type document relations define a: [user] @@ -2563,13 +2755,18 @@ func (testCases ListUsersTests) runListUsersTestCases(t *testing.T) { require.Equal(t, test.expectedErrorMsg, actualErrorMsg) actualUsers := resp.GetUsers() - actualCompare := make([]string, len(actualUsers)) - for i, u := range resp.GetUsers() { + for i, u := range actualUsers { actualCompare[i] = tuple.UserProtoToString(u) } - require.ElementsMatch(t, actualCompare, test.expectedUsers) + + exceptUsers := resp.GetExcludedUsers() + actualCompare = make([]string, len(exceptUsers)) + for i, u := range exceptUsers { + actualCompare[i] = tuple.FromObjectOrUsersetProto(u) + } + require.ElementsMatch(t, actualCompare, test.butNot) }) } } @@ -2705,15 +2902,15 @@ func TestListUsersDatastoreQueryCount(t *testing.T) { model := parser.MustTransformDSLToProto(`model schema 1.1 type user - + type company relations define member: [user] - + type org relations define member: [user] - + type document relations define wildcard: [user:*] @@ -3108,7 +3305,6 @@ func TestListUsersConfig_Deadline(t *testing.T) { allResults: []*openfgav1.User{ {User: &openfgav1.User_Object{Object: &openfgav1.Object{Type: "user", Id: "1"}}}, }, - expectError: "context deadline exceeded", }, `deadline_very_high_returns_everything`: { inputModel: ` diff --git a/pkg/server/list_users.go b/pkg/server/list_users.go index 6c7d8b02ce..eb583e1d0a 100644 --- a/pkg/server/list_users.go +++ b/pkg/server/list_users.go @@ -92,7 +92,8 @@ func (s *Server) ListUsers( ).Observe(datastoreQueryCount) return &openfgav1.ListUsersResponse{ - Users: resp.GetUsers(), + Users: resp.GetUsers(), + ExcludedUsers: resp.GetExcludedUsers(), }, nil } diff --git a/pkg/server/list_users_test.go b/pkg/server/list_users_test.go index 042500a889..fb31c4a39e 100644 --- a/pkg/server/list_users_test.go +++ b/pkg/server/list_users_test.go @@ -181,7 +181,15 @@ func TestModelIdNotFound(t *testing.T) { ctx := context.Background() req := &openfgav1.ListUsersRequest{ - StoreId: "some-store-id", + StoreId: ulid.Make().String(), + Object: &openfgav1.Object{ + Type: "document", + Id: "1", + }, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + {Type: "user"}, + }, } mockController := gomock.NewController(t) @@ -208,13 +216,23 @@ func TestModelIdNotFound(t *testing.T) { func TestExperimentalListUsers(t *testing.T) { ctx := context.Background() - req := &openfgav1.ListUsersRequest{} + storeID := ulid.Make().String() + req := &openfgav1.ListUsersRequest{ + StoreId: storeID, + Object: &openfgav1.Object{ + Type: "document", + Id: "1", + }, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + {Type: "user"}, + }, + } mockController := gomock.NewController(t) defer mockController.Finish() mockDatastore := mockstorage.NewMockOpenFGADatastore(mockController) - mockDatastore.EXPECT().FindLatestAuthorizationModel(gomock.Any(), gomock.Any()).Return(nil, storage.ErrNotFound) // error demonstrates that main code path is reached server := MustNewServerWithOpts( WithDatastore(mockDatastore), @@ -231,12 +249,39 @@ func TestExperimentalListUsers(t *testing.T) { require.Equal(t, codes.Unimplemented, e.Code()) }) - t.Run("list_users_does_not_error_if_experimentally_enabled", func(t *testing.T) { + t.Run("list_users_returns_error_if_latest_model_not_found", func(t *testing.T) { + mockDatastore.EXPECT().FindLatestAuthorizationModel(gomock.Any(), gomock.Any()).Return(nil, storage.ErrNotFound) // error demonstrates that main code path is reached + server.experimentals = []ExperimentalFeatureFlag{ExperimentalEnableListUsers} _, err := server.ListUsers(ctx, req) - require.Error(t, err) - require.Equal(t, "rpc error: code = Code(2020) desc = No authorization models found for store ''", err.Error()) + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.Code(openfgav1.ErrorCode_latest_authorization_model_not_found), st.Code()) + }) + + t.Run("list_users_returns_error_if_model_not_found", func(t *testing.T) { + mockDatastore.EXPECT(). + ReadAuthorizationModel(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, storage.ErrNotFound) + + server.experimentals = []ExperimentalFeatureFlag{ExperimentalEnableListUsers} + _, err := server.ListUsers(ctx, &openfgav1.ListUsersRequest{ + StoreId: storeID, + AuthorizationModelId: ulid.Make().String(), + Object: &openfgav1.Object{ + Type: "document", + Id: "1", + }, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + {Type: "user"}, + }, + }) + + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.Code(openfgav1.ErrorCode_authorization_model_not_found), st.Code()) }) } diff --git a/pkg/tuple/tuple.go b/pkg/tuple/tuple.go index 9a92cbdf68..37a763fa39 100644 --- a/pkg/tuple/tuple.go +++ b/pkg/tuple/tuple.go @@ -173,6 +173,47 @@ func UserProtoToString(obj *openfgav1.User) UserString { } } +// FromObjectOrUsersetProto returns a string from a ObjectOrUserset proto. Ex: 'user:maria' or 'group:fga#member'. It is +// the opposite from StringToObjectOrUserset function. +func FromObjectOrUsersetProto(obj *openfgav1.ObjectOrUserset) UserString { + switch user := obj.GetUser().(type) { + case *openfgav1.ObjectOrUserset_Object: + return fmt.Sprintf("%s:%s", user.Object.GetType(), user.Object.GetId()) + case *openfgav1.ObjectOrUserset_Userset: + return fmt.Sprintf("%s:%s#%s", user.Userset.GetType(), user.Userset.GetId(), user.Userset.GetRelation()) + default: + panic("unsupported type") + } +} + +// StringToObjectOrUserset returns a ObjectOrUserset proto from a string. Ex: 'group:fga#member'. +// It is the opposite from FromObjectOrUsersetProto function. +func StringToObjectOrUserset(userKey UserString) *openfgav1.ObjectOrUserset { + userObj, userRel := SplitObjectRelation(userKey) + userObjType, userObjID := SplitObject(userObj) + + if userRel == "" { + return &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Object{ + Object: &openfgav1.Object{ + Type: userObjType, + Id: userObjID, + }, + }, + } + } + + return &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Userset{ + Userset: &openfgav1.UsersetUser{ + Type: userObjType, + Id: userObjID, + Relation: userRel, + }, + }, + } +} + // StringToUserProto returns a User proto from a string. Ex: 'user:maria#member'. // It is the opposite from FromUserProto function. func StringToUserProto(userKey UserString) *openfgav1.User { diff --git a/pkg/tuple/tuple_test.go b/pkg/tuple/tuple_test.go index 95373e52c2..f6fe0b636a 100644 --- a/pkg/tuple/tuple_test.go +++ b/pkg/tuple/tuple_test.go @@ -545,3 +545,81 @@ func TestParseTupleString(t *testing.T) { }) } } + +func TestFromObjectOrUsersetProto(t *testing.T) { + for _, tc := range []struct { + name string + input *openfgav1.ObjectOrUserset + expected string + }{ + { + name: "user_with_id", + input: &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Object{ + Object: &openfgav1.Object{ + Type: "user", + Id: "id-123", + }, + }, + }, + expected: "user:id-123", + }, + { + name: "user_with_id_and_relation", + input: &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Userset{ + Userset: &openfgav1.UsersetUser{ + Type: "user", + Id: "id-123", + Relation: "member", + }, + }, + }, + expected: "user:id-123#member", + }, + } { + t.Run(tc.name, func(t *testing.T) { + actual := FromObjectOrUsersetProto(tc.input) + require.Equal(t, tc.expected, actual) + }) + } +} + +func TestToObjectOrUsersetProto(t *testing.T) { + for _, tc := range []struct { + name string + input string + expected *openfgav1.ObjectOrUserset + }{ + { + name: "user_with_id", + input: "user:id-123", + expected: &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Object{ + Object: &openfgav1.Object{ + Type: "user", + Id: "id-123", + }, + }, + }, + }, + { + name: "user_with_id_and_relation", + input: "user:id-123#member", + expected: &openfgav1.ObjectOrUserset{ + User: &openfgav1.ObjectOrUserset_Userset{ + Userset: &openfgav1.UsersetUser{ + Type: "user", + Id: "id-123", + Relation: "member", + }, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + actual := StringToObjectOrUserset(tc.input) + require.Equal(t, tc.expected, actual) + }) + } +} diff --git a/tests/listusers/listusers.go b/tests/listusers/listusers.go index d9c543c9cf..8b60f8f036 100644 --- a/tests/listusers/listusers.go +++ b/tests/listusers/listusers.go @@ -170,7 +170,9 @@ func runTest(t *testing.T, test individualTest, client ClientInterface, contextT if assertion.ErrorCode == 0 { require.NoError(t, err, detailedInfo) - require.ElementsMatch(t, assertion.Expectation, listuserstest.FromProtoResponse(resp), detailedInfo) + require.ElementsMatch(t, assertion.Expectation, listuserstest.FromUsersProto(resp.GetUsers()), detailedInfo) + + require.ElementsMatch(t, assertion.ExpectedExcludedUsers, listuserstest.FromObjectOrUsersetProto(resp.GetExcludedUsers()), detailedInfo) // assert 2: each user in the response of ListUsers should return check -> true for _, user := range resp.GetUsers() { From e9099e7f7ce5ab9e6015b5ffb5ae72a9eb0bdc7f Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Mon, 6 May 2024 17:32:04 -0600 Subject: [PATCH 02/22] test: drop residual test that was used for debugging --- .../commands/listusers/list_users_rpc_test.go | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index 097861ba6b..be01ee63f5 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -37,40 +37,6 @@ type ListUsersTests []struct { const maximumRecursiveDepth = 25 -func TestBlah(t *testing.T) { - tests := ListUsersTests{ - { - name: "blah_1", - req: &openfgav1.ListUsersRequest{ - Object: &openfgav1.Object{ - Type: "document", - Id: "1", - }, - Relation: "can_view", - UserFilters: []*openfgav1.UserTypeFilter{ - {Type: "user"}, - }, - }, - model: ` - model - schema 1.1 - type user - type document - relations - define restricted: [user] - define viewer: [user:*] but not restricted - define can_view: viewer`, - tuples: []*openfgav1.TupleKey{ - tuple.NewTupleKey("document:1", "viewer", "user:*"), - tuple.NewTupleKey("document:1", "restricted", "user:bob"), - }, - expectedUsers: []string{"user:*"}, - butNot: []string{"user:bob"}, - }, - } - tests.runListUsersTestCases(t) -} - func TestListUsersDirectRelationship(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) From 0a26449107fc62710fb4a6b83c3642f8c5b4142d Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 10:36:23 -0600 Subject: [PATCH 03/22] chore: restructure control flow in expandExclusion block --- .../commands/listusers/list_users_rpc.go | 58 +++++++++---------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 7255742455..778e8f4489 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -619,13 +619,8 @@ func (l *listUsersQuery) expandExclusion( } subtractFoundUsersMap := make(map[string]foundUser, len(baseFoundUsersMap)) - negatedSubtractionMap := make(map[string]struct{}) for fu := range subtractFoundUsersCh { - for _, s := range fu.excludedUsers { - negatedSubtractionMap[tuple.UserProtoToString(s)] = struct{}{} - } - key := tuple.UserProtoToString(fu.user) subtractFoundUsersMap[key] = fu } @@ -691,18 +686,22 @@ func (l *listUsersQuery) expandExclusion( user: tuple.StringToUserProto(userKey), noRelationship: false, }, foundUsersChan) - } else { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: true, - excludedUsers: []*openfgav1.User{ - tuple.StringToUserProto(userKey), - }, - }, foundUsersChan) + + continue } + + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: true, + excludedUsers: []*openfgav1.User{ + tuple.StringToUserProto(userKey), + }, + }, foundUsersChan) } } - } else if subtractWildcardExists { + } + + if !baseWildcardExists && subtractWildcardExists { for userKey := range baseFoundUsersMap { if !tuple.IsTypedWildcard(userKey) { subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] @@ -719,10 +718,11 @@ func (l *listUsersQuery) expandExclusion( user: tuple.StringToUserProto(userKey), noRelationship: true, }, foundUsersChan) - } } - } else { + } + + if !baseWildcardExists && !subtractWildcardExists { for key, fu := range baseFoundUsersMap { subtractedUser, userIsSubtracted := subtractFoundUsersMap[key] @@ -733,25 +733,23 @@ func (l *listUsersQuery) expandExclusion( noRelationship: false, }, foundUsersChan) - continue - } else { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(key), - noRelationship: true, - }, foundUsersChan) - continue } - } else { - if !fu.noRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(key), - noRelationship: false, - }, foundUsersChan) - } + + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(key), + noRelationship: true, + }, foundUsersChan) continue } + + if !fu.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(key), + noRelationship: false, + }, foundUsersChan) + } } } From f3875d7edea6f805219244373e5a260828980dcc Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 10:37:36 -0600 Subject: [PATCH 04/22] chore(lint): fix lint warnings --- pkg/server/commands/listusers/list_users_rpc.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 778e8f4489..2ea7dcee92 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -270,7 +270,6 @@ func (l *listUsersQuery) expand( req *internalListUsersRequest, foundUsersChan chan<- foundUser, ) expandResponse { - ctx, span := tracer.Start(ctx, "expand") defer span.End() span.SetAttributes(attribute.Int("depth", int(req.depth))) From 4af1cd38bad14d66c0f2611e05d25d4cfbc4c044 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 10:44:05 -0600 Subject: [PATCH 05/22] chore: invert control flow blocks in expandExclusion --- .../commands/listusers/list_users_rpc.go | 100 +++++++++--------- 1 file changed, 49 insertions(+), 51 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 2ea7dcee92..e028b9bed4 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -640,68 +640,70 @@ func (l *listUsersQuery) expandExclusion( _, baseWildcardExists := baseFoundUsersMap[wildcardKey] subtractWildcardUser, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] - if baseWildcardExists { - if !subtractWildcardExists || subtractWildcardUser.noRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(wildcardKey), - }, foundUsersChan) - } - - residualSubtractedUserMap := maps.Clone(subtractFoundUsersMap) + for userKey, fu := range baseFoundUsersMap { + subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] - for userKey := range baseFoundUsersMap { - _, userIsSubtracted := subtractFoundUsersMap[userKey] - _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] - - if !userIsSubtracted && !wildcardSubtracted { + if baseWildcardExists { + if !subtractWildcardExists || subtractWildcardUser.noRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), + user: tuple.StringToUserProto(wildcardKey), }, foundUsersChan) - - delete(residualSubtractedUserMap, userKey) } - } - for userKey, fu := range residualSubtractedUserMap { - excludedUsers := map[string]struct{}{} - if tuple.IsTypedWildcard(userKey) { - for _, excludedUser := range fu.excludedUsers { - excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} + residualSubtractedUserMap := maps.Clone(subtractFoundUsersMap) + + for userKey := range baseFoundUsersMap { + _, userIsSubtracted := subtractFoundUsersMap[userKey] + _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] + if !userIsSubtracted && !wildcardSubtracted { trySendResult(ctx, foundUser{ - user: excludedUser, - noRelationship: false, + user: tuple.StringToUserProto(userKey), }, foundUsersChan) - } - if !fu.noRelationship { - continue + delete(residualSubtractedUserMap, userKey) } } - if _, userIsExcluded := excludedUsers[userKey]; !userIsExcluded { - if fu.noRelationship { + for userKey, fu := range residualSubtractedUserMap { + excludedUsers := map[string]struct{}{} + if tuple.IsTypedWildcard(userKey) { + for _, excludedUser := range fu.excludedUsers { + excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} + + trySendResult(ctx, foundUser{ + user: excludedUser, + noRelationship: false, + }, foundUsersChan) + } + + if !fu.noRelationship { + continue + } + } + + if _, userIsExcluded := excludedUsers[userKey]; !userIsExcluded { + if fu.noRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + noRelationship: false, + }, foundUsersChan) + + continue + } + trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), - noRelationship: false, + noRelationship: true, + excludedUsers: []*openfgav1.User{ + tuple.StringToUserProto(userKey), + }, }, foundUsersChan) - - continue } - - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: true, - excludedUsers: []*openfgav1.User{ - tuple.StringToUserProto(userKey), - }, - }, foundUsersChan) } } - } - if !baseWildcardExists && subtractWildcardExists { - for userKey := range baseFoundUsersMap { + if !baseWildcardExists && subtractWildcardExists { if !tuple.IsTypedWildcard(userKey) { subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] if (userIsSubtracted && subtractedUser.noRelationship) || subtractWildcardUser.noRelationship { @@ -719,16 +721,12 @@ func (l *listUsersQuery) expandExclusion( }, foundUsersChan) } } - } - - if !baseWildcardExists && !subtractWildcardExists { - for key, fu := range baseFoundUsersMap { - subtractedUser, userIsSubtracted := subtractFoundUsersMap[key] + if !baseWildcardExists && !subtractWildcardExists { if userIsSubtracted { if subtractedUser.noRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(key), + user: tuple.StringToUserProto(userKey), noRelationship: false, }, foundUsersChan) @@ -736,7 +734,7 @@ func (l *listUsersQuery) expandExclusion( } trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(key), + user: tuple.StringToUserProto(userKey), noRelationship: true, }, foundUsersChan) @@ -745,7 +743,7 @@ func (l *listUsersQuery) expandExclusion( if !fu.noRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(key), + user: tuple.StringToUserProto(userKey), noRelationship: false, }, foundUsersChan) } From a00f6d2eb64a5835db801af6b63a666c3749cd08 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 11:02:28 -0600 Subject: [PATCH 06/22] chore: rework control flow for expandExclusion case with base wildcard --- .../commands/listusers/list_users_rpc.go | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index e028b9bed4..2c8991bd53 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "maps" "sync" "sync/atomic" "time" @@ -642,6 +641,7 @@ func (l *listUsersQuery) expandExclusion( for userKey, fu := range baseFoundUsersMap { subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] + _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] if baseWildcardExists { if !subtractWildcardExists || subtractWildcardUser.noRelationship { @@ -650,25 +650,20 @@ func (l *listUsersQuery) expandExclusion( }, foundUsersChan) } - residualSubtractedUserMap := maps.Clone(subtractFoundUsersMap) - - for userKey := range baseFoundUsersMap { - _, userIsSubtracted := subtractFoundUsersMap[userKey] - _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] - - if !userIsSubtracted && !wildcardSubtracted { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - }, foundUsersChan) + if !userIsSubtracted && !wildcardSubtracted { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + }, foundUsersChan) + } - delete(residualSubtractedUserMap, userKey) + for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { + if subtractedUserKey == userKey && !userIsSubtracted && !wildcardSubtracted { + continue } - } - for userKey, fu := range residualSubtractedUserMap { excludedUsers := map[string]struct{}{} - if tuple.IsTypedWildcard(userKey) { - for _, excludedUser := range fu.excludedUsers { + if tuple.IsTypedWildcard(subtractedUserKey) { + for _, excludedUser := range subtractedFu.excludedUsers { excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} trySendResult(ctx, foundUser{ @@ -677,15 +672,15 @@ func (l *listUsersQuery) expandExclusion( }, foundUsersChan) } - if !fu.noRelationship { + if !subtractedFu.noRelationship { continue } } - if _, userIsExcluded := excludedUsers[userKey]; !userIsExcluded { - if fu.noRelationship { + if _, userIsExcluded := excludedUsers[subtractedUserKey]; !userIsExcluded { + if subtractedFu.noRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), + user: tuple.StringToUserProto(subtractedUserKey), noRelationship: false, }, foundUsersChan) @@ -693,10 +688,10 @@ func (l *listUsersQuery) expandExclusion( } trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), + user: tuple.StringToUserProto(subtractedUserKey), noRelationship: true, excludedUsers: []*openfgav1.User{ - tuple.StringToUserProto(userKey), + tuple.StringToUserProto(subtractedUserKey), }, }, foundUsersChan) } From 0bc63487339612a3657fc96486be04288b8d3520 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 11:08:24 -0600 Subject: [PATCH 07/22] chore: more control flow improvements to expandExclusion --- pkg/server/commands/listusers/list_users_rpc.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 2c8991bd53..c4f049b2e0 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -639,17 +639,17 @@ func (l *listUsersQuery) expandExclusion( _, baseWildcardExists := baseFoundUsersMap[wildcardKey] subtractWildcardUser, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] + if baseWildcardExists && (!subtractWildcardExists || subtractWildcardUser.noRelationship) { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(wildcardKey), + }, foundUsersChan) + } + for userKey, fu := range baseFoundUsersMap { subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] if baseWildcardExists { - if !subtractWildcardExists || subtractWildcardUser.noRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(wildcardKey), - }, foundUsersChan) - } - if !userIsSubtracted && !wildcardSubtracted { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), @@ -657,10 +657,6 @@ func (l *listUsersQuery) expandExclusion( } for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { - if subtractedUserKey == userKey && !userIsSubtracted && !wildcardSubtracted { - continue - } - excludedUsers := map[string]struct{}{} if tuple.IsTypedWildcard(subtractedUserKey) { for _, excludedUser := range subtractedFu.excludedUsers { @@ -700,7 +696,6 @@ func (l *listUsersQuery) expandExclusion( if !baseWildcardExists && subtractWildcardExists { if !tuple.IsTypedWildcard(userKey) { - subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] if (userIsSubtracted && subtractedUser.noRelationship) || subtractWildcardUser.noRelationship { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), From b872f91c05f8c130e369656e4bddf8dc3ac47914 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 11:22:28 -0600 Subject: [PATCH 08/22] chore: add some godoc and swap to a relationship status enum --- .../commands/listusers/list_users_rpc.go | 75 ++++++++++++------- 1 file changed, 48 insertions(+), 27 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index c4f049b2e0..5515e13088 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -47,10 +47,31 @@ type expandResponse struct { err error } +// userRelationshipStatus represents the status of a relationship that a given user/subject has with respect to a specific relation. +// +// A user/subject either does or does not have a relationship, which represents that +// they either explicitly do have a relationship or explicitly do not. +type userRelationshipStatus int + +const ( + HasRelationship userRelationshipStatus = iota + NoRelationship +) + type foundUser struct { - user *openfgav1.User - excludedUsers []*openfgav1.User - noRelationship bool + user *openfgav1.User + excludedUsers []*openfgav1.User + + // relationshipStatus indicates whether the user explicitly does or does not have + // a specific relationship with respect to the relation being expanded. It almost + // exclusively applies to behavior stemming from exclusion rewrite rules. + // + // As users/subjects are being expanded we propagate the relationship status with + // respect to the relation being evaluated so that we can handle subjects which + // have been explicitly excluded from a relationship and where that relation is + // contained under the subtracted branch of another exclusion. This allows us to + // buble up the subject from the subtracted branch of the exclusion. + relationshipStatus userRelationshipStatus } type ListUsersQueryOption func(l *listUsersQuery) @@ -224,7 +245,7 @@ func (l *listUsersQuery) ListUsers( continue } - if !foundUser.noRelationship { + if foundUser.relationshipStatus == HasRelationship { foundUsers = append(foundUsers, tuple.StringToUserProto(foundUserKey)) } } @@ -537,7 +558,7 @@ func (l *listUsersQuery) expandIntersection( defer wg.Done() foundUsersMap := make(map[string]uint32, 0) for foundUser := range foundUsersChan { - if foundUser.noRelationship { + if foundUser.relationshipStatus == NoRelationship { continue } @@ -639,7 +660,7 @@ func (l *listUsersQuery) expandExclusion( _, baseWildcardExists := baseFoundUsersMap[wildcardKey] subtractWildcardUser, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] - if baseWildcardExists && (!subtractWildcardExists || subtractWildcardUser.noRelationship) { + if baseWildcardExists && (!subtractWildcardExists || subtractWildcardUser.relationshipStatus == NoRelationship) { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(wildcardKey), }, foundUsersChan) @@ -663,29 +684,29 @@ func (l *listUsersQuery) expandExclusion( excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} trySendResult(ctx, foundUser{ - user: excludedUser, - noRelationship: false, + user: excludedUser, + relationshipStatus: HasRelationship, }, foundUsersChan) } - if !subtractedFu.noRelationship { + if subtractedFu.relationshipStatus == HasRelationship { continue } } if _, userIsExcluded := excludedUsers[subtractedUserKey]; !userIsExcluded { - if subtractedFu.noRelationship { + if subtractedFu.relationshipStatus == NoRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(subtractedUserKey), - noRelationship: false, + user: tuple.StringToUserProto(subtractedUserKey), + relationshipStatus: HasRelationship, }, foundUsersChan) continue } trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(subtractedUserKey), - noRelationship: true, + user: tuple.StringToUserProto(subtractedUserKey), + relationshipStatus: NoRelationship, excludedUsers: []*openfgav1.User{ tuple.StringToUserProto(subtractedUserKey), }, @@ -696,45 +717,45 @@ func (l *listUsersQuery) expandExclusion( if !baseWildcardExists && subtractWildcardExists { if !tuple.IsTypedWildcard(userKey) { - if (userIsSubtracted && subtractedUser.noRelationship) || subtractWildcardUser.noRelationship { + if (userIsSubtracted && subtractedUser.relationshipStatus == NoRelationship) || subtractWildcardUser.relationshipStatus == NoRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: false, + user: tuple.StringToUserProto(userKey), + relationshipStatus: HasRelationship, }, foundUsersChan) continue } trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: true, + user: tuple.StringToUserProto(userKey), + relationshipStatus: NoRelationship, }, foundUsersChan) } } if !baseWildcardExists && !subtractWildcardExists { if userIsSubtracted { - if subtractedUser.noRelationship { + if subtractedUser.relationshipStatus == NoRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: false, + user: tuple.StringToUserProto(userKey), + relationshipStatus: HasRelationship, }, foundUsersChan) continue } trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: true, + user: tuple.StringToUserProto(userKey), + relationshipStatus: NoRelationship, }, foundUsersChan) continue } - if !fu.noRelationship { + if fu.relationshipStatus == HasRelationship { trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - noRelationship: false, + user: tuple.StringToUserProto(userKey), + relationshipStatus: HasRelationship, }, foundUsersChan) } } From 5f20ffb21dfee1e6aef05b6360dd4e8208bf3425 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Wed, 15 May 2024 16:19:09 -0400 Subject: [PATCH 09/22] Simplifying logic --- .../commands/listusers/list_users_rpc.go | 105 ++++++------------ 1 file changed, 37 insertions(+), 68 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 5515e13088..9c1d3ebaa7 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -53,6 +53,13 @@ type expandResponse struct { // they either explicitly do have a relationship or explicitly do not. type userRelationshipStatus int +func (r *userRelationshipStatus) InvertRelationshipStatus() userRelationshipStatus { + if *r == NoRelationship { + return HasRelationship + } + return NoRelationship +} + const ( HasRelationship userRelationshipStatus = iota NoRelationship @@ -658,20 +665,15 @@ func (l *listUsersQuery) expandExclusion( wildcardKey := tuple.TypedPublicWildcard(req.GetUserFilters()[0].GetType()) _, baseWildcardExists := baseFoundUsersMap[wildcardKey] - subtractWildcardUser, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] - - if baseWildcardExists && (!subtractWildcardExists || subtractWildcardUser.relationshipStatus == NoRelationship) { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(wildcardKey), - }, foundUsersChan) - } + _, subtractWildcardExists := subtractFoundUsersMap[wildcardKey] for userKey, fu := range baseFoundUsersMap { subtractedUser, userIsSubtracted := subtractFoundUsersMap[userKey] _, wildcardSubtracted := subtractFoundUsersMap[wildcardKey] - if baseWildcardExists { - if !userIsSubtracted && !wildcardSubtracted { + switch { + case baseWildcardExists: + if !wildcardSubtracted { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), }, foundUsersChan) @@ -680,18 +682,16 @@ func (l *listUsersQuery) expandExclusion( for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { excludedUsers := map[string]struct{}{} if tuple.IsTypedWildcard(subtractedUserKey) { + if subtractedFu.relationshipStatus == HasRelationship { + continue + } for _, excludedUser := range subtractedFu.excludedUsers { excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} - trySendResult(ctx, foundUser{ user: excludedUser, relationshipStatus: HasRelationship, }, foundUsersChan) } - - if subtractedFu.relationshipStatus == HasRelationship { - continue - } } if _, userIsExcluded := excludedUsers[subtractedUserKey]; !userIsExcluded { @@ -700,64 +700,33 @@ func (l *listUsersQuery) expandExclusion( user: tuple.StringToUserProto(subtractedUserKey), relationshipStatus: HasRelationship, }, foundUsersChan) - - continue + } else if subtractedFu.relationshipStatus == HasRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(subtractedUserKey), + relationshipStatus: NoRelationship, + excludedUsers: []*openfgav1.User{ + tuple.StringToUserProto(subtractedUserKey), + }, + }, foundUsersChan) } - - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(subtractedUserKey), - relationshipStatus: NoRelationship, - excludedUsers: []*openfgav1.User{ - tuple.StringToUserProto(subtractedUserKey), - }, - }, foundUsersChan) - } - } - } - - if !baseWildcardExists && subtractWildcardExists { - if !tuple.IsTypedWildcard(userKey) { - if (userIsSubtracted && subtractedUser.relationshipStatus == NoRelationship) || subtractWildcardUser.relationshipStatus == NoRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: HasRelationship, - }, foundUsersChan) - - continue } - - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: NoRelationship, - }, foundUsersChan) - } - } - - if !baseWildcardExists && !subtractWildcardExists { - if userIsSubtracted { - if subtractedUser.relationshipStatus == NoRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: HasRelationship, - }, foundUsersChan) - - continue - } - - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: NoRelationship, - }, foundUsersChan) - - continue } + case subtractWildcardExists: + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: subtractedUser.relationshipStatus.InvertRelationshipStatus(), + }, foundUsersChan) - if fu.relationshipStatus == HasRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: HasRelationship, - }, foundUsersChan) - } + case userIsSubtracted: + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: subtractedUser.relationshipStatus.InvertRelationshipStatus(), + }, foundUsersChan) + default: + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: fu.relationshipStatus, + }, foundUsersChan) } } From c6ee65546b13b6980b8ca83802c734468803d78e Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Sat, 18 May 2024 13:32:34 -0400 Subject: [PATCH 10/22] Adding excluded users handling for intersection --- .../commands/listusers/list_users_rpc.go | 32 ++++----- .../commands/listusers/list_users_rpc_test.go | 66 +++++++++++++++++++ 2 files changed, 82 insertions(+), 16 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 9c1d3ebaa7..89d5cc25d6 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -560,17 +560,20 @@ func (l *listUsersQuery) expandIntersection( wildcardCount := atomic.Uint32{} wildcardKey := tuple.TypedPublicWildcard(req.GetUserFilters()[0].GetType()) foundUsersCountMap := make(map[string]uint32, 0) + excludedUsersMap := make(map[string]struct{}, 0) for _, foundUsersChan := range intersectionFoundUsersChans { go func(foundUsersChan chan foundUser) { defer wg.Done() foundUsersMap := make(map[string]uint32, 0) for foundUser := range foundUsersChan { - if foundUser.relationshipStatus == NoRelationship { - continue - } - key := tuple.UserProtoToString(foundUser.user) foundUsersMap[key]++ + for _, excludedUser := range foundUser.excludedUsers { + key := tuple.UserProtoToString(excludedUser) + mu.Lock() + excludedUsersMap[key] = struct{}{} + mu.Unlock() + } } _, wildcardExists := foundUsersMap[wildcardKey] @@ -593,6 +596,11 @@ func (l *listUsersQuery) expandIntersection( } wg.Wait() + excludedUsers := []*openfgav1.User{} + for key := range excludedUsersMap { + excludedUsers = append(excludedUsers, tuple.StringToUserProto(key)) + } + for key, count := range foundUsersCountMap { // Compare the number of times the specific user was returned for // all intersection operands plus the number of wildcards. @@ -600,7 +608,8 @@ func (l *listUsersQuery) expandIntersection( // the intersection expression and can be sent on `foundUsersChan` if (count + wildcardCount.Load()) == uint32(len(childOperands)) { fu := foundUser{ - user: tuple.StringToUserProto(key), + user: tuple.StringToUserProto(key), + excludedUsers: excludedUsers, } trySendResult(ctx, fu, foundUsersChan) } @@ -680,20 +689,11 @@ func (l *listUsersQuery) expandExclusion( } for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { - excludedUsers := map[string]struct{}{} if tuple.IsTypedWildcard(subtractedUserKey) { - if subtractedFu.relationshipStatus == HasRelationship { - continue - } - for _, excludedUser := range subtractedFu.excludedUsers { - excludedUsers[tuple.UserProtoToString(excludedUser)] = struct{}{} - trySendResult(ctx, foundUser{ - user: excludedUser, - relationshipStatus: HasRelationship, - }, foundUsersChan) - } + continue } + excludedUsers := map[string]struct{}{} if _, userIsExcluded := excludedUsers[subtractedUserKey]; !userIsExcluded { if subtractedFu.relationshipStatus == NoRelationship { trySendResult(ctx, foundUser{ diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index be01ee63f5..d4348ac3ee 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -1280,6 +1280,72 @@ func TestListUsersIntersection(t *testing.T) { }, expectedUsers: []string{"user:will"}, }, + { + name: "intersection_and_excluded_users_1", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + { + Type: "user", + }, + }, + }, + model: `model + schema 1.1 + type user + + type document + relations + define viewer: viewer1 and viewer2 + define viewer1: [user,user:*] but not blocked + define viewer2: [user,user:*] but not blocked + define blocked: [user,user:*]`, + + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer1", "user:*"), + tuple.NewTupleKey("document:1", "viewer2", "user:*"), + + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + }, + expectedUsers: []string{"user:*"}, + butNot: []string{"user:maria"}, + }, + { + name: "intersection_and_excluded_users_2", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + { + Type: "user", + }, + }, + }, + model: `model + schema 1.1 + type user + + type document + relations + define viewer: viewer1 and viewer2 + define viewer1: [user,user:*] but not blocked1 + define viewer2: [user,user:*] but not blocked2 + define blocked1: [user,user:*] + define blocked2: [user,user:*]`, + + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer1", "user:*"), + tuple.NewTupleKey("document:1", "viewer2", "user:*"), + + tuple.NewTupleKey("document:1", "blocked1", "user:will"), + + tuple.NewTupleKey("document:1", "blocked1", "user:maria"), + tuple.NewTupleKey("document:1", "blocked2", "user:maria"), + }, + expectedUsers: []string{"user:*"}, + butNot: []string{"user:maria", "user:will"}, + }, } tests.runListUsersTestCases(t) } From 3ac4faa28b9786381309a6af589e68fbfbc5da04 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Sat, 18 May 2024 15:15:05 -0400 Subject: [PATCH 11/22] Adding excluded users handling for union --- .../commands/listusers/list_users_rpc.go | 112 +++++++++++++++--- .../commands/listusers/list_users_rpc_test.go | 66 +++++++++++ 2 files changed, 164 insertions(+), 14 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 89d5cc25d6..b1b2fcdef1 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -383,20 +383,22 @@ func (l *listUsersQuery) expandRewrite( case *openfgav1.Userset_Difference: resp = l.expandExclusion(ctx, req, rewrite, foundUsersChan) case *openfgav1.Userset_Union: - pool := pool.New().WithContext(ctx) - pool.WithCancelOnError() - pool.WithMaxGoroutines(int(l.resolveNodeBreadthLimit)) - - children := rewrite.Union.GetChild() - for _, childRewrite := range children { - childRewriteCopy := childRewrite - pool.Go(func(ctx context.Context) error { - resp := l.expandRewrite(ctx, req, childRewriteCopy, foundUsersChan) - return resp.err - }) - } - - resp.err = pool.Wait() + resp = l.expandUnion(ctx, req, rewrite, foundUsersChan) + // case *openfgav1.Userset_Union: + // pool := pool.New().WithContext(ctx) + // pool.WithCancelOnError() + // pool.WithMaxGoroutines(int(l.resolveNodeBreadthLimit)) + + // children := rewrite.Union.GetChild() + // for _, childRewrite := range children { + // childRewriteCopy := childRewrite + // pool.Go(func(ctx context.Context) error { + // resp := l.expandRewrite(ctx, req, childRewriteCopy, foundUsersChan) + // return resp.err + // }) + // } + + // resp.err = pool.Wait() default: panic("unexpected userset rewrite encountered") } @@ -620,6 +622,88 @@ func (l *listUsersQuery) expandIntersection( } } +func (l *listUsersQuery) expandUnion( + ctx context.Context, + req *internalListUsersRequest, + rewrite *openfgav1.Userset_Union, + foundUsersChan chan<- foundUser, +) expandResponse { + ctx, span := tracer.Start(ctx, "expandUnion") + defer span.End() + pool := pool.New().WithContext(ctx) + pool.WithCancelOnError() + pool.WithMaxGoroutines(int(l.resolveNodeBreadthLimit)) + + childOperands := rewrite.Union.GetChild() + unionFoundUsersChans := make([]chan foundUser, len(childOperands)) + for i, rewrite := range childOperands { + i := i + rewrite := rewrite + unionFoundUsersChans[i] = make(chan foundUser, 1) + pool.Go(func(ctx context.Context) error { + resp := l.expandRewrite(ctx, req, rewrite, unionFoundUsersChans[i]) + return resp.err + }) + } + + errChan := make(chan error, 1) + + go func() { + err := pool.Wait() + for i := range unionFoundUsersChans { + close(unionFoundUsersChans[i]) + } + errChan <- err + close(errChan) + }() + + var mu sync.Mutex + + var wg sync.WaitGroup + wg.Add(len(childOperands)) + + foundUsersMap := make(map[string]struct{}, 0) + excludedUsersCountMap := make(map[string]uint32, 0) + for _, foundUsersChan := range unionFoundUsersChans { + go func(foundUsersChan chan foundUser) { + defer wg.Done() + + for foundUser := range foundUsersChan { + key := tuple.UserProtoToString(foundUser.user) + mu.Lock() + foundUsersMap[key] = struct{}{} + mu.Unlock() + for _, excludedUser := range foundUser.excludedUsers { + key := tuple.UserProtoToString(excludedUser) + mu.Lock() + excludedUsersCountMap[key]++ + mu.Unlock() + } + } + }(foundUsersChan) + } + wg.Wait() + + excludedUsers := []*openfgav1.User{} + for key, count := range excludedUsersCountMap { + if count == uint32(len(childOperands)) { + excludedUsers = append(excludedUsers, tuple.StringToUserProto(key)) + } + } + + for key := range foundUsersMap { + fu := foundUser{ + user: tuple.StringToUserProto(key), + excludedUsers: excludedUsers, + } + trySendResult(ctx, fu, foundUsersChan) + } + + return expandResponse{ + err: <-errChan, + } +} + func (l *listUsersQuery) expandExclusion( ctx context.Context, req *internalListUsersRequest, diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index d4348ac3ee..9c09845ed1 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -1448,6 +1448,72 @@ func TestListUsersUnion(t *testing.T) { }, expectedUsers: []string{"user:jon", "user:maria", "user:will"}, }, + { + name: "union_and_excluded_users_1", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + { + Type: "user", + }, + }, + }, + model: `model + schema 1.1 + type user + + type document + relations + define viewer: viewer1 or viewer2 + define viewer1: [user,user:*] but not blocked + define viewer2: [user,user:*] but not blocked + define blocked: [user,user:*]`, + + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer1", "user:*"), + tuple.NewTupleKey("document:1", "viewer2", "user:*"), + + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + }, + expectedUsers: []string{"user:*"}, + butNot: []string{"user:maria"}, + }, + { + name: "union_and_excluded_users_2", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + { + Type: "user", + }, + }, + }, + model: `model + schema 1.1 + type user + + type document + relations + define viewer: viewer1 or viewer2 + define viewer1: [user,user:*] but not blocked1 + define viewer2: [user,user:*] but not blocked2 + define blocked1: [user] + define blocked2: [user]`, + + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer1", "user:*"), + tuple.NewTupleKey("document:1", "viewer2", "user:*"), + + tuple.NewTupleKey("document:1", "blocked1", "user:will"), + + tuple.NewTupleKey("document:1", "blocked1", "user:maria"), + tuple.NewTupleKey("document:1", "blocked2", "user:maria"), + }, + expectedUsers: []string{"user:*", "user:will"}, + butNot: []string{"user:maria"}, + }, } tests.runListUsersTestCases(t) } From 60aed6dfa7cd6c07fcd44f896768bd8446193d9e Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Sat, 18 May 2024 15:35:10 -0400 Subject: [PATCH 12/22] Fixing integration tests --- pkg/server/commands/listusers/list_users_rpc.go | 14 ++++++++++---- .../commands/listusers/list_users_rpc_test.go | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index b1b2fcdef1..2449bb1f42 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -569,13 +569,16 @@ func (l *listUsersQuery) expandIntersection( foundUsersMap := make(map[string]uint32, 0) for foundUser := range foundUsersChan { key := tuple.UserProtoToString(foundUser.user) - foundUsersMap[key]++ for _, excludedUser := range foundUser.excludedUsers { key := tuple.UserProtoToString(excludedUser) mu.Lock() excludedUsersMap[key] = struct{}{} mu.Unlock() } + if foundUser.relationshipStatus == NoRelationship { + continue + } + foundUsersMap[key]++ } _, wildcardExists := foundUsersMap[wildcardKey] @@ -670,15 +673,18 @@ func (l *listUsersQuery) expandUnion( for foundUser := range foundUsersChan { key := tuple.UserProtoToString(foundUser.user) - mu.Lock() - foundUsersMap[key] = struct{}{} - mu.Unlock() for _, excludedUser := range foundUser.excludedUsers { key := tuple.UserProtoToString(excludedUser) mu.Lock() excludedUsersCountMap[key]++ mu.Unlock() } + if foundUser.relationshipStatus == NoRelationship { + continue + } + mu.Lock() + foundUsersMap[key] = struct{}{} + mu.Unlock() } }(foundUsersChan) } diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index 9c09845ed1..adce623e55 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -1511,7 +1511,7 @@ func TestListUsersUnion(t *testing.T) { tuple.NewTupleKey("document:1", "blocked1", "user:maria"), tuple.NewTupleKey("document:1", "blocked2", "user:maria"), }, - expectedUsers: []string{"user:*", "user:will"}, + expectedUsers: []string{"user:*"}, butNot: []string{"user:maria"}, }, } From 9ac6e3c7835ba71dd6ed14bd764ae95abcc635a0 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Sun, 19 May 2024 13:55:07 -0400 Subject: [PATCH 13/22] Removing commented-out code --- pkg/server/commands/listusers/list_users_rpc.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 2449bb1f42..3dd9d1def9 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -384,21 +384,6 @@ func (l *listUsersQuery) expandRewrite( resp = l.expandExclusion(ctx, req, rewrite, foundUsersChan) case *openfgav1.Userset_Union: resp = l.expandUnion(ctx, req, rewrite, foundUsersChan) - // case *openfgav1.Userset_Union: - // pool := pool.New().WithContext(ctx) - // pool.WithCancelOnError() - // pool.WithMaxGoroutines(int(l.resolveNodeBreadthLimit)) - - // children := rewrite.Union.GetChild() - // for _, childRewrite := range children { - // childRewriteCopy := childRewrite - // pool.Go(func(ctx context.Context) error { - // resp := l.expandRewrite(ctx, req, childRewriteCopy, foundUsersChan) - // return resp.err - // }) - // } - - // resp.err = pool.Wait() default: panic("unexpected userset rewrite encountered") } From e13e51843edfc5cdd3d5ddae7a4a8bc4531fdf76 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Mon, 20 May 2024 10:18:39 -0400 Subject: [PATCH 14/22] Adding and fixing test case --- .../commands/listusers/list_users_rpc.go | 9 +++++- .../commands/listusers/list_users_rpc_test.go | 31 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 3dd9d1def9..c2451fe560 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -757,13 +757,20 @@ func (l *listUsersQuery) expandExclusion( switch { case baseWildcardExists: - if !wildcardSubtracted { + if !userIsSubtracted && !wildcardSubtracted { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), }, foundUsersChan) } for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { + if tuple.IsTypedWildcard(subtractedUserKey) && !userIsSubtracted { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: NoRelationship, + }, foundUsersChan) + } + if tuple.IsTypedWildcard(subtractedUserKey) { continue } diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index adce623e55..e4a985634d 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -2556,6 +2556,7 @@ func TestListUsersChainedNegation(t *testing.T) { tuples: []*openfgav1.TupleKey{ tuple.NewTupleKey("document:1", "viewer", "user:jon"), tuple.NewTupleKey("document:1", "unblocked", "user:*"), + tuple.NewTupleKey("document:1", "unblocked", "user:maria"), }, expectedUsers: []string{"user:jon"}, }, @@ -2563,9 +2564,10 @@ func TestListUsersChainedNegation(t *testing.T) { name: "chained_negation_4", tuples: []*openfgav1.TupleKey{ tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "viewer", "user:maria"), tuple.NewTupleKey("document:1", "unblocked", "user:jon"), }, - expectedUsers: []string{"user:*"}, + expectedUsers: []string{"user:*", "user:maria"}, }, { name: "chained_negation_5", @@ -2573,8 +2575,13 @@ func TestListUsersChainedNegation(t *testing.T) { tuple.NewTupleKey("document:1", "viewer", "user:*"), tuple.NewTupleKey("document:1", "blocked", "user:*"), tuple.NewTupleKey("document:1", "unblocked", "user:*"), + + tuple.NewTupleKey("document:1", "blocked", "user:jon"), + + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + tuple.NewTupleKey("document:1", "unblocked", "user:maria"), }, - expectedUsers: []string{"user:*"}, + expectedUsers: []string{"user:*", "user:maria", "user:jon"}, }, { name: "chained_negation_6", @@ -2620,6 +2627,7 @@ func TestListUsersChainedNegation(t *testing.T) { tuple.NewTupleKey("document:1", "blocked", "user:jon"), tuple.NewTupleKey("document:1", "blocked", "user:maria"), tuple.NewTupleKey("document:1", "unblocked", "user:jon"), + tuple.NewTupleKey("document:1", "unblocked", "user:poovam"), }, expectedUsers: []string{"user:*", "user:jon"}, butNot: []string{"user:maria"}, @@ -2654,6 +2662,25 @@ func TestListUsersChainedNegation(t *testing.T) { }, expectedUsers: []string{"user:*", "user:jon", "user:maria"}, }, + { + name: "chained_negation_14", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:*", "user:maria"}, + }, + { + name: "chained_negation_15", + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "viewer", "user:*"), + tuple.NewTupleKey("document:1", "blocked", "user:maria"), + tuple.NewTupleKey("document:1", "unblocked", "user:*"), + }, + expectedUsers: []string{"user:*", "user:maria"}, + }, } for i := range tests { From 571f71fb708095314167cc99798dc2f2144619b0 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Mon, 20 May 2024 10:21:34 -0400 Subject: [PATCH 15/22] Removing pointer receiver, adding tests for InvertRelationshipStatus --- pkg/server/commands/listusers/list_users_rpc.go | 4 ++-- pkg/server/commands/listusers/list_users_rpc_test.go | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index c2451fe560..f1520ebc77 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -53,8 +53,8 @@ type expandResponse struct { // they either explicitly do have a relationship or explicitly do not. type userRelationshipStatus int -func (r *userRelationshipStatus) InvertRelationshipStatus() userRelationshipStatus { - if *r == NoRelationship { +func (r userRelationshipStatus) InvertRelationshipStatus() userRelationshipStatus { + if r == NoRelationship { return HasRelationship } return NoRelationship diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index e4a985634d..abf4188b63 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -3602,3 +3602,8 @@ func TestListUsersConfig_MaxConcurrency(t *testing.T) { }) } } + +func TestInvertRelationshipStatus(t *testing.T) { + require.Equal(t, HasRelationship, NoRelationship.InvertRelationshipStatus()) + require.Equal(t, NoRelationship, HasRelationship.InvertRelationshipStatus()) +} From 243ad9c007de38cf2561c4265931796f721064c7 Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Wed, 8 May 2024 14:32:12 -0600 Subject: [PATCH 16/22] test: add another ListUsers test involving excluded users --- .../commands/listusers/list_users_rpc_test.go | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index abf4188b63..54cad29f4b 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -1281,7 +1281,7 @@ func TestListUsersIntersection(t *testing.T) { expectedUsers: []string{"user:will"}, }, { - name: "intersection_and_excluded_users_1", + name: "excluded_subjects_of_subtracted_branches_propagate", req: &openfgav1.ListUsersRequest{ Object: &openfgav1.Object{Type: "document", Id: "1"}, Relation: "viewer", @@ -1311,6 +1311,39 @@ func TestListUsersIntersection(t *testing.T) { expectedUsers: []string{"user:*"}, butNot: []string{"user:maria"}, }, + { + name: "intersection_and_excluded_users_1", + req: &openfgav1.ListUsersRequest{ + Object: &openfgav1.Object{Type: "document", Id: "1"}, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{ + { + Type: "user", + }, + }, + }, + model: ` + model + schema 1.1 + + type user + type document + relations + define b: [user:*] but not b1 + define b1: [user] + define a: [user:*] but not a1 + define a1: [user] + define viewer: a and b`, + + tuples: []*openfgav1.TupleKey{ + tuple.NewTupleKey("document:1", "b", "user:*"), + tuple.NewTupleKey("document:1", "b1", "user:will"), + tuple.NewTupleKey("document:1", "a", "user:*"), + tuple.NewTupleKey("document:1", "a1", "user:maria"), + }, + expectedUsers: []string{"user:*"}, + butNot: []string{"user:maria", "user:will"}, + }, { name: "intersection_and_excluded_users_2", req: &openfgav1.ListUsersRequest{ From b22fad608a1e951b039a9e6f82b031343279f32b Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Mon, 20 May 2024 09:15:58 -0600 Subject: [PATCH 17/22] test: add a unit test for expandExclusion handler --- .../commands/listusers/list_users_rpc_test.go | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index 54cad29f4b..3a4e8dd7f9 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -3,6 +3,7 @@ package listusers import ( "context" "fmt" + "sync/atomic" "testing" "time" @@ -20,6 +21,7 @@ import ( "github.com/openfga/openfga/pkg/storage/memory" "github.com/openfga/openfga/pkg/storage/storagewrappers" + storagetest "github.com/openfga/openfga/pkg/storage/test" "github.com/openfga/openfga/pkg/testutils" "github.com/openfga/openfga/pkg/tuple" "github.com/openfga/openfga/pkg/typesystem" @@ -3640,3 +3642,86 @@ func TestInvertRelationshipStatus(t *testing.T) { require.Equal(t, HasRelationship, NoRelationship.InvertRelationshipStatus()) require.Equal(t, NoRelationship, HasRelationship.InvertRelationshipStatus()) } + +func TestListUsers_ExpandExclusionHandler(t *testing.T) { + t.Cleanup(func() { + goleak.VerifyNone(t) + }) + + ds := memory.New() + t.Cleanup(ds.Close) + + t.Run("avoid_producing_explicitly_negated_subjects", func(t *testing.T) { + modelStr := ` + model + schema 1.1 + + type user + + type document + relations + define restricted: [user, user:*] + define viewer: [user, user:*] but not restricted + ` + storeID, model := storagetest.BootstrapFGAStore(t, ds, modelStr, []string{ + "document:1#viewer@user:*", + "document:1#viewer@user:jon", + "document:1#restricted@user:jon", + }) + + l := NewListUsersQuery(ds, WithResolveNodeLimit(maximumRecursiveDepth)) + channelDone := make(chan struct{}) + channelWithResults := make(chan foundUser) + channelWithError := make(chan error, 1) + + typesys := typesystem.New(model) + ctx := typesystem.ContextWithTypesystem(context.Background(), typesys) + + relation, err := typesys.GetRelation("document", "viewer") + require.NoError(t, err) + + rewrite := relation.GetRewrite().GetUserset().(*openfgav1.Userset_Difference) + + go func() { + resp := l.expandExclusion(ctx, &internalListUsersRequest{ + ListUsersRequest: &openfgav1.ListUsersRequest{ + StoreId: storeID, + AuthorizationModelId: model.GetId(), + Object: &openfgav1.Object{ + Type: "document", + Id: "1", + }, + Relation: "viewer", + UserFilters: []*openfgav1.UserTypeFilter{{ + Type: "user", + }}, + }, + visitedUsersetsMap: map[string]struct{}{}, + datastoreQueryCount: new(atomic.Uint32), + }, rewrite, channelWithResults) + if resp.err != nil { + channelWithError <- resp.err + return + } + + channelDone <- struct{}{} + }() + + var actualUsers []string + + OUTER: + for { + select { + case <-channelWithError: + require.FailNow(t, "expected 0 errors") + case result := <-channelWithResults: + actualUsers = append(actualUsers, tuple.UserProtoToString(result.user)) + case <-channelDone: + break OUTER + } + } + + require.Len(t, actualUsers, 1) + require.Equal(t, []string{"user:*"}, actualUsers) + }) +} From 4e62efebff85531d9272c3d04e7a533551b533a9 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Mon, 20 May 2024 15:51:49 -0400 Subject: [PATCH 18/22] Fixing TestListUsers_ExpandExclusionHandler --- .../commands/listusers/list_users_rpc.go | 10 +++---- .../commands/listusers/list_users_rpc_test.go | 30 ++++++++++++++++--- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index f1520ebc77..12ca0a7045 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -247,14 +247,14 @@ func (l *listUsersQuery) ListUsers( foundUsers := make([]*openfgav1.User, 0, len(foundUsersUnique)) for foundUserKey, foundUser := range foundUsersUnique { - _, userIsExcluded := excludedUsersUnique[foundUserKey] - if userIsExcluded { + if foundUser.relationshipStatus == NoRelationship { continue } - - if foundUser.relationshipStatus == HasRelationship { - foundUsers = append(foundUsers, tuple.StringToUserProto(foundUserKey)) + if _, userIsExcluded := excludedUsersUnique[foundUserKey]; userIsExcluded { + continue } + + foundUsers = append(foundUsers, tuple.StringToUserProto(foundUserKey)) } var excludedUsers []*openfgav1.ObjectOrUserset diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index 3a4e8dd7f9..0c262f0520 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -3707,7 +3707,10 @@ func TestListUsers_ExpandExclusionHandler(t *testing.T) { channelDone <- struct{}{} }() - var actualUsers []string + var actualResults []struct { + user string + relationshipStatus userRelationshipStatus + } OUTER: for { @@ -3715,13 +3718,32 @@ func TestListUsers_ExpandExclusionHandler(t *testing.T) { case <-channelWithError: require.FailNow(t, "expected 0 errors") case result := <-channelWithResults: - actualUsers = append(actualUsers, tuple.UserProtoToString(result.user)) + actualResults = append(actualResults, struct { + user string + relationshipStatus userRelationshipStatus + }{user: tuple.UserProtoToString(result.user), relationshipStatus: result.relationshipStatus}) case <-channelDone: break OUTER } } - require.Len(t, actualUsers, 1) - require.Equal(t, []string{"user:*"}, actualUsers) + require.Len(t, actualResults, 3) + require.Equal(t, []struct { + user string + relationshipStatus userRelationshipStatus + }{ + { + user: "user:*", + relationshipStatus: HasRelationship, + }, + { + user: "user:jon", + relationshipStatus: NoRelationship, + }, + { + user: "user:jon", + relationshipStatus: NoRelationship, + }, + }, actualResults) }) } From 9a11034af74a3f1f3f52d311edda2e229697ffd8 Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Mon, 20 May 2024 15:58:06 -0400 Subject: [PATCH 19/22] Removing invert method --- .../commands/listusers/list_users_rpc.go | 29 ++++++++----------- .../commands/listusers/list_users_rpc_test.go | 5 ---- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 12ca0a7045..92170b0742 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -53,13 +53,6 @@ type expandResponse struct { // they either explicitly do have a relationship or explicitly do not. type userRelationshipStatus int -func (r userRelationshipStatus) InvertRelationshipStatus() userRelationshipStatus { - if r == NoRelationship { - return HasRelationship - } - return NoRelationship -} - const ( HasRelationship userRelationshipStatus = iota NoRelationship @@ -793,17 +786,19 @@ func (l *listUsersQuery) expandExclusion( } } } - case subtractWildcardExists: - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: subtractedUser.relationshipStatus.InvertRelationshipStatus(), - }, foundUsersChan) + case subtractWildcardExists || userIsSubtracted: + if subtractedUser.relationshipStatus == HasRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: NoRelationship, + }, foundUsersChan) + } else if subtractedUser.relationshipStatus == NoRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: HasRelationship, + }, foundUsersChan) + } - case userIsSubtracted: - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: subtractedUser.relationshipStatus.InvertRelationshipStatus(), - }, foundUsersChan) default: trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index 0c262f0520..df92b93b91 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -3638,11 +3638,6 @@ func TestListUsersConfig_MaxConcurrency(t *testing.T) { } } -func TestInvertRelationshipStatus(t *testing.T) { - require.Equal(t, HasRelationship, NoRelationship.InvertRelationshipStatus()) - require.Equal(t, NoRelationship, HasRelationship.InvertRelationshipStatus()) -} - func TestListUsers_ExpandExclusionHandler(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) From f46eb5e41e510120ecfcdde0abb32ce793c9f7fc Mon Sep 17 00:00:00 2001 From: Will Vedder Date: Mon, 20 May 2024 16:31:35 -0400 Subject: [PATCH 20/22] Addressing PR feedback --- pkg/server/commands/listusers/list_users_rpc.go | 15 +++++++-------- .../commands/listusers/list_users_rpc_test.go | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index 92170b0742..c076d2164e 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -757,14 +757,13 @@ func (l *listUsersQuery) expandExclusion( } for subtractedUserKey, subtractedFu := range subtractFoundUsersMap { - if tuple.IsTypedWildcard(subtractedUserKey) && !userIsSubtracted { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(userKey), - relationshipStatus: NoRelationship, - }, foundUsersChan) - } - if tuple.IsTypedWildcard(subtractedUserKey) { + if !userIsSubtracted { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(userKey), + relationshipStatus: NoRelationship, + }, foundUsersChan) + } continue } @@ -786,7 +785,7 @@ func (l *listUsersQuery) expandExclusion( } } } - case subtractWildcardExists || userIsSubtracted: + case subtractWildcardExists, userIsSubtracted: if subtractedUser.relationshipStatus == HasRelationship { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), diff --git a/pkg/server/commands/listusers/list_users_rpc_test.go b/pkg/server/commands/listusers/list_users_rpc_test.go index df92b93b91..80c6cdbab1 100644 --- a/pkg/server/commands/listusers/list_users_rpc_test.go +++ b/pkg/server/commands/listusers/list_users_rpc_test.go @@ -3723,7 +3723,7 @@ func TestListUsers_ExpandExclusionHandler(t *testing.T) { } require.Len(t, actualResults, 3) - require.Equal(t, []struct { + require.ElementsMatch(t, []struct { user string relationshipStatus userRelationshipStatus }{ From ad8dadd2a850e5264a51949cac7199692d8d15fe Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Mon, 20 May 2024 15:39:34 -0600 Subject: [PATCH 21/22] chore: drop redundant 'if else' --- pkg/server/commands/listusers/list_users_rpc.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index b4af70d350..c7a96c5994 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -773,7 +773,9 @@ func (l *listUsersQuery) expandExclusion( user: tuple.StringToUserProto(subtractedUserKey), relationshipStatus: HasRelationship, }, foundUsersChan) - } else if subtractedFu.relationshipStatus == HasRelationship { + } + + if subtractedFu.relationshipStatus == HasRelationship { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(subtractedUserKey), relationshipStatus: NoRelationship, @@ -790,7 +792,9 @@ func (l *listUsersQuery) expandExclusion( user: tuple.StringToUserProto(userKey), relationshipStatus: NoRelationship, }, foundUsersChan) - } else if subtractedUser.relationshipStatus == NoRelationship { + } + + if subtractedUser.relationshipStatus == NoRelationship { trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), relationshipStatus: HasRelationship, From 2f34a45e93ae7d9b00f98d2f4c920f2dfadeb1ef Mon Sep 17 00:00:00 2001 From: Jonathan Whitaker Date: Mon, 20 May 2024 15:58:47 -0600 Subject: [PATCH 22/22] chore: simplify code and add a relevant comment --- .../commands/listusers/list_users_rpc.go | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/pkg/server/commands/listusers/list_users_rpc.go b/pkg/server/commands/listusers/list_users_rpc.go index c7a96c5994..308d48828b 100644 --- a/pkg/server/commands/listusers/list_users_rpc.go +++ b/pkg/server/commands/listusers/list_users_rpc.go @@ -766,24 +766,23 @@ func (l *listUsersQuery) expandExclusion( continue } - excludedUsers := map[string]struct{}{} - if _, userIsExcluded := excludedUsers[subtractedUserKey]; !userIsExcluded { - if subtractedFu.relationshipStatus == NoRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(subtractedUserKey), - relationshipStatus: HasRelationship, - }, foundUsersChan) - } + if subtractedFu.relationshipStatus == NoRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(subtractedUserKey), + relationshipStatus: HasRelationship, + }, foundUsersChan) + } - if subtractedFu.relationshipStatus == HasRelationship { - trySendResult(ctx, foundUser{ - user: tuple.StringToUserProto(subtractedUserKey), - relationshipStatus: NoRelationship, - excludedUsers: []*openfgav1.User{ - tuple.StringToUserProto(subtractedUserKey), - }, - }, foundUsersChan) - } + // a found user under the subtracted branch causes the subtracted user to have a negated relationship with respect + // to the base relation and is excluded since a wildcard is contained under the base branch. + if subtractedFu.relationshipStatus == HasRelationship { + trySendResult(ctx, foundUser{ + user: tuple.StringToUserProto(subtractedUserKey), + relationshipStatus: NoRelationship, + excludedUsers: []*openfgav1.User{ + tuple.StringToUserProto(subtractedUserKey), + }, + }, foundUsersChan) } } case subtractWildcardExists, userIsSubtracted: