Add prefs.LoggedOut to fix several state machine bugs.

Fixes: tailscale/corp#1660

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

ipn/ipnlocal/local.go

@@ -445,6 +445,10 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) {
 		// Auth completed, unblock the engine
 		b.blockEngineUpdates(false)
 		b.authReconfig()
+		b.EditPrefs(&ipn.MaskedPrefs{
+			LoggedOutSet: true,
+			Prefs:        ipn.Prefs{LoggedOut: false},
+		})
 		b.send(ipn.Notify{LoginFinished: &empty.Message{}})
 	}
 
@@ -643,7 +647,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		return errors.New("no state key or prefs provided")
 	}
 
-	defer b.stateMachine()
 	if opts.Prefs != nil {
 		b.logf("Start: %v", opts.Prefs.Pretty())
 	} else {
@@ -707,6 +710,8 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		}
 	}
 
+	loggedOut := b.prefs.LoggedOut
+
 	b.inServerMode = b.prefs.ForceDaemon
 	b.serverURL = b.prefs.ControlURLOrDefault()
 	hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...)
@@ -804,8 +809,16 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 	b.send(ipn.Notify{BackendLogID: &blid})
 	b.send(ipn.Notify{Prefs: prefs})
 
-	if wantRunning {
+	if wantRunning && !loggedOut {
 		cc.Login(nil, controlclient.LoginDefault)
+
+		b.mu.Lock()
+		b.state = ipn.Starting
+		b.mu.Unlock()
+
+		b.send(ipn.Notify{State: &b.state})
+	} else {
+		b.stateMachine()
 	}
 	return nil
 }
