From 4b771e7ad5c7030de5007dfb091957b2f05656a2 Mon Sep 17 00:00:00 2001 From: Hendrik Brombeer Date: Wed, 6 May 2026 12:36:20 +0200 Subject: [PATCH] fix(auth): treat refresh_expires_in=0 as no-expiry for offline tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI requests `offline_access` on login, and Keycloak honours it — the issued refresh token has `typ: Offline` and never expires. But Keycloak's wire format signals "no expiry" by setting `refresh_expires_in: 0`, and the CLI was naively computing `RefreshExpiresAt = time.Now().Add(0 * time.Second)`. That stored the expiry as the wall-clock time of login itself, so the very next `grounds ` (or even `grounds doctor`) saw RefreshExpiresAt < now and demanded `grounds login`. The actual offline token sat unused in the keychain the whole time. Fix: - New `RefreshExpiryFromSeconds(seconds int) time.Time` helper that returns the zero `time.Time` when seconds <= 0 (the canonical "no expiry" sentinel) and `time.Now().Add(seconds * time.Second)` otherwise. - New `(*Credentials).IsRefreshAlive()` predicate: zero time means alive; otherwise compare to wall clock. - All three writers (login flow's CredentialsFromToken, source.go's inline refresh, doctor.go's inline refresh) go through the new helper. - All three readers (source.go and doctor.go gates, plus doctor.go's status summary) go through IsRefreshAlive. Doctor's "valid for X" line now prints "no expiry (offline token)" when applicable. Tests cover the boundary cases: seconds=0 → zero time, negative → zero time, positive → finite future, IsRefreshAlive matrix. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/grounds/commands/doctor.go | 20 +++++++++------ internal/auth/credentials.go | 26 +++++++++++++++++++ internal/auth/credentials_test.go | 42 +++++++++++++++++++++++++++++++ internal/auth/refresh.go | 2 +- internal/auth/source.go | 9 ++++++- 5 files changed, 90 insertions(+), 9 deletions(-) diff --git a/cmd/grounds/commands/doctor.go b/cmd/grounds/commands/doctor.go index ad84e13..f3a7c13 100644 --- a/cmd/grounds/commands/doctor.go +++ b/cmd/grounds/commands/doctor.go @@ -239,12 +239,14 @@ func checkAuth(ctx context.Context) checkResult { return checkResult{name: "Auth", status: statusError, summary: "You are not logged in", details: []string{"Run `grounds login` to authenticate."}} } // The access_token Keycloak issues is short-lived (≈5 min by default) - // but the refresh_token lives much longer (≈30 d). Real commands go - // through FileTokenSource.Token which transparently refreshes; doctor - // must do the same or it reports "expired" while everything else - // works fine. Drop down to refresh + persist if needed. + // but the refresh_token is an offline token that doesn't expire + // (Keycloak signals that with refresh_expires_in: 0, which we map + // to a zero `time.Time` — see auth.RefreshExpiryFromSeconds). + // Real commands go through FileTokenSource.Token which transparently + // refreshes; doctor must do the same or it reports "expired" while + // everything else works fine. Drop down to refresh + persist if needed. if time.Now().After(c.ExpiresAt.Add(-30 * time.Second)) { - if time.Now().After(c.RefreshExpiresAt) { + if !c.IsRefreshAlive() { return checkResult{name: "Auth", status: statusError, summary: "Your login session has expired", details: []string{"Run `grounds login` to authenticate again."}} } device := &auth.DeviceClient{ @@ -259,13 +261,17 @@ func checkAuth(ctx context.Context) checkResult { c.AccessToken = fresh.AccessToken c.RefreshToken = fresh.RefreshToken c.ExpiresAt = time.Now().Add(time.Duration(fresh.ExpiresIn) * time.Second) - c.RefreshExpiresAt = time.Now().Add(time.Duration(fresh.RefreshExpiresIn) * time.Second) + c.RefreshExpiresAt = auth.RefreshExpiryFromSeconds(fresh.RefreshExpiresIn) if err := store.Save(c); err != nil { return checkResult{name: "Auth", status: statusError, summary: "Refreshed your session but could not save it", details: []string{err.Error()}} } return checkResult{name: "Auth", status: statusOK, summary: c.PreferredUser + " is logged in (session refreshed, valid for " + time.Until(c.ExpiresAt).Round(time.Minute).String() + ")"} } - return checkResult{name: "Auth", status: statusOK, summary: c.PreferredUser + " is logged in (session valid for " + time.Until(c.ExpiresAt).Round(time.Minute).String() + ", refresh token valid for " + time.Until(c.RefreshExpiresAt).Round(time.Hour).String() + ")"} + refreshSummary := "no expiry (offline token)" + if !c.RefreshExpiresAt.IsZero() { + refreshSummary = "refresh token valid for " + time.Until(c.RefreshExpiresAt).Round(time.Hour).String() + } + return checkResult{name: "Auth", status: statusOK, summary: c.PreferredUser + " is logged in (session valid for " + time.Until(c.ExpiresAt).Round(time.Minute).String() + ", " + refreshSummary + ")"} } func checkAPI(ctx context.Context) checkResult { diff --git a/internal/auth/credentials.go b/internal/auth/credentials.go index e4b8949..fc5e9bb 100644 --- a/internal/auth/credentials.go +++ b/internal/auth/credentials.go @@ -45,3 +45,29 @@ func ParseCredentials(b []byte) (*Credentials, error) { } return c, nil } + +// RefreshExpiryFromSeconds turns the OAuth `refresh_expires_in` field +// into the absolute timestamp we persist. Keycloak returns `0` for +// offline tokens (the canonical "no expiry" indicator), so we map that +// to the zero `time.Time` value and let the IsRefreshAlive predicate +// treat it as "lives forever". Naively doing `time.Now().Add(0)` would +// stamp the token as expired the instant after login, which is exactly +// the regression that bit grounds-cli — see IsRefreshAlive for the +// matching read path. +func RefreshExpiryFromSeconds(seconds int) time.Time { + if seconds <= 0 { + return time.Time{} + } + return time.Now().Add(time.Duration(seconds) * time.Second) +} + +// IsRefreshAlive reports whether the stored refresh token is still +// usable. A zero RefreshExpiresAt means the token is an OIDC offline +// token (no expiry) and is always alive; otherwise compare to wall +// clock. +func (c *Credentials) IsRefreshAlive() bool { + if c.RefreshExpiresAt.IsZero() { + return true + } + return time.Now().Before(c.RefreshExpiresAt) +} diff --git a/internal/auth/credentials_test.go b/internal/auth/credentials_test.go index 7fb5b12..25b008f 100644 --- a/internal/auth/credentials_test.go +++ b/internal/auth/credentials_test.go @@ -83,3 +83,45 @@ func TestParseLegacyFileWithoutVersion(t *testing.T) { t.Errorf("legacy file should be upgraded to v%d, got v%d", CredentialsVersion, c.Version) } } + +// TestRefreshExpiryFromSeconds_offline maps Keycloak's `0` for offline +// tokens to a zero `time.Time` so the CLI doesn't immediately decide +// the refresh token is dead the instant after login. +func TestRefreshExpiryFromSeconds_offline(t *testing.T) { + if !RefreshExpiryFromSeconds(0).IsZero() { + t.Error("seconds=0 should map to zero time.Time") + } + if !RefreshExpiryFromSeconds(-5).IsZero() { + t.Error("negative seconds should also map to zero time.Time") + } +} + +func TestRefreshExpiryFromSeconds_finite(t *testing.T) { + got := RefreshExpiryFromSeconds(60) + if got.IsZero() { + t.Fatal("seconds=60 should produce a non-zero expiry") + } + if got.Before(time.Now()) { + t.Error("seconds=60 should produce an expiry in the future") + } +} + +func TestIsRefreshAlive(t *testing.T) { + now := time.Now() + cases := []struct { + name string + c Credentials + want bool + }{ + {"offline (zero time)", Credentials{}, true}, + {"future", Credentials{RefreshExpiresAt: now.Add(time.Hour)}, true}, + {"past", Credentials{RefreshExpiresAt: now.Add(-time.Hour)}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.c.IsRefreshAlive(); got != tc.want { + t.Errorf("got %v, want %v", got, tc.want) + } + }) + } +} diff --git a/internal/auth/refresh.go b/internal/auth/refresh.go index ff36c30..70fb89d 100644 --- a/internal/auth/refresh.go +++ b/internal/auth/refresh.go @@ -46,7 +46,7 @@ func CredentialsFromToken(t *TokenResponse, email, preferred string) *Credential AccessToken: t.AccessToken, RefreshToken: t.RefreshToken, ExpiresAt: time.Now().Add(time.Duration(t.ExpiresIn) * time.Second), - RefreshExpiresAt: time.Now().Add(time.Duration(t.RefreshExpiresIn) * time.Second), + RefreshExpiresAt: RefreshExpiryFromSeconds(t.RefreshExpiresIn), Email: email, PreferredUser: preferred, } diff --git a/internal/auth/source.go b/internal/auth/source.go index 760fef4..7775e2a 100644 --- a/internal/auth/source.go +++ b/internal/auth/source.go @@ -19,6 +19,13 @@ func (f *FileTokenSource) Token(ctx context.Context) (string, error) { return "", err } if time.Now().After(c.ExpiresAt.Add(-30 * time.Second)) { + // Refuse to even attempt refresh against a known-dead token — + // the only outcome is a useless network round-trip + the + // `_ = Store.Delete()` below would unhelpfully nuke the + // already-stale credentials. + if !c.IsRefreshAlive() { + return "", fmt.Errorf("session expired: run 'grounds login' again") + } fresh, err := f.Device.Refresh(ctx, c.RefreshToken) if err != nil { _ = f.Store.Delete() @@ -27,7 +34,7 @@ func (f *FileTokenSource) Token(ctx context.Context) (string, error) { c.AccessToken = fresh.AccessToken c.RefreshToken = fresh.RefreshToken c.ExpiresAt = time.Now().Add(time.Duration(fresh.ExpiresIn) * time.Second) - c.RefreshExpiresAt = time.Now().Add(time.Duration(fresh.RefreshExpiresIn) * time.Second) + c.RefreshExpiresAt = RefreshExpiryFromSeconds(fresh.RefreshExpiresIn) if err := f.Store.Save(c); err != nil { return "", err }