diff --git a/client/internal/auth/sessionwatch/watcher.go b/client/internal/auth/sessionwatch/watcher.go index 9ad3d306f..e75a7022e 100644 --- a/client/internal/auth/sessionwatch/watcher.go +++ b/client/internal/auth/sessionwatch/watcher.go @@ -62,15 +62,25 @@ var ( ) // StatusRecorder is the side-effect surface the watcher drives on every -// state transition. Production wires this to peer.Status (NotifyStateChange +// state transition. Production wires this to peer.Status (SetSessionExpiresAt // for deadline change/clear, PublishEvent for the two warnings); tests pass // a fake recorder so the same surface is observable without an engine. // +// The watcher is the single owner of the deadline propagated to the +// recorder: every set, clear, sanity-check rejection and Close routes the +// value through SetSessionExpiresAt, so the SubscribeStatus snapshot the UI +// reads can never drift from the watcher's timer state. (SetSessionExpiresAt +// fans out its own state-change notification, so no separate notify is +// needed.) The recorder is server-scoped and outlives this engine-scoped +// watcher — without the Close-time clear a teardown (Down, or the Down+Up of +// a profile switch) would leave the next session showing the previous one's +// stale "expires in" value. +// // PublishEvent's signature mirrors peer.Status.PublishEvent: the watcher // composes the metadata internally so the wire format (MetaSession*) is // owned by sessionwatch, not the caller. type StatusRecorder interface { - NotifyStateChange() + SetSessionExpiresAt(deadline time.Time) PublishEvent( severity cProto.SystemEvent_Severity, category cProto.SystemEvent_Category, @@ -177,7 +187,7 @@ func (w *Watcher) Update(deadline time.Time) error { recorder := w.recorder w.mu.Unlock() if recorder != nil { - recorder.NotifyStateChange() + recorder.SetSessionExpiresAt(deadline) } log.Infof("auth session deadline set to: %s (in %s)", deadline.Format(time.RFC3339), time.Until(deadline).Round(time.Second)) return nil @@ -217,15 +227,30 @@ func (w *Watcher) Dismiss() { log.Infof("auth session final-warning dismissed for deadline %s", w.current.Format(time.RFC3339)) } -// Close stops any pending timer. Update calls after Close are ignored. +// Close stops any pending timer and drops the deadline on the status +// recorder. Update calls after Close are ignored. Clearing the recorder +// here is what keeps a teardown (Down, or the Down+Up of a profile switch) +// from leaving the next session showing this one's stale "expires in" +// value — the recorder is server-scoped and outlives this engine-scoped +// watcher, so nothing else drops the anchor on teardown. func (w *Watcher) Close() { w.mu.Lock() - defer w.mu.Unlock() if w.closed { + w.mu.Unlock() return } w.closed = true w.stopTimerLocked() + hadDeadline := !w.current.IsZero() + w.current = time.Time{} + w.firedAt = time.Time{} + w.finalFiredAt = time.Time{} + w.dismissedAt = time.Time{} + recorder := w.recorder + w.mu.Unlock() + if recorder != nil && hadDeadline { + recorder.SetSessionExpiresAt(time.Time{}) + } } // clearLocked drops the tracked deadline and notifies the recorder so @@ -245,7 +270,7 @@ func (w *Watcher) clearLocked() { recorder := w.recorder w.mu.Unlock() if recorder != nil { - recorder.NotifyStateChange() + recorder.SetSessionExpiresAt(time.Time{}) } log.Infof("auth session deadline cleared") } diff --git a/client/internal/auth/sessionwatch/watcher_test.go b/client/internal/auth/sessionwatch/watcher_test.go index 2d6297064..da2b6add6 100644 --- a/client/internal/auth/sessionwatch/watcher_test.go +++ b/client/internal/auth/sessionwatch/watcher_test.go @@ -10,12 +10,15 @@ import ( ) // fakeRecorder satisfies StatusRecorder and records every call so tests -// can observe what the watcher emits. NotifyStateChange and PublishEvent +// can observe what the watcher emits. SetSessionExpiresAt and PublishEvent // land in the same ordered events slice (with the Kind distinguishing -// them) so tests that care about ordering still work. +// them) so tests that care about ordering still work. lastDeadline holds +// the most recent value passed to SetSessionExpiresAt so tests can assert +// the recorder ended up cleared/set as expected. type fakeRecorder struct { - mu sync.Mutex - events []event + mu sync.Mutex + events []event + lastDeadline time.Time } type eventKind int @@ -34,12 +37,27 @@ type event struct { meta map[string]string } -func (r *fakeRecorder) NotifyStateChange() { +// SetSessionExpiresAt mirrors peer.Status: a same-value write is a no-op, +// a real change records the new value and fans out a state-change (the +// production recorder calls notifyStateChange internally). The baseline +// is the zero time, so an initial clear before any deadline is set emits +// nothing — matching the real recorder. +func (r *fakeRecorder) SetSessionExpiresAt(deadline time.Time) { r.mu.Lock() defer r.mu.Unlock() + if r.lastDeadline.Equal(deadline) { + return + } + r.lastDeadline = deadline r.events = append(r.events, event{kind: stateChange}) } +func (r *fakeRecorder) deadline() time.Time { + r.mu.Lock() + defer r.mu.Unlock() + return r.lastDeadline +} + func (r *fakeRecorder) PublishEvent( severity cProto.SystemEvent_Severity, category cProto.SystemEvent_Category, @@ -341,6 +359,44 @@ func TestCloseSilencesUpdates(t *testing.T) { } } +// TestCloseClearsRecorderDeadline pins the profile-switch fix: a watcher +// holding a live deadline must zero the recorder on Close so the next +// engine's watcher (and the UI reading the shared server-scoped recorder) +// doesn't start out showing the previous session's stale "expires in". +func TestCloseClearsRecorderDeadline(t *testing.T) { + r := &fakeRecorder{} + w := newWatcher(time.Hour, r) + + d := time.Now().Add(2 * time.Hour) + if err := w.Update(d); err != nil { + t.Fatalf("seed Update: %v", err) + } + if got := r.deadline(); !got.Equal(d) { + t.Fatalf("recorder deadline after Update = %v, want %v", got, d) + } + + w.Close() + + if got := r.deadline(); !got.IsZero() { + t.Fatalf("recorder deadline after Close = %v, want zero", got) + } +} + +// TestCloseWithoutDeadlineLeavesRecorderUntouched guards the symmetric +// case: closing a watcher that never held a deadline must not emit a +// redundant clear (the recorder may legitimately hold a value written by +// some other path; the watcher only owns what it set). +func TestCloseWithoutDeadlineLeavesRecorderUntouched(t *testing.T) { + r := &fakeRecorder{} + w := newWatcher(time.Hour, r) + + w.Close() + + if got := r.snapshot(); len(got) != 0 { + t.Fatalf("expected no events from Close on an empty watcher, got %+v", got) + } +} + func TestFinalWarningFiresAfterRegularWarning(t *testing.T) { r := &fakeRecorder{} // Warning fires at deadline-80ms, final at deadline-30ms. diff --git a/client/internal/engine_authsession.go b/client/internal/engine_authsession.go index 9f2ef467a..49e276b77 100644 --- a/client/internal/engine_authsession.go +++ b/client/internal/engine_authsession.go @@ -36,19 +36,21 @@ func (e *Engine) ApplySessionDeadline(ts *timestamppb.Timestamp) { var deadline time.Time // Explicit zero (seconds=0 AND nanos=0) is the sentinel for "disabled". // Everything else flows through Watcher.Update, whose sanity-checks - // reject out-of-range / pre-epoch / far-future / too-stale values; the - // catch-block below converts any rejection into a clear. + // reject out-of-range / pre-epoch / far-future / too-stale values and + // clear on rejection. if ts.GetSeconds() != 0 || ts.GetNanos() != 0 { deadline = ts.AsTime().UTC() } - if e.sessionWatcher != nil { - if err := e.sessionWatcher.Update(deadline); err != nil { - log.Errorf("auth session deadline rejected: %v, clearing", err) - deadline = time.Time{} - } + if e.sessionWatcher == nil { + return } - if e.statusRecorder != nil { - e.statusRecorder.SetSessionExpiresAt(deadline) + // Watcher.Update owns the propagation to the status recorder (the + // SubscribeStatus / Status snapshot the UI reads): a set writes the + // deadline, a clear or a sanity-check rejection writes the zero value. + // Keeping a single writer is what stops the recorder from drifting out + // of sync with the warning timers. + if err := e.sessionWatcher.Update(deadline); err != nil { + log.Errorf("auth session deadline rejected: %v, clearing", err) } } diff --git a/client/internal/peer/status.go b/client/internal/peer/status.go index 010d09846..4dced640c 100644 --- a/client/internal/peer/status.go +++ b/client/internal/peer/status.go @@ -763,10 +763,19 @@ func (d *Status) SetSessionExpiresAt(deadline time.Time) { } // GetSessionExpiresAt returns the most recently recorded SSO session deadline, -// or the zero value when no deadline is tracked. +// or the zero value when no deadline is tracked. A deadline that has already +// slipped into the past reports as "none": once the session has expired it is +// no longer a meaningful countdown, and the sessionwatch.Watcher does not +// arm a timer at the deadline itself to clear it (only the two pre-expiry +// warnings). Without this guard the UI would keep painting a stale +// "expires in …" against a moment that has passed until the next login, +// extend, or teardown rewrote the value. func (d *Status) GetSessionExpiresAt() time.Time { d.mux.Lock() defer d.mux.Unlock() + if !d.sessionExpiresAt.IsZero() && d.sessionExpiresAt.Before(time.Now()) { + return time.Time{} + } return d.sessionExpiresAt }