ipn/ipnlocal: fix profile duplication

Maisem Ali · maisem · commit 3e255d76e15f · 2023-08-08T13:43:37.000-06:00
We would only look for duplicate profiles when a new login occurred but when using `--force-reauth` we could switch users which would end up with duplicate profiles. Updates tailscale#7726 Signed-off-by: Maisem Ali <maisem@tailscale.com>
diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go
@@ -16,9 +16,9 @@ import (
 	"golang.org/x/exp/slices"
 	"tailscale.com/envknob"
 	"tailscale.com/ipn"
-	"tailscale.com/tailcfg"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/cmpx"
 	"tailscale.com/util/winutil"
 )
 
@@ -33,15 +33,9 @@ type profileManager struct {
 	logf  logger.Logf
 
 	currentUserID  ipn.WindowsUserID
-	knownProfiles  map[ipn.ProfileID]*ipn.LoginProfile
-	currentProfile *ipn.LoginProfile // always non-nil
-	prefs          ipn.PrefsView     // always Valid.
-
-	// isNewProfile is a sentinel value that indicates that the
-	// current profile is new and has not been saved to disk yet.
-	// It is reset to false after a call to SetPrefs with a filled
-	// in LoginName.
-	isNewProfile bool
+	knownProfiles  map[ipn.ProfileID]*ipn.LoginProfile // always non-nil
+	currentProfile *ipn.LoginProfile                   // always non-nil
+	prefs          ipn.PrefsView                       // always Valid.
 }
 
 func (pm *profileManager) dlogf(format string, args ...any) {
@@ -107,40 +101,45 @@ func (pm *profileManager) SetCurrentUserID(uid ipn.WindowsUserID) error {
 	}
 	pm.currentProfile = prof
 	pm.prefs = prefs
-	pm.isNewProfile = false
 	return nil
 }
 
