ipn/ipnlocal: always send auth URL notifications when a user requests interactive loginnickkhyl/authurl-notify-backport

This is a backport of #13297 without extra stuff we need for multi-user improvements

Fixes #13296

Signed-off-by: Nick Khyl <nickk@tailscale.com>

2 files changed, 67 insertions, 41 deletions

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index 3da12d7cc..b9f026241 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -263,6 +263,7 @@ type LocalBackend struct {
 	keyExpired       bool
 	authURL          string    // non-empty if not Running
 	authURLTime      time.Time // when the authURL was received from the control server
+	interact         bool      // indicates whether a user requested interactive login
 	egg              bool
 	prevIfState      *netmon.State
 	peerAPIServer    *peerAPIServer // or nil
@@ -1323,11 +1324,8 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 			prefs.Persist = st.Persist.AsStruct()
 		}
 	}
-	if st.URL != "" {
-		b.authURL = st.URL
-		b.authURLTime = b.clock.Now()
-	}
 	if (wasBlocked || b.seamlessRenewalEnabled()) && st.LoginFinished() {
+		b.resetAuthURLLocked()
 		// Interactive login finished successfully (URL visited).
 		// After an interactive login, the user always wants
 		// WantRunning.
@@ -1452,7 +1450,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 	}
 	if st.URL != "" {
 		b.logf("Received auth URL: %.20v...", st.URL)
-		b.popBrowserAuthNow()
+		b.setAuthURL(st.URL)
 	}
 	b.stateMachine()
 	// This is currently (2020-07-28) necessary; conditionally disabling it is fragile!
@@ -2644,27 +2642,11 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 	// TODO(marwan-at-work): streaming background logs?
 	defer b.DeleteForegroundSession(sessionID)
 
