Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field #12096

Merged
merged 1 commit into from
May 14, 2024
Merged

ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field #12096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions ipn/ipnlocal/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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() {
Expand Down Expand Up @@ -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!
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -4653,7 +4658,6 @@ func (b *LocalBackend) ResetForClientDisconnect() {
}
b.keyExpired = false
b.authURL = ""
b.authURLSticky = ""
b.authURLTime = time.Time{}
b.activeLogin = ""
b.resetDialPlan()
Expand Down
17 changes: 8 additions & 9 deletions ipn/ipnlocal/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -421,26 +421,26 @@ 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.
// Since we provided an interactive URL earlier, this shouldn'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())
}

Expand All @@ -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())
}

Expand Down
Loading