-// matchingProfiles returns all profiles that match the given predicate and
-// belong to the currentUserID.
-func (pm *profileManager) matchingProfiles(f func(*ipn.LoginProfile) bool) (out []*ipn.LoginProfile) {
+// allProfiles returns all profiles that belong to the currentUserID.
+// The returned profiles are sorted by Name.
+func (pm *profileManager) allProfiles() (out []*ipn.LoginProfile) {
 	for _, p := range pm.knownProfiles {
-		if p.LocalUserID == pm.currentUserID && f(p) {
+		if p.LocalUserID == pm.currentUserID {
 			out = append(out, p)
 		}
 	}
+	slices.SortFunc(out, func(a, b *ipn.LoginProfile) int {
+		return cmpx.Compare(a.Name, b.Name)
+	})
 	return out
 }
 
-// findProfilesByNodeID returns all profiles that have the provided nodeID and
-// belong to the same control server.
-func (pm *profileManager) findProfilesByNodeID(controlURL string, nodeID tailcfg.StableNodeID) []*ipn.LoginProfile {
-	if nodeID.IsZero() {
-		return nil
+// matchingProfiles returns all profiles that match the given predicate and
+// belong to the currentUserID.
+// The returned profiles are sorted by Name.
+func (pm *profileManager) matchingProfiles(f func(*ipn.LoginProfile) bool) (out []*ipn.LoginProfile) {
+	all := pm.allProfiles()
+	out = all[:0]
+	for _, p := range all {
+		if f(p) {
+			out = append(out, p)
+		}
 	}
-	return pm.matchingProfiles(func(p *ipn.LoginProfile) bool {
-		return p.NodeID == nodeID && p.ControlURL == controlURL
-	})
+	return out
 }
 
-// findProfilesByUserID returns all profiles that have the provided userID and
-// belong to the same control server.
-func (pm *profileManager) findProfilesByUserID(controlURL string, userID tailcfg.UserID) []*ipn.LoginProfile {
-	if userID.IsZero() {
-		return nil
-	}
+// findMatchinProfiles returns all profiles that represent the same node/user as
+// prefs.
+// The returned profiles are sorted by Name.
+func (pm *profileManager) findMatchingProfiles(prefs *ipn.Prefs) []*ipn.LoginProfile {
 	return pm.matchingProfiles(func(p *ipn.LoginProfile) bool {
-		return p.UserProfile.ID == userID && p.ControlURL == controlURL
+		return p.ControlURL == prefs.ControlURL &&
+			(p.UserProfile.ID == prefs.Persist.UserProfile.ID ||
+				p.NodeID == prefs.Persist.NodeID)
 	})
 }
 
@@ -206,40 +205,47 @@ func init() {
 // It also saves the prefs to the StateStore. It stores a copy of the
 // provided prefs, which may be accessed via CurrentPrefs.
 func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView) error {
-	prefs := prefsIn.AsStruct().View()
-	newPersist := prefs.Persist().AsStruct()
+	prefs := prefsIn.AsStruct()
+	newPersist := prefs.Persist
 	if newPersist == nil || newPersist.NodeID == "" || newPersist.UserProfile.LoginName == "" {
-		return pm.setPrefsLocked(prefs)
+		// We don't know anything about this profile, so ignore it for now.
+		return pm.setPrefsLocked(prefs.View())
 	}
 	up := newPersist.UserProfile
 	if up.DisplayName == "" {
 		up.DisplayName = up.LoginName
 	}
 	cp := pm.currentProfile
-	if pm.isNewProfile {
-		pm.isNewProfile = false
-		// Check if we already have a profile for this user.
-		existing := pm.findProfilesByUserID(prefs.ControlURL(), newPersist.UserProfile.ID)
-		// Also check if we have a profile with the same NodeID.
-		existing = append(existing, pm.findProfilesByNodeID(prefs.ControlURL(), newPersist.NodeID)...)
-		if len(existing) == 0 {
-			cp.ID, cp.Key = newUnusedID(pm.knownProfiles)
-		} else {
-			// Only one profile per user/nodeID should exist.
-			for _, p := range existing[1:] {
-				// Best effort cleanup.
-				pm.DeleteProfile(p.ID)
+	// Check if we already have an existing profile that matches the user/node.
+	if existing := pm.findMatchingProfiles(prefs); len(existing) > 0 {
+		// We already have a profile for this user/node we should reuse it. Also
+		// cleanup any other duplicate profiles.
+		cp = existing[0]
+		existing = existing[1:]
+		for _, p := range existing {
+			// Clear the state.
+			if err := pm.store.WriteState(p.Key, nil); err != nil {
+				// We couldn't delete the state, so keep the profile around.
+				continue
 			}
-			cp = existing[0]
+			// Remove the profile, knownProfiles will be persisted below.
+			delete(pm.knownProfiles, p.ID)
 		}
+	} else if cp.ID == "" {
+		// We didn't have an existing profile, so create a new one.
+		cp.ID, cp.Key = newUnusedID(pm.knownProfiles)
 		cp.LocalUserID = pm.currentUserID
+	} else {
+		// This means that there was a force-reauth as a new node that
+		// we haven't seen before.
 	}
-	if prefs.ProfileName() != "" {
-		cp.Name = prefs.ProfileName()
+
+	if prefs.ProfileName != "" {
+		cp.Name = prefs.ProfileName
 	} else {
 		cp.Name = up.LoginName
 	}
-	cp.ControlURL = prefs.ControlURL()
+	cp.ControlURL = prefs.ControlURL
 	cp.UserProfile = newPersist.UserProfile
 	cp.NodeID = newPersist.NodeID
 	pm.knownProfiles[cp.ID] = cp
@@ -250,7 +256,7 @@ func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView) error {
 	if err := pm.setAsUserSelectedProfileLocked(); err != nil {
 		return err
 	}
-	if err := pm.setPrefsLocked(prefs); err != nil {
+	if err := pm.setPrefsLocked(prefs.View()); err != nil {
 		return err
 	}
 	return nil
@@ -273,7 +279,7 @@ func newUnusedID(knownProfiles map[ipn.ProfileID]*ipn.LoginProfile) (ipn.Profile
 // is not new.
 func (pm *profileManager) setPrefsLocked(clonedPrefs ipn.PrefsView) error {
 	pm.prefs = clonedPrefs
-	if pm.isNewProfile {
+	if pm.currentProfile.ID == "" {
 		return nil
 	}
 	if err := pm.writePrefsToStore(pm.currentProfile.Key, pm.prefs); err != nil {
@@ -295,12 +301,9 @@ func (pm *profileManager) writePrefsToStore(key ipn.StateKey, prefs ipn.PrefsVie
 
 // Profiles returns the list of known profiles.
 func (pm *profileManager) Profiles() []ipn.LoginProfile {
-	profiles := pm.matchingProfiles(func(*ipn.LoginProfile) bool { return true })
-	slices.SortFunc(profiles, func(a, b *ipn.LoginProfile) int {
-		return strings.Compare(a.Name, b.Name)
-	})
-	out := make([]ipn.LoginProfile, 0, len(profiles))
-	for _, p := range profiles {
+	allProfiles := pm.allProfiles()
+	out := make([]ipn.LoginProfile, 0, len(allProfiles))
+	for _, p := range allProfiles {
 		out = append(out, *p)
 	}
 	return out
@@ -328,7 +331,6 @@ func (pm *profileManager) SwitchProfile(id ipn.ProfileID) error {
 	}
 	pm.prefs = prefs
 	pm.currentProfile = kp
-	pm.isNewProfile = false
 	return pm.setAsUserSelectedProfileLocked()
 }
 
@@ -380,7 +382,7 @@ var errProfileNotFound = errors.New("profile not found")
 func (pm *profileManager) DeleteProfile(id ipn.ProfileID) error {
 	metricDeleteProfile.Add(1)
 
-	if id == "" && pm.isNewProfile {
+	if id == "" {
 		// Deleting the in-memory only new profile, just create a new one.
 		pm.NewProfile()
 		return nil
@@ -431,7 +433,6 @@ func (pm *profileManager) NewProfile() {
 	metricNewProfile.Add(1)
 
 	pm.prefs = defaultPrefs
-	pm.isNewProfile = true
 	pm.currentProfile = &ipn.LoginProfile{}
 }
 
diff --git a/ipn/ipnlocal/profiles_notwindows.go b/ipn/ipnlocal/profiles_notwindows.go
@@ -16,9 +16,7 @@ import (
 func (pm *profileManager) loadLegacyPrefs() (string, ipn.PrefsView, error) {
 	k := ipn.LegacyGlobalDaemonStateKey
 	switch {
-	case runtime.GOOS == "ios":
-		k = "ipn-go-bridge"
-	case version.IsSandboxedMacOS():
+	case runtime.GOOS == "ios", version.IsSandboxedMacOS():
 		k = "ipn-go-bridge"
 	case runtime.GOOS == "android":
 		k = "ipn-android"
diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go
@@ -203,11 +203,8 @@ func TestProfileDupe(t *testing.T) {
 				{reauth, user1Node1},
 			},
 			profs: []*persist.Persist{
-				// BUG: This is incorrect, and should be:
-				// user1Node1,
-				// user2Node2,
-				user1Node1,
 				user1Node1,
+				user2Node2,
 			},
 		},
 		{
@@ -218,11 +215,8 @@ func TestProfileDupe(t *testing.T) {
 				{reauth, user2Node1},
 			},
 			profs: []*persist.Persist{
-				// BUG: This is incorrect, and should be:
-				// user2Node1,
-				// user3Node3,
-				user1Node1,
 				user2Node1,
+				user3Node3,
 			},
 		},
 		{
@@ -233,11 +227,8 @@ func TestProfileDupe(t *testing.T) {
 				{reauth, user1Node2},
 			},
 			profs: []*persist.Persist{
-				// BUG: This is incorrect, and should be:
-				// user1Node2,
-				// user3Node3,
-				user1Node1,
 				user1Node2,
+				user3Node3,
 			},
 		},
 		{