@@ -2118,12 +2131,17 @@ func (b *LocalBackend) nextState() ipn.State {
 		netMap      = b.netMap
 		state       = b.state
 		wantRunning = b.prefs.WantRunning
+		loggedOut   = b.prefs.LoggedOut
+		hasNodeKey  = b.prefs.Persist != nil &&
+			!b.prefs.Persist.PrivateNodeKey.IsZero()
 	)
 	b.mu.Unlock()
 
 	switch {
+	case !wantRunning && !loggedOut && hasNodeKey:
+		return ipn.Stopped
 	case netMap == nil:
-		if cc.AuthCantContinue() {
+		if cc.AuthCantContinue() || loggedOut {
 			// Auth was interrupted or waiting for URL visit,
 			// so it won't proceed without human help.
 			return ipn.NeedsLogin
@@ -2238,7 +2256,8 @@ func (b *LocalBackend) logout(ctx context.Context, sync bool) error {
 
 	b.EditPrefs(&ipn.MaskedPrefs{
 		WantRunningSet: true,
-		Prefs:          ipn.Prefs{WantRunning: false},
+		LoggedOutSet:   true,
+		Prefs:          ipn.Prefs{WantRunning: false, LoggedOut: true},
 	})
 
 	if cc == nil {

ipn/ipnlocal/state_test.go

@@ -321,6 +321,10 @@ func TestStateMachine(t *testing.T) {
 		assert.NotNil(nn[0].Prefs)
 		assert.NotNil(nn[1].State)
 		prefs := *nn[0].Prefs
+		// Note: a totally fresh system has Prefs.LoggedOut=false by
+		// default. We are logged out, but not because the user asked
+		// for it, so it doesn't count as Prefs.LoggedOut==true.
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
 		assert.Equal(false, prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, *nn[1].State)
 		assert.Equal(ipn.NeedsLogin, b.State())
@@ -343,6 +347,7 @@ func TestStateMachine(t *testing.T) {
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
 		assert.NotNil(nn[1].State)
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
 		assert.Equal(false, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, *nn[1].State)
 		assert.Equal(ipn.NeedsLogin, b.State())
@@ -383,6 +388,7 @@ func TestStateMachine(t *testing.T) {
 		// Trying to log in automatically sets WantRunning.
 		// BUG: that should have happened right after Login().
 		assert.NotNil(nn[0].Prefs)
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
 		assert.Equal(true, nn[0].Prefs.WantRunning)
 	}
 
@@ -555,16 +561,18 @@ func TestStateMachine(t *testing.T) {
 
 	// User wants to logout.
 	t.Logf("\n\nLogout (async)")
-	notifies.expect(1)
+	notifies.expect(2)
 	b.Logout()
 	{
-		nn := notifies.drain(1)
-		assert.Equal([]string{"StartLogout"}, cc.getCalls())
-		assert.NotNil(nn[0].Prefs)
-		// NOTE: WantRunning is false here, which is right. But
-		// it changes to true in subsequent logout attempts, which is
-		// wrong in those attempts.
-		assert.Equal(false, nn[0].Prefs.WantRunning)
+		nn := notifies.drain(2)
+		// BUG: now is not the time to unpause.
+		assert.Equal([]string{"unpause", "StartLogout"}, cc.getCalls())
+		assert.NotNil(nn[0].State)
+		assert.NotNil(nn[1].Prefs)
+		assert.Equal(ipn.NeedsLogin, *nn[0].State)
+		assert.Equal(true, nn[1].Prefs.LoggedOut)
+		assert.Equal(false, nn[1].Prefs.WantRunning)
+		assert.Equal(ipn.NeedsLogin, b.State())
 	}
 
 	// Let's make the logout succeed.
@@ -574,21 +582,27 @@ func TestStateMachine(t *testing.T) {
 	cc.send(nil, "", false, nil)
 	{
 		nn := notifies.drain(1)
-		// BUG: now is not the time to unpause.
-		assert.Equal([]string{"unpause"}, cc.getCalls())
-		assert.NotNil(nn[0].State)
-		assert.Equal(ipn.NeedsLogin, *nn[0].State)
+		assert.Equal([]string{}, cc.getCalls())
+		assert.NotNil(nn[0].Prefs)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
+		// BUG: WantRunning should be false after manual logout.
+		assert.Equal(true, nn[0].Prefs.WantRunning)
+		assert.Equal(ipn.NeedsLogin, b.State())
 	}
 
 	// A second logout should do nothing, since the prefs haven't changed.
 	t.Logf("\n\nLogout2 (async)")
-	notifies.expect(0)
+	notifies.expect(1)
 	b.Logout()
 	{
-		notifies.drain(0)
+		nn := notifies.drain(1)
 		// BUG: the backend has already called StartLogout, and we're
 		// still logged out. So it shouldn't call it again.
 		assert.Equal([]string{"StartLogout"}, cc.getCalls())
+		// BUG: Prefs should not change here. Already logged out.
+		assert.NotNil(nn[0].Prefs)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
+		assert.Equal(false, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, b.State())
 	}
 
@@ -601,6 +615,7 @@ func TestStateMachine(t *testing.T) {
 		nn := notifies.drain(1)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
 		// BUG: second logout shouldn't cause WantRunning->true !!
 		assert.Equal(true, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, b.State())
@@ -616,6 +631,7 @@ func TestStateMachine(t *testing.T) {
 		nn := notifies.drain(1)
 		assert.Equal([]string{"Logout"}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
 		assert.Equal(false, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, b.State())
 	}
@@ -629,6 +645,7 @@ func TestStateMachine(t *testing.T) {
 		nn := notifies.drain(1)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
 		// BUG: third logout shouldn't cause WantRunning->true !!
 		assert.Equal(true, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, b.State())
@@ -664,16 +681,14 @@ func TestStateMachine(t *testing.T) {
 	{
 		// BUG: We already called Shutdown(), no need to do it again.
 		// BUG: Way too soon for UpdateEndpoints.
-		// BUG: We've forgotten that we logged out earlier.
-		//   Shouldn't cc.Login() until the user engages, or else we
-		//   defeat the "no traffic until `tailscale up`" logic.
-		// BUG: strictly, it should pause, not unpause, here.
-		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "Login", "unpause"}, cc.getCalls())
+		// BUG: don't unpause because we're not logged in.
+		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "unpause"}, cc.getCalls())
 
 		nn := notifies.drain(2)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
 		assert.NotNil(nn[1].State)
+		assert.Equal(true, nn[0].Prefs.LoggedOut)
 		assert.Equal(true, nn[0].Prefs.WantRunning)
 		assert.Equal(ipn.NeedsLogin, *nn[1].State)
 		assert.Equal(ipn.NeedsLogin, b.State())
@@ -684,17 +699,19 @@ func TestStateMachine(t *testing.T) {
 	// (This simulates an admin reviving a key that you previously
 	// disabled.)
 	t.Logf("\n\nLoginFinished3")
-	notifies.expect(2)
+	notifies.expect(3)
 	cc.setAuthBlocked(false)
 	cc.send(nil, "", true, &netmap.NetworkMap{
 		MachineStatus: tailcfg.MachineAuthorized,
 	})
 	{
-		nn := notifies.drain(2)
+		nn := notifies.drain(3)
 		assert.Equal([]string{"unpause"}, cc.getCalls())
-		assert.NotNil(nn[0].LoginFinished)
-		assert.NotNil(nn[1].State)
-		assert.Equal(ipn.Starting, *nn[1].State)
+		assert.NotNil(nn[0].Prefs)
+		assert.NotNil(nn[1].LoginFinished)
+		assert.NotNil(nn[2].State)
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
+		assert.Equal(ipn.Starting, *nn[2].State)
 	}
 
 	// Now we've logged in successfully. Let's disconnect.
@@ -711,6 +728,7 @@ func TestStateMachine(t *testing.T) {
 		assert.NotNil(nn[0].State)
 		assert.NotNil(nn[1].Prefs)
 		assert.Equal(ipn.Stopped, *nn[0].State)
+		assert.Equal(false, nn[1].Prefs.LoggedOut)
 	}
 
 	// One more restart, this time with a valid key, but WantRunning=false.
@@ -722,20 +740,16 @@ func TestStateMachine(t *testing.T) {
 	{
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
 		// b.Shutdown() explicitly ourselves.
-		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "unpause"}, cc.getCalls())
+		// BUG: UpdateEndpoints should be called here since we're not WantRunning.
+		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "pause"}, cc.getCalls())
 
 		nn := notifies.drain(2)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
 		assert.NotNil(nn[1].State)
-		prefs := *nn[0].Prefs
-		assert.Equal(false, prefs.WantRunning)
-		// BUG: NeedsLogin is incorrect. We are already logged in.
-		//  This is the cause of bug tailscale/corp#1660.
-		//  (Whenever we enter the NeedsLogin state, the UI will
-		//  sent the user a notification that they should log in.)
-		assert.Equal(ipn.NeedsLogin, *nn[1].State)
-		assert.Equal(ipn.NeedsLogin, b.State())
+		assert.Equal(false, nn[0].Prefs.WantRunning)
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
+		assert.Equal(ipn.Stopped, *nn[1].State)
 	}
 
 	// This time, let's have the control server spontaneously
@@ -744,25 +758,35 @@ func TestStateMachine(t *testing.T) {
 	// it. (Normally we should call b.Login() first, but we already
 	// tested that up above.)
 	t.Logf("\n\nLoginFinished4")
-	notifies.expect(3)
+	notifies.expect(1)
 	cc.setAuthBlocked(false)
 	cc.send(nil, "", true, &netmap.NetworkMap{
 		MachineStatus: tailcfg.MachineAuthorized,
 	})
 	{
-		nn := notifies.drain(3)
-		assert.Equal([]string{"unpause"}, cc.getCalls())
+		nn := notifies.drain(1)
+		// NOTE: No call to Login() here since !WantRunning at
+		// startup time.
+		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].LoginFinished)
+		// State is still ipn.Stopped, as expected.
+	}
+
+	// Request connection.
+	// The state machine didn't call Login() earlier, so now it needs to.
+	t.Logf("\n\nWantRunning4 -> true")
+	notifies.expect(2)
+	b.EditPrefs(&ipn.MaskedPrefs{
+		WantRunningSet: true,
+		Prefs:          ipn.Prefs{WantRunning: true},
+	})
+	{
+		nn := notifies.drain(2)
+		assert.Equal([]string{"unpause", "Login"}, cc.getCalls())
+		// BUG: I would expect Prefs to change first, and state after.
+		assert.NotNil(nn[0].State)
 		assert.NotNil(nn[1].Prefs)
-		assert.NotNil(nn[2].State)
-		// BUG: WantRunning spontaneously shifted to true.
-		// This should happen if a user calls b.Login(), but not if
-		// a login spontaneously finishes.
-		// This could lead to a bug where the control server could
-		// remotely cause a node to re-connect when it's supposed to
-		// be in the Stopped state.
-		assert.Equal(true, nn[1].Prefs.WantRunning)
-		assert.Equal(ipn.Starting, *nn[2].State)
+		assert.Equal(ipn.Starting, *nn[0].State)
 	}
 
 	// The last test case is the most common one: restarting when both
@@ -775,40 +799,32 @@ func TestStateMachine(t *testing.T) {
 	{
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
 		// b.Shutdown() ourselves.
-		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "Login", "unpause"}, cc.getCalls())
+		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "Login"}, cc.getCalls())
 
 		nn := notifies.drain(2)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
 		assert.NotNil(nn[1].State)
-		prefs := *nn[0].Prefs
-		assert.Equal(true, prefs.WantRunning)
-		// BUG: NeedsLogin is very wrong, it could trigger a login
-		//  notification. (It goes away quickly once the control
-		//  server gets back to us, but this race condition is
-		//  enough to probably cause spurious notifications. I think
-		//  I've seen this on Windows.)
-		//
-		//  Probably we should go to Starting right away here, since
-		//  WantRunning==true and we believe (so far) that our key
-		//  is valid.
-		assert.Equal(ipn.NeedsLogin, *nn[1].State)
-		assert.Equal(ipn.NeedsLogin, b.State())
+		assert.Equal(false, nn[0].Prefs.LoggedOut)
+		assert.Equal(true, nn[0].Prefs.WantRunning)
+		assert.Equal(ipn.Starting, *nn[1].State)
+		assert.Equal(ipn.Starting, b.State())
 	}
 
 	// Control server accepts our valid key from before.
 	t.Logf("\n\nLoginFinished5")
-	notifies.expect(2)
+	notifies.expect(1)
 	cc.setAuthBlocked(false)
 	cc.send(nil, "", true, &netmap.NetworkMap{
 		MachineStatus: tailcfg.MachineAuthorized,
 	})
 	{
-		nn := notifies.drain(2)
-		assert.Equal([]string{"unpause"}, cc.getCalls())
+		nn := notifies.drain(1)
+		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].LoginFinished)
 		// NOTE: No prefs change this time. WantRunning stays true.
-		assert.NotNil(nn[1].State)
-		assert.Equal(ipn.Starting, *nn[1].State)
+		// We were in Starting in the first place, so that doesn't
+		// change either.
+		assert.Equal(ipn.Starting, b.State())
 	}
 }

ipn/prefs.go

@@ -87,6 +87,14 @@ type Prefs struct {
 	// this node.
 	WantRunning bool
 
+	// LoggedOut indicates whether the user intends to be logged out.
+	// There are other reasons we may be logged out, including no valid
+	// keys.
+	// We need to remember this state so that, on next startup, we can
+	// generate the "Login" vs "Connect" buttons correctly, without having
+	// to contact the server to confirm our nodekey status first.
+	LoggedOut bool
+
 	// ShieldsUp indicates whether to block all incoming connections,
 	// regardless of the control-provided packet filter. If false, we
 	// use the packet filter as provided. If true, we block incoming
@@ -177,6 +185,7 @@ type MaskedPrefs struct {
 	ExitNodeAllowLANAccessSet bool `json:",omitempty"`
 	CorpDNSSet                bool `json:",omitempty"`
 	WantRunningSet            bool `json:",omitempty"`
+	LoggedOutSet              bool `json:",omitempty"`
 	ShieldsUpSet              bool `json:",omitempty"`
 	AdvertiseTagsSet          bool `json:",omitempty"`
 	HostnameSet               bool `json:",omitempty"`
@@ -246,6 +255,9 @@ func (p *Prefs) pretty(goos string) string {
 		sb.WriteString("mesh=false ")
 	}
 	fmt.Fprintf(&sb, "dns=%v want=%v ", p.CorpDNS, p.WantRunning)
+	if p.LoggedOut {
+		sb.WriteString("loggedout=true ")
+	}
 	if p.ForceDaemon {
 		sb.WriteString("server=true ")
 	}
@@ -315,6 +327,7 @@ func (p *Prefs) Equals(p2 *Prefs) bool {
 		p.ExitNodeAllowLANAccess == p2.ExitNodeAllowLANAccess &&
 		p.CorpDNS == p2.CorpDNS &&
 		p.WantRunning == p2.WantRunning &&
+		p.LoggedOut == p2.LoggedOut &&
 		p.NotepadURLs == p2.NotepadURLs &&
 		p.ShieldsUp == p2.ShieldsUp &&
 		p.NoSNAT == p2.NoSNAT &&

ipn/prefs_clone.go

@@ -41,6 +41,7 @@ func (src *Prefs) Clone() *Prefs {
 	ExitNodeAllowLANAccess bool
 	CorpDNS                bool
 	WantRunning            bool
+	LoggedOut              bool
 	ShieldsUp              bool
 	AdvertiseTags          []string
 	Hostname               string

ipn/prefs_test.go

@@ -42,6 +42,7 @@ func TestPrefsEqual(t *testing.T) {
 		"ExitNodeAllowLANAccess",
 		"CorpDNS",
 		"WantRunning",
+		"LoggedOut",
 		"ShieldsUp",
 		"AdvertiseTags",
 		"Hostname",