-	var lastURLPop string // to dup suppress URL popups
 	for {
 		select {
 		case <-ctx.Done():
 			return
 		case n, ok := <-ch:
-			// URLs flow into Notify.BrowseToURL via two means:
-			//    1. From MapResponse.PopBrowserURL, which already says they're dup
-			//       suppressed if identical, and that's done by the controlclient,
-			//       so this added later adds nothing.
-			//
-			//    2. From the controlclient auth routes, on register. This makes sure
-			//       we don't tell clients (mac, windows, android) to pop the same URL
-			//       multiple times.
-			if n != nil && n.BrowseToURL != nil {
-				if v := *n.BrowseToURL; v == lastURLPop {
-					n.BrowseToURL = nil
-				} else {
-					lastURLPop = v
-				}
-			}
 			if !ok || !fn(n) {
 				return
 			}
@@ -2795,20 +2777,46 @@ func (b *LocalBackend) sendFileNotify() {
 	b.send(n)
 }
 
-// popBrowserAuthNow shuts down the data plane and sends an auth URL
-// to the connected frontend, if any.
-func (b *LocalBackend) popBrowserAuthNow() {
+// setAuthURL sets the authURL and triggers [LocalBackend.popBrowserAuthNow] if the URL has changed.
+// This method is called when a new authURL is received from the control plane, meaning that either a user
+// has started a new interactive login (e.g., by running `tailscale login` or clicking Login in the GUI),
+// or the control plane was unable to authenticate this node non-interactively (e.g., due to key expiration).
+// b.interact indicates whether an interactive login is in progress.
+func (b *LocalBackend) setAuthURL(url string) {
+	var popBrowser, keyExpired bool
+
 	b.mu.Lock()
-	url := b.authURL
-	expired := b.keyExpired
+	switch {
+	case url == "":
+		b.resetAuthURLLocked()
+	case b.authURL != url:
+		b.authURL = url
+		b.authURLTime = b.clock.Now()
+		// Always open the browser if the URL has changed.
+		// This includes the transition from no URL -> some URL.
+		popBrowser = true
+	default:
+		// Otherwise, only open it if the user explicitly requests interactive login.
+		popBrowser = b.interact
+	}
+	keyExpired = b.keyExpired
+	b.interact = false // consume the StartLoginInteractive call, if any, that caused the control plane to send us this URL
 	b.mu.Unlock()
 
-	b.logf("popBrowserAuthNow: url=%v, key-expired=%v, seamless-key-renewal=%v", url != "", expired, b.seamlessRenewalEnabled())
+	if popBrowser {
+		b.popBrowserAuthNow(url, keyExpired)
+	}
+}
+
+// popBrowserAuthNow shuts down the data plane and sends an auth URL
+// to the connected frontend, if any.
+func (b *LocalBackend) popBrowserAuthNow(url string, keyExpired bool) {
+	b.logf("popBrowserAuthNow: url=%v, key-expired=%v, seamless-key-renewal=%v", url != "", keyExpired, b.seamlessRenewalEnabled())
 
 	// Deconfigure the local network data plane if:
 	// - seamless key renewal is not enabled;
 	// - key is expired (in which case tailnet connectivity is down anyway).
-	if !b.seamlessRenewalEnabled() || expired {
+	if !b.seamlessRenewalEnabled() || keyExpired {
 		b.blockEngineUpdates(true)
 		b.stopEngineAndWait()
 	}
@@ -3134,16 +3142,25 @@ func (b *LocalBackend) StartLoginInteractive(ctx context.Context) error {
 		panic("LocalBackend.assertClient: b.cc == nil")
 	}
 	url := b.authURL
+	keyExpired := b.keyExpired
 	timeSinceAuthURLCreated := b.clock.Since(b.authURLTime)
-	cc := b.cc
-	b.mu.Unlock()
-	b.logf("StartLoginInteractive: url=%v", url != "")
-
 	// Only use an authURL if it was sent down from control in the last
 	// 6 days and 23 hours. Avoids using a stale URL that is no longer valid
 	// server-side. Server-side URLs expire after 7 days.
-	if url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour)) {
-		b.popBrowserAuthNow()
+	hasValidURL := url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour))
+	if !hasValidURL {
+		// A user wants to log in interactively, but we don't have a valid authURL.
+		// Set a flag to indicate that interactive login is in progress, forcing
+		// a BrowseToURL notification once the authURL becomes available.
+		b.interact = true
+	}
+	cc := b.cc
+	b.mu.Unlock()
+
+	b.logf("StartLoginInteractive: url=%v", hasValidURL)
+
+	if hasValidURL {
+		b.popBrowserAuthNow(url, keyExpired)
 	} else {
 		cc.Login(b.loginFlags | controlclient.LoginInteractive)
 	}
@@ -4678,8 +4695,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
@@ -4961,7 +4977,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
@@ -4981,6 +4997,12 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client {
 	return prev
 }
 
+func (b *LocalBackend) resetAuthURLLocked() {
+	b.authURL = ""
+	b.authURLTime = time.Time{}
+	b.interact = false
+}
+
 // 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
@@ -5006,8 +5028,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..0b20539df 100644
--- a/ipn/ipnlocal/state_test.go
+++ b/ipn/ipnlocal/state_test.go
@@ -437,10 +437,13 @@ func TestStateMachine(t *testing.T) {
 	// ask control to do anything. Instead backend will emit an event
 	// indicating that the UI should browse to the given URL.
 	t.Logf("\n\nLogin (interactive)")
-	notifies.expect(0)
+	notifies.expect(1)
 	b.StartLoginInteractive(context.Background())
 	{
+		nn := notifies.drain(1)
 		cc.assertCalls()
+		c.Assert(nn[0].BrowseToURL, qt.IsNotNil)
+		c.Assert(url1, qt.Equals, *nn[0].BrowseToURL)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}
 
@@ -450,11 +453,13 @@ func TestStateMachine(t *testing.T) {
 	// the login URL expired. If they start another interactive login,
 	// we must always get a *new* login URL first.
 	t.Logf("\n\nLogin2 (interactive)")
+	b.authURLTime = time.Now().Add(-time.Hour * 24 * 7) // simulate URL expiration
 	notifies.expect(0)
 	b.StartLoginInteractive(context.Background())
 	{
+		notifies.drain(0)
 		// backend asks control for another login sequence
-		cc.assertCalls()
+		cc.assertCalls("Login")
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}