Skip to content

feat: consolidate Keychain auth via shared Core accessor - #40

Merged
victorhqc merged 2 commits into
mainfrom
refactor/authentication-module
Jul 25, 2026
Merged

feat: consolidate Keychain auth via shared Core accessor#40
victorhqc merged 2 commits into
mainfrom
refactor/authentication-module

Conversation

@victorhqc

@victorhqc victorhqc commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

Routes every Keychain access in Filbert through one session-scoped LAContext owned by Core, and exposes a public Core accessor so providers stop reinventing SecItem code. Removes the post-write verification read so saves stop costing a second prompt.

Why

Each Keychain entry point used to allocate its own LAContext and Cursor duplicated the SecItem read query because Core's types were internal. The verification read after every write doubled prompts on save. Spec (core 07) consolidates this to the macOS-allowed minimum.

What changed

  • New shared context (Sources/Core/KeychainAuthenticationContext.swift)KeychainAuthenticationContext.shared is created lazily on first use and reused for the session. KeychainLifecycleObserver invalidates the LAContext on sleep, wake, screen lock, and unlock so macOS can re-prompt within its new window. The context holds no credentials; it only batches authorization.
  • Public Core accessor (Sources/Core/KeychainStorage.swift)KeychainStorage, SecurityKeychainStorage, and KeychainStorageError are now public. Keychain uses this accessor for the consolidated item; providers use it for external items.
  • Removed verification machinery (Sources/Core/Keychain.swift) — Deleted replaceAndVerify, restore, cachedData, and the LoadedStore struct. mutateStore trusts the Security framework status and only updates the cache on success. Keychain.lock remains the sole integrity guarantee against concurrent in-process access.
  • Cursor drops its duplicates (Sources/Providers/Cursor/CursorTokenStore.swift)CursorKeychainAuthenticationContext and defaultReadKeychain are gone. loadExternalPair calls externalStorage.readData(...) with the shared context. Absence versus denial and malformed-record errors still surface as the existing typed CursorExternalCredentialErrors.
  • Tests — Added CursorExternalAccessorTests covering shared-context reuse, absence-versus-denial, and malformed payloads. Added Core tests for public accessor visibility, shared-context reuse, and invalidate() swapping the LAContext. Removed the obsolete verification-failure test and its dead corruptNextReplacement helper. Added a ClosureKeychainStorage test adapter that conforms to the now-public KeychainStorage protocol.
  • Spec (specs/core/07-keychain-auth-consolidation.md) — Appended the macOS Keychain smoke-test checklist for first Cursor import, Cursor refresh, API-key save, and sleep/wake/lock re-authorization. To be run on a real Mac before release.

Notes for review

  • Removing the verification read is a one-way behavior change. The reviewer signed off on the lost safety net in (core 07) Risks; the consolidated item is only written by Filbert and serialized by Keychain.lock. Worth re-confirming at review.
  • Public surface area grew. KeychainStorage, SecurityKeychainStorage, KeychainStorageError, and KeychainAuthenticationContext.shared are now public. Future signature changes become breaking.
  • The macOS smoke test is unchecked. CI cannot exercise real Keychain prompts. The checklist at the end of the spec must be run manually before release; prompt counts cannot be trusted until then.
  • swiftformat, swiftlint (0 serious), swift build, swift build -c release, and swift test (264 tests) all pass.

Remove duplicate Keychain authentication contexts from providers by
promoting Core's KeychainAuthenticationContext and storage types to
public. All SecItem operations now share one session-scoped LAContext,
eliminating redundant password prompts.

Drop post-write verification reads in Keychain (replaceAndVerify,
restore, cachedData). The NSLock remains the sole integrity guarantee.

Wire system sleep/wake and session lock observers to invalidate the
shared context, ensuring macOS re-authorizes when biometric or session
state changes.
@victorhqc
victorhqc merged commit 847346a into main Jul 25, 2026
1 check passed
@victorhqc
victorhqc deleted the refactor/authentication-module branch July 25, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant