fix(auth): [breaking change] reduce macOS keychain prompts from 3 → 1 and add Touch ID support#250
Merged
Merged
Conversation
Fixes issue #247 — users were seeing 3 separate keychain authorization dialogs during `pup auth login`. This change reduces that to 1. Changes: - Remove the `__pup_test__` probe read in `KeychainStorage::new()`; the probe was accessing a throwaway keychain entry that triggered a superfluous macOS authorization dialog on every fresh install - Consolidate the separate `tokens_<site>` and `client_<site>` keychain entries into a single `state_<site>` entry holding a combined `SiteData { tokens, client }` struct — reduces 2 distinct items to 1, so macOS only asks for authorization once per site - Add `TouchIdStorage` (macOS only) backed by the modern `SecItemAdd`/`SecItemCopyMatching` API with `kSecAccessControlUserPresence`, replacing the legacy `SecKeychain` API used by the `keyring` crate. On code-signed builds (Homebrew releases) this presents a Touch ID prompt instead of a password dialog; on unsigned dev builds it degrades gracefully to a plain keychain item (errSecMissingEntitlement fallback) Note: existing users will need to re-run `pup auth login` once after upgrading since the keychain key names changed. Use `DD_TOKEN_STORAGE=file` or `DD_TOKEN_STORAGE=keychain` to opt out of Touch ID. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
platinummonkey
changed the title
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
1 participant
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.
Fixes #247
Summary
Reduces macOS keychain authorization dialogs during
pup auth loginfrom 3 to 1, and adds Touch ID support on code-signed builds (Homebrew releases).Root Cause
Three separate keychain items were being accessed on every login:
pup/__pup_test__— a throwaway probe inKeychainStorage::new()to test availabilitypup/client_<site>— OAuth2 client credentialspup/tokens_<site>— access/refresh tokensmacOS requires a separate authorization dialog for each distinct keychain item the first time an app accesses it.
Changes
__pup_test__):KeychainStorage::new()no longer reads a test entry; it just constructs the struct. Availability errors surface on first real use.client_<site>andtokens_<site>are merged into a singlestate_<site>entry storingSiteData { tokens, client }as JSON. 2 items → 1, so only one authorization dialog.TouchIdStorage(macOS only,#[cfg(target_os = "macos")]): Uses the modernSecItemAdd/SecItemCopyMatchingAPI withkSecAccessControlUserPresenceinstead of the legacySecKeychainAPI (keyringcrate). On code-signed binaries (Homebrew releases), macOS presents Touch ID instead of a password dialog. On unsigned dev builds, silently falls back to a plain keychain item (errSecMissingEntitlement→ retry without access control).Testing
pup auth loginandpup auth statuswork on an unsigned dev buildNotes for users upgrading
Keychain key names changed (
tokens_*/client_*→state_*), so existing stored sessions won't be read after upgrade. Users will need to runpup auth loginonce. UseDD_TOKEN_STORAGE=fileorDD_TOKEN_STORAGE=keychainto opt out of the new Touch ID backend.🤖 Generated with Claude Code