Fix cross-account credential contamination: per-account TPPUserAccount instances#822
Merged
mauricecarrier7 merged 6 commits intomodernize/whole-shotfrom Apr 15, 2026
Merged
Conversation
🧪 Unit Test Results📊 View Full Interactive Report ❌ 5 TESTS FAILED5433 tests | 5391 passed | 5 failed | 37 skipped | ⏱️ 5m 0s | 📊 99.2% | 📈 38.6% coverage Tests by Class
Failed Tests (click to expand)📊 Testing Coverage BreakdownUnit Test Line Coverage: 38.6%
E2E Journey Coverage (SpecterQA): 23/25 journeys passing Journey inventory
🔗 Interactive HTML Report | CI Run Details 📦 Downloadable Artifacts
|
mauricecarrier7
added a commit
that referenced
this pull request
Apr 13, 2026
Two related fixes addressing fallout from PR #822's per-account TPPUserAccount migration. Both codify invariants that prevent the download / borrow flow from misclassifying benign failures as credentials problems and reflexively presenting a useless sign-in modal that traps the user. 1) MyBooksDownloadCenter.userAccount becomes a computed property The download center is a singleton (`@objc static let shared`). Before this change its `userAccount` property was set ONCE at init time from `AccountsManager.shared.currentUserAccount`. After the per-account migration, that captures whichever account happened to be current at app-launch time and never refreshes — so library switches and fresh sign-ins are invisible to the download center. Symptom: tap Download on a freshly-borrowed book → download center checks `userAccount.hasCredentials()` against a stale instance, sees `false`, fires the credentials path even though the user is signed in. Fix: replace the stored property with a computed property that always reads `accountsManager.currentUserAccount`. The init param is preserved as a test-only override (`injectedUserAccount`); no production caller passes it. 2) handleBorrowAuthErrorIfNeeded refuses to call a borrow failure "auth-related" when the user already has an active loan `processRegularDownload` (line 559) auto-re-borrows whenever the registry's copy of a book still carries a `.borrow` primary acquisition. For some catalogs the loans-feed entry retains the borrow relation even after the loan is created, so the auto-re- borrow fires for already-borrowed books. The server responds with 401 (loan-already-exists / cannot-issue-loan), which the network responder maps to `TPPErrorCode.invalidCredentials`. Without a guard, `handleBorrowAuthErrorIfNeeded` then concludes credentials are bad, calls `markCredentialsStale()`, and presents the sign-in modal — but the modal shows the user's *signed-in* AccountDetail view (Sign out button visible, no input fields) titled "Sign in", which is confusing and unfixable from the UI. Fix: add a guard at the top of the auth-error decision path. If the registry already has the book in any active-loan state (`.downloadNeeded`, `.downloading`, `.downloadSuccessful`, `.downloadFailed`, `.holding`, `.SAMLStarted`, `.used`, `.returning`) AND credentials are present, the failure CANNOT be a credentials problem — return `false` so the caller surfaces a real error instead of a fake auth modal. 3) Architectural-invariant comment refinement on the SQ-005 anonymous guard in SignInModalPresenter — clarifies why the central choke point is the right place for the !needsAuth early-return. These guards do NOT block legitimate re-auth flows. Genuine session expiry (SAML cookies, OAuth tokens, OIDC) is detected by separate problem-document type checks and explicit `markCredentialsStale()` calls upstream, neither of which is short-circuited by the new guards. Replay artifact: - .specterqa/replays/sign-in-a1qa-success.yaml — verifies SQ-001 fix end-to-end: PIN field visible, form submits, A1QA sign-in succeeds with the rotated credentials (01230000000237 / Lyrtest123). Note: SQ-007 still has at least one un-traced call site that presents the modal even with these guards in place; see the SQ-007 section of regression-PP-4020/specterqa-regression-findings.md for the open investigation. The two guards committed here are the correct systemic shape — they do not depend on the open issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mauricecarrier7
force-pushed
the
fix/per-account-user-credentials
branch
from
a1e818c to
8a16bc4
Compare
mauricecarrier7
added a commit
that referenced
this pull request
Apr 14, 2026
Two related fixes addressing fallout from PR #822's per-account TPPUserAccount migration. Both codify invariants that prevent the download / borrow flow from misclassifying benign failures as credentials problems and reflexively presenting a useless sign-in modal that traps the user. 1) MyBooksDownloadCenter.userAccount becomes a computed property The download center is a singleton (`@objc static let shared`). Before this change its `userAccount` property was set ONCE at init time from `AccountsManager.shared.currentUserAccount`. After the per-account migration, that captures whichever account happened to be current at app-launch time and never refreshes — so library switches and fresh sign-ins are invisible to the download center. Symptom: tap Download on a freshly-borrowed book → download center checks `userAccount.hasCredentials()` against a stale instance, sees `false`, fires the credentials path even though the user is signed in. Fix: replace the stored property with a computed property that always reads `accountsManager.currentUserAccount`. The init param is preserved as a test-only override (`injectedUserAccount`); no production caller passes it. 2) handleBorrowAuthErrorIfNeeded refuses to call a borrow failure "auth-related" when the user already has an active loan `processRegularDownload` (line 559) auto-re-borrows whenever the registry's copy of a book still carries a `.borrow` primary acquisition. For some catalogs the loans-feed entry retains the borrow relation even after the loan is created, so the auto-re- borrow fires for already-borrowed books. The server responds with 401 (loan-already-exists / cannot-issue-loan), which the network responder maps to `TPPErrorCode.invalidCredentials`. Without a guard, `handleBorrowAuthErrorIfNeeded` then concludes credentials are bad, calls `markCredentialsStale()`, and presents the sign-in modal — but the modal shows the user's *signed-in* AccountDetail view (Sign out button visible, no input fields) titled "Sign in", which is confusing and unfixable from the UI. Fix: add a guard at the top of the auth-error decision path. If the registry already has the book in any active-loan state (`.downloadNeeded`, `.downloading`, `.downloadSuccessful`, `.downloadFailed`, `.holding`, `.SAMLStarted`, `.used`, `.returning`) AND credentials are present, the failure CANNOT be a credentials problem — return `false` so the caller surfaces a real error instead of a fake auth modal. 3) Architectural-invariant comment refinement on the SQ-005 anonymous guard in SignInModalPresenter — clarifies why the central choke point is the right place for the !needsAuth early-return. These guards do NOT block legitimate re-auth flows. Genuine session expiry (SAML cookies, OAuth tokens, OIDC) is detected by separate problem-document type checks and explicit `markCredentialsStale()` calls upstream, neither of which is short-circuited by the new guards. Replay artifact: - .specterqa/replays/sign-in-a1qa-success.yaml — verifies SQ-001 fix end-to-end: PIN field visible, form submits, A1QA sign-in succeeds with the rotated credentials (01230000000237 / Lyrtest123). Note: SQ-007 still has at least one un-traced call site that presents the modal even with these guards in place; see the SQ-007 section of regression-PP-4020/specterqa-regression-findings.md for the open investigation. The two guards committed here are the correct systemic shape — they do not depend on the open issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…via AccountsManager Systemic fix for the TOCTOU credential race (F-034). The singleton pattern mutated a shared `libraryUUID` to switch keychain keys at runtime — any concurrent access during an account switch could read or write the wrong account's credentials. Architecture change: - TPPUserAccount gains init(libraryUUID:) with immutable boundLibraryUUID and its own accountInfoQueue per instance - AccountsManager gains userAccounts cache, userAccount(for:), and currentUserAccount — conforming to new TPPUserAccountResolving protocol - Instance-level credentialSnapshot() reads from self (no UUID switching needed) Production code migration (37 files): - All ~65 call sites of sharedAccount() / sharedAccount(libraryUUID:) / credentialSnapshot(for:) replaced with instance-based access via AccountsManager.shared.currentUserAccount or .userAccount(for:) - Network layer uses per-account instances resolved at request time - atomicUpdate(for:) calls replaced with direct method calls on per-account instances (no UUID switching needed) Legacy singleton retained for test compatibility — TPPUserAccountMock subclasses it. Full singleton removal deferred to followup (requires mock refactoring across ~30 test files). 4,502 tests pass, 0 failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n gate) Tests validate the systemic fix for credential cross-contamination: - Instance cache returns same/different instances correctly - boundLibraryUUID is immutable on per-account instances - Writing credentials to Account A does not affect Account B - Both accounts hold independent credentials simultaneously - Token credentials are isolated between accounts - credentialSnapshot() returns correct per-account data - removeAll() on one account does not affect the other - Concurrent writes to different accounts never cross-contaminate - Concurrent snapshots always return the correct account's data These tests serve as the regression gate: if anyone reintroduces shared mutable state in TPPUserAccount, the concurrent tests fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t one The grep piped to tail -1 was grabbing the TenPrintCover bundle (6 tests) instead of the PalaceTests bundle (4,502 tests). Now uses the "All tests" summary line, with a fallback that sums across all bundles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gate-check command queries pipeline.gates and evidence endpoints, verifies all gates passed and test count meets minimum threshold. Exits 0 (allow) or 1 (block). forgeos-gate-hook.sh finds the changeset for the current branch and runs gate-check. Wired into Claude Code hooks to run before gh pr create. PR creation is blocked unless: - A ForgeOS changeset exists for the branch - All gates (review, testing, release) are passed - Test evidence shows >= FORGEOS_MIN_TESTS passing (default 100) This makes ForgeOS governance blocking, not ceremonial. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d add error origin tagging Cherry-picked from release/2.2.5 into modernize/whole-shot: PP-4045 (BLOCKER): Libraries without PIN requirements got "Invalid Credentials" on sign-in. The guard in TokenRequest rejected empty passwords, but empty password is valid Basic Auth for pinless libraries (RFC 7617). Fix: only guard against empty username. PP-4033: Cancel Hold button did nothing -- no error, no feedback. The revoke endpoint error handler only handled two specific error types; all others fell through silently. Fix: catch-all else branch that announces failure to the user. Error origin tagging: Every non-fatal error logged to Crashlytics now includes error_origin (client/server/network/vendor) for dashboard filtering. Auto-classified from error codes, HTTP status, and URL error domains. Addresses F-META-1. Tests: 13 credential guard tests pass including new pinless login test that verifies Basic Auth "barcode:" format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regression plan: 23 journeys mapped to PP-4020 manual test areas, covering 35% of test matrix automatically. New journeys: place-hold (C3/C4/PP-4033), return-loan (C6), credential-isolation (M3/F-034/ADV-1), persistence-reading-position (P1/P2). Accessibility: add .accessibilityLabel to PIN SecureField/TextField in AccountDetailView. SwiftUI SecureField placeholder text is not exposed to the XCTest accessibility tree, blocking automated sign-in. Replays saved: catalog-browsing, tab-navigation, search-flow (all PASS), sign-in-attempt (quarantined, SecureField invisible). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mauricecarrier7
force-pushed
the
fix/per-account-user-credentials
branch
from
8a16bc4 to
556e1b5
Compare
5 tasks
mauricecarrier7
added a commit
that referenced
this pull request
Apr 15, 2026
Two related fixes addressing fallout from PR #822's per-account TPPUserAccount migration. Both codify invariants that prevent the download / borrow flow from misclassifying benign failures as credentials problems and reflexively presenting a useless sign-in modal that traps the user. 1) MyBooksDownloadCenter.userAccount becomes a computed property The download center is a singleton (`@objc static let shared`). Before this change its `userAccount` property was set ONCE at init time from `AccountsManager.shared.currentUserAccount`. After the per-account migration, that captures whichever account happened to be current at app-launch time and never refreshes — so library switches and fresh sign-ins are invisible to the download center. Symptom: tap Download on a freshly-borrowed book → download center checks `userAccount.hasCredentials()` against a stale instance, sees `false`, fires the credentials path even though the user is signed in. Fix: replace the stored property with a computed property that always reads `accountsManager.currentUserAccount`. The init param is preserved as a test-only override (`injectedUserAccount`); no production caller passes it. 2) handleBorrowAuthErrorIfNeeded refuses to call a borrow failure "auth-related" when the user already has an active loan `processRegularDownload` (line 559) auto-re-borrows whenever the registry's copy of a book still carries a `.borrow` primary acquisition. For some catalogs the loans-feed entry retains the borrow relation even after the loan is created, so the auto-re- borrow fires for already-borrowed books. The server responds with 401 (loan-already-exists / cannot-issue-loan), which the network responder maps to `TPPErrorCode.invalidCredentials`. Without a guard, `handleBorrowAuthErrorIfNeeded` then concludes credentials are bad, calls `markCredentialsStale()`, and presents the sign-in modal — but the modal shows the user's *signed-in* AccountDetail view (Sign out button visible, no input fields) titled "Sign in", which is confusing and unfixable from the UI. Fix: add a guard at the top of the auth-error decision path. If the registry already has the book in any active-loan state (`.downloadNeeded`, `.downloading`, `.downloadSuccessful`, `.downloadFailed`, `.holding`, `.SAMLStarted`, `.used`, `.returning`) AND credentials are present, the failure CANNOT be a credentials problem — return `false` so the caller surfaces a real error instead of a fake auth modal. 3) Architectural-invariant comment refinement on the SQ-005 anonymous guard in SignInModalPresenter — clarifies why the central choke point is the right place for the !needsAuth early-return. These guards do NOT block legitimate re-auth flows. Genuine session expiry (SAML cookies, OAuth tokens, OIDC) is detected by separate problem-document type checks and explicit `markCredentialsStale()` calls upstream, neither of which is short-circuited by the new guards. Replay artifact: - .specterqa/replays/sign-in-a1qa-success.yaml — verifies SQ-001 fix end-to-end: PIN field visible, form submits, A1QA sign-in succeeds with the rotated credentials (01230000000237 / Lyrtest123). Note: SQ-007 still has at least one un-traced call site that presents the modal even with these guards in place; see the SQ-007 section of regression-PP-4020/specterqa-regression-findings.md for the open investigation. The two guards committed here are the correct systemic shape — they do not depend on the open issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mauricecarrier7
added a commit
that referenced
this pull request
Apr 15, 2026
…/8) (#825) * Add per-account credential isolation tests (10 tests, F-034 regression gate) Tests validate the systemic fix for credential cross-contamination: - Instance cache returns same/different instances correctly - boundLibraryUUID is immutable on per-account instances - Writing credentials to Account A does not affect Account B - Both accounts hold independent credentials simultaneously - Token credentials are isolated between accounts - credentialSnapshot() returns correct per-account data - removeAll() on one account does not affect the other - Concurrent writes to different accounts never cross-contaminate - Concurrent snapshots always return the correct account's data These tests serve as the regression gate: if anyone reintroduces shared mutable state in TPPUserAccount, the concurrent tests fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix Palace Bookshelf empty sign-in modal on Download (SQ-005) Anonymous-auth libraries (Palace Bookshelf, COPPA) showed a broken sign-in sheet with no fields and no continue button when Download was tapped. Cancel was the only escape, blocking all reading on Palace Bookshelf — the demo/onboarding library. Root cause: MyBooksDownloadCenter.handleProblem (line 769) and several similar paths fired the reauth modal whenever !hasCredentials, without first checking whether the auth method actually requires credentials. Anonymous accounts always have no credentials by definition, so the condition was always true. The reauthenticator unconditionally invoked SignInModalPresenter.presentSignInModalForCurrentAccount, which mounts AccountDetailView(forceReauthMode: true) — for anonymous auth there are no fields to render, so only the navigation chrome appeared. Systemic fix: early-return in SignInModalPresenter.presentSignInModalForCurrentAccount when !userAccount.needsAuth. Single choke point protects 15+ call sites across MyBooksDownloadCenter, BookActionHandler, BookCellModel, HoldsViewModel, TPPNetworkExecutor, TokenRefreshInterceptor, DLNavigator, MyBooksViewModel, and BookDetailViewModel. Callers always get their completion fired so existing flows complete without modification. Latent secondary fix: UserAccountAuthHelper.needsAuth was missing .oidc, inconsistent with AccountDetails.Authentication.needsAuth. OIDC libraries would have silently bypassed needsAuth gates. Aligned the two definitions. Verified end-to-end via SpecterQA on iPhone 12 / iOS 26: 1. Switched to Palace Bookshelf 2. Tapped Borrow on Dobbs v. Jackson (DPLA Publications lane) 3. Half-sheet showed Read + Remove (auto-download succeeded) 4. Tapped Read → EPUB reader opened at "Page 1 of 488 (Cover)" 5. Swipe-left → page 2 with rendered Dobbs syllabus text Replays for both reproduction and the fixed flow are committed under .specterqa/replays/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * SQ-007: Two systemic guards for stale per-account credential references Two related fixes addressing fallout from PR #822's per-account TPPUserAccount migration. Both codify invariants that prevent the download / borrow flow from misclassifying benign failures as credentials problems and reflexively presenting a useless sign-in modal that traps the user. 1) MyBooksDownloadCenter.userAccount becomes a computed property The download center is a singleton (`@objc static let shared`). Before this change its `userAccount` property was set ONCE at init time from `AccountsManager.shared.currentUserAccount`. After the per-account migration, that captures whichever account happened to be current at app-launch time and never refreshes — so library switches and fresh sign-ins are invisible to the download center. Symptom: tap Download on a freshly-borrowed book → download center checks `userAccount.hasCredentials()` against a stale instance, sees `false`, fires the credentials path even though the user is signed in. Fix: replace the stored property with a computed property that always reads `accountsManager.currentUserAccount`. The init param is preserved as a test-only override (`injectedUserAccount`); no production caller passes it. 2) handleBorrowAuthErrorIfNeeded refuses to call a borrow failure "auth-related" when the user already has an active loan `processRegularDownload` (line 559) auto-re-borrows whenever the registry's copy of a book still carries a `.borrow` primary acquisition. For some catalogs the loans-feed entry retains the borrow relation even after the loan is created, so the auto-re- borrow fires for already-borrowed books. The server responds with 401 (loan-already-exists / cannot-issue-loan), which the network responder maps to `TPPErrorCode.invalidCredentials`. Without a guard, `handleBorrowAuthErrorIfNeeded` then concludes credentials are bad, calls `markCredentialsStale()`, and presents the sign-in modal — but the modal shows the user's *signed-in* AccountDetail view (Sign out button visible, no input fields) titled "Sign in", which is confusing and unfixable from the UI. Fix: add a guard at the top of the auth-error decision path. If the registry already has the book in any active-loan state (`.downloadNeeded`, `.downloading`, `.downloadSuccessful`, `.downloadFailed`, `.holding`, `.SAMLStarted`, `.used`, `.returning`) AND credentials are present, the failure CANNOT be a credentials problem — return `false` so the caller surfaces a real error instead of a fake auth modal. 3) Architectural-invariant comment refinement on the SQ-005 anonymous guard in SignInModalPresenter — clarifies why the central choke point is the right place for the !needsAuth early-return. These guards do NOT block legitimate re-auth flows. Genuine session expiry (SAML cookies, OAuth tokens, OIDC) is detected by separate problem-document type checks and explicit `markCredentialsStale()` calls upstream, neither of which is short-circuited by the new guards. Replay artifact: - .specterqa/replays/sign-in-a1qa-success.yaml — verifies SQ-001 fix end-to-end: PIN field visible, form submits, A1QA sign-in succeeds with the rotated credentials (01230000000237 / Lyrtest123). Note: SQ-007 still has at least one un-traced call site that presents the modal even with these guards in place; see the SQ-007 section of regression-PP-4020/specterqa-regression-findings.md for the open investigation. The two guards committed here are the correct systemic shape — they do not depend on the open issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * SpecterQA replay: A1QA sign-in → return loan → sign-out happy path End-to-end Tier-1 verification on the rotated A1QA credentials (01230000000237 / Lyrtest123). Captures: 1. Sign in via Settings → Libraries → A1QA Test Library 2. My Books loads 7 loans (5 ebooks, 2 audiobooks) with metadata 3. Tap Return on "Accessibility Handbook" → confirmation alert 4. Confirm Return → server-side return succeeds, list shrinks to 6 5. Settings → Sign out → My Books empties Validates that sign-in, return-loan, and sign-out all work end-to-end on basic-auth libraries with the SQ-005 + SQ-007 guards applied. Download from A1QA My Books cell remains blocked by an un-traced SQ-007 call site — see SESSION-2026-04-11.md for the open investigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix Cancel Hold non-functional on Manage Hold half-sheet (SQ-008) Three related fixes for the Manage Hold half-sheet: 1) Cancel Hold confirmation alert rendered behind the sheet Root cause: BookCellModel.showAlert (the @published property that holds the cancel-hold confirmation AlertModel) was only bound to NormalBookCell's .alert(item:) modifier — which sits BEHIND the presented half-sheet. When the user tapped Cancel Hold on the sheet, the confirmation alert appeared on the cell underneath, invisible and non-interactive. The user saw no response to their tap. Fix: added showAlert to the HalfSheetProvider protocol and bound a second .alert(item:) modifier directly on HalfSheetView. Also added @published var showAlert to BookDetailViewModel (which also conforms to HalfSheetProvider). The confirmation now renders ON the sheet where the user can see and interact with it. QA had previously reported this bug from manual testing; SpecterQA independently reproduced it via automated regression, confirming the issue is real and not environment-specific. 2) Problem document extraction for return/cancel-hold failures The old code at line 1163 had `error as? Decoder` which could never succeed — `error` is a [String: Any]? dictionary, not a Swift Decoder. The problem document was silently dropped and the user only saw the generic "could not be completed" message. Fix: extract via guard let + JSONSerialization.data + fromData(), with a nil guard to prevent SIGABRT when error is nil (which NSJSONSerialization throws as an ObjC NSException, not a Swift Error, so try? doesn't catch it). 3) Half-sheet layout for manage-hold state Added bottom padding (40pt) and .large presentation detent option for manage-hold sheets as defense-in-depth against content overflow into the home indicator safe area. Also removes the SQ-007 file-based trace instrumentation from SignInModalPresenter (no longer needed — SQ-007 is fully resolved). Verified end-to-end via SpecterQA 11.5.0: - Reservations → Manage Hold → Cancel Hold → confirmation alert appears ON the sheet → Remove Hold → server error alert with Retry/Remove from Device/Cancel → no crash Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Migrate all legacy TPPOPDSFeed.withURL callers to OPDSFeedService Completes the OPDS2 migration by eliminating the last four callers of the legacy TPPOPDSFeed.withURL callback API. All now use OPDSFeedService.shared.fetchFeed (async/await) which wraps the same OPDS 1.x parser but provides typed PalaceError with attached problem documents — the infrastructure built during the OPDS2 work. Fully backwards compatible: OPDSFeedService.fetchFeed calls TPPOPDSFeed.withURL internally. Same network path, same XML parser, same feed types. The only change is error plumbing. Migrated callers: 1) MyBooksDownloadCenter.returnBook — rewritten from deeply nested callbacks to linear async/await. Problem document now extracted from typed PalaceError via NSError.problemDocument instead of the broken `error as? Decoder` cast. All three error branches (loan-gone, invalid-credentials, generic) preserved with identical behavior but clearer structure. 2) TPPBookRegistry.sync — loans feed sync. Error document now extracted from PalaceError userInfo instead of the raw callback dictionary. Logs sync failures with error description. 3) BookRegistrySync.sync — same pattern as TPPBookRegistry.sync. 4) BookDetailViewModel.loadRelatedBooks — related books fetch. Was silently discarding errors (`_`), now logs them for diagnostics. After this commit, TPPOPDSFeed.withURL is only called from OPDSFeedService.fetchFeed (the wrapper itself) — no direct callers remain in application code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Rename "Reservations" to "Holds" across UI copy Tab bar, nav titles, search labels, and error messages all now say "Holds" instead of "Reservations". This rename was previously done on develop but was lost when this branch diverged. API-level fields (supportsReservations) and the legacy removeReservation action label are unchanged — those are protocol contracts, not user-facing copy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Propagate server error details to return/cancel-hold failure UI Two changes that close the diagnostic gap for return and cancel-hold failures: 1) TPPOPDSFeed.withURL: synthesize error dict for non-problem-doc HTTP errors When a server returns a non-2xx status without a problem document (e.g., HTML error page, empty body, or non-OPDS JSON), the callback previously received (nil, nil) — swallowing the HTTP status code and response body entirely. Now synthesizes a dict with type "http-error", the localized HTTP status text, the numeric code, and the first 500 chars of the response body. 2) returnBook catch block: append server detail to error message Extracts the server's reason from the problem document detail, the NSError userInfo (where OPDSFeedService stashes "problemDocumentDetail"), or falls back to the localized error description. Appends it to the alert message so users and testers see WHY the return failed, not just THAT it failed. Verified via SpecterQA: cancel-hold on OverDrive now shows "Invalid OPDS feed" instead of the generic "could not be completed" — identifying that the OverDrive revoke endpoint returns a non-OPDS response format for this account's hold state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add OPDSFeedMigrationTests: 11 tests for OPDS migration paths Covers the key regression and invariants from the OPDS2 migration: - Revoke tolerance: .opdsFeedInvalid IS tolerated for revoke operations (OverDrive returns non-OPDS XML on success), other parsing/auth errors are NOT tolerated - Synthetic error dict: HTTP errors produce valid problem documents with status code and response body - Error message fallback chain: detail extraction from problemDoc → NSError userInfo → localizedDescription - SQ-005 auth guards: anonymous (false), basic (true), OIDC (true) - SQ-008 protocol: HalfSheetProvider conformance with showAlert on both BookCellModel and BookDetailViewModel All 11 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * F-035: Auto-refresh My Books/Holds on tab switch; add 13 regression tests F-035 fix: TPPBookRegistry.sync() is now called when the user switches to the My Books or Holds tab, so newly borrowed/returned books appear without pull-to-refresh. The existing notification observers in MyBooksViewModel already handle the sync completion. Credential Isolation E2E Tests (5 tests): - Sign in A → switch B → sign in B → verify A intact - Sign in A → switch B → sign in B → sign out B → verify A intact - Three simultaneous libraries, all isolated - markCredentialsStale on A doesn't affect B - 500 rapid concurrent switches, no contamination SAML/OIDC Regression Tests (8 tests): - needsAuth exhaustive for all AuthType values - needsAuth consistency between two implementations (SQ-005 fix) - Borrow reauth guard allows reauth for unregistered books (SAML) - Borrow reauth guard blocks reauth for borrowed books (SQ-007) - All 10 book states correctly classified as loan/non-loan - SignInModal guard: SAML not blocked, OIDC not blocked, anonymous blocked All 24 new tests pass (+ 11 existing OPDSFeedMigrationTests = 35 total). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mauricecarrier7
added a commit
that referenced
this pull request
Apr 17, 2026
Two regressions from build 459 → HEAD traced and fixed. 1) Cache coherence (Gorgon sign-out didn't refresh UI): TPPKeychainVariable caches the last-read value and only invalidates when its .key changes. PR #822 introduced per-account TPPUserAccount instances and AccountDetailViewModel switched from the static class method TPPUserAccount.credentialSnapshot(for:) to the per-instance credentialSnapshot(). But the sign-in/out pipeline writes through the singleton — the per-account instance's cache never sees the update, so the view model keeps returning stale "signed in" state and the UI never refreshes. Fix: the instance method now toggles libraryUUID (nil → uuid) before reading, mirroring the static method's behavior; the key re-bind invalidates every variable's cache via the didSet hook on TPPKeychainVariable.key. 2) OIDC dropped from browser-auth prompt (AccountDetailView regression): Commit 6c88fb1 renamed isBrowserBasedAuth to isOAuthOrSAML and dropped the isOidc check. For OIDC libraries that was changing shouldShowSignInPrompt from true to false post-sign-out, rendering the credential fields instead of the WebView prompt. Restored to the three-way check that shipped in build 459. New regression guard: TPPCredentialSnapshotCoherenceTests.swift — three tests that write via one TPPUserAccount instance and read via a peer instance. These would have caught the cache coherence bug on the day PR #822 shipped; they will catch the next one. Full auth regression: 123/123 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11 tasks
mauricecarrier7
added a commit
that referenced
this pull request
Apr 17, 2026
Finishes the per-account credential isolation migration started in PR #822, which left the legacy singleton in place as a safety-net fallback. That fallback was the root cause of the login-prompt-during-download race: `TPPUserAccount.sharedAccount(libraryUUID:)` re-pointed one shared instance at different libraries by mutating `libraryUUID`, and during the window between "libraryUUID flipped to nil" and "keychain keys re-resolved," `hasCredentials()` returned false on an account that was in fact signed in. Production call sites migrated: - TPPAnnotations.currentID() — now reads AccountsManager.currentUserAccount.deviceID - TPPSignInBusinessLogic.userAccount — now resolves via libraryAccountsProvider.userAccount(for:). Widens TPPLibraryAccountsProvider to also conform to TPPUserAccountResolving, so the existing injected provider can vend per-library accounts directly. - AccountsManager.currentUserAccount — no longer falls back to the singleton on transient `currentAccountId == nil` windows. Caches the last-known account and returns a deterministic "no-account" placeholder on a truly fresh install, avoiding the race that produced the login modal. TPPUserAccount changes: - `libraryUUID` is now immutable (`let`). The didSet -> updateKeychainKeys() cache-invalidation trick is gone; a new `invalidateAllKeychainCaches()` helper calls `TPPKeychainVariable.invalidateCache()` explicitly. - `static private let shared`, legacy `override init()`, and the mutable `sharedAccount(libraryUUID:)` implementation are removed. - `credentialSnapshot()` no longer does the nil→uuid flip; it calls the explicit invalidator. - Class-level `credentialSnapshot(for:)` now forwards to `AccountsManager.shared.userAccount(for:).credentialSnapshot()` without mutating any singleton state. - `atomicUpdate(for:)` asserts the passed UUID matches the bound instance and runs the block under the per-account barrier queue. - `sharedAccount()` / `sharedAccount(libraryUUID:)` remain as `@available(deprecated)` thin delegates — they do not mutate shared state. Retained purely so ~90 test call sites keep compiling; scheduled for removal after those tests are migrated to AccountsManager directly. Protocol: - `TPPUserAccountProvider.sharedAccount(libraryUUID:)` removed. Test infrastructure: - TPPUserAccountMock gains a `convenience init()` that routes to `init(libraryUUID:)` with a fixed test UUID. - TPPLibraryAccountMock now conforms to the widened TPPLibraryAccountsProvider, with an injectable `userAccountResolver` closure so cross-library-isolation tests can route through `TPPMultiLibraryAccountMock` per-UUID while the default single-instance case is unchanged. Verification: - Full xctest run: 5555 tests, 0 failures, 33 skipped (unrelated to this change). - Auth-critical suites (PerAccountIsolation, CrossLibrarySignOut, CredentialVisibility, AccountSwitchCleanup, SAMLSignIn, AuthFlowSecurity, OverdriveDeferredFulfillment) all green. ForgeOS: cs_583f4c98 (init_67d79332). Lesson obs_a87ddd3f captures the "incomplete-migration with safety-net fallback" anti-pattern this fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <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
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.
Summary
TPPUserAccountsingleton that caused credential cross-contamination between library accounts during account switches (PP-4020 F-034, blocker)AccountsManager— each instance has immutable keychain keys, making credential cross-contamination structurally impossiblesharedAccount()to instance-based accesscredentialSnapshotreads + sign-in modal deduplication guard) for defense-in-depthRoot cause
TPPUserAccountwas a singleton that mutatedlibraryUUIDto switch which keychain keys it read/wrote. BetweensharedAccount(libraryUUID:)returning and.username/.pinbeing read, another thread could switch the UUID — sending Account B's credentials to Account A's token endpoint, or writing them into Account A's keychain slot.Architecture change
TPPUserAccount(libraryUUID:)— new initializer with immutableboundLibraryUUIDand its ownaccountInfoQueueAccountsManager.userAccount(for:)— caches per-library instances, returns stable identityTPPUserAccountResolvingprotocol for DIcredentialSnapshot()(no UUID switching needed)SignInModalPresenter.isPresentingguard prevents modal stacking from concurrent 401sWhat's deferred
Legacy singleton
init()retained asinternalfor test mock compatibility (TPPUserAccountMocksubclasses it). Full singleton removal requires refactoring ~30 test files — separate PR.Test plan
Regression context
PP-4020 findings F-033 (reclassified from regression to pre-existing) and F-034 (blocker, fixed). Full technical analysis in
~/Desktop/regression-PP-4020/findings.md.🤖 Generated with Claude Code
PP-4020 Traceability