mirror of
https://github.com/netbirdio/netbird.git
synced 2026-05-31 04:59:54 +00:00
session: clear stale SSO deadline on teardown and after expiry
The session deadline lived in two sinks kept in sync by hand: ApplySessionDeadline wrote both the (engine-scoped) sessionwatch.Watcher and the (server-scoped) peer.Status recorder. The clear paths only touched the watcher, so the recorder — which is what the Status RPC / SubscribeStatus snapshot the UI reads from — kept reporting a deadline that had gone stale, surfacing as a frozen "expires in …" countdown. Two cases were leaking: - Profile switch / Down: the watcher is recreated per engine but the recorder outlives it, so a switch to a profile whose server sends no deadline left the previous profile's value in place. - In-place expiry: the watcher arms warning timers at T-WarningLead and T-FinalWarningLead but nothing at the deadline itself, so once the moment passed the recorder kept the now-past value indefinitely. Make the watcher the single writer of the recorder deadline (Update / clearLocked / Close all route through SetSessionExpiresAt) so teardown clears it, and guard GetSessionExpiresAt to report a past deadline as none so in-place expiry stops painting a stale countdown.
This commit is contained in:
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user