Skip to content

feat(auth): add credential storage policies - #105

Merged
gnapse merged 1 commit into
mainfrom
ernesto/credential-store-policy
Jul 29, 2026
Merged

feat(auth): add credential storage policies#105
gnapse merged 1 commit into
mainfrom
ernesto/credential-store-policy

Conversation

@gnapse

@gnapse gnapse commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add CredentialStore policies to createKeyringTokenStore: system, plaintext, and backward-compatible fallback.
  • Make strict system writes fail without persisting a plaintext credential; explicit plaintext writes bypass the keyring and always return a warning.
  • Allow a policy resolver for CLI options parsed after store construction, and document the API.

Example

let credentialStore: CredentialStore = 'system'

const store = createKeyringTokenStore({
    serviceName: 'example-cli',
    userRecords,
    recordsLocation: configPath,
    credentialStore: () => credentialStore,
})

fallback remains the default to preserve existing consumers. A CLI can expose only system and plaintext, with system as its own default.

Motivated by Doist/todoist-cli#424.

Verification

  • npm run build
  • npm run type-check
  • npm test (35 files, 534 tests)
  • npm run check

@gnapse
gnapse requested a review from scottlovegrove July 29, 2026 16:35

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds system, plaintext, and fallback credential storage policies to createKeyringTokenStore, with a resolver pattern for lazy policy selection, full backward compatibility via the fallback default, and appropriate test/README coverage. No inline issues were flagged.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (3)
  • P3 src/auth/keyring/record-write.ts:200: credentialStoreUnavailableError hardcodes 'The system credential manager could not store the credential.', but the existing SECURE_STORE_DESCRIPTION constant (exported from ./secure-store.js) already encapsulates that label and is used for the same purpose in token-store.ts (e.g. accessReadError, fallbackResult). Import and reuse it for consistency: ts import { type SecureStore, SECURE_STORE_DESCRIPTION, SecureStoreUnavailableError } from './secure-store.js' // ... `${SECURE_STORE_DESCRIPTION} could not store the credential.`
  • P3 src/auth/keyring/record-write.ts:197: credentialStoreUnavailableError() drops the original SecureStoreUnavailableError's message and cause entirely. SecureStoreUnavailableError is specifically designed to preserve platform diagnostics on cause (per its doc comment in secure-store.ts), and the read path surfaces the underlying reason via accessReadError's (${outcome.detail}). In strict system mode — where knowing why the keyring was unavailable matters most — the user only sees a generic message. Consider threading the original error's message into the CliError (e.g. via getErrorMessage(error)) so system-mode write failures are diagnosable.
  • P3 src/auth/keyring/token-store.test.ts:155: This is the only rejection test in the file that pins the exact message string; every other rejects.toMatchObject asserts only the stable code. The wording is an implementation detail, so rewording the user-facing message would break this test without a behavioral regression. Consider dropping the message field to match the rest of the suite (the unit-level test in record-write.test.ts already checks only code).

Share FeedbackReview Logs

@gnapse gnapse self-assigned this Jul 29, 2026
@gnapse
gnapse merged commit 68f0f7e into main Jul 29, 2026
5 checks passed
@gnapse
gnapse deleted the ernesto/credential-store-policy branch July 29, 2026 17:03
doist-release-bot Bot added a commit that referenced this pull request Jul 29, 2026
## [1.1.0](v1.0.1...v1.1.0) (2026-07-29)

### Features

* **auth:** add credential storage policies ([#105](#105)) ([68f0f7e](68f0f7e))
@doist-release-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants