feat(list-users): ListUsers with Excluded Users #1604

jon-whit · 2024-05-06T21:18:13Z

Description

This PR introduces the concept of excluded users, which represent the list of users that do not apply to an exclusion clause. This occurs in models that have chained exclusion where the intermediate exclusion includes a typed public wildcard.

Example:

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:*]

"document:1", "viewer", "user:jon"
"document:1", "blocked", "user:*"
"document:1", "unblocked", "user:jon"

In this example, the "unblocked" status of user:jon was being lost when evaluating the outer exclusion because the exception to the exclusion was not being communicated. This is considered an "excluded user". The solution here is to keep track of the excluded users so that the outer negation context can un-exclude the proper subjects. We also return the excluded users along with the response for the client to handle appropriately.

Consider the same model above but the following tuples:

"document:1", "viewer", "user:*"
"document:1", "blocked", "user:*"
"document:1", "unblocked", "user:jon"

ListUsers(document:1#viewer, user_filters=[{"type": "user"}]) will/should propagate up the user:jon subject from the inner most exclusion. That is, intermediate expansions should always yield a result, but that result may indicate the subject does or does not have the relationship for that intermediate relationships. In this case, that means the following:

ListUsers(document:1#viewer, user_filters=[{"type": "user"}]) --> [{user: "user:jon"}]
--> expandExclusion(document:1#viewer, filter={type: "user"}) --> [{user: "user:jon"}]
-----> base --> [{user: "user:*"}]
-----> sub  --> [{user: "user:*", noRelationship: false}, {user: "user:jon", noRelationship: true}]
--------> expandExclusion(document:1#blocked, filter={type: "user"}) --> [{user: "user:*", noRelationship: false}, {user: "user:jon", noRelationship: true}]
-----------> base --> [{user: "user:*}]
-----------> sub  --> [{user: "user:jon"}]

References

Related API PR: openfga/api#144

Review Checklist

I have clicked on "allow edits by maintainers".
I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
The correct base branch is being used, if not main
I have added tests to validate that the change in functionality is working as expected

minder-by-stacklok · 2024-05-06T21:18:17Z

Minder Vulnerability Report ✅

Minder analyzed this PR and found no vulnerable dependencies.

Vulnerability scan of c38697b1:

🐞 vulnerable packages: 0

🛠 fixes available for: 0

minder-by-stacklok · 2024-05-06T21:18:18Z

Minder analyzed this PR with Trusty and found no dependencies scored lower than your profile threshold.

jon-whit · 2024-05-06T21:20:25Z

Supersedes #1590

pkg/server/commands/listusers/list_users_rpc.go

willvedd

Still need to look over more but some immediate comments.

pkg/server/commands/listusers/list_users_rpc.go

miparnisari

I wrote two tests that I've corroborated with Check API manually (not sure why we're not doing that within runListUsersTestCases) and that are failing with this PR:

{
			name: "intersection",
			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"),
				// b -> user:* but not user:will
				tuple.NewTupleKey("document:1", "a", "user:*"),
				tuple.NewTupleKey("document:1", "a1", "user:maria"),
				// a -> user:* but not user:maria
				// therefore:
				// a and -b -> (user:* but not user:maria) and (user:* but not user:will)
				//          -> user:* but not (user:maria, user:will)
			},
			expectedUsers: []string{"user:*"},
			butNot:        []string{"user:maria", "user:will"},
		},
		{
			name: "union",
			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 or b`,

			tuples: []*openfgav1.TupleKey{
				tuple.NewTupleKey("document:1", "b", "user:*"),
				tuple.NewTupleKey("document:1", "b1", "user:will"),
				// b -> user:* but not user:will
				tuple.NewTupleKey("document:1", "a", "user:*"),
				tuple.NewTupleKey("document:1", "a1", "user:maria"),
				// a -> user:* but not user:maria
				// therefore:
				// a or -b -> (user:* but not user:maria) or (user:* but not user:will)
				//          -> user:*, user:maria, user:will
			},
			expectedUsers: []string{"user:*", "user:maria", "user:will"},
		},

I'm not very sure on the changes you made but it seems to be that we need changes in union and intersection handlers too.

More specifically I think we need a helper method that recursively computes the transformations of intersection, union and exclusion.

E.g.

(user:* but not user:maria) or (user:* but not user:will) -> user:*, user:maria, user:will

(user:* but not user:maria) and (user:* but not user:will) -> user:* but not (user:maria, user:will)

I'm pretty sure this is https://en.wikipedia.org/wiki/De_Morgan%27s_laws 🤔

…openfga into listUsers-excludedUsers

miparnisari · 2024-05-10T16:24:14Z

Are we going to address the case of excluded usersets in a different PR?

{
			name: "usersets",
			model: `model
			schema 1.1

			type user
			type group
				relations
					define member: [user]

			type document
			  relations
				define blocked: [group#member]
				define viewer: [group#member] but not blocked
			`,
			tuples: []*openfgav1.TupleKey{
				tuple.NewTupleKey("document:1", "viewer", "group:eng#member"),
				tuple.NewTupleKey("document:1", "blocked", "group:fga#member"),
			},
			req: &openfgav1.ListUsersRequest{
				Object:      &openfgav1.Object{Type: "document", Id: "1"},
				Relation:    "viewer",
				UserFilters: []*openfgav1.UserTypeFilter{{Type: "group", Relation: "member"}},
			},
			expectedUsers: []string{"group:eng#member"},
			butNot:        []string{"group:fga#member"},
		},

pkg/server/commands/listusers/list_users_rpc.go

jon-whit · 2024-05-15T21:22:27Z

pkg/server/commands/listusers/list_users_rpc.go

-			if !userIsSubtracted && !wildcardSubtracted {
+		switch {
+		case baseWildcardExists:
+			if !wildcardSubtracted {


This conditional behavior is not quite correct and has deviated from the prior commit. Previously we were only producing the wildcard subject if baseWildcardExists && !wildcardSubtracted. See

openfga/pkg/server/commands/listusers/list_users_rpc.go

Line 665 in b872f91

user: tuple.StringToUserProto(wildcardKey),

And we'd only produce an explicit (non-wildcard) subject if baseWildcardExists && !userIsSubtracted && !wildcardSubtracted. See

openfga/pkg/server/commands/listusers/list_users_rpc.go

Lines 673 to 678 in b872f91

if baseWildcardExists {

if !userIsSubtracted && !wildcardSubtracted {

trySendResult(ctx, foundUser{

user: tuple.StringToUserProto(userKey),

}, foundUsersChan)

}

Consider the following scenario:

type user type document relations define restricted: [user, user:*] define viewer: [user, user:*] but not restricted

with tuples:

document:1#viewer@user:*

document:1#viewer@user:jon

document:1#restricted@user:jon

When ranging over the baseFoundUsersMap we'll find user:jon and yield it here, because there is not a subtracted wildcard. The only reason a test related to this scenario is passing against this commit is because of other code in the callstack above this function, which isn't obvious and is actually misleading when reading the code. See

openfga/pkg/server/commands/listusers/list_users_rpc.go

Lines 251 to 253 in 5f20ffb

if userIsExcluded {

continue

}

But this logic itself is not quite correct, becase we're producing on the foundUsersChan superfluously only to be filtered out above upstream since it is not applicable (the subject user:jon is not just excluded but is explicitly subtracted). The rewrite handler should only produce accurate "found user candidates" with respect to the found users channel.

Are you ok with proving your above theory with a unit test? Then we can address appropriately in code.

https://github.com/openfga/openfga/pull/1604/files#r1606950464

willvedd · 2024-05-20T13:16:38Z

This is one test case I'm still pondering though, could see it going either way:

		{
			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:*"}, // should `user:maria` be included?
		},

jon-whit · 2024-05-20T15:35:30Z

This is one test case I'm still pondering though, could see it going either way:

		{
			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:*"}, // should `user:maria` be included?
		},

For consistency, user:maria should be included. This is for the same reason that we return the following:

  tuple.NewTupleKey("document:1", "viewer", "user:*"),
  tuple.NewTupleKey("document:1", "blocked", "user:*"),
  tuple.NewTupleKey("document:1", "unblocked", "user:maria"),

  expectedUsers: []string{"user:maria"}

Maria is explicitly blocked, but then subsequently unblocked, so she explicitly does not have the blocked relationshiop. For any subject that shows up in the base operand, if the subtracted operand contains a wildcard subject of that same type, then every base subject of the exclusion should have a relationship status NO_RELATIONSHIP. If this exclusion is then included as an operand of another chained exclusion, then those subjects will propagate up appropriately.

jon-whit · 2024-05-20T15:47:27Z

pkg/server/commands/listusers/list_users_rpc_test.go

+	ds := memory.New()
+	t.Cleanup(ds.Close)
+
+	t.Run("avoid_producing_explicitly_negated_subjects", func(t *testing.T) {


This fails right now and demonstrates we're producing more subjects than we should from the exclusion handler. For example, we should only produce user:* with the tuples and model provided, but we end up producing 3-4 subjects (user:* and multiple copies of user:jon).

…openfga into listUsers-excludedUsers

jon-whit · 2024-05-20T17:49:56Z

pkg/server/commands/listusers/list_users_rpc.go

+					}
+				}
+			}
+		case subtractWildcardExists:


This case can be collapsed along with the case below

case userIsSubtracted, subtractWildcardExists: trySendResult(ctx, foundUser{ user: tuple.StringToUserProto(userKey), relationshipStatus: subtractedUser.relationshipStatus.InvertRelationshipStatus(), }, foundUsersChan)

pkg/server/commands/listusers/list_users_rpc.go

…listUsers-excludedUsers

willvedd

🙏

jon-whit requested a review from a team as a code owner May 6, 2024 21:18

jon-whit changed the base branch from main to list-users May 6, 2024 21:18

feat(list-users): ListUsers with Excluded Users

a1ca45f

jon-whit force-pushed the listUsers-excludedUsers branch from c38697b to a1ca45f Compare May 6, 2024 21:19

jon-whit mentioned this pull request May 6, 2024

feat(list-users): ListUsers with Excluded Users #1590

Closed

4 tasks

test: drop residual test that was used for debugging

e9099e7

miparnisari reviewed May 7, 2024

View reviewed changes

pkg/server/commands/listusers/list_users_rpc.go Outdated Show resolved Hide resolved

willvedd reviewed May 7, 2024

View reviewed changes

miparnisari reviewed May 8, 2024

View reviewed changes

pkg/server/commands/listusers/list_users_rpc.go Outdated Show resolved Hide resolved

miparnisari requested changes May 8, 2024

View reviewed changes

jon-whit and others added 7 commits May 8, 2024 10:36

chore: restructure control flow in expandExclusion block

0a26449

chore(lint): fix lint warnings

f3875d7

chore: invert control flow blocks in expandExclusion

4af1cd3

chore: rework control flow for expandExclusion case with base wildcard

a00f6d2

chore: more control flow improvements to expandExclusion

0bc6348

chore: add some godoc and swap to a relationship status enum

b872f91

Merge branch 'listUsers-excludedUsers' of https://github.com/openfga/…

04ef107

…openfga into listUsers-excludedUsers

Simplifying logic

5f20ffb

willvedd reviewed May 15, 2024

View reviewed changes

pkg/server/commands/listusers/list_users_rpc.go Outdated Show resolved Hide resolved

jon-whit commented May 15, 2024

View reviewed changes

pkg/server/commands/listusers/list_users_rpc.go Outdated Show resolved Hide resolved

jon-whit commented May 15, 2024

View reviewed changes

willvedd added 4 commits May 18, 2024 13:32

Adding excluded users handling for intersection

c6ee655

Adding excluded users handling for union

3ac4faa

Fixing integration tests

60aed6d

Removing commented-out code

9ac6e3c

willvedd added 2 commits May 20, 2024 10:18

Adding and fixing test case

e13e518

Removing pointer receiver, adding tests for InvertRelationshipStatus

571f71f

jon-whit added 2 commits May 20, 2024 09:42

test: add another ListUsers test involving excluded users

243ad9c

test: add a unit test for expandExclusion handler

b22fad6

jon-whit commented May 20, 2024

View reviewed changes

Merge branch 'listUsers-excludedUsers' of https://github.com/openfga/…

4ecf4bc

…openfga into listUsers-excludedUsers

jon-whit commented May 20, 2024

View reviewed changes

pkg/server/commands/listusers/list_users_rpc.go Outdated Show resolved Hide resolved

willvedd and others added 6 commits May 20, 2024 15:51

Fixing TestListUsers_ExpandExclusionHandler

4e62efe

Removing invert method

9a11034

Addressing PR feedback

f46eb5e

Merge branch 'list-users' of https://github.com/openfga/openfga into …

67424c0

…listUsers-excludedUsers

chore: drop redundant 'if else'

ad8dadd

chore: simplify code and add a relevant comment

2f34a45

willvedd approved these changes May 20, 2024

View reviewed changes

jon-whit merged commit c170551 into list-users May 20, 2024
5 of 7 checks passed

jon-whit deleted the listUsers-excludedUsers branch May 20, 2024 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(list-users): ListUsers with Excluded Users #1604

feat(list-users): ListUsers with Excluded Users #1604

jon-whit commented May 6, 2024 •

edited

minder-by-stacklok bot commented May 6, 2024

Vulnerability scan of `c38697b1:`

minder-by-stacklok bot commented May 6, 2024

jon-whit commented May 6, 2024

willvedd left a comment

miparnisari left a comment •

edited

miparnisari commented May 10, 2024 •

edited

jon-whit May 15, 2024 •

edited

willvedd May 20, 2024

jon-whit May 20, 2024

willvedd commented May 20, 2024

jon-whit commented May 20, 2024 •

edited

jon-whit May 20, 2024

jon-whit May 20, 2024

willvedd left a comment

	if baseWildcardExists {
	if !userIsSubtracted && !wildcardSubtracted {
	trySendResult(ctx, foundUser{
	user: tuple.StringToUserProto(userKey),
	}, foundUsersChan)
	}

feat(list-users): ListUsers with Excluded Users #1604

feat(list-users): ListUsers with Excluded Users #1604

Conversation

jon-whit commented May 6, 2024 • edited

Description

References

Review Checklist

minder-by-stacklok bot commented May 6, 2024

Minder Vulnerability Report ✅

Vulnerability scan of c38697b1:

minder-by-stacklok bot commented May 6, 2024

jon-whit commented May 6, 2024

willvedd left a comment

Choose a reason for hiding this comment

miparnisari left a comment • edited

Choose a reason for hiding this comment

miparnisari commented May 10, 2024 • edited

jon-whit May 15, 2024 • edited

Choose a reason for hiding this comment

willvedd May 20, 2024

Choose a reason for hiding this comment

jon-whit May 20, 2024

Choose a reason for hiding this comment

willvedd commented May 20, 2024

jon-whit commented May 20, 2024 • edited

jon-whit May 20, 2024

Choose a reason for hiding this comment

jon-whit May 20, 2024

Choose a reason for hiding this comment

willvedd left a comment

Choose a reason for hiding this comment

jon-whit commented May 6, 2024 •

edited

Vulnerability scan of `c38697b1:`

miparnisari left a comment •

edited

miparnisari commented May 10, 2024 •

edited

jon-whit May 15, 2024 •

edited

jon-whit commented May 20, 2024 •

edited