From 2f4fca65a1438a2f9cb695ead93465cef1013cf5 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Wed, 16 Nov 2022 16:17:36 +0500 Subject: [PATCH] ipn/ipnlocal: prevent duplicate profiles of the same user Updates #713 Signed-off-by: Maisem Ali --- ipn/ipnlocal/profiles.go | 21 ++++++++++++++++-- ipn/ipnlocal/profiles_test.go | 42 ++++++++++++++++++++++++++++++++--- ipn/prefs.go | 18 +++++++++++++-- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index cb06d625a..86f427289 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -14,6 +14,7 @@ import ( "golang.org/x/exp/slices" "tailscale.com/ipn" + "tailscale.com/tailcfg" "tailscale.com/types/logger" "tailscale.com/util/clientmetric" "tailscale.com/util/strs" @@ -69,6 +70,15 @@ func (pm *profileManager) SetCurrentUser(uid string) error { return nil } +func (pm *profileManager) findProfileByUserID(userID tailcfg.UserID) *ipn.LoginProfile { + for _, p := range pm.knownProfiles { + if p.UserProfile.ID == userID { + return p + } + } + return nil +} + func (pm *profileManager) findProfileByName(name string) *ipn.LoginProfile { for _, p := range pm.knownProfiles { if p.Name == name { @@ -129,8 +139,14 @@ func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView) error { wasNamedWithLoginName := cp.Name == cp.UserProfile.LoginName if pm.isNewProfile { pm.isNewProfile = false - cp.ID, cp.Key = newUnusedID(pm.knownProfiles) - cp.Name = ps.LoginName + // Check if we already have a profile for this user. + existing := pm.findProfileByUserID(ps.UserProfile.ID) + if existing != nil && existing.ID != "" { + cp = existing + } else { + cp.ID, cp.Key = newUnusedID(pm.knownProfiles) + cp.Name = ps.LoginName + } cp.UserProfile = ps.UserProfile cp.LocalUserID = pm.currentUserID } else { @@ -140,6 +156,7 @@ func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView) error { cp.Name = ps.LoginName } pm.knownProfiles[cp.ID] = cp + pm.currentProfile = cp if err := pm.writeKnownProfiles(); err != nil { return err } diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go index 38e631527..51b4f1779 100644 --- a/ipn/ipnlocal/profiles_test.go +++ b/ipn/ipnlocal/profiles_test.go @@ -9,6 +9,8 @@ import ( "tailscale.com/ipn" "tailscale.com/ipn/store/mem" + "tailscale.com/tailcfg" + "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/persist" ) @@ -53,15 +55,27 @@ func TestProfileManagement(t *testing.T) { t.Fatal(err) } // Use Hostname as a proxy for all prefs. - if got.Hostname() != wantProfiles[p.Name].Hostname() { - t.Fatalf("Prefs for profile %q = %v; want %v", p, got.Pretty(), wantProfiles[p.Name].Pretty()) + if !got.Equals(wantProfiles[p.Name]) { + t.Fatalf("Prefs for profile %q =\n got=%+v\nwant=%v", p, got.Pretty(), wantProfiles[p.Name].Pretty()) } } } + logins := make(map[string]tailcfg.UserID) setPrefs := func(t *testing.T, loginName string) ipn.PrefsView { + t.Helper() p := pm.CurrentPrefs().AsStruct() + id := logins[loginName] + if id.IsZero() { + id = tailcfg.UserID(len(logins) + 1) + logins[loginName] = id + } p.Persist = &persist.Persist{ - LoginName: loginName, + LoginName: loginName, + PrivateNodeKey: key.NewNode(), + UserProfile: tailcfg.UserProfile{ + ID: id, + LoginName: loginName, + }, } if err := pm.SetPrefs(p.View()); err != nil { t.Fatal(err) @@ -117,6 +131,18 @@ func TestProfileManagement(t *testing.T) { t.Fatal(err) } checkProfiles(t) + + t.Logf("Create new profile again") + pm.NewProfile() + wantCurProfile = "" + wantProfiles[""] = emptyPrefs + checkProfiles(t) + + t.Logf("Login with the existing profile") + wantProfiles["user@2.example.com"] = setPrefs(t, "user@2.example.com") + delete(wantProfiles, "") + wantCurProfile = "user@2.example.com" + checkProfiles(t) } // TestProfileManagementWindows tests going into and out of Unattended mode on @@ -143,11 +169,21 @@ func TestProfileManagementWindows(t *testing.T) { t.Fatalf("CurrentPrefs = %+v; want %+v", p.Pretty(), wantProfiles[wantCurProfile].Pretty()) } } + logins := make(map[string]tailcfg.UserID) setPrefs := func(t *testing.T, loginName string, forceDaemon bool) ipn.PrefsView { + id := logins[loginName] + if id.IsZero() { + id = tailcfg.UserID(len(logins) + 1) + logins[loginName] = id + } p := pm.CurrentPrefs().AsStruct() p.ForceDaemon = forceDaemon p.Persist = &persist.Persist{ LoginName: loginName, + UserProfile: tailcfg.UserProfile{ + ID: id, + LoginName: loginName, + }, } if err := pm.SetPrefs(p.View()); err != nil { t.Fatal(err) diff --git a/ipn/prefs.go b/ipn/prefs.go index 109c59bca..00fb3f2e0 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -696,13 +696,27 @@ type ProfileID string // LoginProfile represents a single login profile as managed // by the ProfileManager. type LoginProfile struct { - ID ProfileID + // ID is a unique identifier for this profile. + // It is assigned on creation and never changes. + // It may seem redundant to have both ID and UserProfile.ID + // but they are different things. UserProfile.ID may change + // over time (e.g. if a device is tagged). + ID ProfileID + + // Name is the user-visible name of this profile. + // It is filled in from the UserProfile.LoginName field. Name string - Key StateKey + // Key is the StateKey under which the profile is stored. + // It is assigned once at profile creation time and never changes. + Key StateKey + + // UserProfile is the server provided UserProfile for this profile. + // This is updated whenever the server provides a new UserProfile. UserProfile tailcfg.UserProfile // LocalUserID is the user ID of the user who created this profile. // It is only relevant on Windows where we have a multi-user system. + // It is assigned once at profile creation time and never changes. LocalUserID string }