diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c6486c503..41d463851 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1342,20 +1342,26 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control prefs.Persist = st.Persist.AsStruct() } } + if st.LoginFinished() { + if b.authURL != "" { + b.resetAuthURLLocked() + // Interactive login finished successfully (URL visited). + // After an interactive login, the user always wants + // WantRunning. + if !prefs.WantRunning { + prefs.WantRunning = true + prefsChanged = true + } + } + if prefs.LoggedOut { + prefs.LoggedOut = false + prefsChanged = true + } + } if st.URL != "" { b.authURL = st.URL b.authURLTime = b.clock.Now() } - if (wasBlocked || b.seamlessRenewalEnabled()) && st.LoginFinished() { - // Interactive login finished successfully (URL visited). - // After an interactive login, the user always wants - // WantRunning. - if !prefs.WantRunning || prefs.LoggedOut { - prefsChanged = true - } - prefs.WantRunning = true - prefs.LoggedOut = false - } if shouldAutoExitNode() { // Re-evaluate exit node suggestion in case circumstances have changed. _, err := b.suggestExitNodeLocked(curNetMap) @@ -4704,8 +4710,7 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlock activeLogin := b.activeLogin authURL := b.authURL if newState == ipn.Running { - b.authURL = "" - b.authURLTime = time.Time{} + b.resetAuthURLLocked() // Start a captive portal detection loop if none has been // started. Create a new context if none is present, since it @@ -4987,7 +4992,7 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { return nil } - b.authURL = "" + b.resetAuthURLLocked() // When we clear the control client, stop any outstanding netmap expiry // timer; synthesizing a new netmap while we don't have a control @@ -5007,6 +5012,11 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { return prev } +func (b *LocalBackend) resetAuthURLLocked() { + b.authURL = "" + b.authURLTime = time.Time{} +} + // ResetForClientDisconnect resets the backend for GUI clients running // in interactive (non-headless) mode. This is currently used only by // Windows. This causes all state to be cleared, lest an unrelated user @@ -5034,8 +5044,7 @@ func (b *LocalBackend) ResetForClientDisconnect() { b.currentUser = nil } b.keyExpired = false - b.authURL = "" - b.authURLTime = time.Time{} + b.resetAuthURLLocked() b.activeLogin = "" b.resetDialPlan() b.setAtomicValuesFromPrefsLocked(ipn.PrefsView{}) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index d9ed608d8..4746f3969 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -651,25 +651,55 @@ func TestStateMachine(t *testing.T) { c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) } + // Explicitly set the ControlURL to avoid defaulting to [ipn.DefaultControlURL]. + // This prevents [LocalBackend] from using the production control server during tests + // and ensures that [LocalBackend.validPopBrowserURL] returns true for the + // fake interactive login URLs used below. Otherwise, we won't be receiving + // BrowseToURL notifications as expected. + // See tailscale/tailscale#11393. + notifies.expect(1) + b.EditPrefs(&ipn.MaskedPrefs{ + ControlURLSet: true, + Prefs: ipn.Prefs{ + ControlURL: "https://localhost:1/", + }, + }) + notifies.drain(1) + + t.Logf("\n\nStartLoginInteractive3") b.StartLoginInteractive(context.Background()) - t.Logf("\n\nLoginFinished3") + // We've been logged out, and the previously created profile is now deleted. + // We're attempting an interactive login for the first time with the new profile, + // this should result in a call to the control server, which in turn should provide + // an interactive login URL to visit. + notifies.expect(2) + url3 := "https://localhost:1/3" + cc.send(nil, url3, false, nil) + { + nn := notifies.drain(2) + cc.assertCalls("Login") + c.Assert(nn[1].BrowseToURL, qt.IsNotNil) + c.Assert(*nn[1].BrowseToURL, qt.Equals, url3) + } + t.Logf("%q visited", url3) notifies.expect(3) cc.persist.UserProfile.LoginName = "user2" cc.persist.NodeID = "node2" cc.send(nil, "", true, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) + t.Logf("\n\nLoginFinished3") { nn := notifies.drain(3) - cc.assertCalls("Login") c.Assert(nn[0].LoginFinished, qt.IsNotNil) c.Assert(nn[1].Prefs, qt.IsNotNil) c.Assert(nn[1].Prefs.Persist(), qt.IsNotNil) - c.Assert(nn[2].State, qt.IsNotNil) // Prefs after finishing the login, so LoginName updated. c.Assert(nn[1].Prefs.Persist().UserProfile().LoginName, qt.Equals, "user2") c.Assert(nn[1].Prefs.LoggedOut(), qt.IsFalse) + // If a user initiates an interactive login, they also expect WantRunning to become true. c.Assert(nn[1].Prefs.WantRunning(), qt.IsTrue) + c.Assert(nn[2].State, qt.IsNotNil) c.Assert(ipn.Starting, qt.Equals, *nn[2].State) } @@ -767,18 +797,12 @@ func TestStateMachine(t *testing.T) { // We want to try logging in as a different user, while Stopped. // First, start the login process (without logging out first). t.Logf("\n\nLoginDifferent") - notifies.expect(2) - b.EditPrefs(&ipn.MaskedPrefs{ - ControlURLSet: true, - Prefs: ipn.Prefs{ - ControlURL: "https://localhost:1/", - }, - }) + notifies.expect(1) b.StartLoginInteractive(context.Background()) - url3 := "https://localhost:1/3" - cc.send(nil, url3, false, nil) + url4 := "https://localhost:1/4" + cc.send(nil, url4, false, nil) { - nn := notifies.drain(2) + nn := notifies.drain(1) // It might seem like WantRunning should switch to true here, // but that would be risky since we already have a valid // user account. It might try to reconnect to the old account @@ -787,8 +811,8 @@ func TestStateMachine(t *testing.T) { // Because the login hasn't yet completed, the old login // is still valid, so it's correct that we stay paused. cc.assertCalls("Login") - c.Assert(nn[1].BrowseToURL, qt.IsNotNil) - c.Assert(*nn[1].BrowseToURL, qt.Equals, url3) + c.Assert(nn[0].BrowseToURL, qt.IsNotNil) + c.Assert(*nn[0].BrowseToURL, qt.Equals, url4) } // Now, let's complete the interactive login, using a different