fix(auth): an expired token was sent to /patrons/me/, and read back as "signed out" - #1453
Open
mauricecarrier7 wants to merge 1 commit into
Open
fix(auth): an expired token was sent to /patrons/me/, and read back as "signed out"#1453mauricecarrier7 wants to merge 1 commit into
mauricecarrier7 wants to merge 1 commit into
Conversation
…s "signed out" The gate in `getProfileDocument` checked that credentials EXIST. It did not check they are USABLE. `hasCredentials()` is `hasAuthToken || hasBarcodeAndPIN` — presence, not validity — so an expired bearer token passes it. The request goes out, `/patrons/me/` answers 401 with the same OPDS auth-document body the gate exists to avoid, and because this path passes `enableTokenRefresh: false` that 401 cannot self-heal. It returns nil, and callers (NotificationService FCM registration) read nil as "signed out" and re-present sign-in on an account that is fine. Observed on device 2026-09-03, build 499, Icarus Test Library: an app updated over a stale session sent the old token, took a 401, and put the login sheet back up. It cleared on relaunch once a fresh token was stored — which is the signature of a credential that is present but no longer usable, not a missing one. Reported as "login isn't loading", then "it keeps re-appearing, but I'm actually logged in after a restart". This is NOT the PP-5065 defect recurring. That was a 401 body concatenated onto a retry body, breaking XML parsing. This is a clean, well-formed 401 from an independent cause, and it is present in a build that carries the PP-5065 fix. The two share a symptom and nothing else. **The existing test for this gate does not test it.** `AccountProfileDocumentTests` carries an F-007 regression guard whose docstring claims it "kills the gate-removal mutation". Measured: with the ENTIRE credentials gate deleted, all five tests in the file still passed. They assert a nil document and sub-second timing against `https://example.invalid/`, and DNS fails fast enough that both hold whether or not the request was issued. That gate has been protecting nothing since it was written, and the docstring asserted the opposite. The blocker is a seam: `getProfileDocument` reaches `AppContainer.production()` internally, so no test can observe whether the request was sent. Rather than work around it, the decision is lifted into `Account.canAuthenticateProfileRequest(hasCredentials:tokenHasExpired:)` — a pure function over two booleans — and its four cells are asserted directly. Same pattern as `BookDetailView.opensContentDirectly` (PP-5059). Proven to bite: reverting the predicate to presence-only fails `testCanAuthenticate_WithExpiredToken_IsFalse` by name. Not a compile error — a named failing test. 33 tests green across this file plus NotificationServiceStateMachineTests and FCMRegistrationReadinessLintTests. `tokenHasExpired` is false for barcode/PIN credentials and for tokens with no expiry date (`isTokenExpired` returns false for both), so basic-auth libraries are unaffected — pinned by the no-credentials and degenerate cells. **Scope:** the one gate in `Account+profileDocument.swift`, plus its tests. **Not done:** not verified on device. Reproducing needs an app updated over a session whose token has since expired, which is time-dependent rather than scriptable. The unit tests pin the decision, not the live flow. I also did NOT establish what re-presents the login sheet — the 401s are explained, but the link from "profile fetch returned nil" to the sheet re-appearing is inferred from the log ordering, not traced. `Retry borrow failed after OIDC re-auth` and an `ASWebAuthenticationSession` error 3 (presentationContextInvalid, context "Settings Tab") both appear in the same window and neither is attributed. **Deferred:** the F-007 test is left in place and still weak. It is not made worse by this change, and rewriting it needs the `AppContainer.production()` seam opened, which is a larger change than a release-adjacent fix should carry. Its docstring's claim is now contradicted in a comment directly beneath it. A census found ~12 other `hasCredentials()` guards; `TPPMigrationManager:60` already pairs it with an expiry check, so the distinction is known here. The rest were NOT audited for whether they gate an authenticated request with refresh disabled — that is the condition that turns this from early to fatal, and it should be checked before assuming they are safe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🏛️ Architecture Analysis
🔄 Dependency Cycles (2)2 distinct dependency cycles (largest spans 10 components).
Cycle 1 — 10 components, 22 edges between them
edgesCycle 2 — 3 components, 4 edges between them
edgesDiscounted 📋 Architecture Findings
🔍 Reachability Analysis
ℹ️ No architecture-relevant files in this diff — reachability not evaluated. 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ❌ 1 TEST FAILED8469 tests | 8457 passed | 1 failed | 11 skipped | ⏱️ 13m 12s | 📊 99.9% | 📈 49.2% coverage All 971 classes — 1 with failures (full matrix)
📊 Full interactive matrix: report ❌ Failed testsNames only — open the interactive report above for messages + stack frames. 📊 Testing Coverage BreakdownUnit Test Line Coverage (testable surfaces): 49.2% Total coverage incl. UI/lifecycle: 48.0% (17 files excluded from testable denominator — see
📈 What changed vs. baseTest count: -62 🔗 Interactive HTML Report | CI Run Details Counts above were produced by this CI run's xcresult parse — reproduce via the run link. 📦 Downloadable Artifacts
|
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.
What
getProfileDocument's gate checked that credentials exist. It did not check they are usable.hasCredentials()ishasAuthToken || hasBarcodeAndPIN— presence, not validity — so an expired bearer token passes it. The request goes out,/patrons/me/answers 401 with the OPDS auth-document body the gate exists to avoid, and because this path passesenableTokenRefresh: falsethat 401 cannot self-heal. It returns nil, and callers (NotificationService FCM registration) read nil as "signed out" and re-present sign-in on an account that is fine.How it was found
On device 2026-09-03, build 499, Icarus Test Library. An app updated over a stale session sent the old token, took a 401, and put the login sheet back up — then cleared on relaunch once a fresh token was stored. That "fine after restart" behaviour is the signature of a credential that is present but no longer usable, rather than a missing one.
Not PP-5065 recurring. That was a 401 body concatenated onto a retry body, breaking XML parsing. This is a clean, well-formed 401 from an independent cause, in a build that already carries the PP-5065 fix. Same symptom, unrelated mechanism.
The existing test for this gate does not test it
AccountProfileDocumentTestscarries an F-007 regression guard whose docstring says it "kills the gate-removal mutation".Measured: with the entire credentials gate deleted, all five tests still passed. They assert a nil document and sub-second timing against
https://example.invalid/; DNS fails fast enough that both hold whether or not the request was issued.The blocker is a seam —
getProfileDocumentreachesAppContainer.production()internally, so no test can observe whether the request was sent. Rather than work around it, the decision is lifted into a pure function and its four cells asserted directly. Same pattern asBookDetailView.opensContentDirectly(PP-5059).Evidence
Proven to bite. Reverting the predicate to presence-only fails
testCanAuthenticate_WithExpiredToken_IsFalseby name — a named failing test, not a compile error.33 tests green across
AccountProfileDocumentTests+NotificationServiceStateMachineTests+FCMRegistrationReadinessLintTests.Basic auth unaffected:
isTokenExpiredreturns false for barcode/PIN and for tokens with no expiry date. Pinned by the no-credentials and degenerate cells.Risk
Not done: not verified on device — reproducing needs an app updated over a session whose token has since expired, which is time-dependent rather than scriptable. I also did not establish what re-presents the login sheet; the 401s are explained, but the link to the sheet re-appearing is inferred from log ordering, not traced.
Retry borrow failed after OIDC re-authand anASWebAuthenticationSessionerror 3 (presentationContextInvalid, context "Settings Tab") both appear in the same window and neither is attributed.Deferred: the F-007 test is left in place and still weak — not made worse here, and fixing it needs the
AppContainer.production()seam opened. Its docstring's claim is now contradicted in a comment directly beneath it. A census found ~12 otherhasCredentials()guards;TPPMigrationManager:60already pairs it with an expiry check, so the distinction is known in this codebase. The rest were not audited for whether they gate an authenticated request with refresh disabled — the condition that turns this from early to fatal.🤖 Generated with Claude Code