Repository navigation
feat(cli): add the file credential store and the credential resolver (P2) - #245
Merged
Merged
Conversation
ReadAsync returns the warning with the result, CredentialStores owns the read order, an unsafe file's entries are never carried into a safe one, and the resolver takes CliConfiguration and leaves when-to-fall-back to the authenticated pipeline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(P2) CredentialRecord, ICredentialStore and FileCredentialStore keep sign-ins in ~/.xping/credentials.json (0600 in a 0700 directory, owner-only ACL on Windows, atomic replace). Files others can read are refused with chmod instructions; corrupt entries read as nothing with a warning and are kept. CredentialStores holds the keychain-then-file read order, and CredentialResolver picks --api-key, the stored login, then the ambient key, without a network call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failing credential store no longer stops the lookup or the sign-out, an unreadable store is reported separately from a corrupt entry, and a delete leaves a file it cannot parse untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iew (P2) - A ~/.xping the user cannot search is a CredentialStoreException, not a raw UnauthorizedAccessException from the mode check. - CredentialStores keeps reading after a store fails, so a locked keychain cannot hide a file login, and DeleteAllAsync tries every store before reporting the failures. - The resolver reports unreadable stores as StoreFailures, apart from corrupt-entry warnings, so auth status can exit 17. - Signing out leaves an unparseable file untouched instead of deleting every Cloud URL's sign-in. - Reads share the file for delete, so on Windows a reader no longer makes the replacing move fail. - XpingHome.Display checks for an empty profile before GetRelativePath. - CredentialStoresTests split out; Display tested without the real HOME; no dictionary copy, one mode check per delete, no Lazy in the selector. Co-Authored-By: Claude Opus 5.5 <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.
Phase 2 of
docs/internals/implementation-specs/cli-auth-cli-spec.md(§19). Merges into thefeat/cli-authintegration branch, notmain.Implements contract §10.4 (credential storage) and spec §7.2–§7.7 and §8.1 for the file backend.
What changes
CredentialRecord: the §7.2 shape.IsValidFor(cloudUrl)applies the §7.7 corruption rules.ToStringleaves both tokens out.ICredentialStore,CredentialReadResult,CredentialStoreException.FileCredentialStore:~/.xping/credentials.json, one entry per Cloud URL.~/.xping(an existing open one is tightened), owner-only ACL on Windows. Writes are atomic throughPrivateFiles.chmodmessage.Redaction.CredentialStores+CredentialStoreSelector: read order keychain → file, write to the selected store, delete from all. Only the file backend exists until phase 5.CredentialResolver:--api-key, then the stored login, thenXPING_APIKEY/Xping:ApiKey, then none (A-8). It makes no network call. It reportsShadowedLoginandFallbackApiKey.FallbackToApiKey()gives the row-3 credential after an invalid login. A store that cannot be read counts as no login, with a warning.AddXpingCliAuth. No command uses them yet (phase 3), so behaviour is unchanged.Spec amendments (separate commit)
ReadAsyncreturnsCredentialReadResult(record + warning). Corrupt and refused entries have to reachauth statusandreportas text.CredentialStoresowns the §7.4 read/write/delete rules.cloudUrldoesn't match its key ordataGatewayUriis missing. The same goes for a whole file that won't parse.ResolveAsync(CliConfiguration). The resolver decides where to fall back and the pipeline decides when, so "no fallback on network error" is tested in phase 6.Tests
CredentialRecordTests,FileCredentialStoreTests(including the chmod 644 / group-write refusal),CredentialStoreSelectorTests,CredentialResolverTests. 70 new tests. All 1830 CLI tests pass on macOS. The credential tests also pass on Linux (mcr.microsoft.com/dotnet/sdk:10.0).Review fixes
~/.xping(for example it was left root-owned), the store now throwsCredentialStoreException. Before, the mode check let a rawUnauthorizedAccessExceptionescape.CredentialStoreskeeps reading after a store fails, so a locked keychain no longer hides a file login.DeleteAllAsynctries every store before it reports the failures.StoreFailures, kept apart from corrupt-entry warnings. In phase 3,auth statususes this to exit 17 instead of 10. The spec was amended first (§3.4, §7.3, §7.4, §8.1).FileShare.Delete. On Windows, a reader without it made the replacing move fail, which could lose a rotated refresh token.XpingHome.Displaynow checks for an empty home path beforeGetRelativePath, which threw on it.CredentialStoresTestsis its own class.Displayis tested without the realHOME. Removed the dictionary copy, the second mode check per delete and theLazyin the selector.The regression tests for the unsearchable
~/.xping, the unparseable-file sign-out and the empty home path were checked to fail with their fixes reverted. TheCredentialStoresandStoreFailurestests use API added by the fix, so they could not be run against the old code. The Windows sharing test can only fail on Windows, and PR CI runs on Ubuntu.🤖 Generated with Claude Code