Warn about the keyring fallback on the first credential read, not only on login

Merged
JE
2mo ago
+109-7

The failure

The credential store printed its warning: system keyring unavailable, credentials stored in plaintext at … only on Save. Every other command merely reads, so a process whose keyring probe failed served whatever an earlier fallback had left in credentials.json — on one machine today, tokens that expired months ago and profiles that no longer existed — with no word about why. Under the concurrent-probe race fixed in basecamp/cli#70, 19 of 20 parallel basecamp auth status -j returned that stale file silently, and basecamp me --profile clawdito said "Not authenticated" 19/20.

The change

Store.Load runs the same once-per-process warnFallback as Save, so the first read after a fallen-back probe says so on stderr. The inner store becomes a small credStore interface so the test can stand in a fallen-back store without failing a real keyring probe; swapNewCredStore's signature follows.

Manager.SetUserEmail — the one credential read a BASECAMP_TOKEN session could reach (basecamp people me stores the fetched email) — now returns without touching the store when the env token is set, matching AccessToken() / AccountID() precedence. The email names the env token's user, not whoever the stored credentials belong to, so writing it there mislabeled them; and the load ran the keyring probe, which with this PR would have warned a CI host with a locked keychain about plaintext credentials it neither stored nor read. TestSetUserEmailSkipsStoreOnEnvToken fails with the guard removed.

TestLoadWarnsOnceWhenKeyringFellBack asserts the warning appears on the first of two reads and that a later write stays quiet. Checked that it fails with only the Load call removed (an earlier draft passed for the wrong reason — the Save in the same capture window warned — which is why the test is split into a read phase and a write phase).

Verified end to end against basecamp/cli#70 with a temporary local replace (not committed): a bounded probe that fails yields

Load error: credentials not found for profile:clawdito: system keyring unavailable (keyring probe timed out after 1ns: context deadline exceeded), fell back to …/credentials.json
stderr:     warning: system keyring unavailable (keyring probe timed out after 1ns: context deadline exceeded), credentials stored in plaintext at …/credentials.json

This PR works against the currently pinned credstore too — the warning just lacks the reason until the bump.

Noise

Hosts with no keyring at all (Linux without a secret service) will now see the warning on every invocation, not only at login. That is the honest state — their probe fails every run — and hosts that mean to use file storage set BASECAMP_NO_KEYRING=1, which skips the probe and never warns. CI and agents on BASECAMP_TOKEN never read or write the store: AccessToken, IsAuthenticated, AccountID, AuthorizationEndpoint, auth status, and doctor short-circuit on the env token, and SetUserEmail now does too, so the warning cannot fire there.

Declined

  • Print from credstore. A library shouldn't write to stderr; credstore's FallbackWarning() carries the reason and its docstring asks callers to surface it on reads.
  • Warn on Delete / MigrateToKeyring too. Both are preceded by a read or a login in every path that reaches them.
  • Give SetUserIdentity the same BASECAMP_TOKEN guard. Its callers are the login flows (auth login, profile add, the setup wizard), which have just saved fresh credentials and fetch the profile to label them; skipping the write there would leave a login done under an exported token unlabeled. Whether that fetch should use the fresh token rather than the env one is a separate, pre-existing question.
  • Guard GetUserEmail / GetOAuthType too. Neither is reached from a token session — AuthorizationEndpoint consults GetOAuthType only after its env-token branch — and both are display-only reads whose stored answer some callers may want alongside an env token.
  • Gate the call in people.go instead of the setter. The precedence belongs where AccessToken and AccountID already keep it, so every caller gets it.

Follow-up

Bump credstore once basecamp/cli#70 merges: go get github.com/basecamp/cli@main && go mod tidy.