ADFA-5361 fix(portal): mint the Calibre-Web session for the WebView, and stop the guest cookie shadowing it - #511
Merged
Conversation
… reconcile the cookie jar The Books card opened as box admin only when the launcher remembered to pass an AUTH_SERVICE extra, and two of the three launchers did not: a local book opened from "your books" (the screen reached right after Get More) and the module action sheet loaded Calibre-Web unauthenticated, leaving a guest session in the shared WebView cookie jar. The injection then only ever set cookies at "path=/", which appends rather than replaces, so a same-name cookie at the served prefix survived and outranked the fresh one — that is how a single guest page load became a permanent guest session. PortalActivity now derives the service from the resolved target URL (AutoLoginPolicy), so no call site can forget it, and clears the service's cookies on every path they can live at before installing the fresh ones (SessionCookies). Both are pure and JVM-unit-tested. A failed sign-in leaves the jar untouched, so a still-valid remember-me is not discarded over a transient failure. Removes the AUTH_SERVICE extra and LibraryHomeFragment.authServiceFor(). No new state, no new strings.
…nd stop shadowing it Opening Books never actually signed you in as the box admin. Three defects stacked: 1. The auto-login service was an Intent extra each launcher had to remember, and two of the three did not (a local book from "your books" — the screen reached right after Get More — and the module action sheet), so those opened Calibre-Web unauthenticated. 2. The session cookies were only ever set at "path=/", which appends rather than replaces: the box fronts Calibre-Web under /books, and a same-name cookie at that deeper path outranks ours in the Cookie header (RFC 6265 5.4) and was never cleared. 3. The session was minted under dash-node's own agent. Flask-Login binds a session to a fingerprint of the User-Agent, so the WebView's first request had it rejected: identity dropped, remember_token deleted, guest session planted at /books. Measured on device — the same cookies render Admin under the minting agent and Guest under the WebView's. The "logged in as Admin" flash renders from the injected session even when anonymous, which is why this looked like it worked. PortalActivity now derives the service from the target URL (AutoLoginPolicy), clears the service's cookies on every path they can live at before installing the fresh ones (SessionCookies), and asks the box to mint the session for the WebView's own User-Agent, which dash-node forwards through the whole login handshake for Calibre-Web and Kolibri. The callers that consume a session themselves keep dash-node's agent. The three land together on purpose: the clear pass alone would destroy the manual sign-in that is currently the only way to administer the library. Removes the AUTH_SERVICE extra and LibraryHomeFragment.authServiceFor(). Pure logic is JVM-unit-tested; an instrumentation test pins the cookie reconciliation against the real WebView store (device-verified). No new state, no new strings. dash-node 1.2.10 -> 1.2.11.
…icate Calibre-Web login The auto-login failure path was invisible and self-poisoning: AuthClient swallowed the exception without a single log line, and PortalActivity just loaded the page, so a failed sign-in looked exactly like a permissions problem in Calibre-Web rather than a sign-in that did not happen. It now logs the real cause (which already distinguishes 401 from 503 from a timeout), retries once for the failures worth retrying, and tells the user, reusing the string the Authentication screen already shows for this fact. Retried are the FAST failures — a refused connection or a 5xx, what a content service that is restarting or saturated by a books job answers in milliseconds — after a short pause, because those are precisely the failures an immediate retry asks again too soon to get a different answer. A timeout is not retried: it already spent the full read budget, and asking again buys the same answer for twice the wait with the overlay on screen. A 4xx is not retried either; it is the box's final word. Same rule the box uses for its own retries (sockets/net-retry.ts). dash-node: the books runner's private copy of the Calibre-Web login is gone; it imports getCalibreSession. Two implementations of one fact had already drifted — the ADFA-5043 remember_me reached one and not the other — which is the drift this whole bug rode in on. The runner consumes its own session, so it passes no consumer User-Agent. No new strings, no new state. dash-node stays at 1.2.11: not REST-facing, so by convention it does not bump, and the change rides in that version's changelog entry.
…nnot reach On a healthy box the UI never reaches AuthClient's retry: a card whose service is down is not Ready, so it opens the action sheet instead of the portal, and Get More hides the entry outright — both device-verified. The retry exists for the narrow race the bug rode in on, so a scripted local server is the only way to hold that case still. Six cases against a socket that answers a scripted sequence: a 5xx is retried and the second answer is used; the retry pauses before asking again (an immediate one would ask a restarting service too soon to get a different answer); a persistent 5xx gives up after exactly one retry; a 401 and a 404 are final and asked only once — the server has a 200 queued behind them that the test requires us never to reach; and a first-try success pays neither a second request nor the pause. Plain JVM, no emulator: fetchCookie touches only HttpURLConnection and Log, so it runs in the normal unit-test task. It drops from private to package-private for this, with a comment saying why.
…sumer, and refuse the task that wipes the box The auto-login of ADFA-5043 never delivered admin: Flask-Login binds a session to a fingerprint of the requesting agent, so one minted by the box was rejected on the WebView's first request, which also deleted the remember cookie and planted an anonymous session under the service's path prefix — where it outranked anything the app installed at the root. The ADR records the measured evidence, the decision (mint for the agent that asks; one owner for "opens as admin"; reconcile the jar), the options rejected, and the consequences left open. It also records the two things that cost the most time: the flash message that renders from the injected session even when the viewer is anonymous, which is why a broken feature looked healthy, and the fact that the failure path is unreachable through the UI, so it is covered by a test rather than by device steps. build.gradle now refuses connectedAndroidTest unless -PallowUninstall=true. That task uninstalls both APKs when it finishes, and on a device holding an installed rootfs that deletes the box — one run destroyed an installed system while this ticket was being written. The safe invocation is in the error message and in the ADR.
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.
Books opened as Guest after adding content through Get more, permanently — the owner could not
manage the books they had just added.
Cause. Flask-Login binds a session to the requesting agent. dash-node minted under its own
agent, so the WebView's first request had it rejected, the
remember_tokendeleted, and ananonymous session planted at
Path=/books, outranking anything the app set at/and nevercleared. The ADFA-5043 auto-login never delivered admin at all; the "logged in as Admin" flash
renders from the injected session even when the viewer is anonymous, which is why it looked fine.
Two launchers also opened Calibre-Web unauthenticated, because the service was an Intent extra
only one of them remembered to pass.
Evidence, options, cookie map and open consequences:
ADR-5361.
Changes
forwards it through the handshake, Calibre-Web and Kolibri).
string, no new l10n).
remember_me, which is the drift this bug rode in on.−1 Intent extra, −3 call sites that had to know, −1 login implementation. No new state or strings.
connectedAndroidTestnow refuses to runIt uninstalls both APKs when it finishes, which on a device holding a rootfs deletes the box —
one run destroyed an installed system while writing this.
build.gradlefails it with the safeinvocation in the message;
-PallowUninstall=trueopts out.Verification — nothing outstanding
:app:testDebugUnitTest+:app:lintDebuggreen (URL→service rule, cookie directives, retryagainst a scripted server). Instrumentation 3/3: a deeper-path cookie outranks the root one and
Max-Age=0deletes at its path. dash-nodeyarn typecheckclean, 91/91 tests, on the box'sown Node 22. On device: all three entry points open as Admin, a full Get more → add → reopen
cycle keeps admin with Edit Metadata and delete working, Courses unaffected, and a forced 401
shows the toast, logs
HTTP 401with no retry line, and recovers. The jar was wiped first, so theresult is attributable.
Follow-ups left open are in ADR §5. dash-node 1.2.11 (the runner dedupe is not REST-facing, so it
rides in the same entry).