ipnlocal: setting WantRunning with EditPrefs was special.

EditPrefs should be just a wrapper around the action of changing prefs,
but someone had added a side effect of calling Login() sometimes. The
side effect happened *after* running the state machine, which would
sometimes result in us going into NeedsLogin immediately before calling
cc.Login().

This manifested as the macOS app not being able to Connect if you
launched it with LoggedOut=false and WantRunning=false. Trying to
Connect() would sent us to the NeedsLogin state instead.

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

ipn/ipnlocal/local.go

@@ -1438,14 +1438,12 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (*ipn.Prefs, error) {
 		b.mu.Unlock()
 		return p1, nil
 	}
-	cc := b.cc
 	b.logf("EditPrefs: %v", mp.Pretty())
 	b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock
 
-	if !p0.WantRunning && p1.WantRunning {
-		b.logf("EditPrefs: transitioning to running; doing Login...")
-		cc.Login(nil, controlclient.LoginDefault)
-	}
+	// Note: don't perform any actions for the new prefs here. Not
+	// every prefs change goes through EditPrefs. Put your actions
+	// in setPrefsLocksOnEntry instead.
 	return p1, nil
 }
 
@@ -1479,6 +1477,7 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
 	b.hostinfo = newHi
 	hostInfoChanged := !oldHi.Equal(newHi)
 	userID := b.userID
+	cc := b.cc
 
 	b.mu.Unlock()
 
@@ -1518,6 +1517,11 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
 		b.e.SetDERPMap(netMap.DERPMap)
 	}
 
+	if !oldp.WantRunning && newp.WantRunning {
+		b.logf("transitioning to running; doing Login...")
+		cc.Login(nil, controlclient.LoginDefault)
+	}
+
 	if oldp.WantRunning != newp.WantRunning {
 		b.stateMachine()
 	} else {
@@ -2145,8 +2149,23 @@ func (b *LocalBackend) nextState() ipn.State {
 			// Auth was interrupted or waiting for URL visit,
 			// so it won't proceed without human help.
 			return ipn.NeedsLogin
+		} else if state == ipn.Stopped {
+			// If we were already in the Stopped state, then
+			// we can assume auth is in good shape (or we would
+			// have been in NeedsLogin), so transition to Starting
+			// right away.
+			return ipn.Starting
+		} else if state == ipn.NoState {
+			// Our first time connecting to control, and we
+			// don't know if we'll NeedsLogin or not yet.
+			// UIs should print "Loading..." in this state.
+			return ipn.NoState
+		} else if state == ipn.Starting ||
+			state == ipn.Running ||
+			state == ipn.NeedsLogin {
+			return state
 		} else {
-			// Auth or map request needs to finish
+			b.logf("unexpected no-netmap state transition for %v", state)
 			return state
 		}
 	case !wantRunning:

ipn/ipnlocal/state_test.go

@@ -106,20 +106,24 @@ func (cc *mockControl) SetStatusFunc(fn func(controlclient.Status)) {
 	cc.statusFunc = fn
 }
 
-func (cc *mockControl) populateKeys() {
+func (cc *mockControl) populateKeys() (newKeys bool) {
 	cc.mu.Lock()
 	defer cc.mu.Unlock()
 
 	if cc.machineKey.IsZero() {
 		cc.logf("Copying machineKey.")
 		cc.machineKey, _ = cc.opts.GetMachinePrivateKey()
+		newKeys = true
 	}
 
 	if cc.persist.PrivateNodeKey.IsZero() {
 		cc.logf("Generating a new nodekey.")
 		cc.persist.OldPrivateNodeKey = cc.persist.PrivateNodeKey
 		cc.persist.PrivateNodeKey, _ = wgkey.NewPrivate()
+		newKeys = true
 	}
+
+	return newKeys
 }
 
 // send publishes a controlclient.Status notification upstream.
@@ -191,7 +195,11 @@ func (cc *mockControl) Shutdown() {
 func (cc *mockControl) Login(t *tailcfg.Oauth2Token, flags controlclient.LoginFlags) {
 	cc.logf("Login token=%v flags=%v", t, flags)
 	cc.called("Login")
-	cc.populateKeys()
+	newKeys := cc.populateKeys()
+
+	interact := (flags & controlclient.LoginInteractive) != 0
+	cc.logf("Login: interact=%v newKeys=%v", interact, newKeys)
+	cc.setAuthBlocked(interact || newKeys)
 }
 
 func (cc *mockControl) StartLogout() {
@@ -526,7 +534,7 @@ func TestStateMachine(t *testing.T) {
 		nn := notifies.drain(2)
 		// BUG: UpdateEndpoints isn't needed here.
 		// BUG: Login isn't needed here. We never logged out.
-		assert.Equal([]string{"unpause", "UpdateEndpoints", "Login"}, cc.getCalls())
+		assert.Equal([]string{"Login", "unpause", "UpdateEndpoints"}, cc.getCalls())
 		// BUG: I would expect Prefs to change first, and state after.
 		assert.NotNil(nn[0].State)
 		assert.NotNil(nn[1].Prefs)
@@ -752,26 +760,6 @@ func TestStateMachine(t *testing.T) {
 		assert.Equal(ipn.Stopped, *nn[1].State)
 	}
 
-	// This time, let's have the control server spontaneously
-	// authenticate us, even though you didn't ask. That wouldn't
-	// normally happen, but the state machine should be able to handle
-	// it. (Normally we should call b.Login() first, but we already
-	// tested that up above.)
-	t.Logf("\n\nLoginFinished4")
-	notifies.expect(1)
-	cc.setAuthBlocked(false)
-	cc.send(nil, "", true, &netmap.NetworkMap{
-		MachineStatus: tailcfg.MachineAuthorized,
-	})
-	{
-		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")
@@ -782,7 +770,7 @@ func TestStateMachine(t *testing.T) {
 	})
 	{
 		nn := notifies.drain(2)
-		assert.Equal([]string{"unpause", "Login"}, cc.getCalls())
+		assert.Equal([]string{"Login", "unpause"}, cc.getCalls())
 		// BUG: I would expect Prefs to change first, and state after.
 		assert.NotNil(nn[0].State)
 		assert.NotNil(nn[1].Prefs)
@@ -804,9 +792,9 @@ func TestStateMachine(t *testing.T) {
 		nn := notifies.drain(2)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
-		assert.NotNil(nn[1].State)
 		assert.Equal(false, nn[0].Prefs.LoggedOut)
 		assert.Equal(true, nn[0].Prefs.WantRunning)
+		assert.NotNil(nn[1].State)
 		assert.Equal(ipn.Starting, *nn[1].State)
 		assert.Equal(ipn.Starting, b.State())
 	}