From 8aa5c3534da70118d8878d7053957a10817052ed Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 10 May 2024 13:25:08 -0700 Subject: [PATCH] ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field The previous LocalBackend & CLI 'up' changes improved some stuff, but might've been too aggressive in some edge cases. This simplifies the authURL vs authURLSticky distinction and removes the interact field, which seemed to just just be about duplicate URL suppression in IPN bus, back from when the IPN bus was a single client at a time. This moves that suppression to a different spot. Fixes #12119 Updates #12028 Updates #12042 Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4 Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/local.go | 38 +++++++++++++++++++++----------------- ipn/ipnlocal/state_test.go | 17 ++++++++--------- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 414d49b27..9fea49870 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -259,10 +259,8 @@ type LocalBackend struct { endpoints []tailcfg.Endpoint blocked bool keyExpired bool - authURL string // cleared on Notify - authURLSticky string // not cleared on Notify + authURL string // non-empty if not Running authURLTime time.Time // when the authURL was received from the control server - interact bool egg bool prevIfState *netmon.State peerAPIServer *peerAPIServer // or nil @@ -785,7 +783,7 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { s.Version = version.Long() s.TUN = !b.sys.IsNetstack() s.BackendState = b.state.String() - s.AuthURL = b.authURLSticky + s.AuthURL = b.authURL if prefs := b.pm.CurrentPrefs(); prefs.Valid() && prefs.AutoUpdate().Check { s.ClientVersion = b.lastClientVersion } @@ -1139,7 +1137,6 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control prefsChanged := false prefs := b.pm.CurrentPrefs().AsStruct() netMap := b.netMap - interact := b.interact if prefs.ControlURL == "" { // Once we get a message from the control plane, set @@ -1158,7 +1155,6 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control } if st.URL != "" { b.authURL = st.URL - b.authURLSticky = st.URL b.authURLTime = b.clock.Now() } if (wasBlocked || b.seamlessRenewalEnabled()) && st.LoginFinished() { @@ -1276,9 +1272,7 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control } if st.URL != "" { b.logf("Received auth URL: %.20v...", st.URL) - if interact { - b.popBrowserAuthNow() - } + b.popBrowserAuthNow() } b.stateMachine() // This is currently (2020-07-28) necessary; conditionally disabling it is fragile! @@ -2281,8 +2275,8 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa if mask&ipn.NotifyInitialState != 0 { ini.SessionID = sessionID ini.State = ptr.To(b.state) - if b.state == ipn.NeedsLogin && b.authURLSticky != "" { - ini.BrowseToURL = ptr.To(b.authURLSticky) + if b.state == ipn.NeedsLogin && b.authURL != "" { + ini.BrowseToURL = ptr.To(b.authURL) } } if mask&ipn.NotifyInitialPrefs != 0 { @@ -2336,11 +2330,27 @@ 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 } @@ -2476,8 +2486,6 @@ func (b *LocalBackend) sendFileNotify() { func (b *LocalBackend) popBrowserAuthNow() { b.mu.Lock() url := b.authURL - b.interact = false - b.authURL = "" // but NOT clearing authURLSticky expired := b.keyExpired b.mu.Unlock() @@ -2805,7 +2813,6 @@ func (b *LocalBackend) StartLoginInteractive(ctx context.Context) error { if b.cc == nil { panic("LocalBackend.assertClient: b.cc == nil") } - b.interact = true url := b.authURL timeSinceAuthURLCreated := b.clock.Since(b.authURLTime) cc := b.cc @@ -4347,7 +4354,6 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlock authURL := b.authURL if newState == ipn.Running { b.authURL = "" - b.authURLSticky = "" b.authURLTime = time.Time{} } else if oldState == ipn.Running { // Transitioning away from running. @@ -4607,7 +4613,6 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { } b.authURL = "" - b.authURLSticky = "" // When we clear the control client, stop any outstanding netmap expiry // timer; synthesizing a new netmap while we don't have a control @@ -4653,7 +4658,6 @@ func (b *LocalBackend) ResetForClientDisconnect() { } b.keyExpired = false b.authURL = "" - b.authURLSticky = "" b.authURLTime = time.Time{} b.activeLogin = "" b.resetDialPlan() diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index ae8871619..f1b11e737 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -329,7 +329,7 @@ func TestStateMachine(t *testing.T) { (n.Prefs != nil && n.Prefs.Valid()) || n.BrowseToURL != nil || n.LoginFinished != nil { - logf("%v\n\n", n) + logf("%+v\n\n", n) notifies.put(n) } else { logf("(ignored) %v\n\n", n) @@ -406,7 +406,7 @@ func TestStateMachine(t *testing.T) { // the user needs to visit a login URL. t.Logf("\n\nLogin (url response)") - notifies.expect(2) + notifies.expect(3) b.EditPrefs(&ipn.MaskedPrefs{ ControlURLSet: true, Prefs: ipn.Prefs{ @@ -421,12 +421,15 @@ func TestStateMachine(t *testing.T) { // ...but backend eats that notification, because the user // didn't explicitly request interactive login yet, and // we're already in NeedsLogin state. - nn := notifies.drain(2) + nn := notifies.drain(3) c.Assert(nn[1].Prefs, qt.IsNotNil) c.Assert(nn[1].Prefs.LoggedOut(), qt.IsTrue) c.Assert(nn[1].Prefs.WantRunning(), qt.IsFalse) c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) + c.Assert(nn[2].BrowseToURL, qt.IsNotNil) + c.Assert(url1, qt.Equals, *nn[2].BrowseToURL) + c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) } // Now we'll try an interactive login. @@ -434,13 +437,10 @@ 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(1) + notifies.expect(0) 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()) } @@ -453,9 +453,8 @@ func TestStateMachine(t *testing.T) { notifies.expect(0) b.StartLoginInteractive(context.Background()) { - notifies.drain(0) // backend asks control for another login sequence - cc.assertCalls("Login") + cc.assertCalls() c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) }