ipn/ipnlocal: always send auth URL notifications when a user requests interactive login

This PR changes how LocalBackend handles interactive (initiated via StartLoginInteractive) and non-interactive (e.g., due to key expiration) logins,
and when it sends the authURL to the connected clients.

Specifically,
 - When a user initiates an interactive login by clicking Log In in the GUI, the LocalAPI calls StartLoginInteractive.
   If an authURL is available and hasn't expired, we immediately send it to all connected clients, suggesting them to open that URL in a browser.
   Otherwise, we send a login request to the control plane and set a flag indicating that an interactive login is in progress.
 - When LocalBackend receives an authURL from the control plane, we check if it differs from the previous one and whether an interactive login
   is in progress. If either condition is true, we notify all connected clients with the new authURL and reset the interactive login flag.

We reset the auth URL and flags upon a successful authentication, when a different user logs in and when switching Tailscale login profiles.

Finally, we remove the redundant dedup logic added to WatchNotifications in #12096 and revert the tests to their original state to ensure that
calling StartLoginInteractive always produces BrowseToURL notifications, either immediately or when the authURL is received from the control plane.

Fixes #13296

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

ipn/ipnlocal/local.go

@@ -268,6 +268,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
@@ -1359,10 +1360,6 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 			prefsChanged = true
 		}
 	}
-	if st.URL != "" {
-		b.authURL = st.URL
-		b.authURLTime = b.clock.Now()
-	}
 	if shouldAutoExitNode() {
 		// Re-evaluate exit node suggestion in case circumstances have changed.
 		_, err := b.suggestExitNodeLocked(curNetMap)
@@ -1478,7 +1475,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!
@@ -2682,27 +2679,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 := <-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 !fn(n) {
 				return
 			}
@@ -2833,20 +2814,52 @@ func (b *LocalBackend) sendFileNotify() {
 	b.send(n)
 }
 
-// popBrowserAuthNow shuts down the data plane and sends an auth URL
+// setAuthURL sets the authURL and triggers [LocalBackend.popBrowserAuthNow] if the URL has changed.
-// to the connected frontend, if any.
+// This method is called when a new authURL is received from the control plane, meaning that either a user
-func (b *LocalBackend) popBrowserAuthNow() {
+// 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.
+// If url is "", it is equivalent to calling [LocalBackend.resetAuthURLLocked] with b.mu held.
+func (b *LocalBackend) setAuthURL(url string) {
+	var popBrowser, keyExpired bool
+
 	b.mu.Lock()
-	url := b.authURL
+	switch {
-	expired := b.keyExpired
+	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
+	// Consume the StartLoginInteractive call, if any, that caused the control
+	// plane to send us this URL.
+	b.interact = false
 	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.
+// keyExpired is the value of b.keyExpired upon entry and indicates
+// whether the node's key has expired.
+// It must not be called with b.mu held.
+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()
 	}
@@ -3169,16 +3182,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)) {
+	hasValidURL := url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour))
-		b.popBrowserAuthNow()
+	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)
 	}
@@ -5019,9 +5041,11 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client {
 	return prev
 }
 
+// resetAuthURLLocked resets authURL, canceling any pending interactive login.
 func (b *LocalBackend) resetAuthURLLocked() {
 	b.authURL = ""
 	b.authURLTime = time.Time{}
+	b.interact = false
 }
 
 // ResetForClientDisconnect resets the backend for GUI clients running

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())
 	}