Audit COR-389 Auth Paths and Token Resolution · Entire
take a look at this PR, and the intent of COR-389 and thoroughly audit if all the paths do the right thing:
- all API calls (data & core & whatever else) use contexts mechanism and get refreshed JWTs if possible
- we've simplified all we can about this PR.
Bottom line
The PR is sound and does the right thing for the data & core first-class paths — both now resolve through the context machinery and refresh JWTs when a refresh token exists. The discovery refactor is a clean consolidation. But "all API calls refresh if possible" is not literally true: three paths still don't refresh, and one of them (RepoScopedToken) carries a now-stale justification comment. Two of the three are explicitly deferred to COR-395, so they're acceptable for this slice — but you should know they're open.
Path-by-path refresh audit
| Call site | Resolves via | Refreshes? | Verdict |
|---|---|---|---|
Core API coreapi.New (status/dispatch/activity), active context |
ResolveControlPlaneTarget → NewRefreshingLoginProvider |
✅ | correct, keyed on c.CoreURL |
| Core API, no active context | staticControlPlaneTarget → TokenForResource → defaultManager (no RefreshPath) |
❌ | OK — only legacy/no-refresh-token logins land here |
Data API: api_client, recap, search, dispatch/mode_cloud |
ResolveDataAPIToken → discovery → NewRefreshingResourceProvider |
✅ | correct |
| Data API: discovery-unavailable fallback | TokenForResource → defaultManager |
❌ | see Finding 2 |
auth status / logout, active context |
RefreshedLoginToken → NewRefreshingLoginProvider |
✅ | correct, with raw-token fallback |
RepoScopedToken (mirror probe) |
direct LookupCurrentToken + sts.Client |
❌ | see Finding 1 (COR-395) |
resolveAuthHostToken (auth.go:83) |
TokenForResource |
❌ | dead — tests only (COR-395) |
All four data-API call sites and the live control-plane path are correctly on contexts+refresh. Build green, affected package tests green.
Findings (ranked)
1. RepoScopedToken doesn't refresh, and its comment is now wrong. COR-389's acceptance #2 explicitly names it. The comment in repo_token.go:60-74 justifies the skip with "entire login (device flow) stores only a bare access token — no refresh token." That premise is stale: login now requests Scope: "cli offline_access" (auth/client.go:72) and contexts.go:84 persists the refresh token. So an expired-but-refreshable login will fail here. This is real, but COR-395 ("RepoScopedToken bypasses cluster discovery… + remove dead resolveAuthHostToken") tracks it — so deferring is defensible. Fix the comment regardless, or it will mislead the next reader.
2. Data-API discovery-unavailable fallback silently won't refresh AND retains the multi-core bug. When /.well-known/entire-api.json is absent, ResolveDataAPIToken drops to TokenForResource → singleton defaultManager, keyed on api.AuthBaseURL(). If the active context lives on a different core, the exchange targets the wrong core with a token minted elsewhere — the exact bug COR-389 calls out — and won't refresh even though the active context has a refresh token. Only hit on un-rolled-out deployments, justified as "never worse than before," but it's a latent inconsistency. The cleaner design (see below) would eliminate it.
3. Legacy-bridge asymmetry. ResolveDataAPIToken calls MigrateLegacyLoginContext(); ResolveControlPlaneTarget does not. A legacy-only login therefore gets bridged-and-refreshed on a data command but falls to the non-refreshing static path on a control-plane command. Practical impact ≈ nil (legacy logins predate offline_access), but the two entry points should behave the same. One-liner to add the bridge call to ResolveControlPlaneTarget, or a note saying why not.
Simplification assessment
Mostly yes — the refactor is genuinely simpler: fetchWellKnownJSON, selectContext(subject, …), and renderLoginHint(subject, …) are well-extracted; sharing the ClusterCoresCache machinery (separate file to avoid host-name collisions) is the right call. No redundant abstractions stood out. Minor nits:
resolveAuthHostTokenis dead production code (onlyauth_test.gocalls it). You could delete it now instead of waiting for COR-395 — that's strictly less code in this PR.- The two token stacks still coexist.
defaultManager+NewContextStore(read-only, no refresh) survives purely as the fallback for findings 2 & the no-context control-plane case. The end-state COR-389 wants is retiring it. The natural simplification: route both fallbacks throughNewRefreshingResourceProvider/NewRefreshingLoginProvideron the active context (try active context first, singleton only when no context exists). That would refresh, fix the multi-core mismatch in finding 2, and let the singleton shrink. But that's the larger COR-389 consolidation — reasonable to keep out of this data-API slice.
Net: ship-able as the data-API slice. Before merge I'd (a) fix the stale RepoScopedToken comment, and (b) either delete resolveAuthHostToken or drop a // TODO(COR-395). Findings 2 and 3 are fine to leave as tracked follow-ups, but call them out in the PR description so they're not mistaken for "done."