policy/v2: match no one for an ambiguous user outside policy writes

A change to user identifiers must not block startup or later logins.
Keep policy writes strict; log ambiguous references on load and refresh.

hscontrol/policy/v2/compiled_test.go

@@ -320,14 +320,15 @@ func TestCompileOtherDestsMatchesReference(t *testing.T) {
 					hp("undefined"),
 				}
 			case "partial-group":
+				// An ambiguous member matches no one; the others stay.
 				users = append(users, types.User{ID: 100, Name: users[0].Name})
 				ips, err := grant.Destinations[0].Resolve(pol, users, nodes.ViewSlice())
-				require.Error(t, err)
+				require.NoError(t, err)
 				require.NotNil(t, ips)
 				require.False(
 					t,
 					ips.Empty(),
-					"exercise a partial result alongside the error",
+					"exercise a partial result",
 				)
 			}
 

hscontrol/policy/v2/policy.go

@@ -135,8 +135,7 @@ type filterAndPolicy struct {
 	Policy *Policy
 }
 
-// validateUserReferences surfaces ambiguous user@ tokens at policy load so
-// duplicate DB rows fail loudly instead of silently dropping rules.
+// validateUserReferences reports ambiguous user@ tokens so writes can refuse them.
 // Missing-user tokens stay tolerant. Empty users → no-op for
 // syntax-only checks.
 func validateUserReferences(pol *Policy, users types.Users) error {
@@ -247,7 +246,7 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node
 
 	err = validateUserReferences(policy, users)
 	if err != nil {
-		return nil, fmt.Errorf("validating policy user references: %w", err)
+		log.Error().Err(err).Msg("ambiguous policy users at boot")
 	}
 
 	pm := PolicyManager{
@@ -1122,6 +1121,11 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) {
 		return false, false, err
 	}
 
+	err = validateUserReferences(pm.pol, users)
+	if err != nil {
+		log.Error().Err(err).Msg("ambiguous policy users after updating users")
+	}
+
 	// SSH rules and per-node filters embed user identity outside the filter
 	// hash, so a user change needs a client refresh even when it did not move.
 	if hadPerNodeFilter || pm.needsPerNodeFilter ||

hscontrol/policy/v2/policy_test.go

@@ -2862,14 +2862,16 @@ func TestNewPolicyManager_DuplicateUsername(t *testing.T) {
   ]
 }`)
 
-	_, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice())
-	require.Error(t, err, "NewPolicyManager must reject policy with ambiguous username")
+	pm, err := NewPolicyManager(polB, users, types.Nodes{}.ViewSlice())
+	require.NoError(t, err, "ambiguous usernames must not block loading a stored policy")
+
+	_, err = pm.SetPolicy(polB)
 	require.ErrorIs(t, err, ErrMultipleUsersFound)
 	require.Contains(t, err.Error(), "yala@",
 		"error must name the offending token")
 }
 
-// Missing-user tokens stay tolerant per #2863; only multi-match blocks load.
+// Missing-user tokens stay tolerant per #2863.
 func TestNewPolicyManager_UnknownUsernameTolerant(t *testing.T) {
 	users := types.Users{
 		{ID: 1, Name: "alice"},
@@ -4013,9 +4015,9 @@ func TestSetNodesCachedResultsMatchFresh(t *testing.T) {
 	}
 }
 
-// TestFailedSetUsersKeepsCompiledPolicy renames alice to charlie and adds
-// a second bob, so the recompile resolves charlie's grant and then fails on
-// the ambiguous nodeAttrs target. A failed Set* must leave every compiled result as it was:
+// TestFailedSetUsersKeepsCompiledPolicy renames alice to charlie while
+// an unresolvable tag owner prevents the recompile from succeeding.
+// A failed Set* must leave every compiled result as it was:
 // a partial compile would hand alice's nodes charlie's port 22, and the
 // reverse rename, which SetUsers sees as no change, would not clear it.
 func TestFailedSetUsersKeepsCompiledPolicy(t *testing.T) {
@@ -4063,9 +4065,13 @@ func TestFailedSetUsersKeepsCompiledPolicy(t *testing.T) {
 
 	renamed := slices.Clone(users)
 	renamed[0].Name = "charlie"
-	renamed = append(renamed, types.User{ID: 3, Name: "bob"})
+	good := pm.pol.TagOwners
+	missing := Tag("tag:missing")
+	pm.pol.TagOwners = TagOwners{"tag:srv": Owners{&missing}}
 
 	_, _, err = pm.SetUsers(renamed)
+	pm.pol.TagOwners = good
+
 	require.Error(t, err)
 	require.Equal(t, before, read(pm, nodes[0]), "a failed SetUsers must not change compiled results")
 
@@ -4634,8 +4640,8 @@ func TestWithResolvedGroups(t *testing.T) {
 		}
 	}
 
-	require.ErrorIs(t,
-		resolved.resolvedGroups["group:ambiguous"].err, ErrMultipleUsersFound)
+	// An ambiguous member matches no one, as outside a policy write.
+	require.NoError(t, resolved.resolvedGroups["group:ambiguous"].err)
 
 	unknown := Group("group:unknown")
 	got, err := unknown.resolve(resolved, users, nodes)
@@ -4803,3 +4809,52 @@ func TestSetNodesUnapprovedRoutesKeepGeneration(t *testing.T) {
 		})
 	}
 }
+
+func TestAmbiguousUserKeepsOtherGroupMembers(t *testing.T) {
+	users := types.Users{
+		{ID: 1, Name: "alice"},
+		{ID: 2, Name: "dup"},
+		{ID: 3, Name: "dup"},
+	}
+
+	nodes := types.Nodes{
+		node("alice", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]),
+		node("first-dup", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]),
+		node("second-dup", "100.64.0.3", "fd7a:115c:a1e0::3", users[2]),
+	}
+	for i, n := range nodes {
+		n.ID = types.NodeID(i + 1) //nolint:gosec
+	}
+
+	pol := []byte(`{
+		"groups": {"group:members": ["alice@", "dup@"]},
+		"acls": [{"action":"accept", "src":["group:members"], "dst":["*:*"]}],
+		"nodeAttrs": [{"target":["group:members"], "attr":["randomize-client-port"]}]
+	}`)
+	pm, err := NewPolicyManager(pol, users[:2], nodes.ViewSlice())
+	require.NoError(t, err)
+	require.NotEmpty(t, pm.NodeCapMap(nodes[0].ID))
+	require.NotEmpty(t, pm.NodeCapMap(nodes[1].ID))
+	require.Empty(t, pm.NodeCapMap(nodes[2].ID))
+
+	_, usersChanged, err := pm.SetUsers(users)
+	require.NoError(t, err)
+	require.True(t, usersChanged)
+
+	filter, _ := pm.Filter()
+	require.Len(t, filter, 1)
+	require.ElementsMatch(t, []string{"100.64.0.1", "fd7a:115c:a1e0::1"}, filter[0].SrcIPs)
+	require.Equal(t, tailcfg.NodeCapMap{"randomize-client-port": nil}, pm.NodeCapMap(nodes[0].ID))
+	require.Empty(t, pm.NodeCapMap(nodes[1].ID))
+	require.Empty(t, pm.NodeCapMap(nodes[2].ID))
+
+	fresh, err := NewPolicyManager(pol, users, nodes.ViewSlice())
+	require.NoError(t, err)
+
+	freshFilter, _ := fresh.Filter()
+	require.Equal(t, filter, freshFilter)
+	require.Equal(t, pm.NodeCapMaps(), fresh.NodeCapMaps())
+
+	_, err = pm.SetPolicy(pol)
+	require.ErrorIs(t, err, ErrMultipleUsersFound)
+}

hscontrol/policy/v2/tailscale_users_compat_test.go

@@ -111,16 +111,6 @@ var userTokenSkipReasons = map[string]string{
 	// 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/no-email-two/boot":                ambiguousLoad,
-	"users-bare-at-nodeattrs/no-email-two/boot":            ambiguousLoad,
-	"users-bare-at-nodeattrs/no-email-two/runtime":         ambiguousLoad,
-	"users-bare-at-src/no-email-two/boot":                  ambiguousLoad,
-	"users-bare-at-tagowner/no-email-two/boot":             ambiguousLoad,
-	"users-unknown-direct/duplicate-unknown-email/boot":    ambiguousLoad,
-	"users-unknown-member/duplicate-unknown-email/boot":    ambiguousLoad,
-	"users-unknown-member/duplicate-unknown-email/runtime": ambiguousLoad,
-
 	"users-unknown-direct/duplicate-unknown-email/verdict": ambiguousWrite,
 	"users-unknown-member/duplicate-unknown-email/verdict": ambiguousWrite,
 
@@ -170,7 +160,6 @@ var userTokenSkipReasons = map[string]string{
 	"users-bare-at-tagowner/no-email-one/overlay-1/tag-owner":                     bareAtNoEmail,
 	"users-bare-at-tagowner/no-email-two/user-list":                               bareAtNoEmail,
 	"users-bare-at-tagowner/no-email-two/verdict":                                 bareAtNoEmail,
-	"users-bare-at-test-dst/no-email-two/user-list":                               bareAtNoEmail,
 	"users-bare-at-test-src/no-email-two/user-list":                               bareAtNoEmail,
 	"users-bare-at-test-src/okta-name-only/verdict":                               bareAtNoEmail,
 
@@ -180,7 +169,6 @@ var userTokenSkipReasons = map[string]string{
 const (
 	shortUsername        = "HEADSCALE_SHORT_USERNAME: headscale lets odin@ name the user called odin; SaaS matches it to no one"
 	shortUsernameRow     = "HEADSCALE_SHORT_USERNAME: a row storing the address as its name leaves odin@ nothing to match"
-	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"
 	usernameAnswersEmail = "USERNAME_ANSWERS_EMAIL_TOKEN: a username shaped like an unclaimed address takes that address's rights"

hscontrol/policy/v2/types.go

@@ -484,21 +484,14 @@ func (u *Username) Resolve(_ *Policy, users types.Users, nodes views.Slice[types
 }
 
 func (u *Username) resolve(_ *Policy, users types.Users, nodes views.Slice[types.NodeView]) (*netipx.IPSet, error) {
-	var (
-		ips  netipx.IPSetBuilder
-		errs []error
-	)
+	var ips netipx.IPSetBuilder
 
 	user, err := u.resolveUser(users)
-	if errors.Is(err, ErrUserNotFound) {
-		// A policy may name users before they register (#2863). Report it
-		// as an empty set, not an error: callers that bail on error would
-		// otherwise drop a whole group for one missing member (#3513).
-		return &netipx.IPSet{}, nil
-	}
-
 	if err != nil {
-		errs = append(errs, err)
+		// Users may be missing before registration (#2863), or become
+		// ambiguous after a login. An empty set preserves other group
+		// members (#3513); the policy write boundary rejects ambiguity.
+		return &netipx.IPSet{}, nil //nolint:nilerr // Unresolved users intentionally match no nodes.
 	}
 
 	for _, node := range nodes.All() {
@@ -517,7 +510,7 @@ func (u *Username) resolve(_ *Policy, users types.Users, nodes views.Slice[types
 		}
 	}
 
-	return buildIPSetMultiErr(&ips, errs)
+	return ips.IPSet()
 }
 
 // Group is a special string which is always prefixed with `group:`.

hscontrol/state/persist_test.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"database/sql"
 	"encoding/json"
 	"errors"
 	"maps"
@@ -2775,3 +2776,48 @@ func TestUpdateUserRefreshesNodeCopiesWhenPolicyUpdateFails(t *testing.T) {
 	assert.Equal(t, "renamed-user", updated.User().Name(),
 		"the committed row and its cached copy must agree")
 }
+
+func TestUserNameCollisionDoesNotBlockCreationOrRestart(t *testing.T) {
+	dbPath := t.TempDir() + "/headscale.db"
+	cfg := persistTestConfig(dbPath)
+	database, err := db.NewHeadscaleDatabase(cfg)
+	require.NoError(t, err)
+
+	user := database.CreateUserForTest("dup")
+	node := database.CreateRegisteredNodeForTest(user, "duplicate-name-node")
+	_, err = database.SetPolicy(`{
+		"acls": [],
+		"nodeAttrs": [{"target":["dup@"], "attr":["randomize-client-port"]}]
+	}`)
+	require.NoError(t, err)
+	require.NoError(t, database.Close())
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		if s != nil {
+			_ = s.Close()
+		}
+	})
+	require.NotEmpty(t, s.polMan.NodeCapMap(node.ID))
+
+	created, _, err := s.CreateUser(types.User{
+		Name:               "dup",
+		Provider:           "oidc",
+		ProviderIdentifier: sql.NullString{String: "https://idp.example/second", Valid: true},
+	})
+	require.NoError(t, err, "a colliding user must not prevent user creation")
+	require.NotNil(t, created)
+
+	_, _, err = s.CreateUser(types.User{Name: "later"})
+	require.NoError(t, err, "a previous collision must not prevent later user creation")
+	assert.Empty(t, s.polMan.NodeCapMap(node.ID))
+	require.NoError(t, s.Close())
+	s = nil
+
+	reopened := persistTestReopen(t, dbPath)
+	users, err := reopened.ListAllUsers()
+	require.NoError(t, err)
+	require.Len(t, users, 3)
+	require.Empty(t, reopened.polMan.NodeCapMap(node.ID))
+}