simplify findUserByToken in ACL, add missing testcases (#2388)

* update users doc on unique constraints Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> * simplify finduser func Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> * add initial tests for findUserFromToken Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> * add changelog Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> --------- Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-10-31 08:01:34 +01:00 · 2025-01-30 10:35:49 +00:00 · 2025-01-30 10:35:49 +00:00 · 7ba6ad3489
commit 7ba6ad3489
parent 2c279e0a7b
4 changed files with 340 additions and 23 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -7,7 +7,14 @@
 - `oidc.map_legacy_users` is now `false` by default
  [#2350](https://github.com/juanfont/headscale/pull/2350)

-## 0.24.1 (2025-01-xx)
+## 0.24.2 (2025-01-30)
+
+### Changes
+
+- Fix issue where email and username being equal fails to match in Policy
+  [#2388](https://github.com/juanfont/headscale/pull/2388)
+
+## 0.24.1 (2025-01-23)

 ### Changes

--- a/hscontrol/policy/acls.go
+++ b/hscontrol/policy/acls.go
@ -381,7 +381,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
 				}

 				for _, userStr := range usersFromGroup {
-					user, err := findUserFromTokenOrErr(users, userStr)
+					user, err := findUserFromToken(users, userStr)
 					if err != nil {
 						log.Trace().Err(err).Msg("user not found")
 						continue
@ -400,7 +400,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
 			// corresponds with the User info in the netmap.
 			// TODO(kradalby): This is a bit of a hack, and it should go
 			// away with the new policy where users can be reliably determined.
-			if user, err := findUserFromTokenOrErr(users, srcToken); err == nil {
+			if user, err := findUserFromToken(users, srcToken); err == nil {
 				principals = append(principals, &tailcfg.SSHPrincipal{
 					UserLogin: user.Username(),
 				})
@ -1001,7 +1001,7 @@ func (pol *ACLPolicy) TagsOfNode(
 			}
 			var found bool
 			for _, owner := range owners {
-				user, err := findUserFromTokenOrErr(users, owner)
+				user, err := findUserFromToken(users, owner)
 				if err != nil {
 					log.Trace().Caller().Err(err).Msg("could not determine user to filter tags by")
 				}
@ -1038,7 +1038,7 @@ func (pol *ACLPolicy) TagsOfNode(
 func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string) types.Nodes {
 	var out types.Nodes

-	user, err := findUserFromTokenOrErr(users, userToken)
+	user, err := findUserFromToken(users, userToken)
 	if err != nil {
 		log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
 		return out
@ -1058,24 +1058,19 @@ var (
 	ErrorMultipleUserMatching = errors.New("multiple users matching")
 )

-func findUserFromTokenOrErr(
-	users []types.User,
-	token string,
-) (types.User, error) {
+// findUserFromToken finds and returns a user based on the given token, prioritizing matches by ProviderIdentifier, followed by email or name.
+// If no matching user is found, it returns an error of type ErrorNoUserMatching.
+// If multiple users match the token, it returns an error indicating multiple matches.
+func findUserFromToken(users []types.User, token string) (types.User, error) {
 	var potentialUsers []types.User
+
 	for _, user := range users {
 		if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == token {
-			// If a user is matching with a known unique field,
-			// disgard all other users and only keep the current
-			// user.
-			potentialUsers = []types.User{user}
+			// Prioritize ProviderIdentifier match and exit early
+			return user, nil
+		}

-			break
-		}
-		if user.Email == token {
-			potentialUsers = append(potentialUsers, user)
-		}
-		if user.Name == token {
+		if user.Email == token || user.Name == token {
 			potentialUsers = append(potentialUsers, user)
 		}
 	}
--- a/hscontrol/policy/acls_test.go
+++ b/hscontrol/policy/acls_test.go
@ -4046,3 +4046,315 @@ func TestValidTagInvalidUser(t *testing.T) {
 		t.Errorf("TestValidTagInvalidUser() unexpected result (-want +got):\n%s", diff)
 	}
 }
+
+func TestFindUserByToken(t *testing.T) {
+	tests := []struct {
+		name    string
+		users   []types.User
+		token   string
+		want    types.User
+		wantErr bool
+	}{
+		{
+			name: "exact match by ProviderIdentifier",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
+				{Email: "user2@example.com"},
+			},
+			token:   "token1",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
+			wantErr: false,
+		},
+		{
+			name: "no matches found",
+			users: []types.User{
+				{Email: "user1@example.com"},
+				{Name: "username"},
+			},
+			token:   "nonexistent-token",
+			want:    types.User{},
+			wantErr: true,
+		},
+		{
+			name: "multiple matches by email and name",
+			users: []types.User{
+				{Email: "token2", Name: "notoken"},
+				{Name: "token2", Email: "notoken@example.com"},
+			},
+			token:   "token2",
+			want:    types.User{},
+			wantErr: true,
+		},
+		{
+			name: "match by email",
+			users: []types.User{
+				{Email: "token3@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "othertoken"}},
+			},
+			token:   "token3@example.com",
+			want:    types.User{Email: "token3@example.com"},
+			wantErr: false,
+		},
+		{
+			name: "match by name",
+			users: []types.User{
+				{Name: "token4"},
+				{Email: "user5@example.com"},
+			},
+			token:   "token4",
+			want:    types.User{Name: "token4"},
+			wantErr: false,
+		},
+		{
+			name: "provider identifier takes precedence over email and name matches",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
+				{Email: "token5@example.com", Name: "token5"},
+			},
+			token:   "token5",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
+			wantErr: false,
+		},
+		{
+			name: "empty token finds no users",
+			users: []types.User{
+				{Email: "user6@example.com"},
+				{Name: "username6"},
+			},
+			token:   "",
+			want:    types.User{},
+			wantErr: true,
+		},
+		// Test case 1: Duplicate Emails with Unique ProviderIdentifiers
+		{
+			name: "duplicate emails with unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid1"}, Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid2"}, Email: "user@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 2: Duplicate Names with Unique ProviderIdentifiers
+		{
+			name: "duplicate names with unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "John Doe"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "John Doe"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 3: Duplicate Emails and Names with Unique ProviderIdentifiers
+		{
+			name: "duplicate emails and names with unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Email: "user@example.com", Name: "John Doe"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid6"}, Email: "user@example.com", Name: "John Doe"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 4: Unique Names without ProviderIdentifiers
+		{
+			name: "unique names without provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "janesmith@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+			wantErr: false,
+		},
+
+		// Test case 5: Duplicate Emails without ProviderIdentifiers but Unique Names
+		{
+			name: "duplicate emails without provider identifiers but unique names",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+			wantErr: false,
+		},
+
+		// Test case 6: Duplicate Names and Emails without ProviderIdentifiers
+		{
+			name: "duplicate names and emails without provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 7: Multiple Users with the Same Email but Different Names and Unique ProviderIdentifiers
+		{
+			name: "multiple users with same email, different names, unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid7"}, Email: "user@example.com", Name: "John Doe"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid8"}, Email: "user@example.com", Name: "Jane Smith"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 8: Multiple Users with the Same Name but Different Emails and Unique ProviderIdentifiers
+		{
+			name: "multiple users with same name, different emails, unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid9"}, Email: "johndoe@example.com", Name: "John Doe"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid10"}, Email: "janedoe@example.com", Name: "John Doe"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 9: Multiple Users with Same Email and Name but Unique ProviderIdentifiers
+		{
+			name: "multiple users with same email and name, unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid11"}, Email: "user@example.com", Name: "John Doe"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid12"}, Email: "user@example.com", Name: "John Doe"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 10: Multiple Users without ProviderIdentifiers but with Unique Names and Emails
+		{
+			name: "multiple users without provider identifiers, unique names and emails",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "janesmith@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+			wantErr: false,
+		},
+
+		// Test case 11: Multiple Users without ProviderIdentifiers and Duplicate Emails but Unique Names
+		{
+			name: "multiple users without provider identifiers, duplicate emails but unique names",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+			wantErr: false,
+		},
+
+		// Test case 12: Multiple Users without ProviderIdentifiers and Duplicate Names but Unique Emails
+		{
+			name: "multiple users without provider identifiers, duplicate names but unique emails",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "janedoe@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 13: Multiple Users without ProviderIdentifiers and Duplicate Both Names and Emails
+		{
+			name: "multiple users without provider identifiers, duplicate names and emails",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 14: Multiple Users with Same Email Without ProviderIdentifiers
+		{
+			name: "multiple users with same email without provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+
+		// Test case 15: Multiple Users with Same Name Without ProviderIdentifiers
+		{
+			name: "multiple users with same name without provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "janedoe@example.com"},
+			},
+			token:   "John Doe",
+			want:    types.User{},
+			wantErr: true,
+		},
+		{
+			name: "Name field used as email address match",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "user@example.com", Email: "another@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "user@example.com", Email: "another@example.com"},
+			wantErr: false,
+		},
+		{
+			name: "multiple users with same name as email and unique provider identifiers",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "user@example.com", Email: "user1@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Name: "user@example.com", Email: "user2@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+		{
+			name: "no provider identifier and duplicate names as emails",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another1@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another2@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+		{
+			name: "name as email with multiple matches when provider identifier is not set",
+			users: []types.User{
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another1@example.com"},
+				{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another2@example.com"},
+			},
+			token:   "user@example.com",
+			want:    types.User{},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotUser, err := findUserFromToken(tt.users, tt.token)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("findUserFromToken() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if diff := cmp.Diff(tt.want, gotUser, util.Comparers...); diff != "" {
+				t.Errorf("findUserFromToken() unexpected result (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
--- a/hscontrol/types/users.go
+++ b/hscontrol/types/users.go
@ -29,8 +29,9 @@ type User struct {
 	// you can have multiple users with the same name in OIDC,
 	// but not if you only run with CLI users.

-	// Username for the user, is used if email is empty
+	// Name (username) for the user, is used if email is empty
 	// Should not be used, please use Username().
+	// It is unique if ProviderIdentifier is not set.
 	Name string

 	// Typically the full name of the user
@ -40,9 +41,11 @@ type User struct {
 	// Should not be used, please use Username().
 	Email string

-	// Unique identifier of the user from OIDC,
-	// comes from `sub` claim in the OIDC token
-	// and is used to lookup the user.
+	// ProviderIdentifier is a unique or not set identifier of the
+	// user from OIDC. It is the combination of `iss`
+	// and `sub` claim in the OIDC token.
+	// It is unique if set.
+	// It is unique together with Name.
 	ProviderIdentifier sql.NullString

 	// Provider is the origin of the user account,