diff --git a/client/android/client.go b/client/android/client.go index 6f5eaacf3..9705db8e0 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -104,8 +104,7 @@ type Client struct { stateChangeMu sync.Mutex stateChangeSubID string - eventSub *peer.EventSubscription - // Closed to stop the watch goroutines from delivering buffered items to a + // Closed to stop the watch goroutine from delivering buffered ticks to a // listener that has been removed or replaced. See stopStateChangeWatchLocked. stateChangeDone chan struct{} diff --git a/client/android/session.go b/client/android/session.go index d5da09c93..1ce97f074 100644 --- a/client/android/session.go +++ b/client/android/session.go @@ -6,13 +6,8 @@ import ( "context" "fmt" - log "github.com/sirupsen/logrus" - "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/internal/auth" - "github.com/netbirdio/netbird/client/internal/auth/sessionwatch" - "github.com/netbirdio/netbird/client/internal/peer" - cProto "github.com/netbirdio/netbird/client/proto" ) // StateChangeListener receives client state notifications. @@ -21,16 +16,11 @@ import ( // changed: connection state, the run-loop status label (e.g. NeedsLogin) or // the session deadline. It mirrors the daemon's SubscribeStatus stream // trigger — on each signal the consumer pulls the fresh values via -// Status() / SessionExpiresAtUnix(). -// -// OnSessionExpiring forwards the engine's session-expiry warnings, fired at -// sessionwatch.WarningLead before the deadline and again at FinalWarningLead -// (finalWarning true). The second one is suppressed when the user dismissed -// the first via DismissSessionWarning. The daemon turns the same events into -// its tray notification. +// Status() / SessionExpiresAtUnix(). The engine arms no expiry-warning +// timers on Android; the app schedules the warnings from the deadline it +// reads here. type StateChangeListener interface { OnStateChanged() - OnSessionExpiring(expiresAtUnix int64, leadMinutes int64, finalWarning bool) } // Status returns the connect run-loop's status label — the same value the @@ -110,11 +100,11 @@ func (c *Client) SetStateChangeListener(listener StateChangeListener) { return } - // Both subscriptions are buffered (one pending tick, ten pending events), - // so unsubscribing is not enough to stop callbacks: the loops would drain - // what is already queued and deliver it to a listener the caller has - // already removed or replaced. Gate every callback on this registration's - // own signal, which is closed before unsubscribing. + // The subscription is buffered (one pending tick), so unsubscribing is + // not enough to stop callbacks: the loop would drain what is already + // queued and deliver it to a listener the caller has already removed or + // replaced. Gate every callback on this registration's own signal, which + // is closed before unsubscribing. done := make(chan struct{}) c.stateChangeDone = done @@ -133,9 +123,6 @@ func (c *Client) SetStateChangeListener(listener StateChangeListener) { listener.OnStateChanged() } }() - - c.eventSub = c.recorder.SubscribeToEvents() - go watchSessionWarnings(c.eventSub, listener, done) } // RemoveStateChangeListener unregisters the state notification listener. @@ -145,21 +132,6 @@ func (c *Client) RemoveStateChangeListener() { c.stopStateChangeWatchLocked() } -// DismissSessionWarning records the user's "Dismiss" on the first expiry -// warning and suppresses the final one for the current deadline. A refreshed -// deadline re-arms both. No-op while the engine is not running. -func (c *Client) DismissSessionWarning() { - cc := c.getConnectClient() - if cc == nil { - return - } - engine := cc.Engine() - if engine == nil { - return - } - engine.DismissSessionWarning() -} - // ExtendAuthSession runs the interactive SSO flow to obtain a fresh JWT and // asks the management server to extend the session deadline. The tunnel is // untouched: no resync, no reconnect. Async; the result arrives on the @@ -201,8 +173,8 @@ func (c *Client) CancelExtendAuthSession() { } func (c *Client) stopStateChangeWatchLocked() { - // Signal first, unsubscribe second: closing the channels only stops new - // items, and the loops would still hand whatever is buffered to a listener + // Signal first, unsubscribe second: closing the channel only stops new + // items, and the loop would still hand whatever is buffered to a listener // that is no longer registered. if c.stateChangeDone != nil { close(c.stateChangeDone) @@ -212,49 +184,6 @@ func (c *Client) stopStateChangeWatchLocked() { c.recorder.UnsubscribeFromStateChanges(c.stateChangeSubID) c.stateChangeSubID = "" } - if c.eventSub != nil { - // Closes the channel, which ends watchSessionWarnings. - c.recorder.UnsubscribeFromEvents(c.eventSub) - c.eventSub = nil - } -} - -// watchSessionWarnings forwards the engine's session-expiry warnings to the -// listener. The event stream also carries unrelated traffic — network-map -// updates on every sync, DNS and route errors — so everything but an -// AUTHENTICATION event carrying the session-warning marker is dropped. Exits -// when the subscription is closed by UnsubscribeFromEvents, or earlier when -// done is closed — the stream buffers up to ten events, and a deregistered -// listener must not receive the ones already queued. -func watchSessionWarnings(sub *peer.EventSubscription, listener StateChangeListener, done <-chan struct{}) { - for ev := range sub.Events() { - select { - case <-done: - return - default: - } - if ev.GetCategory() != cProto.SystemEvent_AUTHENTICATION { - continue - } - meta := ev.GetMetadata() - if meta[sessionwatch.MetaSessionWarning] != "true" { - // Other AUTHENTICATION events exist (e.g. a deadline rejected as - // out of range); they carry no warning marker. - continue - } - deadline, err := sessionwatch.ParseExpiresAt(meta[sessionwatch.MetaSessionExpiresAt]) - if err != nil { - log.Warnf("session warning event with unparsable deadline: %v", err) - continue - } - lead, err := sessionwatch.ParseLeadMinutes(meta[sessionwatch.MetaSessionLeadMinutes]) - if err != nil { - // Informational only — the deadline above is what drives the UI. - lead = 0 - } - listener.OnSessionExpiring(deadline.Unix(), int64(lead), - meta[sessionwatch.MetaSessionFinal] == "true") - } } func (c *Client) beginExtend() (context.Context, error) { diff --git a/client/internal/auth/sessionwatch/watcher.go b/client/internal/auth/sessionwatch/watcher.go index e685c28d0..496903044 100644 --- a/client/internal/auth/sessionwatch/watcher.go +++ b/client/internal/auth/sessionwatch/watcher.go @@ -90,8 +90,9 @@ type StatusRecorder interface { // fallback T-FinalWarningLead dialog (suppressed when the user dismissed // the first one for the same deadline). Safe for concurrent use. type Watcher struct { - lead time.Duration - finalLead time.Duration + lead time.Duration + finalLead time.Duration + deadlineOnly bool mu sync.Mutex current time.Time @@ -102,6 +103,7 @@ type Watcher struct { dismissedAt time.Time // deadline value the user dismissed via Dismiss(); gates fireFinal closed bool recorder StatusRecorder + nowFn func() time.Time } // New returns a watcher with the package defaults WarningLead and @@ -122,9 +124,17 @@ func NewWithLeads(lead, final time.Duration, recorder StatusRecorder) *Watcher { lead: lead, finalLead: final, recorder: recorder, + nowFn: time.Now, } } +// NewDeadlineOnly returns a watcher that validates and records deadlines but arms no warning timers. +func NewDeadlineOnly(recorder StatusRecorder) *Watcher { + w := New(recorder) + w.deadlineOnly = true + return w +} + // Update sets the latest deadline. Pass the zero time to clear (e.g. when // a Sync push from the server omits the field because login expiration // was disabled). @@ -181,7 +191,7 @@ func (w *Watcher) Update(deadline time.Time) error { w.finalFiredAt = time.Time{} w.dismissedAt = time.Time{} - if deadline.After(now) { + if deadline.After(now) && !w.deadlineOnly { w.armTimerLocked(deadline) } recorder := w.recorder @@ -303,6 +313,11 @@ func (w *Watcher) fire(armedFor time.Time) { w.mu.Unlock() return } + now := w.nowFn() + if isLate(now, armedFor, max(w.finalLead, 0)) { + w.fireLateLocked(armedFor, now) + return + } w.firedAt = armedFor recorder := w.recorder w.mu.Unlock() @@ -331,6 +346,14 @@ func (w *Watcher) fireFinal(armedFor time.Time) { log.Infof("auth session final-warning skipped (dismissed by user)") return } + now := w.nowFn() + if isLate(now, armedFor, 0) { + w.finalFiredAt = armedFor + w.mu.Unlock() + log.Infof("auth session final-warning skipped for deadline %s (passed %s ago)", + armedFor.Format(time.RFC3339), now.Round(0).Sub(armedFor).Round(time.Second)) + return + } w.finalFiredAt = armedFor recorder := w.recorder w.mu.Unlock() @@ -341,6 +364,39 @@ func (w *Watcher) fireFinal(armedFor time.Time) { publishWarning(recorder, armedFor, true) } +// fireLateLocked handles a T-WarningLead callback that fired inside the +// final-warning window: it sends the final warning in its place while the +// deadline has not passed and the user has not dismissed it, so a resume +// with time left still warns. The caller must hold w.mu; this helper +// releases it. +func (w *Watcher) fireLateLocked(armedFor, now time.Time) { + w.firedAt = armedFor + switch { + case w.dismissedAt.Equal(armedFor): + w.mu.Unlock() + log.Infof("auth session expiry soon warning skipped (dismissed by user)") + return + case w.finalFiredAt.Equal(armedFor): + w.mu.Unlock() + log.Infof("auth session expiry soon warning skipped (final warning already fired)") + return + case isLate(now, armedFor, 0): + w.mu.Unlock() + log.Infof("auth session expiry soon warning skipped for deadline %s (passed %s ago)", + armedFor.Format(time.RFC3339), now.Round(0).Sub(armedFor).Round(time.Second)) + return + } + w.finalFiredAt = armedFor + recorder := w.recorder + w.mu.Unlock() + if recorder == nil { + return + } + log.Infof("auth session expiry soon warning fired inside the final-warning window, sending final warning for deadline %s", + armedFor.Format(time.RFC3339)) + publishWarning(recorder, armedFor, true) +} + // armOneShotLocked schedules cb at fireAt. When fireAt is already in the // past it dispatches on the next scheduler tick so a state-change recorder // notification (invoked after w.mu is released) lands first. Caller must @@ -380,3 +436,11 @@ func publishWarning(recorder StatusRecorder, deadline time.Time, final bool) { meta, ) } + +// isLate reports whether the wall clock now has already reached armedFor +// minus cutoffLead. The timers run on the monotonic clock, which can stall +// while the host sleeps, so a timer can fire long after the window it was +// armed for. +func isLate(now, armedFor time.Time, cutoffLead time.Duration) bool { + return !now.Round(0).Before(armedFor.Add(-cutoffLead).Round(0)) +} diff --git a/client/internal/auth/sessionwatch/watcher_test.go b/client/internal/auth/sessionwatch/watcher_test.go index 4b49a94b6..cb2800978 100644 --- a/client/internal/auth/sessionwatch/watcher_test.go +++ b/client/internal/auth/sessionwatch/watcher_test.go @@ -527,3 +527,201 @@ func TestDismissBeforeUpdateIsNoop(t *testing.T) { } t.Fatalf("final-warning did not publish after no-op pre-Update Dismiss, events=%+v", r.snapshot()) } + +func TestIsLate(t *testing.T) { + armedFor := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC) + lead := 2 * time.Minute + tests := []struct { + name string + now time.Time + cutoffLead time.Duration + want bool + }{ + {"before cutoff", armedFor.Add(-3 * time.Minute), lead, false}, + {"at cutoff", armedFor.Add(-lead), lead, true}, + {"after cutoff", armedFor.Add(-time.Minute), lead, true}, + {"zero lead before deadline", armedFor.Add(-time.Second), 0, false}, + {"zero lead at deadline", armedFor, 0, true}, + {"zero lead after deadline", armedFor.Add(time.Second), 0, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isLate(tt.now, armedFor, tt.cutoffLead); got != tt.want { + t.Fatalf("isLate(%s, %s, %s) = %v, want %v", tt.now, armedFor, tt.cutoffLead, got, tt.want) + } + }) + } +} + +func TestIsLateIgnoresMonotonicReading(t *testing.T) { + now := time.Now() + wallOnly := now.Round(0) + if isLate(now, wallOnly.Add(time.Second), 0) { + t.Fatalf("now with monotonic reading must compare as wall clock before a later wall-only deadline") + } + if !isLate(now, wallOnly, 0) { + t.Fatalf("now with monotonic reading must compare as wall clock at an equal wall-only deadline") + } +} + +func TestLateTimerFiring(t *testing.T) { + tests := []struct { + name string + final bool + beforeDl time.Duration + wantWarns int + wantFinals int + }{ + {"warning on resume inside window", false, 3 * time.Minute, 1, 0}, + {"warning promoted to final inside final window", false, time.Minute, 0, 1}, + {"warning skipped past deadline", false, -time.Minute, 0, 0}, + {"final on resume before deadline", true, time.Minute, 0, 1}, + {"final skipped past deadline", true, -time.Minute, 0, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &fakeRecorder{} + w := New(r) + defer w.Close() + + // The deadline is an hour out so the real timers never fire + // during the test; the late callback is invoked directly with an + // injected clock that simulates a resume near the deadline. + d := time.Now().Add(time.Hour).Round(0) + w.nowFn = func() time.Time { return d.Add(-tt.beforeDl) } + if err := w.Update(d); err != nil { + t.Fatalf("Update: %v", err) + } + + if tt.final { + w.fireFinal(d) + } else { + w.fire(d) + } + + events := r.snapshot() + if got := countWhere(events, event.isWarning); got != tt.wantWarns { + t.Fatalf("expected %d warning publishes, got %d: %+v", tt.wantWarns, got, events) + } + if got := countWhere(events, event.isFinalWarning); got != tt.wantFinals { + t.Fatalf("expected %d final-warning publishes, got %d: %+v", tt.wantFinals, got, events) + } + }) + } +} + +func TestPromotedFinalWarningIsNotRepeated(t *testing.T) { + r := &fakeRecorder{} + w := New(r) + defer w.Close() + + d := time.Now().Add(time.Hour).Round(0) + now := d.Add(-time.Minute) + w.nowFn = func() time.Time { return now } + if err := w.Update(d); err != nil { + t.Fatalf("Update: %v", err) + } + + w.fire(d) + // The final timer was suspended too, so it fires even later than the + // warning timer, here still just before the deadline. + now = d.Add(-30 * time.Second) + w.fireFinal(d) + + events := r.snapshot() + if got := countWhere(events, event.isFinalWarning); got != 1 { + t.Fatalf("expected exactly 1 final-warning publish, got %d: %+v", got, events) + } + if got := countWhere(events, event.isWarning); got != 0 { + t.Fatalf("expected no regular warning publish, got %d: %+v", got, events) + } +} + +func TestPromotionRespectsDismiss(t *testing.T) { + r := &fakeRecorder{} + w := New(r) + defer w.Close() + + d := time.Now().Add(time.Hour).Round(0) + w.nowFn = func() time.Time { return d.Add(-time.Minute) } + if err := w.Update(d); err != nil { + t.Fatalf("Update: %v", err) + } + + w.Dismiss() + w.fire(d) + + events := r.snapshot() + if got := countWhere(events, func(e event) bool { return e.kind == publish }); got != 0 { + t.Fatalf("expected no publish after dismiss, got %d: %+v", got, events) + } +} + +func TestPromotionSkippedWhenFinalAlreadyFired(t *testing.T) { + r := &fakeRecorder{} + w := New(r) + defer w.Close() + + // Both timers fall in the past after a long suspend and are dispatched + // with a zero delay, so the final callback can run before the warning one. + d := time.Now().Add(time.Hour).Round(0) + w.nowFn = func() time.Time { return d.Add(-time.Minute) } + if err := w.Update(d); err != nil { + t.Fatalf("Update: %v", err) + } + + w.fireFinal(d) + w.fire(d) + + events := r.snapshot() + if got := countWhere(events, event.isFinalWarning); got != 1 { + t.Fatalf("expected exactly 1 final-warning publish, got %d: %+v", got, events) + } + if got := countWhere(events, event.isWarning); got != 0 { + t.Fatalf("expected no regular warning publish, got %d: %+v", got, events) + } +} + +func TestDeadlineOnlyRecordsDeadlineWithoutWarnings(t *testing.T) { + r := &fakeRecorder{} + w := NewDeadlineOnly(r) + defer w.Close() + + // With the default leads this deadline would otherwise fire both + // timers on the next tick. + d := time.Now().Add(50 * time.Millisecond).Round(0) + if err := w.Update(d); err != nil { + t.Fatalf("Update: %v", err) + } + if got := r.deadline(); !got.Equal(d) { + t.Fatalf("expected recorder deadline %v, got %v", d, got) + } + + time.Sleep(100 * time.Millisecond) + + events := r.snapshot() + if got := countWhere(events, func(e event) bool { return e.kind == publish }); got != 0 { + t.Fatalf("expected no publish in deadline-only mode, got %d: %+v", got, events) + } + if w.timer != nil || w.finalTimer != nil { + t.Fatal("expected no timers armed in deadline-only mode") + } +} + +func TestDeadlineOnlyStillRejectsOutOfRangeDeadlines(t *testing.T) { + r := &fakeRecorder{} + w := NewDeadlineOnly(r) + defer w.Close() + + if err := w.Update(time.Now().Add(time.Hour)); err != nil { + t.Fatalf("Update: %v", err) + } + + err := w.Update(time.Now().Add(-maxPastHorizon - time.Hour)) + if !errors.Is(err, ErrDeadlineInPast) { + t.Fatalf("expected ErrDeadlineInPast, got %v", err) + } + if got := r.deadline(); !got.IsZero() { + t.Fatalf("expected recorder cleared after rejection, got %v", got) + } +} diff --git a/client/internal/engine_sessionwatch.go b/client/internal/engine_sessionwatch.go index a46d73f87..05b46a465 100644 --- a/client/internal/engine_sessionwatch.go +++ b/client/internal/engine_sessionwatch.go @@ -1,4 +1,4 @@ -//go:build !js +//go:build !js && !android package internal @@ -7,10 +7,12 @@ import ( "github.com/netbirdio/netbird/client/internal/peer" ) -// newSessionWatcher returns the real SSO session expiry watcher for every -// non-wasm build. The js/wasm build gets a no-op stub from -// engine_sessionwatch_js.go so the sessionwatch package (and its timer -// machinery) never links into the wasm binary. +// newSessionWatcher returns the real SSO session expiry watcher. The js/wasm +// build gets a no-op stub from engine_sessionwatch_js.go so the sessionwatch +// package (and its timer machinery) never links into the wasm binary; the +// android build gets a deadline-only watcher from +// engine_sessionwatch_android.go because the app schedules the warnings +// itself. func newSessionWatcher(recorder *peer.Status) sessionDeadlineWatcher { return sessionwatch.New(recorder) } diff --git a/client/internal/engine_sessionwatch_android.go b/client/internal/engine_sessionwatch_android.go new file mode 100644 index 000000000..8317f9165 --- /dev/null +++ b/client/internal/engine_sessionwatch_android.go @@ -0,0 +1,12 @@ +//go:build android + +package internal + +import ( + "github.com/netbirdio/netbird/client/internal/auth/sessionwatch" + "github.com/netbirdio/netbird/client/internal/peer" +) + +func newSessionWatcher(recorder *peer.Status) sessionDeadlineWatcher { + return sessionwatch.NewDeadlineOnly(recorder) +}