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 }