policy/v2: prefer the user owning an email over a username match

docs/ref/oidc.md

@@ -185,6 +185,11 @@ You may refer to users in the Headscale policy via:
 - Provider identifier (this value is currently only available from the [API](api.md), database or directly from your
   identity provider)
 
+References match a provider identifier first, then a stored email address, then a username. A token shaped like an
+email address also matches a username with that value when no user has that email. If one user has the email and
+another has the username, the email owner wins. Multiple matching emails, or multiple matching usernames when no email
+matches, remain ambiguous.
+
 !!! note "A user identifier in the policy must contain a single `@`"
 
     The Headscale policy requires a single `@` to reference a user. If the username or provider identifier doesn't
@@ -200,6 +205,8 @@ You may refer to users in the Headscale policy via:
     consequences for Headscale where a policy might no longer work or a user might obtain more access by hijacking an
     existing username or email address.
 
+    Use provider identifiers in policies when your identity provider lets users change their own usernames.
+
 !!! tip "Howto use the provider identifier in the policy"
 
     The provider identifier uniquely identifies an OIDC user and a well-behaving identity provider guarantees that this

hscontrol/policy/v2/tailscale_users_compat_test.go

@@ -99,33 +99,27 @@ var userTokenSkipReasons = map[string]string{
 
 	"users-bare-at-test-dst/breakdown":                         testUnknownAlias,
 	"users-bare-at-test-dst/duplicate-unknown-email/breakdown": testUnknownAlias,
+	"users-bare-at-test-dst/name-is-owner-email/breakdown":     testUnknownAlias,
 	"users-bare-at-test-dst/name-is-unknown-email/breakdown":   testUnknownAlias,
 	"users-bare-at-test-dst/no-email-two/breakdown":            testUnknownAlias,
 	"users-bare-at-test-src/breakdown":                         testUnknownAlias,
 	"users-bare-at-test-src/duplicate-unknown-email/breakdown": testUnknownAlias,
+	"users-bare-at-test-src/name-is-owner-email/breakdown":     testUnknownAlias,
 	"users-bare-at-test-src/name-is-unknown-email/breakdown":   testUnknownAlias,
 	"users-bare-at-test-src/no-email-two/breakdown":            testUnknownAlias,
 
-	"users-bare-at-dst/name-is-owner-email/boot":           ambiguousLoad,
+	// The evaluation error repeats the test's source unquoted, here a user's name.
+	"users-bare-at-test-dst/name-is-owner-email/user-list": testUnknownAlias,
+
 	"users-bare-at-dst/no-email-two/boot":                  ambiguousLoad,
-	"users-bare-at-group/name-is-owner-email/boot":         ambiguousLoad,
 	"users-bare-at-group/no-email-two/boot":                ambiguousLoad,
-	"users-bare-at-nodeattrs/name-is-owner-email/boot":     ambiguousLoad,
 	"users-bare-at-nodeattrs/no-email-two/boot":            ambiguousLoad,
 	"users-bare-at-nodeattrs/no-email-two/runtime":         ambiguousLoad,
-	"users-bare-at-src/name-is-owner-email/boot":           ambiguousLoad,
 	"users-bare-at-src/no-email-two/boot":                  ambiguousLoad,
-	"users-bare-at-tagowner/name-is-owner-email/boot":      ambiguousLoad,
 	"users-bare-at-tagowner/no-email-two/boot":             ambiguousLoad,
-	"users-bare-at-test-dst/name-is-owner-email/boot":      ambiguousLoad,
-	"users-bare-at-test-src/name-is-owner-email/boot":      ambiguousLoad,
-	"users-short-name/name-is-owner-email/boot":            ambiguousLoad,
 	"users-unknown-direct/duplicate-unknown-email/boot":    ambiguousLoad,
-	"users-unknown-direct/name-is-owner-email/boot":        ambiguousLoad,
 	"users-unknown-member/duplicate-unknown-email/boot":    ambiguousLoad,
 	"users-unknown-member/duplicate-unknown-email/runtime": ambiguousLoad,
-	"users-unknown-member/name-is-owner-email/boot":        ambiguousLoad,
-	"users-unknown-member/name-is-owner-email/runtime":     ambiguousLoad,
 
 	"users-unknown-direct/duplicate-unknown-email/verdict": ambiguousWrite,
 	"users-unknown-member/duplicate-unknown-email/verdict": ambiguousWrite,
@@ -133,32 +127,6 @@ var userTokenSkipReasons = map[string]string{
 	"users-unknown-direct/duplicate-unknown-email/user-list": ambiguityListsUsers,
 	"users-unknown-member/duplicate-unknown-email/user-list": ambiguityListsUsers,
 
-	"users-bare-at-dst/name-is-owner-email/verdict":             emailOwnerAmbiguous,
-	"users-bare-at-group/name-is-owner-email/verdict":           emailOwnerAmbiguous,
-	"users-bare-at-nodeattrs/name-is-owner-email/server/filter": emailOwnerAmbiguous,
-	"users-bare-at-nodeattrs/name-is-owner-email/verdict":       emailOwnerAmbiguous,
-	"users-bare-at-src/name-is-owner-email/verdict":             emailOwnerAmbiguous,
-	"users-bare-at-tagowner/name-is-owner-email/server/filter":  emailOwnerAmbiguous,
-	"users-bare-at-tagowner/name-is-owner-email/verdict":        emailOwnerAmbiguous,
-	"users-bare-at-test-dst/name-is-owner-email/breakdown":      emailOwnerAmbiguous,
-	"users-bare-at-test-dst/name-is-owner-email/message":        emailOwnerAmbiguous,
-	"users-bare-at-test-src/name-is-owner-email/breakdown":      emailOwnerAmbiguous,
-	"users-bare-at-test-src/name-is-owner-email/message":        emailOwnerAmbiguous,
-	"users-short-name/name-is-owner-email/verdict":              emailOwnerAmbiguous,
-	"users-unknown-direct/name-is-owner-email/verdict":          emailOwnerAmbiguous,
-	"users-unknown-member/name-is-owner-email/verdict":          emailOwnerAmbiguous,
-
-	"users-bare-at-dst/name-is-owner-email/user-list":       emailOwnerAmbiguous,
-	"users-bare-at-group/name-is-owner-email/user-list":     emailOwnerAmbiguous,
-	"users-bare-at-nodeattrs/name-is-owner-email/user-list": emailOwnerAmbiguous,
-	"users-bare-at-src/name-is-owner-email/user-list":       emailOwnerAmbiguous,
-	"users-bare-at-tagowner/name-is-owner-email/user-list":  emailOwnerAmbiguous,
-	"users-bare-at-test-dst/name-is-owner-email/user-list":  emailOwnerAmbiguous,
-	"users-bare-at-test-src/name-is-owner-email/user-list":  emailOwnerAmbiguous,
-	"users-short-name/name-is-owner-email/user-list":        emailOwnerAmbiguous,
-	"users-unknown-direct/name-is-owner-email/user-list":    emailOwnerAmbiguous,
-	"users-unknown-member/name-is-owner-email/user-list":    emailOwnerAmbiguous,
-
 	"users-unknown-direct/name-is-unknown-email-unverified/overlay-1/filter":         usernameAnswersEmail,
 	"users-unknown-direct/name-is-unknown-email-unverified/overlay-1/route-approver": usernameAnswersEmail,
 	"users-unknown-direct/name-is-unknown-email-unverified/overlay-1/tag-owner":      usernameAnswersEmail,
@@ -215,7 +183,6 @@ const (
 	ambiguousLoad        = "AMBIGUOUS_USER_LOAD: a user token matching two users stops the policy loading; SaaS keeps serving it"
 	ambiguousWrite       = "AMBIGUOUS_USER_WRITE: headscale refuses to store a policy whose user token matches two users (#3160)"
 	ambiguityListsUsers  = "AMBIGUITY_LISTS_USERS: headscale's refusal of a token matching two users lists both users by ID and name"
-	emailOwnerAmbiguous  = "EMAIL_OWNER_FIRST: a username equal to another user's e-mail makes that e-mail match two users"
 	usernameAnswersEmail = "USERNAME_ANSWERS_EMAIL_TOKEN: a username shaped like an unclaimed address takes that address's rights"
 	bareAtNoEmail        = `BARE_AT_NO_EMAIL: "@" matches the users without an e-mail; SaaS matches it to no one`
 )

hscontrol/policy/v2/types.go

@@ -434,13 +434,13 @@ func (u *Username) CanBeAutoApprover() bool {
 }
 
 // resolveUser attempts to find a user in the provided [types.Users] slice that matches the [Username].
-// It prioritizes matching the [types.User.ProviderIdentifier], and if not found, it falls back to matching
-// the [types.User.Email] or [types.User.Name].
+// It prioritizes matching the [types.User.ProviderIdentifier], then the [types.User.Email],
+// and falls back to the [types.User.Name] only when no email matches.
 // If no matching user is found, it returns an error indicating no user matching.
 // If multiple matching users are found, it returns an error indicating multiple users matching.
 // It returns the matched [types.User] and a nil error if exactly one match is found.
 func (u *Username) resolveUser(users types.Users) (types.User, error) {
-	var potentialUsers types.Users
+	var emailMatches, nameMatches types.Users
 
 	// At parsetime, we require all usernames to contain an "@" character, if the
 	// username token does not naturally do so (like email), the user have to
@@ -454,9 +454,18 @@ func (u *Username) resolveUser(users types.Users) (types.User, error) {
 			return user, nil
 		}
 
-		if user.Email == uTrimmed || user.Name == uTrimmed {
-			potentialUsers = append(potentialUsers, user)
+		if user.Email == uTrimmed {
+			emailMatches = append(emailMatches, user)
 		}
+
+		if user.Name == uTrimmed {
+			nameMatches = append(nameMatches, user)
+		}
+	}
+
+	potentialUsers := emailMatches
+	if len(potentialUsers) == 0 {
+		potentialUsers = nameMatches
 	}
 
 	if len(potentialUsers) == 0 {

hscontrol/policy/v2/types_test.go

@@ -2,9 +2,11 @@ package v2
 
 import (
 	"bytes"
+	"database/sql"
 	"encoding/json"
 	"fmt"
 	"net/netip"
+	"slices"
 	"strings"
 	"testing"
 	"time"
@@ -6485,3 +6487,98 @@ func TestUnmarshalPolicyMaxDstPorts(t *testing.T) {
 		})
 	}
 }
+
+func TestResolveUserPrecedence(t *testing.T) {
+	const (
+		address    = "alice@example.com"
+		providerID = "https://idp.example/alice"
+	)
+
+	owner := types.User{ID: 1, Name: "alice", Email: address}
+	oidcName := types.User{ID: 2, Name: address, Provider: "oidc"}
+	localName := types.User{ID: 3, Name: address}
+	both := types.User{ID: 4, Name: address, Email: address}
+	provider := types.User{
+		ID:                 5,
+		Name:               "provider-user",
+		ProviderIdentifier: sql.NullString{String: providerID, Valid: true},
+	}
+
+	tests := []struct {
+		name    string
+		token   Username
+		users   types.Users
+		want    types.User
+		wantErr error
+	}{
+		{
+			name:  "email owner beats OIDC username",
+			token: Username(address),
+			users: types.Users{owner, oidcName},
+			want:  owner,
+		},
+		{
+			name:  "email owner beats local username",
+			token: Username(address),
+			users: types.Users{owner, localName},
+			want:  owner,
+		},
+		{
+			name:  "address-shaped username without email owner",
+			token: Username(address),
+			users: types.Users{oidcName, {ID: 6, Name: "other", Email: "other@example.com"}},
+			want:  oidcName,
+		},
+		{
+			name:  "email and username on the same user count once",
+			token: Username(address),
+			users: types.Users{both, oidcName},
+			want:  both,
+		},
+		{
+			name:    "duplicate usernames remain ambiguous",
+			token:   Username(address),
+			users:   types.Users{oidcName, localName},
+			wantErr: ErrMultipleUsersFound,
+		},
+		{
+			name:    "duplicate emails remain ambiguous despite a unique username",
+			token:   Username(address),
+			users:   types.Users{owner, {ID: 7, Name: "second-owner", Email: address}, oidcName},
+			wantErr: ErrMultipleUsersFound,
+		},
+		{
+			name:  "provider identifier wins",
+			token: Username(providerID + "@"),
+			users: types.Users{provider, {ID: 8, Name: providerID}},
+			want:  provider,
+		},
+		{
+			name:    "unknown user",
+			token:   "missing@",
+			users:   types.Users{owner, oidcName, localName},
+			wantErr: ErrUserNotFound,
+		},
+	}
+
+	for _, tt := range tests {
+		for _, reverse := range []bool{false, true} {
+			t.Run(fmt.Sprintf("%s/reverse=%t", tt.name, reverse), func(t *testing.T) {
+				users := slices.Clone(tt.users)
+				if reverse {
+					slices.Reverse(users)
+				}
+
+				got, err := tt.token.resolveUser(users)
+				if tt.wantErr != nil {
+					require.ErrorIs(t, err, tt.wantErr)
+
+					return
+				}
+
+				require.NoError(t, err)
+				require.Equal(t, tt.want, got)
+			})
+		}
+	}
+}