From 2dc123c1b23be4f3247b2226278699225cec85be Mon Sep 17 00:00:00 2001 From: t Date: Fri, 31 Jul 2026 17:37:15 -0400 Subject: [PATCH] refactor(accounts): extract AccountCredentialResolver credential-resolution collaborator (Wave 3 / 3a-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth 3a collaborator split. Per-account credential resolution — the per-library TPPUserAccount cache (immutable keychain keys), the currentUserAccount ride-out over the account-switch nil window, and the fresh-install placeholder — moves out of AccountsManager into an injected AccountCredentialResolver. Structurally small but the highest user-facing risk: this is the credential-isolation boundary (F-034 cross-account leak) + the sign-in-modal ride-out (F-016). Behaviour-identical. Off develop (all four prior extractions merged). **AccountsManager 955 → 915 LOC.** Across the five extractions the hub is 2383 → 915. - New AccountCredentialResolver.swift: a `final class @unchecked Sendable` (not actor — currentUserAccount is a synchronous @objc TPPUserAccountResolving requirement reached from the non-async currentAccount setter). Owns the userAccounts cache + userAccountsLock + lastKnownCurrentUserAccount + noAccountPlaceholder. ONE injected dep: `currentAccountIdProvider: () -> String?` — a LIVE closure (not a snapshot) so the ride-out observes the transient nil window in real time; TPPUserAccount(libraryUUID:) is self-contained so no defaults/keychain is injected. lazy var (closure captures self). - Invariants preserved VERBATIM: F-034 — userAccount(for:) does check-build-insert in ONE userAccountsLock span (splitting it reopens the 6-year TOCTOU, PP-4020); F-016 — currentUserAccount writes lastKnown under lock on every id-present resolution, and in the nil window returns lastKnown ?? placeholder (placeholder evaluated after unlock). - NO TPPUserAccount.sharedAccount() fallback (the 3a-4 BLOCK + PR #822 lesson) — the only fallbacks are lastKnown then the placeholder. - Hub keeps the @objc TPPUserAccountResolving conformance + thin userAccount(for:)/ currentUserAccount facades → the ~136 external call sites + the Obj-C bridge are byte-identical (zero external edits). @unchecked Sendable audit refreshed. - New AccountCredentialResolverTests: cache stability, id-present resolution, the F-016 ride-out pin (front-and-center), fresh-install placeholder (no credentials), and a concurrentPerform single-instance check for the lock span. Verified: god-class freeze re-baselined DOWN 955 → 915; locator 302 / shared-read 212; Palace-noDRM BUILD SUCCEEDED (production compile). Architect pre-review APPROVED. **Scope:** per-account credential resolution out of AccountsManager into an injected collaborator; Accounts area only, single-module. **Not done:** the F-034 500-iteration chaos gate (TPPCredentialIsolationE2ETests) + TPPPerAccountIsolationTests + the new seam tests run in CI (worktree can't build the Adobe host); they pass through the retained facades unchanged. 3 MyBooks docstrings naming AccountsManager.noAccountSentinelUUID go stale (compile-safe; the literal is value-duplicated). **Deferred:** the last collaborator, CurrentAccountStore (the currentAccount switch pipeline). Co-Authored-By: Claude Opus 4.8 (1M context) --- Palace.xcodeproj/project.pbxproj | 10 ++ .../Library/AccountCredentialResolver.swift | 121 ++++++++++++++++++ Palace/Accounts/Library/AccountsManager.swift | 84 ++++-------- .../AccountCredentialResolverTests.swift | 120 +++++++++++++++++ scripts/godclass-loc-baseline.txt | 8 +- 5 files changed, 280 insertions(+), 63 deletions(-) create mode 100644 Palace/Accounts/Library/AccountCredentialResolver.swift create mode 100644 PalaceTests/Accounts/AccountCredentialResolverTests.swift diff --git a/Palace.xcodeproj/project.pbxproj b/Palace.xcodeproj/project.pbxproj index 0180456b1..656f4b164 100644 --- a/Palace.xcodeproj/project.pbxproj +++ b/Palace.xcodeproj/project.pbxproj @@ -97,6 +97,7 @@ 114C14F01FFD0AD65BD9E2A8 /* TPPSettingsAuthBridge.swift in Sources */ = {isa = PBXBuildFile; fileRef = CB91C36D9DC90CDAC98FA568 /* TPPSettingsAuthBridge.swift */; }; 119503E71993F914009FB788 /* libxml2.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 119503E61993F914009FB788 /* libxml2.dylib */; }; 119503E91993F919009FB788 /* libz.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 119503E81993F919009FB788 /* libz.dylib */; }; + 121523B3892A09CCEDACF8CC /* AccountCredentialResolver.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8DB71A079F6AD144689D1E63 /* AccountCredentialResolver.swift */; }; 121E8B66B8634CD720E7C524 /* CatalogRepositoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBA39820CBA75D6BA9B763E2 /* CatalogRepositoryTests.swift */; }; 1246CC54E04F756C2913D438 /* LCPPDFDiskExtractTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 01F470EA0E88BDCE56E27B86 /* LCPPDFDiskExtractTests.swift */; }; 12695B15A63B66C4B42E9B13 /* TPPReaderFootnoteAccessibilityTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = FD60FEA388441F72F8C8D85C /* TPPReaderFootnoteAccessibilityTests.swift */; }; @@ -125,6 +126,7 @@ 161DDED234AF46D984276897 /* EmailAddressTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2B222C03630C4B2984C38A28 /* EmailAddressTests.swift */; }; 165C30CC09A69EA31390E6B5 /* RatingReviewRequester.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4F892EAF356E802A7E33A657 /* RatingReviewRequester.swift */; }; 1698C14BA6720F592D8A0834 /* TestAppContainerFactory.swift in Sources */ = {isa = PBXBuildFile; fileRef = C2F955BD354E7BE3DEFB5B74 /* TestAppContainerFactory.swift */; }; + 16AEE219FDEB9C1FB699C6F9 /* AccountCredentialResolver.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8DB71A079F6AD144689D1E63 /* AccountCredentialResolver.swift */; }; 16D1246F3608F88A422AE6BA /* SideloadedBookRegistry.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8F96D2A6AAB3B6C802BDBFCF /* SideloadedBookRegistry.swift */; }; 16E9A34D3D9D5833B73139CF /* iPadOnMacRMSDKGuardTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C20222DD2B8B50BD8CDF984D /* iPadOnMacRMSDKGuardTests.swift */; }; 17071065242A923400E2648F /* TPPSecrets.swift in Sources */ = {isa = PBXBuildFile; fileRef = 17071060242A923400E2648F /* TPPSecrets.swift */; }; @@ -1921,6 +1923,7 @@ F9118F50C0F64CE998AC7527 /* OPDS2ParsingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45081C08344C4032BAB1EE86 /* OPDS2ParsingTests.swift */; }; F91715377B924C738EEAC8F3 /* FirebaseManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = B79DD7FF880843F7BE282614 /* FirebaseManager.swift */; }; F9497743E78564D80AEC155F /* TPPReaderPageListBusinessLogic.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0D848CD4CD0E01B3A2BF67FE /* TPPReaderPageListBusinessLogic.swift */; }; + F957246841935C2C67540008 /* AccountCredentialResolverTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = BF3A15BCE8862A26CC2EF7A6 /* AccountCredentialResolverTests.swift */; }; FA346754BE76A48C92CF0754 /* OPDS2FeedParsingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CD86C00D774B74F80D5B713 /* OPDS2FeedParsingTests.swift */; }; FA60C0B01CF7E5F52C5C3A0C /* TPPBookRegistryAccountCaptureContractTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = BC0A032C501D578FE2D25C40 /* TPPBookRegistryAccountCaptureContractTests.swift */; }; FA8184531683826DDA00EB04 /* BorrowOperationContractTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 958964493F9D85E5FCFAAAE2 /* BorrowOperationContractTests.swift */; }; @@ -2840,6 +2843,7 @@ 8CE9C470237F84820072E964 /* TPPBookDetailsProblemDocumentViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TPPBookDetailsProblemDocumentViewController.swift; sourceTree = ""; }; 8D50821D42294719B5E6F62C /* UIApplication+MainScene.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIApplication+MainScene.swift"; sourceTree = ""; }; 8D899B519469E743D48FFACC /* AudiobookPlaytimesLifecycleTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = AudiobookPlaytimesLifecycleTests.swift; sourceTree = ""; }; + 8DB71A079F6AD144689D1E63 /* AccountCredentialResolver.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = AccountCredentialResolver.swift; sourceTree = ""; }; 8DD49518A593F93C52009D35 /* KeyboardNavigationHandlerTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; name = KeyboardNavigationHandlerTests.swift; path = PalaceTests/Reader/KeyboardNavigationHandlerTests.swift; sourceTree = ""; }; 8E0832A22CC1EC07844CD4AA /* SpyAuthDecisionRecorder.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SpyAuthDecisionRecorder.swift; sourceTree = ""; }; 8E22186CA3D046EA9541D1FE /* TPPBookRegistryRecordTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TPPBookRegistryRecordTests.swift; sourceTree = ""; }; @@ -3051,6 +3055,7 @@ BED352FF5F5E4D307FBF8B42 /* BorrowReducerCoreContractTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = BorrowReducerCoreContractTests.swift; sourceTree = ""; }; BEF1F7C70AC93BF8E2F22148 /* DebugSettings.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; name = DebugSettings.swift; path = Debug/DebugSettings.swift; sourceTree = ""; }; BF102132435465A1B2C3D4E5 /* BookContentResetService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookContentResetService.swift; sourceTree = ""; }; + BF3A15BCE8862A26CC2EF7A6 /* AccountCredentialResolverTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = AccountCredentialResolverTests.swift; sourceTree = ""; }; BF99D1F39D634C56A70BFE75 /* AudiobookReliabilityTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AudiobookReliabilityTests.swift; sourceTree = ""; }; BFC0D1E2F30415263748495B /* BookReturnServiceTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = BookReturnServiceTests.swift; sourceTree = ""; }; BFC0D1E2F30415263748496C /* CredentialPromptCoordinatorTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = CredentialPromptCoordinatorTests.swift; sourceTree = ""; }; @@ -4354,6 +4359,7 @@ 9D32B0D46348D5770A7551C4 /* BorrowReauthResettingTests.swift */, 822C34E7CB9D9E06317B7722 /* CatalogCrawlSchedulerTests.swift */, A64CF3480F3F5C70A6227526 /* AccountsManagerCatalogLoadJoinTests.swift */, + BF3A15BCE8862A26CC2EF7A6 /* AccountCredentialResolverTests.swift */, ); path = Accounts; sourceTree = ""; @@ -4994,6 +5000,7 @@ C49DDACF93DCF459DC310653 /* AccountRegistryStore.swift */, 39D66F77DC2F6D47EEC5AC3A /* AuthDocumentLoader.swift */, 221D9267074DA0CF6A7A3834 /* AccountRegistryLoader.swift */, + 8DB71A079F6AD144689D1E63 /* AccountCredentialResolver.swift */, ); path = Library; sourceTree = ""; @@ -8092,6 +8099,7 @@ 092C37A2AA45C900DCCD7A8E /* AccountRegistryStoreSeamTests.swift in Sources */, 36BD44B00F9DEB4FB6473B9E /* AuthDocumentLoaderSeamTests.swift in Sources */, D669BCA946AA705EA8922727 /* AccountRegistryLoaderSeamTests.swift in Sources */, + F957246841935C2C67540008 /* AccountCredentialResolverTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -8664,6 +8672,7 @@ 0AE214B06AC5C39759675519 /* AccountRegistryStore.swift in Sources */, C3FB1B86998C6337127728A9 /* AuthDocumentLoader.swift in Sources */, 9A060754EB6CCAF39D8F7860 /* AccountRegistryLoader.swift in Sources */, + 121523B3892A09CCEDACF8CC /* AccountCredentialResolver.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -9239,6 +9248,7 @@ 4731E519F95F0E4A6F77BFBF /* AccountRegistryStore.swift in Sources */, 55259176804CBB1F48F10FB3 /* AuthDocumentLoader.swift in Sources */, B86ABC14203789D64ABC9A16 /* AccountRegistryLoader.swift in Sources */, + 16AEE219FDEB9C1FB699C6F9 /* AccountCredentialResolver.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Palace/Accounts/Library/AccountCredentialResolver.swift b/Palace/Accounts/Library/AccountCredentialResolver.swift new file mode 100644 index 000000000..fcfad3dad --- /dev/null +++ b/Palace/Accounts/Library/AccountCredentialResolver.swift @@ -0,0 +1,121 @@ +// +// AccountCredentialResolver.swift +// Palace +// +// god-class decomposition — Wave 3 / 3a-5 (the fifth in-target collaborator split +// out of `AccountsManager`). +// +// Per-account credential resolution: the cache of library-scoped `TPPUserAccount` +// instances (each with immutable keychain keys), the `currentUserAccount` resolution +// with its "ride-out" over the transient `currentAccountId == nil` account-switch +// window, and the fresh-install placeholder. This is the credential-isolation +// boundary — a defect here is a silent cross-account credential leak (F-034) or a +// spurious sign-in modal (F-016) — so the bodies below are relocated VERBATIM from +// the hub and the two invariants are preserved exactly: +// +// * F-034 (TOCTOU): `userAccount(for:)` does check-build-insert in ONE +// `userAccountsLock` span, returning the same cached instance per UUID whose +// keychain keys are immutable for its lifetime. Splitting the read/insert would +// let two instances exist for one UUID and reopen the 6-year race (PP-4020). +// * F-016 (ride-out): `currentUserAccount` writes `lastKnownCurrentUserAccount` +// under the lock on EVERY id-present resolution, and during the nil window +// returns that last-resolved instance (placeholder only on true fresh install) +// so consumers never observe `hasCredentials == false` on a signed-in account. +// +// A `final class` (not an actor): `currentUserAccount` is reached synchronously from +// the `@objc TPPUserAccountResolving` facade on `AccountsManager` (which stays the +// protocol witness) and from the non-async `currentAccount` setter — an actor would +// force `await` through the `@objc` conformance. The resolver is a plain internal +// collaborator; it is NOT `@objc` and does NOT conform to the protocol. +// +// `@unchecked Sendable` invariant: the only mutable state is `userAccounts` and +// `lastKnownCurrentUserAccount`, read/written exclusively under `userAccountsLock` +// (an immutable `NSLock`); `noAccountPlaceholder` is a `lazy var` resolved at most +// once on the fresh-install path and immutable thereafter (write-once); +// `currentAccountIdProvider` is an immutable `let` reading the internally +// thread-safe `UserDefaults` live on every call. +// +// There is NO shared-singleton credential fallback anywhere here — the only fallbacks +// are `lastKnownCurrentUserAccount` then `noAccountPlaceholder`. A shared-singleton +// safety-net is the PR #822 defect that caused spurious sign-in modals. +// +// Copyright © 2026 The Palace Project. All rights reserved. +// + +import Foundation + +/// Resolves per-library `TPPUserAccount` instances with immutable-key isolation and +/// the account-switch ride-out. Injected into `AccountsManager`, which keeps the +/// `@objc TPPUserAccountResolving` facades; tests construct it directly with a spy +/// `currentAccountIdProvider`. +final class AccountCredentialResolver: @unchecked Sendable { + + /// Live read of the current library's UUID. MUST re-read on every call (not a + /// captured snapshot) — the ride-out below depends on observing the transient nil + /// window in real time. + private let currentAccountIdProvider: () -> String? + + /// Cache of per-library `TPPUserAccount` instances. Each instance has immutable + /// keychain keys, eliminating the TOCTOU race that the singleton's mutable + /// `libraryUUID` pattern was subject to. + private var userAccounts = [String: TPPUserAccount]() + private let userAccountsLock = NSLock() + + /// Last account returned from `currentUserAccount`. Used to ride out the brief + /// windows where `currentAccountId` is nil during an account switch — without this, + /// consumers observe a transiently-unauthenticated state on an account that IS + /// signed in, and fire spurious sign-in modals. + private var lastKnownCurrentUserAccount: TPPUserAccount? + + /// Sentinel UUID for the "no account selected" placeholder. Not a real library + /// UUID — keychain reads for this instance return nil, so hasCredentials() + /// deterministically returns false. + private static let noAccountSentinelUUID = "__no_account_selected__" + + /// Placeholder returned by `currentUserAccount` only on a truly fresh install + /// before any account has ever been selected. Lazily created so app launch doesn't + /// pay for a keychain-probed instance. + private lazy var noAccountPlaceholder: TPPUserAccount = TPPUserAccount( + libraryUUID: Self.noAccountSentinelUUID + ) + + init(currentAccountIdProvider: @escaping () -> String?) { + self.currentAccountIdProvider = currentAccountIdProvider + } + + /// Returns a library-scoped `TPPUserAccount` instance. Creates and caches a new one + /// on first access for a given UUID. + func userAccount(for libraryUUID: String) -> TPPUserAccount { + userAccountsLock.lock() + defer { userAccountsLock.unlock() } + if let existing = userAccounts[libraryUUID] { + return existing + } + let account = TPPUserAccount(libraryUUID: libraryUUID) + userAccounts[libraryUUID] = account + return account + } + + /// Convenience for the current library's user account. + /// + /// Thread-safety note: `currentAccountId` can transiently be nil during an account + /// switch (the old id is cleared before the new id is assigned). If we blindly fell + /// back to a fresh/empty instance in that window, consumers like + /// MyBooksDownloadCenter would observe `hasCredentials == false` on an account that + /// IS signed in and fire a spurious login modal. We cache the last-resolved account + /// and return it during the nil window instead. The placeholder path only fires on a + /// true fresh-install state where no account has ever been selected. + var currentUserAccount: TPPUserAccount { + if let id = currentAccountIdProvider() { + let account = userAccount(for: id) + userAccountsLock.lock() + lastKnownCurrentUserAccount = account + userAccountsLock.unlock() + return account + } + userAccountsLock.lock() + let last = lastKnownCurrentUserAccount + userAccountsLock.unlock() + return last ?? noAccountPlaceholder + } +} diff --git a/Palace/Accounts/Library/AccountsManager.swift b/Palace/Accounts/Library/AccountsManager.swift index 2c81ba922..a6ca69563 100644 --- a/Palace/Accounts/Library/AccountsManager.swift +++ b/Palace/Accounts/Library/AccountsManager.swift @@ -73,8 +73,10 @@ private final class AccountsManagerBoolFlag: @unchecked Sendable { /// - the auth-document fetch state (single-flight map + lock) → moved to the /// injected `AuthDocumentLoader` (Wave 3 / 3a-3), which owns its own `NSLock`; the /// hub holds the loader as a `lazy var` and no longer names that state. -/// - `userAccounts`, `lastKnownCurrentUserAccount` → read/written only under -/// `userAccountsLock`. +/// - the per-account credential state (`userAccounts` cache + its lock, +/// `lastKnownCurrentUserAccount`, `noAccountPlaceholder`) → moved to the injected +/// `AccountCredentialResolver` (Wave 3 / 3a-5), which owns their `NSLock`/lazy +/// synchronization; the hub holds it as a `lazy var`. /// - `crawlScheduler` → immutable `Sendable` `let` bound once from `init`, passed /// into the load collaborator. /// - `isAccountSwitching` → storage moved into the lock-backed @@ -82,19 +84,16 @@ private final class AccountsManagerBoolFlag: @unchecked Sendable { /// set/get is serialized by the holder's own `NSLock`; the public /// `private(set) var` computed accessor preserves every call site and the /// value/timing verbatim (see property below). -/// - `networkExecutor`, `noAccountPlaceholder` → `lazy var`, each resolved -/// exactly once from an already-constructed dependency and immutable -/// thereafter; the lazy init runs on the first touch, which for -/// `networkExecutor` is inside the background load path after `AppContainer` -/// finishes constructing, and for `noAccountPlaceholder` only on the fresh- -/// install `currentUserAccount` path. Effectively write-once. +/// - `networkExecutor` → `lazy var`, resolved exactly once from an +/// already-constructed dependency (inside the background load path after +/// `AppContainer` finishes constructing) and immutable thereafter — write-once. /// - `_explicitCancelCalled` → `#if DEBUG` test-only; compiled out of release. Set by /// the hub cancel/drain facades BEFORE delegating to the load collaborator (so the /// 3a-3 `AuthDocumentLoader.isTornDown` binding stays byte-identical). /// /// Immutable (`let`) state — inherently safe: `tppAccountUUID`, `ageCheck`, /// `settings`, `defaults`, `registryStore` (internally synchronized), -/// `registryCache`, `crawlScheduler`, `userAccountsLock`. +/// `registryCache`, `crawlScheduler`. /// /// `currentAccountId` is a computed property backed by the injected /// `UserDefaults` (`defaults`), which is itself internally thread-safe — no @@ -647,65 +646,26 @@ private final class AccountsManagerBoolFlag: @unchecked Sendable { // MARK: - Per-Account User Credentials - /// Cache of per-library `TPPUserAccount` instances. Each instance has - /// immutable keychain keys, eliminating the TOCTOU race that the - /// singleton's mutable `libraryUUID` pattern was subject to. - private var userAccounts = [String: TPPUserAccount]() - private let userAccountsLock = NSLock() - - /// Last account returned from `currentUserAccount`. Used to ride out the - /// brief windows where `currentAccountId` is nil during an account switch - /// — without this, consumers observe a transiently-unauthenticated state - /// on an account that IS signed in, and fire spurious sign-in modals. - private var lastKnownCurrentUserAccount: TPPUserAccount? - - /// Sentinel UUID for the "no account selected" placeholder. Not a real - /// library UUID — keychain reads for this instance return nil, so - /// hasCredentials() deterministically returns false. - private static let noAccountSentinelUUID = "__no_account_selected__" - - /// Placeholder returned by `currentUserAccount` only on a truly fresh - /// install before any account has ever been selected. Lazily created so - /// app launch doesn't pay for a keychain-probed instance. - private lazy var noAccountPlaceholder: TPPUserAccount = TPPUserAccount( - libraryUUID: AccountsManager.noAccountSentinelUUID + /// Per-account credential resolution (Wave 3 / 3a-5): the per-library + /// `TPPUserAccount` cache (immutable keys), the `currentUserAccount` ride-out over + /// the account-switch nil window, and the fresh-install placeholder moved to the + /// injected `AccountCredentialResolver`, which owns the F-034/F-016 invariants. The + /// hub keeps the `@objc TPPUserAccountResolving` witnesses below as thin facades. + /// `lazy var` because the provider closure captures `self` (the authDocLoader + /// precedent); `currentAccountIdProvider` is a LIVE read (not a snapshot) so the + /// ride-out observes the transient nil window in real time. + private lazy var credentialResolver = AccountCredentialResolver( + currentAccountIdProvider: { [weak self] in self?.currentAccountId } ) - /// Returns a library-scoped `TPPUserAccount` instance. Creates and - /// caches a new one on first access for a given UUID. + /// Returns a library-scoped `TPPUserAccount` instance (facade → `credentialResolver`). func userAccount(for libraryUUID: String) -> TPPUserAccount { - userAccountsLock.lock() - defer { userAccountsLock.unlock() } - if let existing = userAccounts[libraryUUID] { - return existing - } - let account = TPPUserAccount(libraryUUID: libraryUUID) - userAccounts[libraryUUID] = account - return account + credentialResolver.userAccount(for: libraryUUID) } - /// Convenience for the current library's user account. - /// - /// Thread-safety note: `currentAccountId` can transiently be nil during an - /// account switch (the old id is cleared before the new id is assigned). - /// If we blindly fell back to a fresh/empty instance in that window, - /// consumers like MyBooksDownloadCenter would observe `hasCredentials == - /// false` on an account that IS signed in and fire a spurious login modal. - /// We cache the last-resolved account and return it during the nil window - /// instead. The placeholder path only fires on a true fresh-install state - /// where no account has ever been selected. + /// Convenience for the current library's user account (facade → `credentialResolver`). var currentUserAccount: TPPUserAccount { - if let id = currentAccountId { - let account = userAccount(for: id) - userAccountsLock.lock() - lastKnownCurrentUserAccount = account - userAccountsLock.unlock() - return account - } - userAccountsLock.lock() - let last = lastKnownCurrentUserAccount - userAccountsLock.unlock() - return last ?? noAccountPlaceholder + credentialResolver.currentUserAccount } // MARK: – Load logic (facade → AccountRegistryLoader) diff --git a/PalaceTests/Accounts/AccountCredentialResolverTests.swift b/PalaceTests/Accounts/AccountCredentialResolverTests.swift new file mode 100644 index 000000000..bad2906ef --- /dev/null +++ b/PalaceTests/Accounts/AccountCredentialResolverTests.swift @@ -0,0 +1,120 @@ +// +// AccountCredentialResolverTests.swift +// PalaceTests +// +// Pins the Wave 3 / 3a-5 `AccountCredentialResolver` seam: per-account credential +// resolution extracted from AccountsManager behind the injected resolver. The +// end-to-end isolation contract is the retained `TPPCredentialIsolationE2ETests` +// (F-034 500-iteration chaos gate) + `TPPPerAccountIsolationTests`, which route +// through the hub facade unchanged. This file pins the resolver directly with a spy +// `currentAccountIdProvider` — most importantly the F-016 ride-out. +// +// Copyright © 2026 The Palace Project. All rights reserved. +// + +import XCTest +@testable import Palace + +final class AccountCredentialResolverTests: XCTestCase { + + // MARK: - Per-account cache isolation (F-034 surface) + + /// Same UUID → identical cached instance; distinct UUIDs → distinct instances with + /// distinct immutable `boundLibraryUUID` keys. Seam-level mirror of the per-account + /// isolation guarantee. + func testUserAccount_cacheStability_perUUID() { + let resolver = AccountCredentialResolver(currentAccountIdProvider: { nil }) + let a1 = resolver.userAccount(for: "urn:uuid:lib-A") + let a2 = resolver.userAccount(for: "urn:uuid:lib-A") + let b = resolver.userAccount(for: "urn:uuid:lib-B") + + XCTAssertTrue(a1 === a2, "same UUID must return the identical cached instance") + XCTAssertFalse(a1 === b, "distinct UUIDs must return distinct instances") + XCTAssertEqual(a1.boundLibraryUUID, "urn:uuid:lib-A") + XCTAssertEqual(b.boundLibraryUUID, "urn:uuid:lib-B") + XCTAssertNotEqual(a1.boundLibraryUUID, b.boundLibraryUUID, "keys must not collide across libraries") + } + + // MARK: - currentUserAccount ride-out (F-016) + + /// id-present resolution returns `userAccount(for: id)` and records it as last-known. + func testCurrentUserAccount_idPresent_resolvesCurrentLibrary() { + let idBox = IdBox("urn:uuid:lib-A") + let resolver = AccountCredentialResolver(currentAccountIdProvider: { idBox.value }) + + let resolved = resolver.currentUserAccount + XCTAssertEqual(resolved.boundLibraryUUID, "urn:uuid:lib-A") + XCTAssertTrue(resolved === resolver.userAccount(for: "urn:uuid:lib-A"), + "currentUserAccount must return the cached instance for the current id") + } + + /// THE isolation-critical pin (F-016): during the account-switch window where + /// `currentAccountId` is transiently nil, `currentUserAccount` returns the + /// LAST-RESOLVED signed-in instance — NOT the fresh-install placeholder — so + /// consumers don't observe `hasCredentials == false` on a signed-in account and + /// fire a spurious login modal. + /// + /// Kill case: dropping the `lastKnownCurrentUserAccount` ride-out (returning the + /// placeholder in the nil window) → this fails (returns the sentinel instance). + func testCurrentUserAccount_ridesOutNilWindow_returnsLastKnownNotPlaceholder() { + let idBox = IdBox("urn:uuid:lib-A") + let resolver = AccountCredentialResolver(currentAccountIdProvider: { idBox.value }) + + let signedIn = resolver.currentUserAccount // id-present → records last-known + idBox.value = nil // enter the transient switch window + + let duringWindow = resolver.currentUserAccount + XCTAssertTrue(duringWindow === signedIn, + "the ride-out must return the last-resolved instance during the nil window") + XCTAssertNotEqual(duringWindow.boundLibraryUUID, "__no_account_selected__", + "the ride-out must NOT return the fresh-install placeholder") + } + + /// True fresh install (nil from the start, no prior resolution) → the placeholder, + /// which is not a real library so `hasCredentials()` is deterministically false. + func testCurrentUserAccount_freshInstall_returnsPlaceholderWithNoCredentials() { + let resolver = AccountCredentialResolver(currentAccountIdProvider: { nil }) + + let placeholder = resolver.currentUserAccount + XCTAssertEqual(placeholder.boundLibraryUUID, "__no_account_selected__", + "fresh install with no last-known returns the sentinel placeholder") + XCTAssertFalse(placeholder.hasCredentials(), + "the placeholder must have no credentials") + } + + // MARK: - Concurrency (the single-lock-span cache) + + /// Concurrent `userAccount(for:)` on one UUID yields exactly ONE cached instance, + /// no crash — exercises the single `userAccountsLock` critical section that closes + /// the F-034 TOCTOU. + func testUserAccount_concurrentSameUUID_singleInstance() { + let resolver = AccountCredentialResolver(currentAccountIdProvider: { nil }) + let sink = InstanceSink() + + DispatchQueue.concurrentPerform(iterations: 300) { _ in + sink.record(resolver.userAccount(for: "urn:uuid:lib-A")) + } + XCTAssertEqual(sink.distinctCount, 1, + "check-build-insert under one lock span must yield a single cached instance per UUID") + } +} + +// MARK: - Test doubles + +/// Mutable current-id source for the spy provider. `@unchecked Sendable`: mutated only +/// on the test thread before/after (not during) each `currentUserAccount` read. +fileprivate final class IdBox: @unchecked Sendable { + var value: String? + init(_ value: String?) { self.value = value } +} + +/// Records distinct object identities seen across concurrent `userAccount(for:)` calls. +fileprivate final class InstanceSink: @unchecked Sendable { + private let lock = NSLock() + private var seen = Set() + func record(_ account: TPPUserAccount) { + lock.lock(); defer { lock.unlock() } + seen.insert(ObjectIdentifier(account)) + } + var distinctCount: Int { lock.lock(); defer { lock.unlock() }; return seen.count } +} diff --git a/scripts/godclass-loc-baseline.txt b/scripts/godclass-loc-baseline.txt index f99fe24e3..73df4c34d 100644 --- a/scripts/godclass-loc-baseline.txt +++ b/scripts/godclass-loc-baseline.txt @@ -60,6 +60,12 @@ # seams were inverted behind the injected `AccountSwitchDependencies` bundle; the # net is slightly NEGATIVE (the extracted nav-pop closure + deleted inline reaches # outweigh the stored dependency + its docs). Re-baselined by the exact delta. +# Wave 3 / 3a-5 (-40 on AccountsManager, 955 -> 915): per-account credential resolution +# (the per-library TPPUserAccount cache + lock, the currentUserAccount ride-out over the +# account-switch nil window, and the fresh-install placeholder) EXTRACTED to the injected +# AccountCredentialResolver (a final class, not an actor). The hub keeps the @objc +# TPPUserAccountResolving facades. Preserves F-034 (single-lock-span cache) + F-016 (ride-out) +# verbatim. Fifth of the 3a splits. Re-baselined DOWN. # Wave 3 / 3a-4 (-954 on AccountsManager, 1910 -> 956): the catalog LOAD orchestration # (loadCatalogs stale-while-revalidate pipeline, the first-page/paginate network crawl + # direct-GET fallbacks, CP-D1 preload + slim-snapshot hydrate/carve/write, the owned @@ -108,7 +114,7 @@ # PalaceDownloads and the account-scope wiring goes with it. Re-baselined by the exact # delta; the freeze still catches any further unrelated growth. 3093 Palace/Audiobooks/AudiobookSessionManager.swift -955 Palace/Accounts/Library/AccountsManager.swift +915 Palace/Accounts/Library/AccountsManager.swift 2184 Palace/MyBooks/MyBooksDownloadCenter.swift 1378 Palace/Book/UI/BookDetail/BookDetailViewModel.swift 1261 Palace/SignInLogic/TPPSignInBusinessLogic.swift