ipn/ipnlocal: don't send LoginFinished unless auth was in progress (#17266)

Before we introduced seamless, the "blocked" state was used to track:

* Whether a login was required for connectivity, and therefore we should
  keep the engine deconfigured until that happened
* Whether authentication was in progress

"blocked" would stop authReconfig from running. We want this when a login is
required: if your key has expired we want to deconfigure the engine and keep
it down, so that you don't keep using exit nodes (which won't work because
your key has expired).

Taking the engine down while auth was in progress was undesirable, so we
don't do that with seamless renewal. However, not entering the "blocked"
state meant that we needed to change the logic for when to send
LoginFinished on the IPN bus after seeing StateAuthenticated from the
controlclient. Initially we changed the "if blocked" check to "if blocked or
seamless is enabled" which was correct in other places.

In this place however, it introduced a bug: we are sending LoginFinished
every time we see StateAuthenticated, which happens even on a down & up, or
a profile switch. This in turn made it harder for UI clients to track when
authentication is complete.

Instead we should only send it out if we were blocked (i.e. seamless is
disabled, or our key expired) or an auth was in progress.

Updates tailscale/corp#31476

Updates tailscale/corp#32645

Fixes #17363

Signed-off-by: James Sanderson <[email protected]>

ipn/ipnlocal/local.go

@@ -1600,6 +1600,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 	}
 
 	wasBlocked := b.blocked
+	authWasInProgress := b.authURL != ""
 	keyExpiryExtended := false
 	if st.NetMap != nil {
 		wasExpired := b.keyExpired
@@ -1617,7 +1618,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 		b.blockEngineUpdates(false)
 	}
 
-	if st.LoginFinished() && (wasBlocked || b.seamlessRenewalEnabled()) {
+	if st.LoginFinished() && (wasBlocked || authWasInProgress) {
 		if wasBlocked {
 			// Auth completed, unblock the engine
 			b.blockEngineUpdates(false)

ipn/ipnlocal/state_test.go

@@ -348,6 +348,14 @@ func (b *LocalBackend) nonInteractiveLoginForStateTest() {
 // predictable, but maybe a bit less thorough. This is more of an overall
 // state machine test than a test of the wgengine+magicsock integration.
 func TestStateMachine(t *testing.T) {
+	runTestStateMachine(t, false)
+}
+
+func TestStateMachineSeamless(t *testing.T) {
+	runTestStateMachine(t, true)
+}
+
+func runTestStateMachine(t *testing.T, seamless bool) {
 	envknob.Setenv("TAILSCALE_USE_WIP_CODE", "1")
 	defer envknob.Setenv("TAILSCALE_USE_WIP_CODE", "")
 	c := qt.New(t)
@@ -545,6 +553,13 @@ func TestStateMachine(t *testing.T) {
 	notifies.expect(3)
 	cc.persist.UserProfile.LoginName = "user1"
 	cc.persist.NodeID = "node1"
+
+	// even if seamless is being enabled by default rather than by policy, this is
+	// the point where it will first get enabled.
+	if seamless {
+		sys.ControlKnobs().SeamlessKeyRenewal.Store(true)
+	}
+
 	cc.send(nil, "", true, &netmap.NetworkMap{})
 	{
 		nn := notifies.drain(3)