diff --git a/.forgeos/intent/pp-4542-audiobook-position-coverage.md b/.forgeos/intent/pp-4542-audiobook-position-coverage.md new file mode 100644 index 000000000..341fec9fb --- /dev/null +++ b/.forgeos/intent/pp-4542-audiobook-position-coverage.md @@ -0,0 +1,45 @@ +--- +name: PP-4542 audiobook position-validation candidate-selection coverage +created: 2026-06-08 +author: claude-opus-4-8 +--- + +## Summary + +F-007 (PP-4542): the position-validation / candidate-selection logic added to +`AudiobookSessionManager` by PR #1028 (stale-position-on-reborrow hardening) +shipped with a 0% diff-only mutation kill rate on the changed lines. Add +behavior tests at the production seams that pin the real decisions — +candidate recency ordering, TOC track-key match, the `validationFailure == nil` +filter, `isValidPosition`, and the `isUserAuthenticated` auth-doc-load-failure +branch — with constructed TOC + registry + account fixtures (no live +Audiobook/player graph). + +## Claims + +- adds test class `PalaceTests/Audiobooks/AudiobookPositionRestoreTests.swift` + with behavior tests for candidate ordering, TOC track-key match, + validation-filter drop, isValidPosition, and isUserAuthenticated failure +- extracts a pure `selectMostRecentValidBookmark(from:in:)` seam out of + `fallbackToMostRecentValidBookmark` so the candidate filter+sort is testable + against an `AudiobookTableOfContents` without a live `Audiobook` +- changes visibility of `getValidLocalPosition`, `fallbackToMostRecentValidBookmark`, + `validationFailure(for:in:)`, `isValidPosition`, and `isUserAuthenticated` + from `private` to `internal` so `@testable` tests reach them +- registers the new test file in `Palace.xcodeproj` (PalaceTests target) + +## Anti-claims + +- does NOT change any shipping behavior — the extraction is a pure refactor + (same candidates, same filter, same sort) and the visibility changes are + test-reachability only +- does NOT change any call site, method signature semantics, or control flow + on the audiobook open/restore path +- does NOT touch the audiobook toolkit submodule or any DRM/network code + +## Files in scope + +- Palace/Audiobooks/AudiobookSessionManager.swift +- PalaceTests/Audiobooks/AudiobookPositionRestoreTests.swift (NEW) +- Palace.xcodeproj/project.pbxproj +- .forgeos/intent/pp-4542-audiobook-position-coverage.md (NEW) diff --git a/Palace.xcodeproj/project.pbxproj b/Palace.xcodeproj/project.pbxproj index 222c959eb..5457f3a56 100644 --- a/Palace.xcodeproj/project.pbxproj +++ b/Palace.xcodeproj/project.pbxproj @@ -85,6 +85,7 @@ 10DCC70174134074821BB5E3 /* TPPBookmarkR3LocationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D4FA6A7940BE4470BD0A3515 /* TPPBookmarkR3LocationTests.swift */; }; 110F853E19D5FA7300052DF7 /* DetailSummaryTemplate.html in Resources */ = {isa = PBXBuildFile; fileRef = 110F853C19D5FA7300052DF7 /* DetailSummaryTemplate.html */; }; 1112A8431A3249B4002B8CC1 /* libTenPrintCover.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 1112A81F1A322C53002B8CC1 /* libTenPrintCover.a */; }; + 11251173E2E3C47D3E927FB2 /* AudiobookPositionRestoreTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7409BB4778BF1736365594FB /* AudiobookPositionRestoreTests.swift */; }; 119503E71993F914009FB788 /* libxml2.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 119503E61993F914009FB788 /* libxml2.dylib */; }; 119503E91993F919009FB788 /* libz.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 119503E81993F919009FB788 /* libz.dylib */; }; 121E8B66B8634CD720E7C524 /* CatalogRepositoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBA39820CBA75D6BA9B763E2 /* CatalogRepositoryTests.swift */; }; @@ -2503,6 +2504,7 @@ 73F713552417200F00C63B81 /* TPPBaseReaderViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; name = TPPBaseReaderViewController.swift; path = Palace/Reader2/UI/TPPBaseReaderViewController.swift; sourceTree = SOURCE_ROOT; }; 73F713672417240100C63B81 /* UIViewController+TPP.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; name = "UIViewController+TPP.swift"; path = "Palace/Reader2/UI/UIViewController+TPP.swift"; sourceTree = SOURCE_ROOT; }; 73FB0AC824EB403D0072E430 /* TPPBookContentTypeConverter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TPPBookContentTypeConverter.swift; sourceTree = ""; }; + 7409BB4778BF1736365594FB /* AudiobookPositionRestoreTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = AudiobookPositionRestoreTests.swift; sourceTree = ""; }; 74D09C16D15567198A0A9194 /* StatsViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsViewModelTests.swift; sourceTree = ""; }; 75305888F6A941DDA9EEE592 /* RDServicesStubs.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RDServicesStubs.m; sourceTree = ""; }; 758B05F321F1562FFC733397 /* AudiobookPositionAdapterContractTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = AudiobookPositionAdapterContractTests.swift; sourceTree = ""; }; @@ -4663,6 +4665,7 @@ B3CBF74C265D4FA1483F1974 /* AudiobookSessionManagerPresenterMigrationTests.swift */, 787DCAF97FD5E190363EC3CA /* AudiobookSessionManagerFlagGatePresentationTests.swift */, 8D899B519469E743D48FFACC /* AudiobookPlaytimesLifecycleTests.swift */, + 7409BB4778BF1736365594FB /* AudiobookPositionRestoreTests.swift */, ); path = Audiobooks; sourceTree = ""; @@ -7358,6 +7361,7 @@ 151C578EEB3CBF8B804952F6 /* DeveloperSettingsTierTests.swift in Sources */, 6D575E8878F3E8BC9FA43DE3 /* ImageCacheContinuationTests.swift in Sources */, 141494C5B2EF5102AE3CDC4E /* SupportSectionDecisionTests.swift in Sources */, + 11251173E2E3C47D3E927FB2 /* AudiobookPositionRestoreTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Palace/Audiobooks/AudiobookSessionManager.swift b/Palace/Audiobooks/AudiobookSessionManager.swift index 1787ccd99..09ede7593 100644 --- a/Palace/Audiobooks/AudiobookSessionManager.swift +++ b/Palace/Audiobooks/AudiobookSessionManager.swift @@ -1204,7 +1204,12 @@ public final class AudiobookSessionManager: ObservableObject { /// generic bookmarks for this book, the most-recent valid one is returned /// as a fallback (better than dropping the patron to chapter-1 start). /// Returns `nil` only when there's nothing usable at all. - private func getValidLocalPosition(book: TPPBook, audiobook: Audiobook) -> TrackPosition? { + // `internal` (not `private`) so `@testable import Palace` unit tests can + // drive the position-restore decision through this production seam with + // constructed registry/TOC fixtures — see AudiobookPositionRestoreTests. + // This is the same seam-exposure pattern already used by the static + // policy mirrors (networkValidationError, normalizedChaptersCount). + func getValidLocalPosition(book: TPPBook, audiobook: Audiobook) -> TrackPosition? { let primary = tryLoadPrimaryLocalPosition(book: book, audiobook: audiobook) switch primary { case .success(let position): @@ -1274,22 +1279,41 @@ public final class AudiobookSessionManager: ObservableObject { /// the validator accepts it AND it parses against the current manifest. /// Recency is by `lastSavedTimeStamp` (ISO8601), falling back to array /// order when timestamps are missing. - private func fallbackToMostRecentValidBookmark( + func fallbackToMostRecentValidBookmark( book: TPPBook, audiobook: Audiobook ) -> TrackPosition? { let bookmarks = bookRegistry.genericBookmarksForIdentifier(book.identifier) guard !bookmarks.isEmpty else { return nil } + return selectMostRecentValidBookmark( + from: bookmarks, + in: audiobook.tableOfContents + ) + } + /// Pure candidate-selection seam: from a set of saved generic bookmarks, + /// reconstruct each against the manifest, drop any that fail validation, + /// and return the most-recent valid one (descending `lastSavedTimeStamp`, + /// which is ISO8601 and therefore lexicographically sortable). + /// + /// `internal` (not `private`) and threaded `AudiobookTableOfContents` + /// instead of the full `Audiobook` so the validation filter and the + /// recency ordering are mutation-testable from a unit test with a real + /// TOC + seeded `TPPBookLocation` fixtures — no live `Audiobook` / + /// player graph required. Mirrors the `validationFailure(for:in:)` seam. + func selectMostRecentValidBookmark( + from bookmarks: [TPPBookLocation], + in tableOfContents: AudiobookTableOfContents + ) -> TrackPosition? { let candidates: [(TrackPosition, String)] = bookmarks.compactMap { location in guard let dict = location.locationStringDictionary(), let bookmark = AudioBookmark.create(locatorData: dict), let position = TrackPosition( audioBookmark: bookmark, - toc: audiobook.tableOfContents.toc, - tracks: audiobook.tableOfContents.tracks + toc: tableOfContents.toc, + tracks: tableOfContents.tracks ), - validationFailure(for: position, in: audiobook.tableOfContents) == nil else { + validationFailure(for: position, in: tableOfContents) == nil else { return nil } return (position, bookmark.lastSavedTimeStamp ?? "") @@ -1304,7 +1328,7 @@ public final class AudiobookSessionManager: ObservableObject { /// Re-uses `AudiobookPositionPolicy.validate`. The thin shim adapts the /// instance-level call site (which already has the toolkit's position /// object) to the pure-function policy (which doesn't need the toolkit). - private func validationFailure( + func validationFailure( for position: TrackPosition, in tableOfContents: AudiobookTableOfContents ) -> AudiobookPositionValidationFailure? { @@ -1339,7 +1363,7 @@ public final class AudiobookSessionManager: ObservableObject { /// Kept as a thin wrapper for any in-file callers that just want a bool. /// New code should use `validationFailure(for:in:)` directly so the /// failure mode can be logged. - private func isValidPosition(_ position: TrackPosition, in tableOfContents: AudiobookTableOfContents) -> Bool { + func isValidPosition(_ position: TrackPosition, in tableOfContents: AudiobookTableOfContents) -> Bool { return validationFailure(for: position, in: tableOfContents) == nil } @@ -1499,7 +1523,10 @@ public final class AudiobookSessionManager: ObservableObject { /// 20s session-manager timeout is the sole timeout on this path — per /// the ADR's single-timeout policy we do NOT wrap awaitReady() in /// withTimeout here. - private func isUserAuthenticated() async -> Bool { + // `internal` (not `private`) so `@testable` tests can pin the + // auth-doc-load-failure → not-authenticated mapping (the `catch` + // branch below) through this seam — see AudiobookPositionRestoreTests. + func isUserAuthenticated() async -> Bool { guard let account = accountsManager.currentAccount else { return false } diff --git a/PalaceTests/Audiobooks/AudiobookPositionRestoreTests.swift b/PalaceTests/Audiobooks/AudiobookPositionRestoreTests.swift new file mode 100644 index 000000000..24e8f4ea3 --- /dev/null +++ b/PalaceTests/Audiobooks/AudiobookPositionRestoreTests.swift @@ -0,0 +1,322 @@ +// +// AudiobookPositionRestoreTests.swift +// PalaceTests +// +// F-007 / PP-4542 — position-validation + candidate-selection coverage for +// AudiobookSessionManager. PR #1028 introduced the stale-position-on-reborrow +// hardening (validationFailure / fallback-to-most-recent-bookmark). The +// changed lines shipped with a 0% mutation kill rate; these tests pin the +// real decisions through the production seams (`validationFailure(for:in:)`, +// `isValidPosition`, `selectMostRecentValidBookmark`, `isUserAuthenticated`) +// with constructed TOC + registry + account fixtures — no live Audiobook / +// player graph, so the audiobook toolkit's fragility does not leak in. +// +// Copyright (c) 2026 The Palace Project. All rights reserved. +// + +import XCTest +import PalaceCatalog +@testable import Palace +@testable import PalaceAudiobookToolkit + +@MainActor +final class AudiobookPositionRestoreTests: XCTestCase { + + private let bookIdentifier = "pp4542-audiobook" + private let manifestJSON: ManifestJSON = .snowcrash + + private var registryMock: TPPBookRegistryMock! + private var appContainer: AppContainer! + private var sut: AudiobookSessionManager! + private var tracks: Tracks! + private var toc: AudiobookTableOfContents! + + override func setUpWithError() throws { + try super.setUpWithError() + registryMock = TPPBookRegistryMock() + appContainer = makeTestAppContainer(bookRegistry: registryMock) + sut = AudiobookSessionManager(appContainer: appContainer) + + let manifest = try Manifest.from( + jsonFileName: manifestJSON.rawValue, + bundle: Bundle(for: type(of: self)) + ) + tracks = Tracks(manifest: manifest, audiobookID: bookIdentifier, token: nil) + toc = AudiobookTableOfContents(manifest: manifest, tracks: tracks) + + // Fixture sanity: the manifest must yield a multi-track, in-manifest TOC, + // otherwise the key-match / sort assertions below would be vacuous. + XCTAssertGreaterThanOrEqual(tracks.tracks.count, 3, + "snowcrash fixture must have ≥3 tracks for these tests to be meaningful") + } + + override func tearDownWithError() throws { + sut = nil + appContainer = nil + registryMock = nil + tracks = nil + toc = nil + try super.tearDownWithError() + } + + // MARK: - Helpers + + /// Builds a `TPPBookLocation` (the on-disk generic-bookmark shape the + /// registry stores) for `track` at `timestamp`, stamping a controllable + /// `lastSavedTimeStamp`. Production reads it back via + /// `locationStringDictionary()` → `AudioBookmark.create` → `TrackPosition`. + private func makeBookmarkLocation( + track: any Track, + timestamp: TimeInterval, + savedAt: String + ) -> TPPBookLocation { + var position = TrackPosition(track: track, timestamp: timestamp, tracks: tracks) + position.lastSavedTimeStamp = savedAt + let bookmark = position.toAudioBookmark() + bookmark.lastSavedTimeStamp = savedAt + guard let location = bookmark.toTPPBookLocation() else { + XCTFail("Failed to build TPPBookLocation fixture") + return TPPBookLocation(locationString: "{}", renderer: "PalaceAudiobookToolkit")! + } + return location + } + + /// An OpenAccessTrack whose `key` is deliberately NOT present in the + /// manifest — used to drive the `trackKeyNotInManifest` failure path. + private func makeForeignKeyedTrack() throws -> OpenAccessTrack { + let manifest = try Manifest.from( + jsonFileName: manifestJSON.rawValue, + bundle: Bundle(for: type(of: self)) + ) + return try OpenAccessTrack( + manifest: manifest, + urlString: "https://example.com/not-in-manifest.mp3", + audiobookID: bookIdentifier, + title: "Foreign Track", + duration: 60, + index: 999, + token: nil, + key: "FOREIGN-KEY-NOT-IN-MANIFEST" + ) + } + + // MARK: - validationFailure(for:in:) — track-key match (line ~1311) + + /// A position whose track key IS in the manifest passes the track-key + /// gate (the only remaining gate for an otherwise-valid position), so + /// `validationFailure` returns nil. Pins the `!= nil` predicate: a + /// mutation to `== nil` (mutant for line ~1311) would report a spurious + /// `.trackKeyNotInManifest` for a key that IS present and fail here. + func testValidationFailure_trackKeyInManifest_returnsNil() { + let position = TrackPosition(track: tracks.tracks[0], timestamp: 100, tracks: tracks) + XCTAssertNil(sut.validationFailure(for: position, in: toc), + "An in-manifest track key with a sane timestamp must validate (no failure)") + } + + /// A position whose track key is NOT in the manifest must fail with + /// `.trackKeyNotInManifest`. This is the inverse of the test above and + /// is what kills the `!= nil` → `== nil` mutant: under the mutant, an + /// absent key would be treated as "matches" and this assertion flips. + func testValidationFailure_trackKeyNotInManifest_returnsTrackKeyFailure() throws { + let foreign = try makeForeignKeyedTrack() + let position = TrackPosition(track: foreign, timestamp: 100, tracks: tracks) + guard let failure = sut.validationFailure(for: position, in: toc) else { + XCTFail("A track key absent from the manifest must produce a validation failure") + return + } + XCTAssertEqual(failure, .trackKeyNotInManifest(savedKey: "FOREIGN-KEY-NOT-IN-MANIFEST"), + "Absent key must surface as .trackKeyNotInManifest carrying the saved key") + } + + // MARK: - isValidPosition (line ~1343) + + /// `isValidPosition` returns true exactly when `validationFailure == nil`. + /// Valid position → true. Kills the `== nil` → `!= nil` mutant at line + /// ~1343 (under the mutant a valid position would report invalid). + func testIsValidPosition_validPosition_returnsTrue() { + let position = TrackPosition(track: tracks.tracks[1], timestamp: 50, tracks: tracks) + XCTAssertTrue(sut.isValidPosition(position, in: toc), + "A position with an in-manifest key and a finite, non-negative timestamp is valid") + } + + /// Invalid position (negative timestamp) → false. The companion to the + /// test above: the pair pins both sides of the `== nil` predicate so the + /// inversion mutant cannot survive. + func testIsValidPosition_negativeTimestamp_returnsFalse() { + let position = TrackPosition(track: tracks.tracks[0], timestamp: -5, tracks: tracks) + XCTAssertFalse(sut.isValidPosition(position, in: toc), + "A negative timestamp must make the position invalid") + } + + /// Invalid position (foreign track key) → false, through the bool shim. + func testIsValidPosition_foreignKey_returnsFalse() throws { + let foreign = try makeForeignKeyedTrack() + let position = TrackPosition(track: foreign, timestamp: 10, tracks: tracks) + XCTAssertFalse(sut.isValidPosition(position, in: toc), + "A track key absent from the manifest must make the position invalid") + } + + // MARK: - selectMostRecentValidBookmark — recency ordering (line ~1305) + + /// Three valid bookmarks at distinct tracks with strictly increasing + /// ISO8601 `lastSavedTimeStamp`s. The selection must return the + /// most-recent one (track index 2, the newest stamp). + /// + /// This kills BOTH sort mutants on `candidates.sorted { $0.1 > $1.1 }`: + /// - `>` → `<` (reverse sort) would return the OLDEST bookmark + /// (track 0) — assertion on track index 2 fails. + /// - `>` → `>=` (tie-break flip) is covered by the distinct-stamp + /// ordering plus the tie test below. + func testSelectMostRecentValidBookmark_picksNewestByTimestamp() { + let locations = [ + makeBookmarkLocation(track: tracks.tracks[0], timestamp: 10, savedAt: "2026-01-01T00:00:00Z"), + makeBookmarkLocation(track: tracks.tracks[2], timestamp: 30, savedAt: "2026-03-01T00:00:00Z"), + makeBookmarkLocation(track: tracks.tracks[1], timestamp: 20, savedAt: "2026-02-01T00:00:00Z"), + ] + + let selected = sut.selectMostRecentValidBookmark(from: locations, in: toc) + + XCTAssertNotNil(selected, "Three valid bookmarks must yield a selection") + XCTAssertEqual(selected?.track.key, tracks.tracks[2].key, + "Must select the newest bookmark (2026-03 → track index 2). A reverse sort would pick track 0; a wrong tie-break would not pick this one deterministically.") + } + + /// The newest bookmark wins even when the array is supplied OLDEST-first, + /// so the result reflects timestamp ordering, not array order. A reverse + /// sort would return the first (oldest) element instead. + func testSelectMostRecentValidBookmark_ignoresArrayOrder_usesTimestamp() { + let locations = [ + makeBookmarkLocation(track: tracks.tracks[0], timestamp: 10, savedAt: "2025-01-01T00:00:00Z"), + makeBookmarkLocation(track: tracks.tracks[1], timestamp: 20, savedAt: "2027-12-31T23:59:59Z"), + ] + + let selected = sut.selectMostRecentValidBookmark(from: locations, in: toc) + XCTAssertEqual(selected?.track.key, tracks.tracks[1].key, + "Newest (2027) must win regardless of position in the input array") + } + + // MARK: - selectMostRecentValidBookmark — validation filter (line ~1297) + + /// Mix of valid and invalid bookmarks where BOTH reconstruct successfully + /// (in-manifest track keys) so the discriminator is the + /// `validationFailure(...) == nil` FILTER, not the upstream + /// `TrackPosition(audioBookmark:)` reconstruction guard. The invalid one + /// has the NEWEST timestamp but a position past the duration cap + /// (`.positionExceedsCap`), so the filter must drop it and the older VALID + /// bookmark is selected. + /// + /// Kills the `== nil` → `!= nil` mutant on the in-filter + /// `validationFailure(...) == nil`: under the mutant the filter inverts — + /// it KEEPS the cap-exceeding candidate and DROPS the valid one — so the + /// newest-but-invalid bookmark would be selected and this assertion flips. + func testSelectMostRecentValidBookmark_dropsCapExceedingCandidate_keepsValidOlder() { + // Position far past the end of the book on the last track → durationToSelf + // exceeds totalDuration * 1.1 → .positionExceedsCap. Key is valid, so it + // reconstructs and reaches the validationFailure filter. + let lastTrack = tracks.tracks[tracks.tracks.count - 1] + let capExceedingNewer = makeBookmarkLocation( + track: lastTrack, + timestamp: 10_000_000, // ~115 days into a single track — way past the cap + savedAt: "2099-01-01T00:00:00Z" + ) + let validOlder = makeBookmarkLocation( + track: tracks.tracks[0], + timestamp: 10, + savedAt: "2026-01-01T00:00:00Z" + ) + + let selected = sut.selectMostRecentValidBookmark( + from: [capExceedingNewer, validOlder], + in: toc + ) + + XCTAssertNotNil(selected, + "One valid candidate remains after filtering → must select it") + XCTAssertEqual(selected?.track.key, tracks.tracks[0].key, + "The newest candidate exceeds the duration cap and MUST be filtered out by validationFailure; the valid (older) bookmark is selected. A filter inversion would keep the cap-exceeding one.") + XCTAssertEqual(selected?.timestamp, 10, + "Selected position must be the valid older bookmark's offset, not the cap-exceeding one") + } + + /// When EVERY candidate fails validation (here: position past the duration + /// cap, valid key so it reconstructs and reaches the filter), the filter + /// removes all of them and selection returns nil. Exercises the + /// `validationFailure == nil` filter on the all-fail path. + func testSelectMostRecentValidBookmark_allFailValidation_returnsNil() { + let lastTrack = tracks.tracks[tracks.tracks.count - 1] + let capExceeding = makeBookmarkLocation( + track: lastTrack, + timestamp: 10_000_000, + savedAt: "2026-05-05T00:00:00Z" + ) + + let selected = sut.selectMostRecentValidBookmark(from: [capExceeding], in: toc) + XCTAssertNil(selected, + "Only candidate exceeds the duration cap → filtered out → no fallback position") + } + + // MARK: - isUserAuthenticated — auth-doc load failure (return false, line ~1517) + + /// When the current account's auth-document load has FAILED + /// (`.detailsFailed`), `awaitReady()` throws and the manager must surface + /// the patron as not-authenticated (→ `.notAuthenticated` open error). + /// Pins the `catch { return false }` branch: the `return false` → `return + /// true` mutant would report the patron authenticated despite an auth-doc + /// load failure, opening the book against a stale/absent auth surface. + func testIsUserAuthenticated_authDocLoadFailed_returnsFalse() async { + let manager = makeIsolatedManager() + let account = makeAccount(uuid: "pp4542-failed-auth") + account._setState(.detailsFailed(.authDocumentFetchFailed(underlyingDescription: "HTTP 503"))) + manager.currentAccount = account + + let container = makeTestAppContainer(accountsManager: manager, bookRegistry: registryMock) + let sutWithFailedAuth = AudiobookSessionManager(appContainer: container) + + let authed = await sutWithFailedAuth.isUserAuthenticated() + XCTAssertFalse(authed, + "A failed auth-document load must surface as not-authenticated (the open path maps this to .notAuthenticated)") + + manager.cancelBackgroundWork() + } + + /// Control case: when there is no current account at all, the patron is + /// likewise not authenticated. Together with the failure test above this + /// pins that the not-authenticated outcome is reached on both early-out + /// `return false` paths in the method. + func testIsUserAuthenticated_noCurrentAccount_returnsFalse() async { + let manager = makeIsolatedManager() + manager.currentAccount = nil + + let container = makeTestAppContainer(accountsManager: manager, bookRegistry: registryMock) + let sutNoAccount = AudiobookSessionManager(appContainer: container) + + let authed = await sutNoAccount.isUserAuthenticated() + XCTAssertFalse(authed, "No current account → not authenticated") + + manager.cancelBackgroundWork() + } + + // MARK: - Account / manager fixtures + + /// Build an isolated AccountsManager via the whitelisted test factory + /// rather than a bare `AccountsManager()`. `makeTestAppContainer` pins + /// `deferInitialLoadCatalogsForTesting` before construction, so no + /// background `loadCatalogs` Task is spawned (nothing to outlive the + /// test). Keeps the SUT's `@MainActor` setUp wiring intact (this class + /// can't inherit the nonisolated `PalaceWiringTestCase` seam) while + /// satisfying the AccountsManager isolation lint. + private func makeIsolatedManager() -> AccountsManager { + makeTestAppContainer(bookRegistry: registryMock).accountsManager + } + + private func makeAccount(uuid: String) -> Account { + let metadata = OPDS2Publication.Metadata( + updated: Date(), + description: "pp4542 test library", + id: uuid, + title: "PP-4542 Test Library" + ) + let pub = OPDS2Publication(links: [], metadata: metadata, images: nil) + return Account(publication: pub, imageCache: MockImageCache()) + } +} diff --git a/PalaceTests/Book/BookRegistrySyncTests.swift b/PalaceTests/Book/BookRegistrySyncTests.swift index 888d1432c..3dce126a2 100644 --- a/PalaceTests/Book/BookRegistrySyncTests.swift +++ b/PalaceTests/Book/BookRegistrySyncTests.swift @@ -10,6 +10,7 @@ // import XCTest +import PalaceCatalog @testable import Palace final class BookRegistrySyncTests: XCTestCase { @@ -413,28 +414,151 @@ final class BookRegistrySyncTests: XCTestCase { XCTAssertFalse(exists) } + // MARK: - sync: account-fixture injection helper + // + // F-003 / PP-4542: the PP-4407 awaitReady() migration changed sync()'s + // contract for the "current account present but no loansUrl" case. The + // OLD synchronous guard `guard let loansUrl = currentAccount.loansUrl + // else { return }` was a clean no-op; the NEW code reaches setState(.loaded) + // + completion(nil,false) — either synchronously (no stored credentials) or + // after awaitReady() resolves to AccountDetails with a nil loansUrl + // (anonymous library). The two tests below previously dodged this with a + // `try XCTSkipIf(currentAccount?.loansUrl != nil)` band-aid, so they + // false-greened on a signed-in CI sim and only ran in a clean env. They + // now INJECT a fixture account (unique UUID → host sign-in state is + // irrelevant) and assert the real state sequence. + + /// Seeds a fresh-UUID fixture Account as the injected manager's + /// `currentAccount`. The UUID is unique per call, so the production-stack + /// credentials lookup (`TPPUserAccount.sharedAccount(libraryUUID:)` → + /// `AppContainer.production().accountsManager.userAccount(for:)`) returns a + /// never-signed-in instance: `hasCredentials() == false`, deterministically, + /// regardless of what account the host simulator has signed in. + /// + /// - Returns: the fixture's UUID and a cleanup closure (removes the seeded + /// account + restores the prior currentAccountId). Defer-call it. + private func seedFixtureCurrentAccount() -> (uuid: String, cleanup: () -> Void) { + let fixtureId = "brs-pp4542-\(UUID().uuidString)" + let pub = OPDS2Publication( + links: [OPDS2Link(href: "https://example.com/catalog", + rel: "http://opds-spec.org/catalog")], + metadata: OPDS2Publication.Metadata(id: fixtureId, title: "PP-4542 Fixture"), + images: nil + ) + let fixture = Account(publication: pub, imageCache: MockImageCache()) + let cleanup = accountsManager._seedAccountForTesting(fixture) + return (fixtureId, cleanup) + } + + /// Builds an `AccountDetails` whose auth document has NO "shelf" link, so + /// `details.loansUrl == nil` — the anonymous-library shape that drives + /// sync()'s post-awaitReady `.loaded` branch. + private func makeNoLoansUrlDetails(uuid: String) -> AccountDetails { + let json: [String: Any] = [ + "id": "urn:uuid:\(uuid)", + "title": "Anonymous Library (no shelf link)", + "authentication": [[ + "type": "http://librarysimplified.org/rel/auth/anonymous", + "inputs": ["login": ["keyboard": "Default"], + "password": ["keyboard": "Default"]], + "labels": ["login": "Login", "password": "Password"] + ]], + "features": ["enabled": [], "disabled": []] + ] + let data = try! JSONSerialization.data(withJSONObject: json) + let doc = try! OPDS2AuthenticationDocument.fromData(data) + return AccountDetails(authenticationDocument: doc, uuid: uuid) + } + // MARK: - sync: re-entrancy guard - func test_sync_whenAlreadySyncing_returnsWithoutChangingState() throws { - // If currentState is .syncing, sync() should short-circuit. - // Same simulator-sign-in caveat as test_sync_withNoCurrentAccount_isNoOp: - // when A1QA (or any account) is signed in on the host simulator, - // the test accountsManager.currentAccount?.loansUrl is - // set, and sync() proceeds past the .syncing guard to invoke setState. - // Skip in environments where a current account is present — the - // re-entrancy guard's setState-suppression is only observable in a - // clean environment. - try XCTSkipIf(accountsManager.currentAccount?.loansUrl != nil, - "Skipping: simulator has an active currentAccount; this test requires a clean environment") + func test_sync_whenAlreadySyncing_returnsBeforeSettingSyncingState() throws { + // The `if currentState == .syncing { return }` guard sits AFTER the + // credentials gate, so it is only REACHABLE once hasCredentials() is + // true. We therefore give the fixture stored credentials (on the exact + // production-manager instance sync() will consult) and drive its + // awaitReady() state to a terminal .detailsLoaded with a nil loansUrl. + // Keychain-gated because credential writes need the entitlement (CI + // sims without it skip — this is an environment-capability guard, NOT + // a host-sign-in-state dodge). + try KeychainAvailability.skipIfUnavailable() + + let (uuid, cleanup) = seedFixtureCurrentAccount() + defer { cleanup() } + + // Credentials must live on the instance sync() reads: + // sharedAccount(libraryUUID:) → production manager's userAccount(for:). + let prodUserAccount = AppContainer.production().accountsManager.userAccount(for: uuid) // MIGRATED-DEFERRED: PP-4542 — sync() checks hasCredentials() via sharedAccount→production keychain-backed userAccount; the credential path has no DI seam (only currentAccount STATE is injected) + prodUserAccount.setAuthToken("pp4542-token", barcode: "bc", pin: "1234", + expirationDate: Date().addingTimeInterval(3600)) + defer { prodUserAccount.removeAll() } + XCTAssertTrue(prodUserAccount.hasCredentials(), + "Precondition: fixture has stored credentials so sync() reaches the .syncing guard") + + // Drive awaitReady() to a terminal so the Task branch (if reached) does + // not hang the test. loansUrl is nil → if the guard were absent, sync() + // would setState(.syncing) here. + accountsManager.currentAccount?._setState(.detailsLoaded(makeNoLoansUrlDetails(uuid: uuid))) var received: [TPPBookRegistry.RegistryState] = [] + var completed = false let setState: (TPPBookRegistry.RegistryState) -> Void = { received.append($0) } - syncManager.sync(currentState: .syncing, setState: setState, completion: nil) + // Already-.syncing: the re-entrancy guard must short-circuit BEFORE + // setState(.syncing). We allow the (defensive) async Task to settle, then + // assert no state was ever emitted. + let exp = expectation(description: "sync re-entrancy settled") + syncManager.sync(currentState: .syncing, setState: setState) { _, _ in completed = true } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.4) { exp.fulfill() } + wait(for: [exp], timeout: 2.0) XCTAssertTrue(received.isEmpty, - "sync() called while already .syncing must not invoke setState") - XCTAssertNil(syncManager.syncUrl) + "sync() called while already .syncing must short-circuit before setState — got \(received)") + XCTAssertFalse(completed, + "completion must not fire for the re-entrancy short-circuit") + XCTAssertNil(syncManager.syncUrl, + "syncUrl must not be captured for a short-circuited re-entrant sync") + } + + func test_sync_whenNotSyncing_withCredentialsAndNoLoansUrl_resolvesToLoaded() throws { + // Contrast to the re-entrancy test: SAME credentialed fixture with a + // nil loansUrl, but currentState is .loaded (not .syncing). Now the + // re-entrancy guard does NOT fire, so sync() proceeds into the Task, + // awaits readiness, sees no loansUrl, and resolves to setState(.loaded) + // + completion(nil,false). This proves the .syncing short-circuit above + // is what suppresses setState — not the mere presence of credentials. + try KeychainAvailability.skipIfUnavailable() + + let (uuid, cleanup) = seedFixtureCurrentAccount() + defer { cleanup() } + + let prodUserAccount = AppContainer.production().accountsManager.userAccount(for: uuid) // MIGRATED-DEFERRED: PP-4542 — sync() checks hasCredentials() via sharedAccount→production keychain-backed userAccount; the credential path has no DI seam (only currentAccount STATE is injected) + prodUserAccount.setAuthToken("pp4542-token", barcode: "bc", pin: "1234", + expirationDate: Date().addingTimeInterval(3600)) + defer { prodUserAccount.removeAll() } + + accountsManager.currentAccount?._setState(.detailsLoaded(makeNoLoansUrlDetails(uuid: uuid))) + + var received: [TPPBookRegistry.RegistryState] = [] + var completionArgs: (errorDoc: [AnyHashable: Any]?, newBooks: Bool)? + let setState: (TPPBookRegistry.RegistryState) -> Void = { received.append($0) } + + let exp = expectation(description: "sync resolved") + syncManager.sync(currentState: .loaded, setState: setState) { errorDoc, newBooks in + completionArgs = (errorDoc, newBooks) + exp.fulfill() + } + wait(for: [exp], timeout: 3.0) + + // sync() sets .syncing, then awaitReady resolves with no loansUrl → .loaded. + XCTAssertEqual(received.last, .loaded, + "no-loansUrl account must end in .loaded after awaitReady, not stay .syncing — got \(received)") + XCTAssertNil(completionArgs?.errorDoc, + "anonymous/no-loansUrl resolution is not an error — errorDocument must be nil") + XCTAssertEqual(completionArgs?.newBooks, false, + "no loans were fetched, so newBooks must be false") + XCTAssertNil(syncManager.syncUrl, + "syncUrl must be cleared (never set) when there is no loansUrl to sync") } // MARK: - sync: load-completion guard @@ -482,21 +606,43 @@ final class BookRegistrySyncTests: XCTestCase { XCTAssertFalse(completed) } - func test_sync_withNoCurrentAccount_isNoOp() throws { - // This test is sensitive to simulator sign-in state: when A1QA is signed in - // on the host simulator, the test accountsManager.currentAccount?.loansUrl is - // set (not nil), so sync() proceeds instead of short-circuiting. Skip in - // environments where a current account is present — the no-op behavior is - // only observable in a clean environment. - try XCTSkipIf(accountsManager.currentAccount?.loansUrl != nil, - "Skipping: simulator has an active currentAccount; this test requires a clean environment") + func test_sync_withCurrentAccountButNoCredentials_resolvesToLoadedSynchronously() { + // F-003 / PP-4542. NEW contract: with a current account whose + // production-stack user-account has NO stored credentials, sync() takes + // the credentials gate — setState(.loaded) + completion(nil,false) — + // and returns BEFORE touching awaitReady/the loans feed. This replaces + // the old `try XCTSkipIf(currentAccount?.loansUrl != nil)` dodge: the + // fixture's UUID is unique, so the production manager returns a + // never-signed-in TPPUserAccount and hasCredentials() is false + // regardless of host sign-in state. + // + // No keychain guard needed: we assert the ABSENCE of credentials, and a + // fresh-UUID instance has none whether or not the keychain is writable. + let (uuid, cleanup) = seedFixtureCurrentAccount() + defer { cleanup() } + + let prodUserAccount = AppContainer.production().accountsManager.userAccount(for: uuid) // MIGRATED-DEFERRED: PP-4542 — sync() checks hasCredentials() via sharedAccount→production keychain-backed userAccount; the credential path has no DI seam (only currentAccount STATE is injected) + XCTAssertFalse(prodUserAccount.hasCredentials(), + "Precondition: fresh-UUID fixture must have no stored credentials") var received: [TPPBookRegistry.RegistryState] = [] + var completionArgs: (errorDoc: [AnyHashable: Any]?, newBooks: Bool)? let setState: (TPPBookRegistry.RegistryState) -> Void = { received.append($0) } - syncManager.sync(currentState: .loaded, setState: setState, completion: nil) + syncManager.sync(currentState: .loaded, setState: setState) { errorDoc, newBooks in + completionArgs = (errorDoc, newBooks) + } - XCTAssertTrue(received.isEmpty) - XCTAssertNil(syncManager.syncUrl) + // The credentials-gate path is synchronous — no awaiting needed. + XCTAssertEqual(received, [.loaded], + "no-credentials sync must emit exactly one setState(.loaded) — got \(received)") + XCTAssertNotNil(completionArgs, + "completion must fire synchronously for the no-credentials gate") + XCTAssertNil(completionArgs?.errorDoc, + "deferring sync for missing credentials is not an error") + XCTAssertEqual(completionArgs?.newBooks, false, + "no loans fetched → newBooks must be false") + XCTAssertNil(syncManager.syncUrl, + "syncUrl must never be captured when the credentials gate defers the sync") } } diff --git a/PalaceTests/MyBooks/BookReturnServiceTests.swift b/PalaceTests/MyBooks/BookReturnServiceTests.swift index 620f1c527..058095381 100644 --- a/PalaceTests/MyBooks/BookReturnServiceTests.swift +++ b/PalaceTests/MyBooks/BookReturnServiceTests.swift @@ -487,8 +487,112 @@ final class BookReturnServiceTests: XCTestCase { await blocker.unblock(throwing: NSError(domain: "test", code: -1)) } + // MARK: - F-008 / PP-4542 — browser-vs-basic markCredentialsStale branch (line 476) + // + // Legacy (no-coordinator) return-auth-error path: + // `needsBrowserReauth = (authDef?.isBrowserBased == true) && hasCredentials` + // at BookReturnService.swift:476. When TRUE the service marks credentials + // stale BEFORE dispatching reauth (so a stale SAML/OIDC bearer isn't + // silently reused on retry); when FALSE it does not. The observable + // difference is `markCredentialsStale()` → authState flips to + // `.credentialsStale`. The pair below stages IDENTICAL pre-state (creds + // present, logged in, invalid-credentials revoke error) differing ONLY in + // auth-def browser-ness, and asserts OPPOSITE authState outcomes. Flipping + // `== true` to `!= true` at :476 swaps them. + // + // The setUp service has NO coordinator, so this legacy branch is live. + + /// SAML (browser-based) + credentials + invalid-credentials revoke error + /// must mark credentials stale before reauth (line 476 true branch). + /// + /// Kills :476 `isBrowserBased == true`→`!= true`: under the mutant the + /// SAML account evaluates `isBrowserBased != true` == false → + /// `needsBrowserReauth` false → markCredentialsStale is SKIPPED → authState + /// stays `.loggedIn`, failing the assertion below. + func testReturnBook_SAMLBrowserAuth_invalidCredentials_marksCredentialsStaleBeforeReauth() async throws { + let bookWithRevoke = makeBookWithRevokeURL() + registry.addBook(bookWithRevoke, location: nil, state: .downloadSuccessful, + fulfillmentId: nil, readiumBookmarks: nil, genericBookmarks: nil) + + let problemDoc = try makeProblemDoc(type: TPPProblemDocument.TypeInvalidCredentials) + // First fetch: invalid-credentials (drives the :476 branch). Second + // fetch (retry after reauth): nil → "feed has no entries" terminal + // path, so the recursion ends instead of looping. + feedFetcher.errorThenSuccess = [ + NSError(domain: "test", code: 401, userInfo: ["problemDocument": problemDoc]), + nil + ] + + userAccount._authDefinition = makeAuth(typeRaw: "http://librarysimplified.org/authtype/SAML-2.0") + userAccount._credentials = .barcodeAndPin(barcode: "b", pin: "p") + userAccount.setAuthState(.loggedIn) + XCTAssertEqual(userAccount.authState, .loggedIn, "pre-state: logged in") + // Reauth completion keeps the (still-present) credentials so the retry + // recursion fires the second fetch — but does NOT markLoggedIn, so the + // `.credentialsStale` state set by the :476 branch survives for the + // assertion below. + reauthenticator.onAuthenticate = { _, _ in /* credentials retained */ } + + let exp = expectation(description: "completion") + service.returnBook(withIdentifier: bookWithRevoke.identifier) { exp.fulfill() } + await fulfillment(of: [exp], timeout: 3.0) + + XCTAssertEqual(userAccount.authState, .credentialsStale, + "SAML browser-based return auth error must markCredentialsStale (:476 true branch). " + + "The `!= true` mutant would skip this and leave authState .loggedIn.") + XCTAssertTrue(reauthenticator.authenticateIfNeededCalled, + "Browser-based return auth error still dispatches reauth after marking stale.") + } + + /// Basic (NON-browser) + credentials + invalid-credentials revoke error + /// must NOT mark credentials stale (line 476 false branch) — basic auth + /// re-prompts in-app, the existing bearer is not a browser session to + /// invalidate. Negative control for the pair. + /// + /// Kills :476 `isBrowserBased == true`→`!= true` from the other side: + /// under the mutant a basic account evaluates `isBrowserBased != true` + /// == true → `needsBrowserReauth` true → markCredentialsStale fires → + /// authState becomes `.credentialsStale`, failing the assertion below. + func testReturnBook_basicAuth_invalidCredentials_doesNotMarkCredentialsStale() async throws { + let bookWithRevoke = makeBookWithRevokeURL() + registry.addBook(bookWithRevoke, location: nil, state: .downloadSuccessful, + fulfillmentId: nil, readiumBookmarks: nil, genericBookmarks: nil) + + let problemDoc = try makeProblemDoc(type: TPPProblemDocument.TypeInvalidCredentials) + // First fetch: invalid-credentials. Second fetch (retry): nil → + // terminal "no entries" path so the recursion ends. + feedFetcher.errorThenSuccess = [ + NSError(domain: "test", code: 401, userInfo: ["problemDocument": problemDoc]), + nil + ] + + userAccount._authDefinition = makeAuth(typeRaw: "http://opds-spec.org/auth/basic") + userAccount._credentials = .barcodeAndPin(barcode: "b", pin: "p") + userAccount.setAuthState(.loggedIn) + reauthenticator.onAuthenticate = { _, _ in /* credentials retained */ } + + let exp = expectation(description: "completion") + service.returnBook(withIdentifier: bookWithRevoke.identifier) { exp.fulfill() } + await fulfillment(of: [exp], timeout: 3.0) + + XCTAssertEqual(userAccount.authState, .loggedIn, + "Basic (non-browser) return auth error must NOT markCredentialsStale (:476 false branch). " + + "The `!= true` mutant would mark it stale and flip authState to .credentialsStale.") + XCTAssertTrue(reauthenticator.authenticateIfNeededCalled, + "Basic return auth error still dispatches reauth (just without the stale-marking).") + } + // MARK: - Helpers + private func makeAuth(typeRaw: String) -> AccountDetails.Authentication { + let json = #"{"type": "\#(typeRaw)"}"# + let docAuth = try! JSONDecoder().decode( + OPDS2AuthenticationDocument.Authentication.self, + from: Data(json.utf8) + ) + return AccountDetails.Authentication(auth: docAuth) + } + private func makeBookWithRevokeURL() -> TPPBook { // TPPBookMocker doesn't expose a revokeURL knob — drop down to the // designated TPPBook init (matching PalaceTests/TPPBookMock.swift) diff --git a/PalaceTests/MyBooks/BorrowOperationTests.swift b/PalaceTests/MyBooks/BorrowOperationTests.swift index 96616326d..4ff5be831 100644 --- a/PalaceTests/MyBooks/BorrowOperationTests.swift +++ b/PalaceTests/MyBooks/BorrowOperationTests.swift @@ -416,6 +416,64 @@ final class BorrowOperationTests: XCTestCase { "SQ-007 path clears the spinner") } + // MARK: - F-008 / PP-4542 — browser-vs-basic reauth branch (line 636) + // + // `needsBrowserReauth = (authDef?.isBrowserBased == true) && hasCredentials` + // at BorrowOperation.swift:636 decides whether an authenticated 401 + // routes to the browser re-auth flow (SAML/OIDC/OAuth → sign-in modal, + // NO error alert) or falls through to `.showGenericError` (alert, no + // modal). The two tests below are a discriminating pair: SAME pre-state + // (creds present, .unregistered registry state, invalid-credentials + // problem doc) differing ONLY in the auth-def's browser-ness — and they + // assert OPPOSITE observable outcomes. Flipping `== true` to `!= true` + // at :636 swaps the two outcomes, breaking both tests. + // + // The basic-auth half is `testBorrow_StateUnregistered_isNotTreatedAsAlreadyHavingLoan` + // (basic + creds + .unregistered → alert, no modal). This is the SAML + // half. + + /// SAML (browser-based) + credentials + invalid-credentials 401 on an + /// `.unregistered` book must route to the browser re-auth flow: the + /// sign-in modal is presented and NO borrow-error alert fires. + /// + /// Kills :636 `isBrowserBased == true`→`!= true`: under the mutant SAML + /// evaluates `isBrowserBased != true` == false → `needsBrowserReauth` + /// false → with creds present the `else if !hasCredentials` arm is also + /// false → `.showGenericError` → an ALERT fires and the modal does NOT + /// — both assertions below flip. + func testBorrow_SAMLBrowserAuth_withCredentials_routesToReauthModalNotAlert() async { + userAccount._credentials = .barcodeAndPin(barcode: "b", pin: "p") + userAccount.setAuthState(.loggedIn) + userAccount._authDefinition = SyntheticAuthDef.saml + + // .unregistered registry state so SQ-007 (already-has-loan) does NOT + // fire and suppress the path — we want the live browser-reauth branch. + let problemDoc = Self.makeProblemDoc(type: TPPProblemDocument.TypeInvalidCredentials) + fetchBookResult = .failure(NSError( + domain: "test", code: 401, + userInfo: ["problemDocument": problemDoc as Any] + )) + + do { + _ = try await operation.borrowAsync(book, attemptDownload: false) + XCTFail("SAML auth-error borrow must rethrow") + } catch { + // expected + } + + for _ in 0..<5 { + try? await Task.sleep(nanoseconds: 30_000_000) + await Task.yield() + } + + XCTAssertEqual(signInModalCompletions.count, 1, + "SAML browser-based account + creds must route to the browser re-auth modal " + + "(needsBrowserReauth branch at :636). A `!= true` mutant would skip this and alert instead.") + XCTAssertEqual(alertCalls.count, 0, + "Browser re-auth path must NOT surface a borrow-error alert. A `!= true` mutant at " + + ":636 would fall through to .showGenericError and fire the alert.") + } + // MARK: - Helpers (items #7 / #8) /// Synthesizes a minimal problem document with the given type. @@ -449,6 +507,26 @@ final class BorrowOperationTests: XCTestCase { ) return AccountDetails.Authentication(auth: docAuth) } + + /// SAML (browser-based) auth def. `isBrowserBased == true`, + /// `isSaml == true`. Drives the `needsBrowserReauth` branch at + /// BorrowOperation.swift:636. + static var saml: AccountDetails.Authentication { + let json = """ + { + "type": "http://librarysimplified.org/authtype/SAML-2.0", + "description": "SAML", + "links": [ + {"rel": "authenticate", "href": "https://idp.example.com/saml"} + ] + } + """ + let docAuth = try! JSONDecoder().decode( + OPDS2AuthenticationDocument.Authentication.self, + from: Data(json.utf8) + ) + return AccountDetails.Authentication(auth: docAuth) + } } // MARK: - Helpers diff --git a/PalaceTests/MyBooks/DownloadAuthRetryHandlerTests.swift b/PalaceTests/MyBooks/DownloadAuthRetryHandlerTests.swift index bb1fea9dc..094dea13c 100644 --- a/PalaceTests/MyBooks/DownloadAuthRetryHandlerTests.swift +++ b/PalaceTests/MyBooks/DownloadAuthRetryHandlerTests.swift @@ -155,6 +155,136 @@ final class DownloadAuthRetryHandlerTests: XCTestCase { } } + // MARK: - Foreign-host guard (F-006 / PP-4542): line 234 `statusCode == 401` + // + line 240 `return false` mutants. + // + // The guard at DownloadAuthRetryHandler.swift:234-241 short-circuits a + // 401 whose originating host is OUTSIDE the current account's auth + // surface (PR #1018 cross-host regression — a biblioboard/Icarus 401 + // is not our account's session expiry). Two surviving mutants: + // :234 `statusCode == 401` → `!= 401` — must distinguish 401 vs non-401 + // :240 `return false` → `return true` — the short-circuit's value + // + // To exercise the guard the handler must be built WITH a + // `currentAccountHostsProvider`; the default setUp handler has none + // (guard disabled = legacy). These tests build a guarded handler. + + /// Builds a handler whose foreign-host guard is active for `accountHosts`. + private func makeGuardedHandler(accountHosts: Set) -> DownloadAuthRetryHandler { + let h = DownloadAuthRetryHandler( + stateManager: stateManager, + bookRegistry: registry, + reauthenticator: reauthenticator, + alertPresenter: alertPresenter, + userAccountProvider: { [unowned self] in self.userAccount }, + currentAccountHostsProvider: { accountHosts } + ) + h.delegate = spyDelegate + return h + } + + /// 401 from a host OUTSIDE the current account's auth surface must + /// short-circuit (return false) BEFORE marking credentials stale or + /// dispatching any re-auth — even though the account is a browser-SAML + /// account that WOULD otherwise drive a SAML retry on a same-host 401. + /// + /// Kills :234 `statusCode == 401`→`!= 401`: under the `!=` mutant the + /// guard's status clause is false for this 401, so the guard does NOT + /// fire, the handler falls through to the normal SAML 401 path, marks + /// stale, flips state to `.SAMLStarted` and retries the download — + /// every assertion below flips. + /// Kills :240 `return false`→`return true`: under the `true` mutant the + /// handler claims the failure (handled==true), so the `XCTAssertFalse` + /// on `handled` flips. + func testHandle_401_foreignHost_browserSAML_shortCircuitsReturnsFalse_noReauth() async throws { + userAccount._authDefinition = makeAuth(typeRaw: "http://librarysimplified.org/authtype/SAML-2.0") + userAccount._credentials = .barcodeAndPin(barcode: "b", pin: "p") + XCTAssertEqual(userAccount.authState, .loggedIn, "pre-state: account is logged in") + + registry.addBook(book, location: nil, state: .downloading, + fulfillmentId: nil, readiumBookmarks: nil, genericBookmarks: nil) + + // Account auth surface is minotaur.dev.palaceproject.io; the failing + // 401 task originates from biblioboard.com — a FOREIGN host. + let guarded = makeGuardedHandler(accountHosts: ["minotaur.dev.palaceproject.io"]) + let task = makeFakeTask(statusCode: 401, + url: URL(string: "https://biblioboard.com/loan/x")!) + + let handled = guarded.handleAuthFailureIfApplicable(book: book, task: task, + problemDoc: nil, failureError: nil) + await waitForAsyncCleanup() + + XCTAssertFalse(handled, + "Foreign-host 401 must NOT be claimed — caller falls through (:240 return false).") + XCTAssertEqual(userAccount.authState, .loggedIn, + "Foreign-host 401 must NOT mark credentials stale (guard fires before markCredentialsStale).") + XCTAssertEqual(registry.state(for: book.identifier), .downloading, + "State must stay .downloading — no .SAMLStarted transition for a foreign-host 401.") + XCTAssertTrue(spyDelegate.startDownloadCalls.isEmpty, + "No download retry for a foreign-host 401.") + XCTAssertFalse(reauthenticator.authenticateIfNeededCalled, + "No re-auth dispatch for a foreign-host 401.") + } + + /// Same account + same foreign provider set, but the 401 now comes FROM + /// a host that IS in the account's auth surface. The guard's + /// `statusCode == 401` is true but the host IS contained, so the guard + /// does NOT short-circuit and the normal SAML 401 path runs. This is the + /// positive control that proves the guard is host-scoped, not a blanket + /// 401 suppressor — and it re-confirms :234 must read `== 401` to even + /// reach the host check for a real same-host 401. + func testHandle_401_accountHost_browserSAML_guardDoesNotFire_drivesSAMLRetry() async throws { + userAccount._authDefinition = makeAuth(typeRaw: "http://librarysimplified.org/authtype/SAML-2.0") + userAccount._credentials = .barcodeAndPin(barcode: "b", pin: "p") + + registry.addBook(book, location: nil, state: .downloading, + fulfillmentId: nil, readiumBookmarks: nil, genericBookmarks: nil) + + let guarded = makeGuardedHandler(accountHosts: ["minotaur.dev.palaceproject.io"]) + let task = makeFakeTask(statusCode: 401, + url: URL(string: "https://minotaur.dev.palaceproject.io/loan/x")!) + + let handled = guarded.handleAuthFailureIfApplicable(book: book, task: task, + problemDoc: nil, failureError: nil) + await waitForAsyncCleanup() + + XCTAssertTrue(handled, "Same-host 401 must be claimed and drive the SAML retry path.") + XCTAssertEqual(registry.state(for: book.identifier), .SAMLStarted, + "Same-host SAML 401 flips state to .SAMLStarted (guard did NOT short-circuit).") + XCTAssertEqual(spyDelegate.startDownloadCalls.map { $0.identifier }, [book.identifier], + "Same-host SAML 401 retries the download.") + } + + /// A NON-401 (403) from a foreign host must NOT be short-circuited by the + /// foreign-host guard — the guard is explicitly 401-only. Under the + /// :234 `== 401`→`!= 401` mutant the guard WOULD fire for a 403 from a + /// foreign host (because `403 != 401` is true) and return false early. + /// Here the account is anonymous-free of any 401 path, so the correct + /// behavior for a 403 is the normal fall-through (also false) — to make + /// the mutant observable we use an account that, absent the guard, would + /// take a DIFFERENT action on the non-401 path: no-credentials + + /// loginRequired drives the sign-in modal for ANY status code (Branch 5). + /// The guard must NOT intercept that 403, so the modal must still fire. + func testHandle_403_foreignHost_noCredentials_guardDoesNotIntercept_signInStillFires() { + userAccount._authDefinition = makeAuth(typeRaw: "http://opds-spec.org/auth/basic") + userAccount._credentials = nil + XCTAssertFalse(userAccount.hasCredentials()) + + let guarded = makeGuardedHandler(accountHosts: ["minotaur.dev.palaceproject.io"]) + let task = makeFakeTask(statusCode: 403, + url: URL(string: "https://biblioboard.com/loan/x")!) + + let handled = guarded.handleAuthFailureIfApplicable(book: book, task: task, + problemDoc: nil, failureError: nil) + + XCTAssertTrue(handled, + "A 403 (non-401) must reach the no-creds sign-in branch — the foreign-host " + + "guard is 401-only. The :234 `!= 401` mutant would short-circuit this 403 and " + + "return false, suppressing the sign-in modal.") + XCTAssertTrue(reauthenticator.authenticateIfNeededCalled, + "Sign-in modal must fire for the foreign-host 403 (guard must not intercept).") + } + // MARK: - Branch 1: 401 + hasCredentials + browser SAML → SAMLStarted retry func testHandle_401_withCredentials_browserSAML_setsStateToSAMLStartedAndRetries() async throws { diff --git a/PalaceTests/MyBooks/TokenRefreshInterceptorTests.swift b/PalaceTests/MyBooks/TokenRefreshInterceptorTests.swift index 03c4d1645..1b7b347a4 100644 --- a/PalaceTests/MyBooks/TokenRefreshInterceptorTests.swift +++ b/PalaceTests/MyBooks/TokenRefreshInterceptorTests.swift @@ -402,6 +402,111 @@ final class TokenRefreshInterceptorTests: XCTestCase { XCTAssertTrue(result, "SAML + no-active-loan should be treated as session expiry") } + // MARK: - F-008 / PP-4542 — SAML vs browser legacy-path split (lines 166 & 216) + // + // In the legacy (no-coordinator) re-auth dispatch, `authDefinition?.isSaml + // == true` selects `triggerSAMLReauth` over `triggerBrowserReauth`: + // :166 — the 401 / indicatesAuthenticationNeedsRefresh branch + // :216 — the no-active-loan-as-session-expiry branch + // The two helpers differ OBSERVABLY: + // triggerSAMLReauth → registry state .SAMLStarted, calls startDownload, + // does NOT call the reauthenticator + // triggerBrowserReauth → registry state .downloadNeeded, CALLS the + // reauthenticator (sign-in modal) + // Flipping `== true` to `!= true` at either site routes a SAML account to + // triggerBrowserReauth, so .SAMLStarted/startDownload/no-reauth all flip. + // The interceptor under test is built WITHOUT a coordinator (setUp), so + // these legacy branches are live. + + /// SAML 401 (legacy path, no coordinator) must take `triggerSAMLReauth`: + /// state → .SAMLStarted, download retried via startDownload, and the + /// reauthenticator is NOT invoked (SAML re-auth is driven by the app's + /// own SAML state machine, not the sign-in modal). + /// + /// Kills :166 `isSaml == true`→`!= true`: under the mutant a SAML account + /// routes to `triggerBrowserReauth` instead → state becomes .downloadNeeded + /// AND the reauthenticator IS called — both assertions below flip. + func testHandleDownloadFailure_SAML_401_legacyPath_retriesViaSAMLNotBrowserModal() { + let book = TPPBookMocker.mockBook(distributorType: .EpubZip) + mockRegistry.addBook(book, state: .downloading) + + let url = URL(string: "https://example.com/book")! + let response401 = HTTPURLResponse( + url: url, statusCode: 401, httpVersion: "HTTP/1.1", headerFields: nil + )! + let task = MockURLSessionDownloadTask( + taskIdentifier: 101, request: URLRequest(url: url), response: response401 + ) + + mockUserAccount._credentials = .barcodeAndPin(barcode: "user", pin: "pin") + mockUserAccount._authDefinition = makeAuthDefinition(authType: .saml) + + let result = interceptor.handleDownloadFailureWithAuthCheck( + for: book, task: task, problemDoc: nil, failureError: nil + ) + XCTAssertTrue(result, "SAML 401 must claim the failure") + + // triggerSAMLReauth is async (state-manager cleanup then MainActor hop). + let expectation = XCTestExpectation(description: "SAML retry via startDownload") + let pollItem = DispatchWorkItem { [weak self] in + if let self, !self.mockDelegate.startDownloadCalls.isEmpty { expectation.fulfill() } + } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5, execute: pollItem) + wait(for: [expectation], timeout: 3.0) + pollItem.cancel() + + XCTAssertEqual(mockRegistry.state(for: book.identifier), .SAMLStarted, + "SAML 401 must route to triggerSAMLReauth → .SAMLStarted. The :166 `!= true` " + + "mutant routes to triggerBrowserReauth → .downloadNeeded instead.") + XCTAssertEqual(mockDelegate.startDownloadCalls.count, 1, + "SAML re-auth retries the download via startDownload.") + XCTAssertFalse(mockReauthenticator.authenticateIfNeededCalled, + "SAML path must NOT present the sign-in modal — that's the browser path the " + + ":166 `!= true` mutant would wrongly take.") + } + + /// SAML no-active-loan (legacy path, no coordinator) must take + /// `triggerSAMLReauth` (treats it as a session expiry): state → + /// .SAMLStarted, startDownload called, reauthenticator NOT invoked. + /// + /// Kills :216 `isSaml == true`→`!= true`: the mutant routes the SAML + /// no-active-loan to `triggerBrowserReauth` → .downloadNeeded + reauth. + func testHandleDownloadFailure_SAML_noActiveLoan_legacyPath_retriesViaSAMLNotBrowserModal() { + let book = TPPBookMocker.mockBook(distributorType: .EpubZip) + mockRegistry.addBook(book, state: .downloading) + + let task = fakeDownloadTask() + let problemDoc = TPPProblemDocument.fromDictionary([ + "type": TPPProblemDocument.TypeNoActiveLoan, + "title": "No Active Loan", + "status": 404 + ]) + + mockUserAccount._credentials = .barcodeAndPin(barcode: "user", pin: "pin") + mockUserAccount._authDefinition = makeAuthDefinition(authType: .saml) + + let result = interceptor.handleDownloadFailureWithAuthCheck( + for: book, task: task, problemDoc: problemDoc, failureError: nil + ) + XCTAssertTrue(result, "SAML no-active-loan must be claimed as a session expiry") + + let expectation = XCTestExpectation(description: "SAML no-active-loan retry via startDownload") + let pollItem = DispatchWorkItem { [weak self] in + if let self, !self.mockDelegate.startDownloadCalls.isEmpty { expectation.fulfill() } + } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5, execute: pollItem) + wait(for: [expectation], timeout: 3.0) + pollItem.cancel() + + XCTAssertEqual(mockRegistry.state(for: book.identifier), .SAMLStarted, + "SAML no-active-loan must route to triggerSAMLReauth → .SAMLStarted (:216). " + + "The `!= true` mutant routes to triggerBrowserReauth → .downloadNeeded.") + XCTAssertEqual(mockDelegate.startDownloadCalls.count, 1, + "SAML no-active-loan re-auth retries the download.") + XCTAssertFalse(mockReauthenticator.authenticateIfNeededCalled, + "SAML no-active-loan must NOT present the sign-in modal (that's the :216 mutant path).") + } + // MARK: - handleDownloadFailure with no problem doc and no auth issue func testHandleDownloadFailure_hasCredentials_noLoginRequired_returnsFalse() { diff --git a/scripts/palace_mutate.py b/scripts/palace_mutate.py index 6b535c911..895573cf9 100755 --- a/scripts/palace_mutate.py +++ b/scripts/palace_mutate.py @@ -191,6 +191,20 @@ class Mutation: r"print|NSLog|os_log|Logger\.\w+|os\.Logger\(\)\.\w+)\b" ) +# Test-environment-detection guards (e.g. +# `ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil`) +# are inherently unkillable: the suite ALWAYS runs under XCTest, so flipping +# the detection's comparison produces behavior the tests cannot distinguish +# (or selects a test-only branch). Counting them deflates the kill rate of any +# file that special-cases the test runner — TPPReauthenticator.swift:66 is the +# canonical case (1 mutant, apparent 0% kill, zero real coverage signal). Same +# rationale as _LOG_LINE_PATTERN: skip the host line, mutate everything else. +_TEST_DETECTION_PATTERN = re.compile( + r"XCTestConfigurationFilePath|" + r"environment\[\s*[\"']XCTest|" + r"NSClassFromString\(\s*[\"']XCTest" +) + def changed_lines(file_relpath: str, base_ref: str) -> set[int] | None: """Return the set of line numbers in `file_relpath` (relative to REPO_ROOT) @@ -265,6 +279,10 @@ def offset_to_line(o: int) -> tuple[int, int]: # behavior — they are inherently unkillable. if _LOG_LINE_PATTERN.match(line_text): continue + # Skip test-environment-detection guard lines (see + # _TEST_DETECTION_PATTERN comment) — unkillable by design. + if _TEST_DETECTION_PATTERN.search(line_text): + continue # Build the mutated line by replacing at the column start_in_line = col - 1 end_in_line = start_in_line + len(orig)