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