diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index e1544f7a2..99839ac50 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -16,9 +16,9 @@ "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 } +// 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 { + out = append(out, p) + } + } + slices.SortFunc(out, func(a, b *ipn.LoginProfile) int { + return cmpx.Compare(a.Name, b.Name) + }) + return out +} + // 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) { - for _, p := range pm.knownProfiles { - if p.LocalUserID == pm.currentUserID && f(p) { + all := pm.allProfiles() + out = all[:0] + for _, p := range all { + if f(p) { out = append(out, p) } } 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 - } +// 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.NodeID == nodeID && p.ControlURL == controlURL - }) -} - -// 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 - } - 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 @@ func (pm *profileManager) CurrentProfile() ipn.LoginProfile { 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 index 5e045d5f2..fc61d2671 100644 --- a/ipn/ipnlocal/profiles_notwindows.go +++ b/ipn/ipnlocal/profiles_notwindows.go @@ -16,9 +16,7 @@ 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 index 80b41a243..8d881376e 100644 --- a/ipn/ipnlocal/profiles_test.go +++ b/ipn/ipnlocal/profiles_test.go @@ -203,11 +203,8 @@ type step struct { {reauth, user1Node1}, }, profs: []*persist.Persist{ - // BUG: This is incorrect, and should be: - // user1Node1, - // user2Node2, - user1Node1, user1Node1, + user2Node2, }, }, { @@ -218,11 +215,8 @@ type step struct { {reauth, user2Node1}, }, profs: []*persist.Persist{ - // BUG: This is incorrect, and should be: - // user2Node1, - // user3Node3, - user1Node1, user2Node1, + user3Node3, }, }, { @@ -233,11 +227,8 @@ type step struct { {reauth, user1Node2}, }, profs: []*persist.Persist{ - // BUG: This is incorrect, and should be: - // user1Node2, - // user3Node3, - user1Node1, user1Node2, + user3Node3, }, }, {