feat(cli): refresh access token before expiry#44
Merged
Conversation
- Add a refresh threshold setting with --refresh-threshold flag and REFRESH_THRESHOLD env, defaulting to 5m - Refresh the stored access token in token get and the main flow when it expires within the threshold window - Reuse the existing token without any network request while it is further than the threshold from expiry - Fall back to the existing valid token with a warning when refresh fails, and only re-authenticate once expired - Document the new threshold setting in the configuration reference Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI’s token handling so both the main run() flow and authgate-cli token get proactively refresh a stored access token when it is within a configurable pre-expiry window (default 5 minutes), aiming to keep users/scripts continuously authenticated.
Changes:
- Introduces
needsRefresh/ensureFreshTokento decide and perform pre-expiry refresh with graceful degradation when the token is still valid. - Adds
--refresh-threshold/REFRESH_THRESHOLDconfiguration plumbing and documentation. - Updates
token getto use full config + endpoint resolution and adds unit/integration tests for proactive refresh behavior and boundary cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
auth.go |
Adds refresh-decision helper and a shared “ensure fresh token” path used by commands. |
main.go |
Switches cached-token decision from “not expired” to “not within refresh threshold” and adds graceful reuse on refresh failure. |
config.go |
Adds refresh-threshold flag/env/default and stores it on AppConfig. |
token_cmd.go |
Updates token get to load full config, resolve endpoints, and refresh near expiry before printing. |
config_test.go |
Adds tests covering refresh-threshold config precedence and default. |
token_cmd_test.go |
Updates existing tests for new runTokenGet signature and adds proactive refresh integration tests + needsRefresh unit tests. |
README.md |
Documents REFRESH_THRESHOLD and corresponding flag behavior. |
Comments suppressed due to low confidence (1)
token_cmd.go:342
- The JSON
expiredfield usestime.Now().After(tok.ExpiresAt), which reports not-expired whennow == ExpiresAt. For an expiry instant, equality should count as expired. Use an inclusive comparison (e.g.!time.Now().Before(tok.ExpiresAt)) so boundary behavior matches typical expiry semantics.
out := tokenGetOutput{
AccessToken: tok.AccessToken,
TokenType: tok.TokenType,
ExpiresAt: tok.ExpiresAt,
Expired: time.Now().After(tok.ExpiresAt),
ClientID: tok.ClientID,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+100
to
+101
| // Graceful degradation: keep using the old token while it's still valid. | ||
| if time.Now().Before(tok.ExpiresAt) { |
Comment on lines
+94
to
+101
| if !needsRefresh(tok, cfg.RefreshThreshold, time.Now()) { | ||
| return tok, false, nil | ||
| } | ||
|
|
||
| newTok, err := refreshAccessToken(ctx, cfg, tok.RefreshToken) | ||
| if err != nil { | ||
| // Graceful degradation: keep using the old token while it's still valid. | ||
| if time.Now().Before(tok.ExpiresAt) { |
| ) | ||
| fmt.Fprintf(stderr, "Hint: run 'authgate-cli' to re-authenticate.\n") | ||
| default: | ||
| fmt.Fprintf(stderr, "Error: failed to load token: %v\n", err) |
Comment on lines
89
to
96
| @@ -95,6 +95,12 @@ func run(ctx context.Context, ui tui.Manager, cfg *AppConfig) int { | |||
| newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken) | |||
| if err != nil { | |||
- Refuse to reuse an empty access token when refresh fails, surfacing the error instead of returning an unusable token - Capture the current time once so the refresh decision and validity check reason about the same instant - Reword the token get fallback error so it no longer implies a load failure when refresh failed - Treat the JSON expired field as inclusive at the expiry instant, matching the refresh boundary Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
85
to
88
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| cfg := loadStoreConfig() | ||
| cfg := loadConfig() | ||
| resolveEndpoints(cmd.Context(), cfg) | ||
| if code := runTokenGet( |
Comment on lines
93
to
95
| @@ -95,6 +95,12 @@ func run(ctx context.Context, ui tui.Manager, cfg *AppConfig) int { | |||
| newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken) | |||
Comment on lines
89
to
90
| if !needsRefresh(existing, cfg.RefreshThreshold, time.Now()) { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenStillValid}) |
Comment on lines
+101
to
+105
| newTok, err := refreshAccessToken(ctx, cfg, tok.RefreshToken) | ||
| if err != nil { | ||
| // Graceful degradation: keep using the old token only while it's still | ||
| // usable — present and not yet expired. An empty access token is never | ||
| // reusable, so a refresh failure there surfaces the error rather than |
| | `USERINFO_TIMEOUT` | `10s` | `/oauth/userinfo` request | | ||
| | `DISCOVERY_TIMEOUT` | `10s` | OIDC Discovery fetch | | ||
| | `REVOCATION_TIMEOUT` | `10s` | RFC 7009 revocation request issued by `token delete` | | ||
| | `REFRESH_THRESHOLD` | `5m` | Refresh the stored access token when it expires within this window; used by 'token get' and the main flow. No network request while the token is further from expiry | |
Comment on lines
+646
to
+648
| if called.Load() { | ||
| t.Error("no network request expected when token is far from expiry") | ||
| } |
- Defer OIDC discovery until a refresh is actually needed so token get makes no network request while the token is far from expiry - Skip the refresh round trip entirely when no refresh token is stored, degrading gracefully when the access token is still valid - Capture the current time once in run() so the refresh decision and reuse check share one instant - Add tests asserting discovery is not contacted far from expiry and is resolved before refreshing near expiry Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+86
to
+88
| cfg := loadConfig() | ||
| // Endpoints are resolved lazily inside ensureFreshToken, only when a | ||
| // refresh is actually needed, so reads far from expiry stay offline. |
Comment on lines
+115
to
+119
| // Without a refresh token a refresh can only fail after a wasted round | ||
| // trip, so skip the network entirely and degrade gracefully. | ||
| if tok.RefreshToken == "" { | ||
| return reuseOrFail(ErrRefreshTokenExpired) | ||
| } |
| | `--version` | — | Print version and exit (also available as `version` subcommand) | | ||
|
|
||
| Each timeout / size environment variable also has a matching flag with the same name in lower-kebab-case (e.g. `--callback-timeout`, `--max-response-body-size`). | ||
| Each timeout / size / threshold environment variable also has a matching flag with the same name in lower-kebab-case (e.g. `--callback-timeout`, `--max-response-body-size`, `--refresh-threshold`). |
Comment on lines
96
to
98
| @@ -95,6 +98,12 @@ func run(ctx context.Context, ui tui.Manager, cfg *AppConfig) int { | |||
| newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken) | |||
| case errors.Is(err, ErrRefreshTokenExpired): | ||
| fmt.Fprintf( | ||
| stderr, | ||
| "Error: stored token expired and refresh token is no longer valid: %v\n", |
- Build the network-capable config lazily so token get validates SERVER_URL and constructs the HTTP client only when a refresh is actually needed, keeping far-from-expiry reads fully offline - Add ErrNoRefreshToken so a missing refresh token is no longer reported as an expired one - Reword the token get re-auth error to avoid asserting expiry for non-expiry cases - Emit the expired status only for truly expired tokens, not ones merely inside the refresh window - List --refresh-threshold in the CLI flags reference table Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+101
to
+110
| if !now.Before(existing.ExpiresAt) { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenExpired}) | ||
| } | ||
| newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken) | ||
| if err != nil { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshFailed, Err: err}) | ||
| // Graceful degradation: reuse the old token while it's still | ||
| // valid; only fall through to re-authentication once expired. | ||
| if now.Before(existing.ExpiresAt) { | ||
| storage = &existing |
| MaxResponseBodySize int64 | ||
|
|
||
| // RefreshThreshold is the window before expiry within which the stored | ||
| // access token is proactively refreshed. Resolved only by loadConfig. |
- Mirror ensureFreshToken in the main flow: skip the doomed network call (and misleading "refresh failed" status) when no refresh token exists, reusing the still-valid token or falling through to re-auth - Resolve RefreshThreshold in loadStoreConfig so the offline token get path and loadConfig share one source, and correct the field comment Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
main.go:120
- In the refresh-failed branch, the graceful-degradation path reuses
existingwhenevernow.Before(existing.ExpiresAt), even ifexisting.AccessTokenis empty. SinceneedsRefreshtreats an empty access token as requiring refresh, a failure here could leavestorageset to an empty token and cause downstream calls (verify/userinfo) to run with an empty Authorization header. Add a non-empty access-token check (as inensureFreshToken) before reusing the cached token.
} else if newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken); err != nil {
ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshFailed, Err: err})
// Graceful degradation: reuse the old token while it's still
// valid; only fall through to re-authentication once expired.
if now.Before(existing.ExpiresAt) {
storage = &existing
flow = "cached"
}
Comment on lines
+104
to
+112
| if existing.RefreshToken == "" { | ||
| // No refresh token means a refresh can only fail, so skip the | ||
| // network call (and the misleading "refresh failed" status). | ||
| // Reuse the old token while it's still valid; otherwise fall | ||
| // through to re-authentication. | ||
| if now.Before(existing.ExpiresAt) { | ||
| storage = &existing | ||
| flow = "cached" | ||
| } |
- Require a non-empty access token (not just a future expiry) before reusing the existing token when refresh is skipped or fails, mirroring ensureFreshToken so a corrupt token never proceeds to fail confusingly downstream Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+118
to
+123
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshFailed, Err: err}) | ||
| // Graceful degradation: reuse the old token while it's still | ||
| // valid; only fall through to re-authentication once expired. | ||
| if reuseValid { | ||
| storage = &existing | ||
| flow = "cached" |
- Emit EventRefreshFailed only when the main flow will actually re-authenticate; when the cached token is still usable, keep using it silently instead of showing "starting new authentication flow" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+97
to
+103
| // Only report expiry when the token is actually expired; a token | ||
| // merely inside the refresh-threshold window is still valid, so the | ||
| // "expired" message would be misleading. The subsequent refresh | ||
| // success/failure status conveys what happened in that case. | ||
| if !now.Before(existing.ExpiresAt) { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenExpired}) | ||
| } |
Comment on lines
+195
to
+199
| // Resolved here (not loadConfig) so the offline `token get` path can decide | ||
| // whether a refresh is due without building the full network config. | ||
| cfg.RefreshThreshold = getDurationConfig( | ||
| flagRefreshThreshold, "REFRESH_THRESHOLD", defaultRefreshThreshold) | ||
|
|
- Resolve REFRESH_THRESHOLD with a dedicated uncapped parser so legitimate values above 10m (e.g. 30m, 1h) are honored; a large threshold only refreshes sooner and can't hang the CLI - Document that the threshold, unlike timeouts, is not capped Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add an EventTokenRefreshing status so proactive (near-expiry) refreshes surface a "Refresh token" step with accurate "nearing expiry" text, instead of either no feedback or a misleading "expired" label - Reuse one neutral "Refresh token" step for expired and proactive refreshes, resolved by success/failure - Drop the "starting new authentication flow" line from the refresh-failed status so it reads correctly whether the CLI degrades to the cached token or re-authenticates - Emit the failure status on every failed refresh so the step never dangles in progress Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+89
to
+131
| // Capture now once so the refresh decision and the graceful-degradation | ||
| // check below reason about the same instant. | ||
| now := time.Now() | ||
| if !needsRefresh(existing, cfg.RefreshThreshold, now) { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenStillValid}) | ||
| storage = &existing | ||
| flow = "cached" | ||
| } else { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenExpired}) | ||
| newStorage, err := refreshAccessToken(ctx, cfg, existing.RefreshToken) | ||
| if err != nil { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshFailed, Err: err}) | ||
| // reuseValid reports whether the old token is still usable as-is — | ||
| // present and not yet expired. Mirrors ensureFreshToken so a corrupt | ||
| // token (empty access token but future expiry) is never reused. | ||
| reuseValid := existing.AccessToken != "" && now.Before(existing.ExpiresAt) | ||
| if existing.RefreshToken == "" { | ||
| // No refresh token means a refresh can only fail, so skip the | ||
| // network call entirely (no refresh step is shown). Reuse the old | ||
| // token while it's still valid; otherwise fall through to re-auth. | ||
| if reuseValid { | ||
| storage = &existing | ||
| flow = "cached" | ||
| } | ||
| } else { | ||
| storage = newStorage | ||
| flow = "refreshed" | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshSuccess}) | ||
| // About to refresh: show an accurate in-progress status — | ||
| // "expired" only when actually expired, otherwise "near expiry". | ||
| if now.Before(existing.ExpiresAt) { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenRefreshing}) | ||
| } else { | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventTokenExpired}) | ||
| } | ||
| newStorage, refreshErr := refreshAccessToken(ctx, cfg, existing.RefreshToken) | ||
| if refreshErr != nil { | ||
| // The refresh genuinely failed, so mark it failed in the UI. | ||
| // Then degrade gracefully (reuse the still-valid token) or | ||
| // fall through to re-authentication once expired. | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshFailed, Err: refreshErr}) | ||
| if reuseValid { | ||
| storage = &existing | ||
| flow = "cached" | ||
| } | ||
| } else { | ||
| storage = newStorage | ||
| flow = "refreshed" | ||
| ui.ShowStatus(tui.StatusUpdate{Event: tui.EventRefreshSuccess}) | ||
| } |
…FreshToken - Extract tokenUsable so the "reuse the existing token" condition (non-empty access token and not yet expired, including the expiry boundary) is defined once and used by both the main flow and token get, removing the duplicated inline check Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+137
to
+155
| // A network refresh is required, so upgrade to the full network-capable | ||
| // config now (deferring SERVER_URL validation and transport setup until | ||
| // this point keeps the far-from-expiry path offline). | ||
| full := cfg | ||
| if loadFull != nil { | ||
| full = loadFull() | ||
| } | ||
|
|
||
| // Resolve endpoints lazily too. Callers that pre-populate Endpoints skip | ||
| // this (e.g. tests). | ||
| if full.Endpoints.TokenURL == "" { | ||
| resolveEndpoints(ctx, full) | ||
| } | ||
|
|
||
| newTok, err := refreshAccessToken(ctx, full, tok.RefreshToken) | ||
| if err != nil { | ||
| return reuseOrFail(err) | ||
| } | ||
| return *newTok, true, nil |
- Pin the lazily-built full config's store and client ID to the originally loaded ones in ensureFreshToken, so a refreshed token is saved to the same backend it came from rather than a freshly resolved store that could differ (e.g. TOKEN_STORE=auto), which would cause repeated refreshes Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| // flow, which prints its own status. | ||
| fmt.Printf("Refresh failed: %v\n", update.Err) | ||
| fmt.Println("Starting new authentication flow...") | ||
| case EventNewAuthFlow, EventNoExistingTokens: |
Comment on lines
+195
to
+197
| // Resolved here (not loadConfig) so the offline `token get` path can decide | ||
| // whether a refresh is due without building the full network config. | ||
| cfg.RefreshThreshold = getRefreshThresholdConfig(flagRefreshThreshold) |
| | `USERINFO_TIMEOUT` | `10s` | `/oauth/userinfo` request | | ||
| | `DISCOVERY_TIMEOUT` | `10s` | OIDC Discovery fetch | | ||
| | `REVOCATION_TIMEOUT` | `10s` | RFC 7009 revocation request issued by `token delete` | | ||
| | `REFRESH_THRESHOLD` | `5m` | Refresh the stored access token when it expires within this window; used by 'token get' and the main flow. No network request while the token is further from expiry | |
…rding - Give EventNewAuthFlow its own "Starting authentication flow..." message so it no longer claims "no existing tokens" when re-auth follows a failed refresh of existing tokens - Update the loadStoreConfig comment to reflect that token get may load network config lazily and refresh when near expiry - Use backticks around `token get` in the REFRESH_THRESHOLD reference row for consistency Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Keep CLI users (and scripts calling
authgate-cli token get) logged in by proactively refreshing the stored access token before it expires. Previously the token was only refreshed after full expiry, andtoken getnever refreshed at all. Now, when the token falls within a configurable threshold of expiry (default 5m), it is exchanged for a fresh one and saved back.AI Authorship
auth.go,config.go,main.go,token_cmd.go,README.md,config_test.go,token_cmd_test.goChange classification
token getand the mainrun()flow. A bug here affects every authenticated invocation.Plan reference
See
plan.md(branch root). Goal: "once logged in, stay logged in" — read the access token, and if it's withinNminutes of expiry (N configurable, default 5m), refresh it proactively. Done = (1)token getandrun()auto-refresh near expiry; (2) threshold tunable via--refresh-thresholdflag andREFRESH_THRESHOLDenv; (3) graceful degradation when refresh fails but the old token is still valid.Verification
TestNeedsRefresh(boundary cases: empty token, far-from-expiry, exactly-at-threshold, inside, expired),TestRefreshThresholdConfig(default 5m + env/flag resolution)TestRunTokenGet_ProactiveRefreshagainst anhttptesttoken endpoint with a temp-file storetoken getrefreshes, prints the new token, storeExpiresAtupdated to the futureinvalid_grant→ non-zero exit, stderr re-auth hintVerifiability check
needsRefresh/ensureFreshTokendoc comments; README config table)Security check
validateTokenResponseformatHTTPError/ OAuth error masking;token get --jsonstill omits the refresh tokenRisk & rollback
token getchanges from a purely local read to a possible network call. Mitigation: it only contacts the server when inside the threshold window; on failure it gracefully returns the still-valid token, so scripts aren't interrupted. Offline use beyond the threshold makes zero network requests (test-covered).token getnow usesloadConfig()+resolveEndpointsinstead ofloadStoreConfig(), so environments missing endpoint config could error during resolution. Mitigation: reuses the existingbuildTokenInspectCmdresolution path and its warnings/fallbacks.token.jsonon-disk format is unchanged, so no data migration is needed.Reviewer guide
auth.go—needsRefresh(boundary semantics: inclusive at-threshold) andensureFreshToken(graceful-degradation branches)main.go—run()now uses!needsRefresh(...)and reuses the old token on refresh failure when still validtoken_cmd.go—runTokenGeterror handling (not-found vsErrRefreshTokenExpiredvs generic) and the switch toloadConfig()+resolveEndpointsconfig.go— mechanical addition mirroring existing timeout settingsREADME.md— docs only