From d4f6efa1dfb7974c5ee9a99bc14aa4735a8220eb Mon Sep 17 00:00:00 2001
From: Maisem Ali <maisem@tailscale.com>
Date: Tue, 22 Nov 2022 05:34:28 -0800
Subject: [PATCH] ipn/ipnlocal: handle case when selected profile is deleted

Profile keys are not deleted but are instead set to `nil` which results
in getting a nil error and we were not handling that correctly.

Updates #713

Signed-off-by: Maisem Ali <maisem@tailscale.com>
---
 ipn/ipnlocal/profiles.go      | 46 ++++++++++++++++++----------
 ipn/ipnlocal/profiles_test.go | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go
index 1e996c357..f67526788 100644
--- a/ipn/ipnlocal/profiles.go
+++ b/ipn/ipnlocal/profiles.go
@@ -51,22 +51,38 @@ func (pm *profileManager) SetCurrentUser(uid string) error {
 	if pm.currentUserID == uid {
 		return nil
 	}
+	prev := pm.currentUserID
 	pm.currentUserID = uid
-	cpk := ipn.CurrentProfileKey(uid)
-	if b, err := pm.store.ReadState(cpk); err == nil {
-		pk := ipn.StateKey(string(b))
-		prefs, err := pm.loadSavedPrefs(pk)
-		if err != nil {
-			return err
-		}
-		pm.currentProfile = pm.findProfileByKey(pk)
-		pm.prefs = prefs
-		pm.isNewProfile = false
-	} else if err == ipn.ErrStateNotExist {
+	if uid == "" && prev != "" {
+		// This is a local user logout, or app shutdown.
+		// Clear the current profile.
+		pm.NewProfile()
+		return nil
+	}
+
+	// Read the CurrentProfileKey from the store which stores
+	// the selected profile for the current user.
+	b, err := pm.store.ReadState(ipn.CurrentProfileKey(uid))
+	if err == ipn.ErrStateNotExist || len(b) == 0 {
+		pm.NewProfile()
+		return nil
+	}
+
+	// Now attempt to load the profile using the key we just read.
+	pk := ipn.StateKey(string(b))
+	prof := pm.findProfileByKey(pk)
+	if prof == nil {
+		pm.NewProfile()
+		return nil
+	}
+	prefs, err := pm.loadSavedPrefs(pk)
+	if err != nil {
 		pm.NewProfile()
-	} else {
 		return err
 	}
+	pm.currentProfile = prof
+	pm.prefs = prefs
+	pm.isNewProfile = false
 	return nil
 }
 
@@ -299,10 +315,10 @@ func (pm *profileManager) setAsUserSelectedProfileLocked() error {
 
 func (pm *profileManager) loadSavedPrefs(key ipn.StateKey) (ipn.PrefsView, error) {
 	bs, err := pm.store.ReadState(key)
+	if err == ipn.ErrStateNotExist || len(bs) == 0 {
+		return emptyPrefs, nil
+	}
 	if err != nil {
-		if err == ipn.ErrStateNotExist {
-			return emptyPrefs, nil
-		}
 		return ipn.PrefsView{}, err
 	}
 	savedPrefs, err := ipn.PrefsFromBytes(bs)
diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go
index 75e98c7d2..3bbfedfb1 100644
--- a/ipn/ipnlocal/profiles_test.go
+++ b/ipn/ipnlocal/profiles_test.go
@@ -16,6 +16,62 @@
 	"tailscale.com/types/persist"
 )
 
+func TestProfileCurrentUserSwitch(t *testing.T) {
+	store := new(mem.Store)
+
+	pm, err := newProfileManagerWithGOOS(store, logger.Discard, "", "linux")
+	if err != nil {
+		t.Fatal(err)
+	}
+	id := 0
+	newProfile := func(t *testing.T, loginName string) ipn.PrefsView {
+		id++
+		t.Helper()
+		pm.NewProfile()
+		p := pm.CurrentPrefs().AsStruct()
+		p.Persist = &persist.Persist{
+			NodeID:         tailcfg.StableNodeID(fmt.Sprint(id)),
+			LoginName:      loginName,
+			PrivateNodeKey: key.NewNode(),
+			UserProfile: tailcfg.UserProfile{
+				ID:        tailcfg.UserID(id),
+				LoginName: loginName,
+			},
+		}
+		if err := pm.SetPrefs(p.View()); err != nil {
+			t.Fatal(err)
+		}
+		return p.View()
+	}
+
+	pm.SetCurrentUser("user1")
+	newProfile(t, "user1")
+	cp := pm.currentProfile
+	pm.DeleteProfile(cp.ID)
+	if pm.currentProfile == nil {
+		t.Fatal("currentProfile is nil")
+	} else if pm.currentProfile.ID != "" {
+		t.Fatalf("currentProfile.ID = %q, want empty", pm.currentProfile.ID)
+	}
+	if !pm.CurrentPrefs().Equals(emptyPrefs) {
+		t.Fatalf("CurrentPrefs() = %v, want emptyPrefs", pm.CurrentPrefs().Pretty())
+	}
+
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "", "linux")
+	if err != nil {
+		t.Fatal(err)
+	}
+	pm.SetCurrentUser("user1")
+	if pm.currentProfile == nil {
+		t.Fatal("currentProfile is nil")
+	} else if pm.currentProfile.ID != "" {
+		t.Fatalf("currentProfile.ID = %q, want empty", pm.currentProfile.ID)
+	}
+	if !pm.CurrentPrefs().Equals(emptyPrefs) {
+		t.Fatalf("CurrentPrefs() = %v, want emptyPrefs", pm.CurrentPrefs().Pretty())
+	}
+}
+
 func TestProfileList(t *testing.T) {
 	store := new(mem.Store)