Guarantee minimum remaining token lifetime - #1
Conversation
Emit only access tokens with at least MinLifetime (20m) remaining, so a consumer that caches the token (e.g. mise's env_cache) can never serve it past its own exp. MSAL Go exposes no force-refresh option and refreshes proactively only when Entra sends refresh_in; when the cached token is inside the window, force a fresh mint by hiding the cached access token on cache reload, so the silent path redeems the refresh token instead. Extract the acquisition logic, cache, and lifetime policy into internal/token with tests beside the package; main.go is now CLI wiring. clientID and tenantID stay in main so the release `-ldflags -X main.*` path is unchanged. README: document the lifetime guarantee and fix a stale "device-code flow" mention (the tool uses an interactive browser sign-in). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change moves Azure DevOps token scopes, MSAL cache persistence, and minimum-lifetime refresh logic into ChangesToken refresh flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant token.Silent
participant MSAL
participant token.FileCache
CLI->>token.Silent: Request Azure DevOps token
token.Silent->>MSAL: AcquireTokenSilent
MSAL->>token.FileCache: Load cached credentials
token.FileCache-->>MSAL: Return cache contents
MSAL-->>token.Silent: Return token and expiry
token.Silent->>token.FileCache: Drop access token if lifetime is below 20 minutes
token.Silent->>MSAL: Retry silent acquisition
MSAL-->>CLI: Return token meeting minimum lifetime
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/token/token.go`:
- Around line 48-60: Update FileCache.Replace to ignore the os.ReadFile error
only when it indicates os.ErrNotExist; propagate all other cache-read failures,
including permission, I/O, and path errors. Preserve the existing
StripAccessTokens and Unmarshal flow for successfully read data.
- Around line 63-77: The FileCache.Export implementation must serialize cache
updates across processes; atomic rename alone can overwrite newer token state
with stale data. Update Export, or the surrounding reload/export flow, to
acquire a cross-process lock covering the latest cache reload through
temporary-file write and rename, then release it reliably on all error paths;
preserve the existing atomic permissions and rename behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbb9e938-2866-40d6-87ec-dc2550fea7ec
📒 Files selected for processing (4)
README.mdinternal/token/token.gointernal/token/token_test.gomain.go
FileCache.Replace returned nil on any os.ReadFile error, masking a permission/IO/path failure as an empty cache (i.e. "not signed in") and tripping golangci-lint nilerr. Ignore only os.ErrNotExist (the normal pre-login state) and wrap the rest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Guarantee that every token
az-devops-tokenprints has at leastMinLifetime(20m) of remaining life, forcing a refresh when the cached token is closer to expiry. Also extracts the acquisition logic, cache, and lifetime policy intointernal/token, leavingmain.goas CLI wiring.Why
A consumer that caches the token — notably mise's
env_cache— would otherwise pin whatever the tool returned for the cache TTL. MSAL hands back a cached access token whose remaining life is variable (as little as minutes before it refreshes), so a cached copy could be served past its ownexp, producing sporadic401s. With a guaranteed minimum lifetime, a consumer can safely cache the token for any TTL below that bound.This is the upstream fix for the
env_cacheconcern raised on energyworldnet/build#106 (where the cache was reverted pending this).How
MSAL Go exposes no force-refresh option and refreshes proactively only when Entra sends
refresh_in. When the cached token is inside the window, the tool forces a fresh mint by hiding the cached access token on cache reload (StripAccessTokens), so MSAL's silent path redeems the refresh token instead (apps/internal/base: "redeem a cached refresh token"). The refresh token and all other cache sections are preserved.Tests / checks
internal/tokenunit tests (StripAccessTokens,NeedsForcedRefresh).go vet,gofmt,go build, and a graceful-degradation smoke run all pass.Follow-up
After release,
buildreintroduces a miseenv_cachewithenv_cache_ttlsafely below the 20m guarantee.