diff --git a/controller/app/build.gradle b/controller/app/build.gradle index ab3e9daba..49c88f130 100644 --- a/controller/app/build.gradle +++ b/controller/app/build.gradle @@ -763,6 +763,31 @@ task refreshKolibriCatalog { } } +// ADFA-5361: `connectedAndroidTest` UNINSTALLS both APKs when it finishes. On a device that holds an +// installed rootfs, uninstalling the app deletes its data directory — that is, the whole box. Observed: +// one run wiped an installed system on a test device. The instrumentation tests are worth keeping, so +// the fix is to refuse the destructive entry point rather than to write a rule someone has to remember. +// Install and run explicitly instead (this installs, runs, and leaves both APKs in place): +// ./gradlew :app:installDebug :app:installDebugAndroidTest +// adb shell am instrument -w -e class org.iiab.controller.test/androidx.test.runner.AndroidJUnitRunner +// A device with nothing to lose can still opt in with -PallowUninstall=true. +tasks.configureEach { t -> + if (t.name.startsWith('connected') && t.name.endsWith('AndroidTest')) { + t.doFirst { + if (!project.hasProperty('allowUninstall')) { + throw new GradleException( + "Refusing to run ${t.name}: it uninstalls the app when it finishes, which on a " + + "device holding an installed rootfs destroys the box (ADFA-5361).\n" + + " Run instead:\n" + + " ./gradlew :app:installDebug :app:installDebugAndroidTest\n" + + " adb shell am instrument -w -e class \\\n" + + " org.iiab.controller.test/androidx.test.runner.AndroidJUnitRunner\n" + + " Or, if this device has nothing to lose: -PallowUninstall=true") + } + } + } +} + // ADFA-4466 / build resilience: apply the Firebase google-services plugin only when its config file is // present. Without it the plugin fails the build; skipping it lets contributors build a working APK with // analytics compiled out (BuildConfig.ANALYTICS_ENABLED = false). Applied at the bottom, as the plugin diff --git a/controller/app/src/androidTest/java/org/iiab/controller/portal/SessionCookieReconcileTest.java b/controller/app/src/androidTest/java/org/iiab/controller/portal/SessionCookieReconcileTest.java new file mode 100644 index 000000000..6278f436e --- /dev/null +++ b/controller/app/src/androidTest/java/org/iiab/controller/portal/SessionCookieReconcileTest.java @@ -0,0 +1,108 @@ +package org.iiab.controller.portal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import android.webkit.CookieManager; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.iiab.controller.portal.domain.AutoLoginPolicy; +import org.iiab.controller.portal.domain.SessionCookies; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * ADFA-5361: on-device proof that the reconcile directives do what the pure tests describe. + * The JVM tests pin the STRINGS; only the real WebView cookie store can say whether + * "Max-Age=0" at a given Path actually deletes, and whether a deeper-path copy outranks the + * one we install — which is the mechanism that turned one guest page load into a permanent + * guest session. + * + *

Uses a test-only origin so it never touches the box's real cookies. + */ +@RunWith(AndroidJUnit4.class) +public class SessionCookieReconcileTest { + + private static final String ORIGIN = "http://cookie-reconcile.test/"; + private static final String FRESH = "session=fresh-admin; remember_token=7|fresh"; + + private CookieManager cm; + + private void wipe() { + for (String name : new String[]{"session", "remember_token"}) { + for (String path : new String[]{"/", "/books", "/books/"}) { + cm.setCookie(ORIGIN, name + "=; Path=" + path + "; Max-Age=0"); + } + } + cm.flush(); + } + + @Before public void setUp() { + cm = CookieManager.getInstance(); + cm.setAcceptCookie(true); + wipe(); + } + + @After public void tearDown() { + wipe(); + } + + /** The stale copy the old code could never replace: same name, deeper path. */ + @Test public void deeperPathCookieOutranksTheRootOne() { + cm.setCookie(ORIGIN, "session=stale-guest; path=/books"); + cm.setCookie(ORIGIN, "session=fresh-admin; path=/"); + cm.flush(); + + String header = cm.getCookie(ORIGIN + "books/"); + assertTrue("both copies are sent: " + header, header.contains("stale-guest")); + assertTrue("both copies are sent: " + header, header.contains("fresh-admin")); + // The service reads the first "session" it is given, and that is the stale one. + assertTrue("stale copy is served first: " + header, + header.indexOf("stale-guest") < header.indexOf("fresh-admin")); + } + + /** The fix: clearing every candidate path first leaves only what we just installed. */ + @Test public void clearThenSetLeavesOnlyTheFreshSession() { + cm.setCookie(ORIGIN, "session=stale-guest; path=/books"); + cm.setCookie(ORIGIN, "session=stale-guest; path=/books/"); + cm.setCookie(ORIGIN, "remember_token=1|stale; path=/books"); + cm.flush(); + + String prefix = AutoLoginPolicy.prefixFor("http://localhost:8085/books/book/12"); + assertEquals("/books", prefix); + for (String directive : SessionCookies.clearDirectives(FRESH, prefix)) { + cm.setCookie(ORIGIN, directive); + } + for (String directive : SessionCookies.setDirectives(FRESH)) { + cm.setCookie(ORIGIN, directive); + } + cm.flush(); + + String header = cm.getCookie(ORIGIN + "books/book/12"); + assertFalse("stale session survived: " + header, header.contains("stale-guest")); + assertFalse("stale remember_token survived: " + header, header.contains("1|stale")); + assertTrue("fresh session missing: " + header, header.contains("session=fresh-admin")); + assertTrue("fresh remember_token missing: " + header, header.contains("remember_token=7|fresh")); + } + + /** Max-Age=0 must delete at the exact path it names, not only at the root. */ + @Test public void clearDeletesAtEveryNamedPath() { + cm.setCookie(ORIGIN, "session=at-root; path=/"); + cm.setCookie(ORIGIN, "session=at-prefix; path=/books"); + cm.setCookie(ORIGIN, "session=at-prefix-slash; path=/books/"); + cm.flush(); + + for (String directive : SessionCookies.clearDirectives("session=x", "/books")) { + cm.setCookie(ORIGIN, directive); + } + cm.flush(); + + String header = cm.getCookie(ORIGIN + "books/"); + assertTrue("expected no session cookie left, got: " + header, + header == null || !header.contains("session=")); + } +} diff --git a/controller/app/src/main/java/org/iiab/controller/PortalActivity.java b/controller/app/src/main/java/org/iiab/controller/PortalActivity.java index 946aae48b..7265ae559 100644 --- a/controller/app/src/main/java/org/iiab/controller/PortalActivity.java +++ b/controller/app/src/main/java/org/iiab/controller/PortalActivity.java @@ -27,11 +27,13 @@ import android.os.Handler; import android.os.Looper; +import org.iiab.controller.portal.domain.AutoLoginPolicy; import org.iiab.controller.portal.domain.NavigationPolicy; import org.iiab.controller.portal.domain.PdfPolicy; import org.iiab.controller.portal.domain.PdfViewerUrl; import org.iiab.controller.portal.domain.PdfViewerBuild; import org.iiab.controller.portal.domain.PdfViewerRouter; +import org.iiab.controller.portal.domain.SessionCookies; import org.iiab.controller.portal.domain.WebViewVersion; import org.iiab.controller.portal.data.PdfViewerCatalog; import org.iiab.controller.util.AppExecutors; @@ -372,8 +374,11 @@ public boolean onConsoleMessage(android.webkit.ConsoleMessage consoleMessage) { // ADFA-5043: Books (Calibre-Web) / Courses (Kolibri) auto-login as box admin — fetch a session // cookie, inject it into the WebView CookieManager, THEN load, so the card opens already // authenticated. Degrades gracefully: if the service isn't installed/ready, just load without it. - String authService = getIntent().getStringExtra("AUTH_SERVICE"); - if (authService != null && !authService.isEmpty()) { + // ADFA-5361: the service is DERIVED from the URL, not passed in by the caller. As an Intent + // extra it was a fact each launcher had to remember, and two of the three did not — those + // opened Calibre-Web unauthenticated and left a guest session in the shared cookie jar. + String authService = AutoLoginPolicy.serviceFor(finalTargetUrl); + if (authService != null) { autoLoginThenLoad(authService, finalTargetUrl); } else { // Native architecture: content is served locally; load it directly. @@ -385,15 +390,29 @@ public boolean onConsoleMessage(android.webkit.ConsoleMessage consoleMessage) { * failure (service absent/not ready) load without a cookie — the card still opens. */ private void autoLoginThenLoad(String service, String targetUrl) { showAuthOverlay(); - org.iiab.controller.redesign.AuthClient.session(service, new org.iiab.controller.redesign.AuthClient.SessionCb() { + // ADFA-5361: the box must mint the session FOR THIS WebView. Calibre-Web binds a session to a + // fingerprint of the User-Agent, so one minted under the box's own agent is rejected on the + // first request here and the card opens as the anonymous Guest. Read verbatim from the very + // WebView that will present it — its only job is to match. + String consumerUa = webView.getSettings().getUserAgentString(); + org.iiab.controller.redesign.AuthClient.session(service, consumerUa, new org.iiab.controller.redesign.AuthClient.SessionCb() { @Override public void onOk(String cookie) { if (isFinishing() || isDestroyed()) return; // ADFA-5043: left mid-sign-in; don't touch dead views android.webkit.CookieManager cm = android.webkit.CookieManager.getInstance(); cm.setAcceptCookie(true); + // ADFA-5361: clear before set, so the jar holds the session we just minted and nothing + // else. Setting alone appends: a same-name cookie at a deeper path (the box fronts the + // service under a prefix) is never overwritten and outranks ours in the Cookie header, + // which is how one guest page load became a permanent guest session. Clear and set are + // one operation — a failed sign-in leaves the jar untouched (see onErr), so a still-valid + // remember-me from an earlier open is not thrown away over a transient failure. + String prefix = AutoLoginPolicy.prefixFor(targetUrl); + for (String directive : SessionCookies.clearDirectives(cookie, prefix)) { + cm.setCookie(BoxEndpoints.BASE + "/", directive); + } // Host-wide on the box origin so every request under the service prefix carries it. - for (String pair : cookie.split(";")) { - String p = pair.trim(); - if (!p.isEmpty()) cm.setCookie(BoxEndpoints.BASE + "/", p + "; path=/"); + for (String directive : SessionCookies.setDirectives(cookie)) { + cm.setCookie(BoxEndpoints.BASE + "/", directive); } cm.flush(); hideAuthOverlay(); @@ -402,6 +421,11 @@ private void autoLoginThenLoad(String service, String targetUrl) { @Override public void onErr() { if (isFinishing() || isDestroyed()) return; // ADFA-5043: left mid-sign-in; don't touch dead views hideAuthOverlay(); + // ADFA-5361: say so. The page still opens — read-only, as the anonymous guest — but + // silently landing there is what made "I can't manage my own books" look like a + // permissions mystery instead of a sign-in that did not happen. Reuses the string the + // Authentication screen already shows for this same fact (no new l10n). + Toast.makeText(PortalActivity.this, R.string.k2go_auth_load_failed, Toast.LENGTH_LONG).show(); webView.loadUrl(targetUrl); } }); diff --git a/controller/app/src/main/java/org/iiab/controller/portal/domain/AutoLoginPolicy.java b/controller/app/src/main/java/org/iiab/controller/portal/domain/AutoLoginPolicy.java new file mode 100644 index 000000000..67a84d781 --- /dev/null +++ b/controller/app/src/main/java/org/iiab/controller/portal/domain/AutoLoginPolicy.java @@ -0,0 +1,102 @@ +/* + * ============================================================================ + * Name : AutoLoginPolicy.java + * Author : AppDevForAll + * Copyright : Copyright (c) 2026 AppDevForAll + * Description : ADFA-5361. Decides, from the URL alone, whether a portal page opens as the box + * admin (Calibre-Web "books" / Kolibri "courses") and under which path prefix the + * box fronts it. Single owner of that fact: ADFA-5043 kept it in a private method of + * one fragment and carried it as an Intent extra, so the two call sites that did not + * pass the extra opened Calibre-Web unauthenticated — which planted a guest session + * in the shared WebView cookie jar. Deriving it here means no call site can forget. + * Pure (no android.*), so it is JVM-unit-testable. + * ============================================================================ + */ +package org.iiab.controller.portal.domain; + +/** + * Maps a portal target URL to the service whose admin session the WebView should carry. + * + *

The service names are the ones the box's credential store uses + * ({@code /k2go-api/auth/<service>/session}), which are NOT the URL segments: Calibre-Web is + * served at {@code /books} but is called {@code calibre}. The path prefix is returned separately + * because the cookie work needs it (see {@link SessionCookies}). + */ +public final class AutoLoginPolicy { + + /** Box credential-store service names. */ + public static final String CALIBRE = "calibre"; + public static final String KOLIBRI = "kolibri"; + + private AutoLoginPolicy() {} + + /** + * The auto-login service for {@code url}, or {@code null} when the page needs no admin + * session. Only pages served by the local box qualify — the session cookie is an admin + * credential and must never be minted for, or injected against, an external host. + */ + public static String serviceFor(String url) { + String segment = firstSegment(url); + if (segment == null) return null; + if (segment.equals("books")) return CALIBRE; + if (segment.equals("kolibri")) return KOLIBRI; + return null; + } + + /** + * The path prefix the box fronts the service under (e.g. {@code "/books"}), or {@code null} + * when the URL has no auto-login service. Derived from the same segment as + * {@link #serviceFor(String)} so the two can never disagree. + */ + public static String prefixFor(String url) { + if (serviceFor(url) == null) return null; + return "/" + firstSegment(url); + } + + /** + * First path segment of an internal-host URL, lowercased; {@code null} if the URL is unusable, + * points at an external host, or has no path segment. + * + *

Hand-parsed rather than via {@code android.net.Uri} because this layer stays pure. + */ + private static String firstSegment(String url) { + if (url == null) return null; + String s = url.trim(); + if (s.isEmpty()) return null; + + int schemeEnd = s.indexOf("://"); + if (schemeEnd < 0) return null; // relative URLs never reach the portal + int authorityStart = schemeEnd + 3; + + int pathStart = indexOfAny(s, authorityStart, '/', '?', '#'); + String authority = pathStart < 0 ? s.substring(authorityStart) : s.substring(authorityStart, pathStart); + if (!NavigationPolicy.isInternalHost(hostOf(authority))) return null; + if (pathStart < 0 || s.charAt(pathStart) != '/') return null; // no path at all + + String path = s.substring(pathStart + 1); + int cut = indexOfAny(path, 0, '/', '?', '#'); + if (cut >= 0) path = path.substring(0, cut); + path = path.trim().toLowerCase(); + return path.isEmpty() ? null : path; + } + + /** Host of an {@code authority} ({@code user@host:port}), without credentials or port. */ + private static String hostOf(String authority) { + String a = authority; + int at = a.lastIndexOf('@'); + if (at >= 0) a = a.substring(at + 1); + int colon = a.lastIndexOf(':'); + if (colon >= 0) a = a.substring(0, colon); + return a; + } + + /** First index at or after {@code from} of any of {@code chars}, or -1. */ + private static int indexOfAny(String s, int from, char... chars) { + for (int i = from; i < s.length(); i++) { + for (char c : chars) { + if (s.charAt(i) == c) return i; + } + } + return -1; + } +} diff --git a/controller/app/src/main/java/org/iiab/controller/portal/domain/SessionCookies.java b/controller/app/src/main/java/org/iiab/controller/portal/domain/SessionCookies.java new file mode 100644 index 000000000..911fcf25b --- /dev/null +++ b/controller/app/src/main/java/org/iiab/controller/portal/domain/SessionCookies.java @@ -0,0 +1,99 @@ +/* + * ============================================================================ + * Name : SessionCookies.java + * Author : AppDevForAll + * Copyright : Copyright (c) 2026 AppDevForAll + * Description : ADFA-5361. Builds the cookie directives that install a freshly minted service + * session in the WebView: first EXPIRE the service's cookies on every path they can + * live at, then set the new ones. ADFA-5043 only ever set them at "path=/", which + * appends rather than replaces: a cookie of the same name at a deeper path (the box + * fronts Calibre-Web under /books) is a different cookie, is never overwritten, and + * wins in the Cookie header (RFC 6265 5.4 orders longer paths first; the service + * reads the first one). That is what turned one guest page load into a permanent + * guest session. Pure string work (no android.*), so it is JVM-unit-testable. + * ============================================================================ + */ +package org.iiab.controller.portal.domain; + +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +/** + * Directive builders for the WebView cookie jar. Both take the {@code Cookie}-style header the box + * returns ({@code "name=value; name2=value2"}) — the names to reconcile come from that response, so + * this class holds no hardcoded knowledge of Calibre-Web's or Kolibri's cookie names. + */ +public final class SessionCookies { + + private SessionCookies() {} + + /** + * Directives that delete the cookies named in {@code cookieHeader} from every path the service + * can have set them at, so the fresh ones cannot be shadowed by a stale copy. + * + * @param prefix the service's path prefix (e.g. {@code "/books"}), or {@code null} for root only + */ + public static List clearDirectives(String cookieHeader, String prefix) { + List out = new ArrayList<>(); + for (String name : names(cookieHeader)) { + for (String path : clearPaths(prefix)) { + out.add(name + "=; Path=" + path + "; Max-Age=0"); + } + } + return out; + } + + /** + * Directives that install {@code cookieHeader} host-wide, so every request under the service + * prefix carries it. Unchanged from ADFA-5043 — the fix is the clear pass above, not the set. + */ + public static List setDirectives(String cookieHeader) { + List out = new ArrayList<>(); + for (String pair : pairs(cookieHeader)) { + out.add(pair + "; path=/"); + } + return out; + } + + /** Cookie names in a {@code Cookie} header, in order, without duplicates. */ + static List names(String cookieHeader) { + Set out = new LinkedHashSet<>(); + for (String pair : pairs(cookieHeader)) { + out.add(pair.substring(0, pair.indexOf('=')).trim()); + } + return new ArrayList<>(out); + } + + /** + * Every path a copy of the service's cookies can live at: the root the box-side login sets + * (it talks to the service directly, with no prefix) and the prefix the WebView reaches it + * through, which the service may set itself. {@code "/books"} and {@code "/books/"} are + * distinct cookie paths, so both are cleared. + */ + static List clearPaths(String prefix) { + List out = new ArrayList<>(); + out.add("/"); + if (prefix == null) return out; + String p = prefix.trim(); + while (p.endsWith("/")) p = p.substring(0, p.length() - 1); + if (p.isEmpty() || p.equals("/")) return out; + out.add(p); + out.add(p + "/"); + return out; + } + + /** Well-formed {@code name=value} pairs of a {@code Cookie} header, trimmed. */ + private static List pairs(String cookieHeader) { + List out = new ArrayList<>(); + if (cookieHeader == null) return out; + for (String raw : cookieHeader.split(";")) { + String pair = raw.trim(); + int eq = pair.indexOf('='); + if (eq <= 0) continue; // no '=' at all, or an empty name: not a usable cookie + out.add(pair); + } + return out; + } +} diff --git a/controller/app/src/main/java/org/iiab/controller/redesign/AuthClient.java b/controller/app/src/main/java/org/iiab/controller/redesign/AuthClient.java index 6bc0da0f1..86f31dec4 100644 --- a/controller/app/src/main/java/org/iiab/controller/redesign/AuthClient.java +++ b/controller/app/src/main/java/org/iiab/controller/redesign/AuthClient.java @@ -17,6 +17,7 @@ import android.os.Handler; import android.os.Looper; +import android.util.Log; import org.iiab.controller.config.BoxEndpoints; import org.iiab.controller.util.AppExecutors; @@ -31,8 +32,34 @@ public final class AuthClient { private AuthClient() {} + private static final String TAG = "K2Go-Auth"; private static final Handler MAIN = new Handler(Looper.getMainLooper()); + /** + * Pause before the single retry. The failures worth retrying are the FAST ones — a refused + * connection or a 5xx from a service that is restarting or saturated — and those are exactly the + * ones an immediate retry asks again too soon to get a different answer, which would make the + * retry decorative. Short enough to be invisible next to the 12 s read budget it guards. + */ + private static final long RETRY_DELAY_MS = 800L; + + /** Thrown so a caller can tell a fast, retryable failure from the server's final word. */ + private static final class HttpStatusException extends Exception { + final int status; + HttpStatusException(int status, String body) { + // Bounded: an unexpected HTML error page must not turn one log line into a screenful. + super("HTTP " + status + (body == null || body.isEmpty() ? "" + : ": " + (body.length() > 200 ? body.substring(0, 200) + "…" : body))); + this.status = status; + } + } + + /** {@code getMessage()} is null for several IO exceptions; the class name is better than "null". */ + private static String describe(Exception e) { + String m = e.getMessage(); + return (m == null || m.isEmpty()) ? e.getClass().getSimpleName() : m; + } + public interface SessionCb { /** {@code cookie} is a ready-to-use Cookie header ("name=value; name2=value2"). */ void onOk(String cookie); @@ -40,22 +67,77 @@ public interface SessionCb { void onErr(); } - /** Ask the box for a signed-in session cookie for a service ("books"/"calibre" or "kolibri"). */ - public static void session(String service, SessionCb cb) { + /** + * Ask the box for a signed-in session cookie for a service ("books"/"calibre" or "kolibri"). + * + *

ADFA-5361: {@code consumerUserAgent} is the User-Agent of the WebView that will USE the + * session, sent as this request's own User-Agent. Calibre-Web (Flask-Login) binds a session to + * a fingerprint of the agent, so a session minted under the box's own agent is rejected on the + * WebView's first request — the identity is dropped, the remember_token deleted, and the card + * opens as the anonymous Guest. It must be the WebView's string verbatim: its only job is to + * match what the WebView will send. + */ + public static void session(String service, String consumerUserAgent, SessionCb cb) { AppExecutors.get().io().execute(() -> { + String url = BoxEndpoints.API + "/auth/" + service + "/session"; try { - String url = BoxEndpoints.API + "/auth/" + service + "/session"; - JSONObject o = new JSONObject(httpGet(url)); - final String cookie = o.optString("cookie", ""); - if (cookie.isEmpty()) MAIN.post(cb::onErr); - else MAIN.post(() -> cb.onOk(cookie)); + final String cookie = fetchCookie(url, consumerUserAgent, service); + if (cookie.isEmpty()) { + // A 200 with no cookie is the box telling us the handshake produced nothing. + Log.w(TAG, service + ": the box returned no session cookie"); + MAIN.post(cb::onErr); + } else { + MAIN.post(() -> cb.onOk(cookie)); + } } catch (Exception e) { + // ADFA-5361: never silent. Without this line the card just opens as the anonymous + // Guest and nothing anywhere says why — which is most of what made this bug expensive. + Log.w(TAG, service + ": sign-in failed, the card will open unauthenticated: " + describe(e)); MAIN.post(cb::onErr); } }); } - private static String httpGet(String urlStr) throws Exception { + /** + * One attempt, plus a single retry for the failures that are worth retrying. + * + *

ADFA-5361: retried are the FAST ones — a refused connection or a 5xx, which is what a + * content service that is busy or has just been restarted answers in milliseconds while a books + * job runs. NOT retried: a timeout (it already spent the full read budget; asking again buys the + * same answer for twice the wait, with the sign-in overlay on screen) and a 4xx (the box's final + * word — wrong credentials do not improve on a second ask). Same rule the box uses for its own + * retries: {@code isTransient: status >= 500} in sockets/net-retry.ts. + */ + // Package-private, not private: this is the branch the UI cannot reach on a healthy box (a card + // whose service is down never opens the portal at all), so the only way to cover it is to call it + // with a scripted server — see AuthClientRetryTest. + static String fetchCookie(String url, String consumerUserAgent, String service) throws Exception { + try { + return cookieOf(httpGet(url, consumerUserAgent)); + } catch (java.net.SocketTimeoutException e) { + throw e; // already waited the full budget + } catch (HttpStatusException e) { + if (e.status < 500) throw e; // 401/403/404: final + Log.w(TAG, service + ": " + describe(e) + " — retrying once"); + } catch (java.io.IOException e) { + Log.w(TAG, service + ": " + describe(e) + " — retrying once"); + } + // Runs on the IO executor, never the main thread. Restore the interrupt flag and give up + // rather than swallowing it: an interrupted worker must not go on to open a new connection. + try { + Thread.sleep(RETRY_DELAY_MS); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw ie; + } + return cookieOf(httpGet(url, consumerUserAgent)); + } + + private static String cookieOf(String json) throws Exception { + return new JSONObject(json).optString("cookie", ""); + } + + private static String httpGet(String urlStr, String consumerUserAgent) throws Exception { HttpURLConnection c = (HttpURLConnection) new URL(urlStr).openConnection(); try { c.setUseCaches(false); @@ -63,9 +145,12 @@ private static String httpGet(String urlStr) throws Exception { // The server does a login handshake with the local service; a missing service fails fast. c.setReadTimeout(12000); c.setRequestProperty("Accept", "application/json"); + if (consumerUserAgent != null && !consumerUserAgent.isEmpty()) { + c.setRequestProperty("User-Agent", consumerUserAgent); + } int code = c.getResponseCode(); String text = readAll(code >= 200 && code < 400 ? c.getInputStream() : c.getErrorStream()); - if (code < 200 || code >= 400) throw new Exception("HTTP " + code); + if (code < 200 || code >= 400) throw new HttpStatusException(code, text); return text; } finally { c.disconnect(); diff --git a/controller/app/src/main/java/org/iiab/controller/redesign/LibraryHomeFragment.java b/controller/app/src/main/java/org/iiab/controller/redesign/LibraryHomeFragment.java index 6825184cb..65303b7df 100644 --- a/controller/app/src/main/java/org/iiab/controller/redesign/LibraryHomeFragment.java +++ b/controller/app/src/main/java/org/iiab/controller/redesign/LibraryHomeFragment.java @@ -327,9 +327,9 @@ private void onCardClick(Card c) { if (c.state == GREEN || contentInFlight(c)) { Intent i = new Intent(requireContext(), PortalActivity.class); i.putExtra("TARGET_URL", BoxEndpoints.BASE + "/" + c.endpoint + "/"); - // ADFA-5043: Books (Calibre-Web) and Courses (Kolibri) auto-login as box admin in the WebView. - String authService = authServiceFor(c.endpoint); - if (authService != null) i.putExtra("AUTH_SERVICE", authService); + // ADFA-5361: Books (Calibre-Web) and Courses (Kolibri) still auto-login as box admin, but + // the portal derives that from the URL (AutoLoginPolicy) — this call site no longer has to + // know, so the entry points that never knew are covered too. startActivity(i); } else if (ModuleCards.byEndpoint(c.endpoint) != null) { // ADFA-4958: module -> action sheet openSheet(c); @@ -352,14 +352,6 @@ private void onCardClick(Card c) { } } - /** ADFA-5043: card endpoint → auto-login service name (server credential store), or null if the - * card has no admin login. */ - private static String authServiceFor(String endpoint) { - if ("kolibri".equals(endpoint)) return "kolibri"; - if ("books".equals(endpoint)) return "calibre"; - return null; - } - /** * ADFA-4958: the module action sheet is the single contextual surface for a module card. * diff --git a/controller/app/src/test/java/org/iiab/controller/portal/domain/AutoLoginPolicyTest.java b/controller/app/src/test/java/org/iiab/controller/portal/domain/AutoLoginPolicyTest.java new file mode 100644 index 000000000..a96632723 --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/portal/domain/AutoLoginPolicyTest.java @@ -0,0 +1,58 @@ +package org.iiab.controller.portal.domain; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import org.junit.Test; + +/** Pure-JVM tests for the portal auto-login policy (ADFA-5361). */ +public class AutoLoginPolicyTest { + + @Test public void booksPagesOpenAsCalibreAdmin() { + assertEquals("calibre", AutoLoginPolicy.serviceFor("http://localhost:8085/books/")); + assertEquals("calibre", AutoLoginPolicy.serviceFor("http://localhost:8085/books")); + // The entry point ADFA-5043 missed: a local book opened from "your books". + assertEquals("calibre", AutoLoginPolicy.serviceFor("http://localhost:8085/books/book/12")); + assertEquals("calibre", AutoLoginPolicy.serviceFor("http://box:8085/books/?q=verne")); + } + + @Test public void coursePagesOpenAsKolibriAdmin() { + assertEquals("kolibri", AutoLoginPolicy.serviceFor("http://localhost:8085/kolibri/")); + assertEquals("kolibri", AutoLoginPolicy.serviceFor("http://127.0.0.1:8085/kolibri/learn#/home")); + } + + @Test public void otherBoxPagesNeedNoSession() { + assertNull(AutoLoginPolicy.serviceFor("http://localhost:8085/home")); + assertNull(AutoLoginPolicy.serviceFor("http://localhost:8085/kiwix/")); + assertNull(AutoLoginPolicy.serviceFor("http://localhost:8085/")); + assertNull(AutoLoginPolicy.serviceFor("http://localhost:8085")); + } + + /** An admin cookie must never be minted for, or injected against, a host that is not the box. */ + @Test public void externalHostsNeverAutoLogin() { + assertNull(AutoLoginPolicy.serviceFor("http://example.org/books/")); + assertNull(AutoLoginPolicy.serviceFor("https://gutenberg.org/books/book/12")); + assertNull(AutoLoginPolicy.prefixFor("http://example.org/books/")); + } + + @Test public void malformedUrlsNeedNoSession() { + assertNull(AutoLoginPolicy.serviceFor(null)); + assertNull(AutoLoginPolicy.serviceFor("")); + assertNull(AutoLoginPolicy.serviceFor(" ")); + assertNull(AutoLoginPolicy.serviceFor("/books/")); // relative: no scheme + assertNull(AutoLoginPolicy.serviceFor("localhost:8085/books")); + } + + @Test public void prefixMatchesTheServedPath() { + assertEquals("/books", AutoLoginPolicy.prefixFor("http://localhost:8085/books/book/12")); + assertEquals("/kolibri", AutoLoginPolicy.prefixFor("http://localhost:8085/kolibri/")); + assertNull(AutoLoginPolicy.prefixFor("http://localhost:8085/home")); + } + + /** Service name and prefix come from one segment, so they cannot disagree. */ + @Test public void serviceAndPrefixStayInStep() { + String url = "http://localhost:8085/BOOKS/book/12"; + assertEquals("calibre", AutoLoginPolicy.serviceFor(url)); + assertEquals("/books", AutoLoginPolicy.prefixFor(url)); + } +} diff --git a/controller/app/src/test/java/org/iiab/controller/portal/domain/SessionCookiesTest.java b/controller/app/src/test/java/org/iiab/controller/portal/domain/SessionCookiesTest.java new file mode 100644 index 000000000..aba390a2e --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/portal/domain/SessionCookiesTest.java @@ -0,0 +1,64 @@ +package org.iiab.controller.portal.domain; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +/** Pure-JVM tests for the WebView cookie reconciliation (ADFA-5361). */ +public class SessionCookiesTest { + + private static final String HEADER = "session=abc123; remember_token=7|deadbeef"; + + @Test public void namesComeFromTheServerResponse() { + assertEquals(Arrays.asList("session", "remember_token"), SessionCookies.names(HEADER)); + } + + @Test public void namesIgnoreJunkAndDuplicates() { + assertEquals(Arrays.asList("a"), SessionCookies.names("a=1; ; noequals; =novalue; a=2")); + assertTrue(SessionCookies.names(null).isEmpty()); + assertTrue(SessionCookies.names("").isEmpty()); + } + + /** The box-side login sets cookies at the root; the service, reached through its prefix, can + * set its own copy there — and "/books" and "/books/" are distinct cookie paths. */ + @Test public void everyPathTheCookieCanLiveAtIsCleared() { + assertEquals(Arrays.asList("/", "/books", "/books/"), SessionCookies.clearPaths("/books")); + assertEquals(Arrays.asList("/", "/books", "/books/"), SessionCookies.clearPaths("/books/")); + assertEquals(Arrays.asList("/"), SessionCookies.clearPaths(null)); + assertEquals(Arrays.asList("/"), SessionCookies.clearPaths("/")); + assertEquals(Arrays.asList("/"), SessionCookies.clearPaths(" ")); + } + + @Test public void clearDirectivesExpireEveryNameOnEveryPath() { + List out = SessionCookies.clearDirectives(HEADER, "/books"); + assertEquals(Arrays.asList( + "session=; Path=/; Max-Age=0", + "session=; Path=/books; Max-Age=0", + "session=; Path=/books/; Max-Age=0", + "remember_token=; Path=/; Max-Age=0", + "remember_token=; Path=/books; Max-Age=0", + "remember_token=; Path=/books/; Max-Age=0"), out); + } + + @Test public void setDirectivesInstallTheFreshSessionHostWide() { + assertEquals(Arrays.asList("session=abc123; path=/", "remember_token=7|deadbeef; path=/"), + SessionCookies.setDirectives(HEADER)); + } + + @Test public void nothingToInstallMeansNothingToClear() { + assertTrue(SessionCookies.clearDirectives(null, "/books").isEmpty()); + assertTrue(SessionCookies.clearDirectives("", "/books").isEmpty()); + assertTrue(SessionCookies.setDirectives(null).isEmpty()); + } + + /** A cookie value may contain '=' (base64 padding); only the first one splits name from value. */ + @Test public void valuesKeepTheirOwnEqualsSigns() { + assertEquals(Arrays.asList("session"), SessionCookies.names("session=YWJjZA==")); + assertEquals(Arrays.asList("session=YWJjZA==; path=/"), + SessionCookies.setDirectives("session=YWJjZA==")); + } +} diff --git a/controller/app/src/test/java/org/iiab/controller/redesign/AuthClientRetryTest.java b/controller/app/src/test/java/org/iiab/controller/redesign/AuthClientRetryTest.java new file mode 100644 index 000000000..ff0b7b505 --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/redesign/AuthClientRetryTest.java @@ -0,0 +1,173 @@ +package org.iiab.controller.redesign; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.junit.Test; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * ADFA-5361: covers the sign-in retry, which nothing else can reach. + * + *

On a real box the UI never gets here: 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 entirely — device-verified while + * testing this ticket. The retry exists for the narrow race the bug actually rode in on (the service + * falling between the probe and the tap, or answering 5xx while it is saturated by a books job), and + * a scripted server is the only way to hold that case still. + * + *

Plain JVM, no emulator: {@code fetchCookie} touches only {@code HttpURLConnection} and + * {@code Log} (stubbed by {@code returnDefaultValues}), so this runs in the normal unit-test task. + */ +public class AuthClientRetryTest { + + private static final String COOKIE_JSON = + "{\"service\":\"calibre\",\"cookie\":\"session=abc123; remember_token=7|deadbeef\"}"; + private static final String ERROR_JSON = "{\"error\":\"sign-in failed\"}"; + private static final String UA = "TestConsumer/1.0"; + + /** A one-connection-per-request HTTP server that answers a scripted sequence of statuses and + * records when each request arrived, so the test can assert both the count and the pause. */ + private static final class ScriptedBox implements AutoCloseable { + private final ServerSocket socket; + private final int[] statuses; + private final String[] bodies; + private final List arrivals = Collections.synchronizedList(new ArrayList<>()); + private volatile boolean running = true; + + ScriptedBox(int[] statuses, String[] bodies) throws IOException { + this.statuses = statuses; + this.bodies = bodies; + this.socket = new ServerSocket(0, 8, InetAddress.getByName("127.0.0.1")); + Thread t = new Thread(this::serve, "scripted-box"); + t.setDaemon(true); + t.start(); + } + + String url() { + return "http://127.0.0.1:" + socket.getLocalPort() + "/k2go-api/auth/calibre/session"; + } + + int requests() { return arrivals.size(); } + + /** Milliseconds between the first and second request; -1 when there was no second. */ + long pauseMs() { return arrivals.size() < 2 ? -1 : arrivals.get(1) - arrivals.get(0); } + + private void serve() { + while (running) { + try (Socket s = socket.accept()) { + arrivals.add(System.currentTimeMillis()); + // Drain the request head; answering before reading it can break the client's pipe. + BufferedReader in = new BufferedReader(new InputStreamReader(s.getInputStream())); + String line; + while ((line = in.readLine()) != null && !line.isEmpty()) { /* headers */ } + + int i = Math.min(arrivals.size() - 1, statuses.length - 1); + byte[] body = bodies[i].getBytes(StandardCharsets.UTF_8); + OutputStream out = s.getOutputStream(); + // Connection: close so every request is its own accept() and the count is exact. + out.write(("HTTP/1.1 " + statuses[i] + " Scripted\r\n" + + "Content-Type: application/json\r\n" + + "Content-Length: " + body.length + "\r\n" + + "Connection: close\r\n\r\n").getBytes(StandardCharsets.UTF_8)); + out.write(body); + out.flush(); + } catch (IOException e) { + if (running) throw new IllegalStateException("scripted box failed", e); + } + } + } + + @Override public void close() { + running = false; + try { socket.close(); } catch (IOException ignored) { } + } + } + + @Test public void a5xxIsRetriedAndTheSecondAnswerIsUsed() throws Exception { + try (ScriptedBox box = new ScriptedBox( + new int[]{503, 200}, new String[]{ERROR_JSON, COOKIE_JSON})) { + String cookie = AuthClient.fetchCookie(box.url(), UA, "calibre"); + assertTrue("the retry's cookie must be the one returned: " + cookie, + cookie.contains("session=abc123")); + assertEquals("exactly one retry", 2, box.requests()); + } + } + + /** The defect this pins: an immediate retry asks a restarting service again too soon to get a + * different answer, which would make the retry decorative. */ + @Test public void theRetryPausesBeforeAskingAgain() throws Exception { + try (ScriptedBox box = new ScriptedBox( + new int[]{503, 200}, new String[]{ERROR_JSON, COOKIE_JSON})) { + AuthClient.fetchCookie(box.url(), UA, "calibre"); + assertTrue("the second request came " + box.pauseMs() + "ms after the first", + box.pauseMs() >= 500); + } + } + + @Test public void aPersistent5xxGivesUpAfterOneRetry() throws Exception { + try (ScriptedBox box = new ScriptedBox( + new int[]{503, 503}, new String[]{ERROR_JSON, ERROR_JSON})) { + try { + AuthClient.fetchCookie(box.url(), UA, "calibre"); + fail("a persistent 5xx must not resolve"); + } catch (Exception expected) { + assertTrue("the status belongs in the message: " + expected.getMessage(), + expected.getMessage().contains("503")); + } + assertEquals("one retry, never two", 2, box.requests()); + } + } + + /** A 4xx is the box's final word — wrong credentials do not improve on a second ask, and the + * user is waiting behind a blocking overlay. Device-verified on a 401 (ADFA-5361). */ + @Test public void a4xxIsFinalAndNotRetried() throws Exception { + try (ScriptedBox box = new ScriptedBox( + new int[]{401, 200}, new String[]{ERROR_JSON, COOKIE_JSON})) { + try { + AuthClient.fetchCookie(box.url(), UA, "calibre"); + fail("a 401 must not resolve by retrying"); + } catch (Exception expected) { + assertTrue("the status belongs in the message: " + expected.getMessage(), + expected.getMessage().contains("401")); + } + assertEquals("no retry on a 4xx", 1, box.requests()); + } + } + + @Test public void a404IsFinalToo() throws Exception { + try (ScriptedBox box = new ScriptedBox( + new int[]{404, 200}, new String[]{ERROR_JSON, COOKIE_JSON})) { + try { + AuthClient.fetchCookie(box.url(), UA, "calibre"); + fail("a 404 must not resolve by retrying"); + } catch (Exception expected) { + // the message carries the status; the point of the assert below is the count + } + assertEquals("no retry on a 4xx", 1, box.requests()); + } + } + + /** A first-try success must not cost the caller a second request (or the pause). */ + @Test public void aFirstTrySuccessAsksOnce() throws Exception { + try (ScriptedBox box = new ScriptedBox(new int[]{200}, new String[]{COOKIE_JSON})) { + long started = System.currentTimeMillis(); + String cookie = AuthClient.fetchCookie(box.url(), UA, "calibre"); + assertTrue(cookie.contains("remember_token=7|deadbeef")); + assertEquals(1, box.requests()); + assertTrue("a success must not pay the retry pause", + System.currentTimeMillis() - started < 500); + } + } +} diff --git a/controller/docs/ADR-5361-service-session-portability.md b/controller/docs/ADR-5361-service-session-portability.md new file mode 100644 index 000000000..48228fb92 --- /dev/null +++ b/controller/docs/ADR-5361-service-session-portability.md @@ -0,0 +1,195 @@ +# ADR-5361 — A service session belongs to the agent that will use it; the box mints it for that agent + +**Status:** accepted — implemented in ADFA-5361 (app + dash-node 1.2.11), device-verified. + +**Scope note (form):** genericized per the ADR authoring convention — no personal names, no specific +device identifiers. "the test device" = the Android target the measurements were taken on; "the box" += the on-device server tree (nginx :8085 fronting the content services and the dash-node REST core). + +--- + +## 1. The bug, stated as a missing fact + +ADFA-5043 gave the Books and Courses cards an auto-login: the app asks the box for a signed-in +session, the box logs in server-side with the stored admin credentials, and the app injects the +returned cookies into the WebView before loading the page. The reported symptom was that adding +books through Get More left the library in read-only Guest mode, permanently — the device owner +could not manage books they had just added themselves. + +The missing fact is not "the cookie is lost" and not "Get More breaks something". It is: + +> **A Calibre-Web session is bound to the agent that created it. The mint had no idea who would use it.** + +Flask-Login binds a session to a fingerprint of the request's `User-Agent` (and address). The box +minted under its own agent; the WebView presented the cookie under a different one; the first +request therefore had the session rejected, the `remember_token` **deleted**, and a fresh +**anonymous** session issued in its place. + +Which means the auto-login never delivered admin at all. It looked like it did — see §2.4 — and that +is why it shipped. + +Two further defects turned a wrong identity into an unrecoverable one; both are recorded here +because each is an instance of a general trap, not a typo. + +## 2. The measured evidence (do not infer it from versions) + +### 2.1 The session does not travel between agents + +The same freshly minted session, replayed through nginx, differing only in the `User-Agent`: + +| Session presented with… | `/books/admin` link | `Set-Cookie: remember_token=; Max-Age=0` on the reply | +|---|---|---| +| the agent that minted it | **present** (Admin) | no | +| a WebView-shaped agent | absent (Guest) | **yes — the reply deletes the remember cookie** | +| a login performed *with* the WebView's agent | **present** (Admin) | no | + +The third row is the fix, proven before it was written: the session works when the agent that +creates it is the agent that will use it. + +### 2.2 The service sets its cookies under its own path prefix + + $ curl -sI http:///books/ | grep -i set-cookie + Set-Cookie: session=…; HttpOnly; Path=/books; SameSite=Lax + +The box-side login talks to the service **directly** (no prefix), so its cookies come back with +`Path=/`. The WebView reaches the same service **through nginx**, under `/books`. + +### 2.3 The sources map — who writes which cookie, where, and who wins + +| Cookie | Written by | Path | Read by | +|---|---|---|---| +| `session` (admin) | the box's server-side login, direct to the service port | `/` | the service | +| `session` (guest) | the service itself, reached through its prefix | `/books` | the service — **and this one wins** | +| `remember_token` | either, depending on who logged in | as above | Flask-Login, but only when there is no session | + +Two cookies of the same name at different paths are two different cookies. RFC 6265 §5.4 orders the +longer path first and the service reads the first one it is given, so the deeper copy shadows the +one the app installs — and nothing in the app ever removed it. An instrumentation test pins this +against the real WebView cookie store rather than against the RFC. + +### 2.4 The signal that made a broken feature look healthy + +Calibre-Web renders `You are now logged in as: 'Admin'` on the first page after a login. It is a +**flash message stored inside the injected session**, so it renders even when the session is then +rejected and the viewer is already anonymous. Every screenshot of the broken build shows it. + +> **Corollary, and the cheapest lesson in this ADR:** a message is not proof of identity. See §7. + +## 3. Decision + +1. **The box mints the session for the agent that asks.** `GET /k2go-api/auth/:service/session` + forwards the caller's own `User-Agent` through the entire login handshake — every request, not + only the credential POST, because the fingerprint is established on the first (anonymous) one. + The app sends its WebView's string, read from the very WebView that will present it. +2. **"This page opens as box admin" has one owner.** The portal derives the service from the target + URL (`portal/domain/AutoLoginPolicy`) instead of each launcher passing an Intent extra. Two of + three launchers had not been passing it, and those were the entry points reached right after + adding content. +3. **The cookie jar is reconciled, not appended to.** Before installing a fresh session the app + expires that service's cookies on every path they can live at (`portal/domain/SessionCookies`). + Clear and set are one operation: a failed sign-in leaves the jar untouched, so a still-valid + session is never discarded over a transient failure. +4. **The failure is legible.** The client logs the real cause, retries once for fast failures (a + refused connection or a 5xx) after a short pause, never for a timeout or a 4xx, and tells the + user. Same rule the box applies to its own retries (`sockets/net-retry.ts`). + +Callers that consume a session *themselves* (the content-download runner, the delete path) pass no +agent and keep the box's own. The fact travels only where it is true. + +## 4. Options considered + +**A. Mint with the consumer's agent — chosen.** Small, local, and it makes the endpoint's contract +honest: *mint a session for the agent that asks*. Any caller — the app, a script, a future surface — +gets a session valid for itself by construction. Verified before implementation (§2.1). + +**B. Log in inside the WebView.** Post the service's own login form from the page, so the session is +created by its consumer by definition. Rejected: it means handling each service's CSRF and form +shape in the client, and re-implementing per service what the box already does once. + +**C. Relax the upstream service.** Disable Flask-Login's session protection, or turn off anonymous +browsing so a rejected session fails loudly instead of degrading to Guest. Rejected: it weakens a +security control of a third-party component to work around our own design, and it does not travel — +the next service will bind sessions its own way. + +**D. Proxy every service request through the box.** The box holds the session and the WebView never +sees a cookie. Rejected as far out of proportion: it puts the REST core on the critical path of all +content browsing, for one identity problem. + +## 5. Consequences + +- The endpoint is now **agent-sensitive**. A caller that sends no `User-Agent` degrades to the + previous behaviour — a session valid only for the box — and that degradation is logged, because + "the session mints but never authenticates" is otherwise invisible. +- **The User-Agent must be the consumer's string verbatim.** The tempting alternative — a branded + `K2Go WebView` — only works if the same string is also forced onto the WebView, and that breaks + things that read it: PDF routing picks a pdf.js build from the Chrome major version in the UA, and + the services' templates sniff it for their mobile layout. It would also be two places that must + agree, which is the class of defect this ADR exists to remove. +- Kolibri receives the same treatment. Django does not bind sessions to the agent, so it is expected + to be a no-op there; it is applied anyway because one rule per endpoint is the point, and a + per-service exception is what later gets forgotten. Verified not to regress (§7). +- The app is one Intent extra and one god-class method lighter; the box has one Calibre-Web login + instead of two. The duplicate had already drifted — the `remember_me` of ADFA-5043 reached one + copy and not the other — which is the drift this bug rode in on. + +### Open consequences, deliberately not closed here + +- **Session rows accumulate with no owner.** Every authenticated open creates a session record in + the service's user database and nothing ever deletes it. Not a regression — those sessions were + already being minted, merely uselessly — but it is a fact without an owner on a device meant to + run for years. +- **The service is derived once, at load.** Navigating inside the WebView from the box's home page + to a service still opens it unauthenticated. No longer permanent, because the next card-opened + session clears and reinstalls, but covering it means hooking navigation rather than startup. +- The inverse mapping (service name → URL segment) still lives in the settings probe. Same fact, + other direction. + +## 6. Checklist for the next service + +Before adding a third service to the auto-login: + +1. Is it fronted under a **path prefix**? Then its own cookies live at that prefix, and the jar must + be reconciled there, not only at `/`. +2. Does the box **mint** the session, or does the consumer log in? If the box mints it, pass the + consumer's `User-Agent`. +3. Does the service allow **anonymous browsing**? Then a rejected session degrades silently to a + guest view instead of failing — assume every "it works" report is about a message, not an + identity, until §7 is satisfied. + +## 7. Verification + +**What counts as proof of identity** — one of: + +- the service's own navigation shows the admin account (not `Guest`), or +- an admin-only surface is present and usable (for Calibre-Web: `Edit Metadata` on a book, and the + delete that follows it). + +**What does not count:** the `You are now logged in as: 'Admin'` flash (§2.4). It renders from the +injected session even when the viewer is anonymous, and it is the reason the original auto-login was +accepted as working. + +**Device-verified for this ADR**, with the WebView cookie store deleted first so nothing could be +attributed to leftovers: all three entry points open as admin; a full add-then-reopen cycle keeps +admin and allows editing and deleting the newly added book; the courses card reaches its super-admin +surface; and after the run the jar holds only the freshly minted, non-persistent cookies — the +persistent one left by an earlier manual sign-in is gone, which is what proves the clear pass ran. + +**Covered by tests, not by device steps:** the sign-in failure path. On a healthy box the UI never +reaches it — 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 were observed while trying to reproduce it by +hand; do not spend an afternoon repeating that. The retry is held still by a unit test against a +scripted local server (5xx retried once after a pause; 4xx asked exactly once, with a success queued +behind it that the test requires us never to reach), and the toast plus the log line were verified on +device by forcing a 401 with a deliberately wrong stored credential. + +### Running the instrumentation test + +The cookie reconciliation is pinned against the real WebView cookie store, which needs a device. +**Never** use `connectedAndroidTest` for it: that task uninstalls both APKs when it finishes, and on +a device holding an installed rootfs, uninstalling the app deletes the box. One run destroyed an +installed system while this ticket was being written; the build now refuses that task unless +`-PallowUninstall=true` is passed. Install and run explicitly instead: + + ./gradlew :app:installDebug :app:installDebugAndroidTest + adb shell am instrument -w -e class org.iiab.controller.portal.SessionCookieReconcileTest \ + org.iiab.controller.test/androidx.test.runner.AndroidJUnitRunner diff --git a/static/dashboard/CHANGELOG.md b/static/dashboard/CHANGELOG.md index e91cc4f51..b65a2adaa 100644 --- a/static/dashboard/CHANGELOG.md +++ b/static/dashboard/CHANGELOG.md @@ -4,6 +4,7 @@ One line per version, newest first. Every REST-facing change bumps the version i (the app surfaces it via `/system/dashboard/update-check` and the "Update available" pill), so this file is the human record of what each bump enables. Keep entries short: `version - change (TICKET)`. +- **1.2.11** - `/auth/:service/session` mints the session **for the agent that asks** (ADFA-5361). Calibre-Web (Flask-Login) binds a session to a fingerprint of the User-Agent, so a session minted under dash-node's own agent was rejected on the WebView's first request: the identity was dropped, the `remember_token` deleted, and the card opened as the anonymous Guest — the "logged in as Admin" flash comes from the injected session and renders even then, which is why the auto-login looked like it worked. The route now forwards the caller's `User-Agent` through the whole login handshake (every request, not just the POST — the fingerprint is established on the first one), for Calibre-Web and Kolibri alike. The callers that consume the session themselves (downloads runner, `removeBook`) are unchanged. No User-Agent on the request degrades to the previous behaviour, logged. Same ticket: the books runner's private copy of the Calibre-Web login is gone — it never got the ADFA-5043 `remember_me` and was the drift this whole bug rode in on — so `getCalibreSession` is the one source. (ADFA-5361) - **1.2.10** - In-proot content-service recovery (ADFA-5343, ADR-5343a §10). New `POST /system/service/:svc/restart`: runs `pdsm restart ` in the one living proot to recover a content service wedged after an environment relaunch (orphaned off proot → `epoll_wait` ENOSYS), for the supported upstream services (mirrors `pdsm_installed_services`; `dash-node` excluded). A server-side watcher auto-heals a present-but-wedged content service (404 = not installed → left alone), cooldown-bounded; the app's future module-card Retry is the manual backstop hitting the same endpoint. Loopback-only, like all of `/k2go-api`. (ADFA-5343) - **1.2.9** - Cancelable dashboard self-update (ADFA-5333). New `POST /system/dashboard/rebuild/cancel`: stops an in-flight rebuild cleanly while it is still **building** (git fetch + staging build + smoke test — none of which touch the live dashboard) by signaling the detached `setsid` session group; **refused during "promoting"** (the short dist-swap + restart window) so the swap is never interrupted mid-flight, and a no-op (409) when nothing is running. To support this, `tools/rebuild-dashboard.sh` now records its coarse phase (`building`/`promoting`) and its session-leader pid, and its cleanup trap fires on TERM/INT so a canceled run leaves no staging behind. Pairs with the app running the update in the background with a Cancel action (ADFA-5333). (ADFA-5333) - **1.2.7** - Books homologated to the ZIM contract (ADFA-4893). The books runner now surfaces its reconnect state on the poll (`retryAttempt`/`retryTotal` via `ctx.reportRetry`), so the app can show "Reconnecting… n of N" like ZIM/rootfs (books' per-item budget is 6 tries → shows n of 5, same label). And the runner is now **idempotent on resume**: it skips books already in the Calibre-Web library (matched by title) instead of re-downloading + re-uploading them — which used to duplicate entries and reset the percent to 0 — so a resume, or a re-run after process death, is safe. (ADFA-4893) diff --git a/static/dashboard/package.json b/static/dashboard/package.json index d638da191..7f7e063b0 100644 --- a/static/dashboard/package.json +++ b/static/dashboard/package.json @@ -1,6 +1,6 @@ { "name": "dashboard-console", - "version": "1.2.10", + "version": "1.2.11", "description": "", "main": "index.js", "scripts": { diff --git a/static/dashboard/routes.ts b/static/dashboard/routes.ts index 6ef6fc99c..38db6c750 100644 --- a/static/dashboard/routes.ts +++ b/static/dashboard/routes.ts @@ -729,14 +729,25 @@ apiRouter.delete('/credentials/:service', (req: Request, res: Response): void => apiRouter.get('/auth/:service/session', async (req: Request, res: Response): Promise => { res.set('Cache-Control', 'no-store'); const service = String(req.params.service); + // ADFA-5361: mint the session FOR THE AGENT THAT ASKS. Calibre-Web (Flask-Login) binds a session + // to a fingerprint of the User-Agent, so a session minted under this process's agent is rejected + // on the consumer's first request — the identity is dropped, the remember_token deleted, and the + // caller silently becomes the anonymous Guest. The caller's own User-Agent is that fact: the app + // sends its WebView's. Missing (a hand-made call) degrades to this process's agent, as before — + // logged, because "the session mints but never authenticates" is otherwise invisible. + const consumerUa = req.get('user-agent'); + if (!consumerUa) { + console.warn(`[auth] ${service}: request carries no User-Agent; the session is minted for ` + + 'this process and will not authenticate another agent'); + } try { if (service === 'kolibri') { - const s = await kolibriLogin(); + const s = await kolibriLogin(undefined, consumerUa); res.json({ service: 'kolibri', cookie: s.cookie }); return; } if (service === 'calibre' || service === 'books') { - const s = await getCalibreSession(); + const s = await getCalibreSession(consumerUa); res.json({ service: 'calibre', cookie: s.cookie }); return; } diff --git a/static/dashboard/sockets/books.exec.ts b/static/dashboard/sockets/books.exec.ts index 93e73d7cf..01073f9d5 100644 --- a/static/dashboard/sockets/books.exec.ts +++ b/static/dashboard/sockets/books.exec.ts @@ -5,9 +5,8 @@ // download_books_batch handler (auth/CSRF + fetch + upload), made durable and reporting // structured per-book progress. A job item is { id, title, url }. import { jobs, RunnerContext, CanceledError, PausedError, classifyStop } from './jobs'; -import { getCredential } from './credentials'; import { withRetry } from './net-retry'; -import { presentTitles } from './books.query'; +import { presentTitles, getCalibreSession } from './books.query'; import fs from 'fs'; import path from 'path'; @@ -27,56 +26,16 @@ const BOOK_RETRY_MAX_MS = 15_000; const CALIBRE_WEB_LOCAL_URL = 'http://127.0.0.1:8083'; const TMP_DIR = '/tmp/books_downloader/'; const SYSTEM_USER_AGENT = 'K2Go Dashboard/1.0 (https://github.com/appdevforall/KnowledgeToGo)'; -// ADFA-4949: the credential override the original comment anticipated. Values now -// come from the shared store (env -> persisted override -> the same Admin/changeme -// factory default), so a device whose Calibre-Web password changed keeps working -// without a rebuild. Behaviour on an untouched device is identical. +// ADFA-4949: the credentials come from the shared store (env -> persisted override -> the +// same Admin/changeme factory default), so a device whose Calibre-Web password changed keeps +// working without a rebuild. Read inside getCalibreSession (books.query.ts). +// ADFA-5361: this file used to carry its OWN copy of that login. Two implementations of "an +// authenticated Calibre-Web session" drifted exactly as expected — the remember_me of ADFA-5043 +// reached one and not the other — so the copy is gone and there is one source. This runner +// consumes the session itself, so it passes no consumer User-Agent and keeps Node's own agent. interface BookItem { id?: string; title?: string; url?: string; } -/** Authenticate against Calibre-Web and return a usable cookie + fresh CSRF token. */ -async function getCalibreSession(): Promise<{ cookie: string; csrfToken: string }> { - const loginPageRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/login`); - const initialCookies = loginPageRes.headers.getSetCookie().map((c) => c.split(';')[0]).join('; '); - const loginHtml = await loginPageRes.text(); - - const csrfMatch = loginHtml.match(/name="csrf_token" value="(.*?)"/); - if (!csrfMatch) throw new Error('Could not find CSRF token on login page'); - const csrfToken = csrfMatch[1]; - - const loginData = new URLSearchParams(); - loginData.append('csrf_token', csrfToken); - const cred = getCredential('calibre'); - loginData.append('username', cred.username); - loginData.append('password', cred.password); - - const authRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/login`, { - method: 'POST', - headers: { - Cookie: initialCookies, - 'Content-Type': 'application/x-www-form-urlencoded', - Referer: `${CALIBRE_WEB_LOCAL_URL}/login`, - }, - body: loginData, - redirect: 'manual', - }); - - if (authRes.status !== 302 && authRes.status !== 303) { - throw new Error('Invalid Calibre-Web credentials'); - } - - const authCookieString = authRes.headers.getSetCookie().map((c) => c.split(';')[0]).join('; '); - - const homePageRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/`, { headers: { Cookie: authCookieString } }); - const homeHtml = await homePageRes.text(); - const finalCsrfMatch = - homeHtml.match(/name="csrf_token"\s+value="([^"]+)"/i) || - homeHtml.match(/value="([^"]+)"\s+name="csrf_token"/i); - const finalCsrfToken = finalCsrfMatch ? finalCsrfMatch[1] : csrfToken; - - return { cookie: authCookieString, csrfToken: finalCsrfToken }; -} - const booksRunner: (ctx: RunnerContext) => Promise = async (ctx) => { if (!fs.existsSync(TMP_DIR)) fs.mkdirSync(TMP_DIR, { recursive: true }); diff --git a/static/dashboard/sockets/books.query.ts b/static/dashboard/sockets/books.query.ts index 2a31cc5e8..93ff13eb8 100644 --- a/static/dashboard/sockets/books.query.ts +++ b/static/dashboard/sockets/books.query.ts @@ -132,9 +132,20 @@ export function listLibrary(): any[] { /** Log into Calibre-Web with the given credentials and return the authenticated session. * A successful login answers with a 302/303 redirect; anything else means the credentials were * rejected (thrown as 'Invalid Calibre-Web credentials'). A connection error (service down) throws - * the underlying fetch error, so callers can tell "wrong password" from "not running". */ -async function loginCalibre(username: string, password: string): Promise<{ cookie: string; csrfToken: string }> { - const loginPageRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/login`); + * the underlying fetch error, so callers can tell "wrong password" from "not running". + * + * ADFA-5361: {@code userAgent} is the agent that will USE the session. Flask-Login binds a session + * to a fingerprint of the User-Agent (+ address), so a session minted here under Node's own agent + * is rejected the moment another agent presents it: the identity is dropped, the remember_token is + * deleted, and the caller silently becomes the anonymous Guest. Callers that consume the session + * themselves (the downloads runner, removeBook) pass nothing and keep Node's agent; the auto-login + * route passes the WebView's, so the session is minted for its real consumer. All three requests + * carry it — the anonymous GET already establishes the fingerprint. */ +async function loginCalibre( + username: string, password: string, userAgent?: string, +): Promise<{ cookie: string; csrfToken: string }> { + const agent: Record = userAgent ? { 'User-Agent': userAgent } : {}; + const loginPageRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/login`, { headers: { ...agent } }); const initialCookies = loginPageRes.headers.getSetCookie().map((c) => c.split(';')[0]).join('; '); const loginHtml = await loginPageRes.text(); const csrfMatch = loginHtml.match(/name="csrf_token" value="(.*?)"/); @@ -153,6 +164,7 @@ async function loginCalibre(username: string, password: string): Promise<{ cooki const authRes = await fetch(`${CALIBRE_WEB_LOCAL_URL}/login`, { method: 'POST', headers: { + ...agent, Cookie: initialCookies, 'Content-Type': 'application/x-www-form-urlencoded', Referer: `${CALIBRE_WEB_LOCAL_URL}/login`, @@ -163,16 +175,20 @@ async function loginCalibre(username: string, password: string): Promise<{ cooki if (authRes.status !== 302 && authRes.status !== 303) throw new Error('Invalid Calibre-Web credentials'); const authCookieString = authRes.headers.getSetCookie().map((c) => c.split(';')[0]).join('; '); - const homeHtml = await (await fetch(`${CALIBRE_WEB_LOCAL_URL}/`, { headers: { Cookie: authCookieString } })).text(); + const homeHtml = await (await fetch(`${CALIBRE_WEB_LOCAL_URL}/`, { + headers: { ...agent, Cookie: authCookieString }, + })).text(); const finalCsrfMatch = homeHtml.match(/name="csrf_token"\s+value="([^"]+)"/i) || homeHtml.match(/value="([^"]+)"\s+name="csrf_token"/i); return { cookie: authCookieString, csrfToken: finalCsrfMatch ? finalCsrfMatch[1] : csrfToken }; } -export async function getCalibreSession(): Promise<{ cookie: string; csrfToken: string }> { +/** ADFA-5361: pass {@code userAgent} when the session is for someone else (the app's WebView). + * Omit it when this process is the consumer — see {@link loginCalibre}. */ +export async function getCalibreSession(userAgent?: string): Promise<{ cookie: string; csrfToken: string }> { const cred = getCredential('calibre'); - return loginCalibre(cred.username, cred.password); + return loginCalibre(cred.username, cred.password, userAgent); } /** ADFA-5044: check credentials against the live Calibre-Web before persisting them. Resolves on a diff --git a/static/dashboard/sockets/kolibri.session.test.ts b/static/dashboard/sockets/kolibri.session.test.ts index 93948a3e2..311bbd08b 100644 --- a/static/dashboard/sockets/kolibri.session.test.ts +++ b/static/dashboard/sockets/kolibri.session.test.ts @@ -15,7 +15,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { cookieValue, mergeCookies, matchesOrigin, KolibriApiError } from './kolibri.session'; +import { cookieValue, mergeCookies, matchesOrigin, KolibriApiError, login } from './kolibri.session'; import { mapPhase, mapPercent, normalizeUuid, buildTaskPayload, overallPercent, sampleSpeed, toRemoteChannel, failureMessage, @@ -361,3 +361,48 @@ test('failureMessage keeps a detail line that does not match the usual shape', ( test('failureMessage does not echo the class name back as if it were detail', () => { assert.equal(failureMessage('HTTPError', 'Traceback:\nHTTPError'), 'HTTPError'); }); + +// ─── ADFA-5361: the session is minted for the agent that will use it ───────── +// +// The only network-shaped test here, and it stubs fetch: what it pins is not Kolibri's +// behaviour but OUR contract — the consumer's User-Agent reaches EVERY request of the +// handshake, because the fingerprint a service binds a session to is established on the +// first (anonymous) one, not only on the login POST. Getting this wrong is invisible: +// the session mints, the cookie looks fine, and the consumer is silently anonymous. + +/** Runs `body` with a stubbed fetch, returning the User-Agent seen on each request. */ +async function captureUserAgents(body: () => Promise): Promise<(string | null)[]> { + const seen: (string | null)[] = []; + const realFetch = globalThis.fetch; + globalThis.fetch = (async (url: RequestInfo | URL, init: RequestInit = {}) => { + seen.push(new Headers(init.headers).get('user-agent')); + const headers = new Headers(); + if (String(url).includes('/current/')) { + headers.append('set-cookie', 'kolibri_csrftoken=csrf1; Path=/'); + return new Response('{}', { status: 200, headers }); + } + headers.append('set-cookie', 'kolibri=sess1; Path=/'); + return new Response(JSON.stringify({ username: 'Admin', can_manage_content: true }), + { status: 200, headers }); + }) as typeof fetch; + try { + await body(); + } finally { + globalThis.fetch = realFetch; + } + return seen; +} + +test('login carries the consumer User-Agent on every request of the handshake', async () => { + const seen = await captureUserAgents( + () => login({ username: 'Admin', password: 'x' }, 'ConsumerUA/1.0')); + assert.equal(seen.length, 2); // CSRF seed + login POST + assert.deepEqual(seen, ['ConsumerUA/1.0', 'ConsumerUA/1.0']); +}); + +test('login without a consumer keeps this process own agent', async () => { + // The callers that consume the session themselves must NOT be given someone else's + // identity: omitting the argument has to leave the header untouched, not empty it. + const seen = await captureUserAgents(() => login({ username: 'Admin', password: 'x' })); + assert.deepEqual(seen, [null, null]); +}); diff --git a/static/dashboard/sockets/kolibri.session.ts b/static/dashboard/sockets/kolibri.session.ts index fec5b23dc..3f289a959 100644 --- a/static/dashboard/sockets/kolibri.session.ts +++ b/static/dashboard/sockets/kolibri.session.ts @@ -105,18 +105,28 @@ async function fetchWithTimeout( * * @param override explicit credentials (used by the validation endpoint before * persisting them); if omitted, they are taken from the store. + * @param userAgent ADFA-5361: the agent that will USE the session, when it is not this + * process (the app's WebView). Django does not bind a session to the + * User-Agent the way Flask-Login does — Calibre-Web is where this was + * measured — but the endpoint's contract is one rule for every service + * ("mint a session for the agent that asks"), and a per-service exception + * is what later gets forgotten. Omitted by the callers that consume the + * session themselves, which keep this process's own agent. */ export async function login( override?: { username: string; password: string }, + userAgent?: string, ): Promise { const cred = override ?? getCredential('kolibri'); + const agent: Record = userAgent ? { 'User-Agent': userAgent } : {}; // 1. Seed the CSRF cookie. The session viewset carries @ensure_csrf_cookie, // so a GET is enough. let cookie = ''; let csrfToken: string | null = null; try { - const seed = await fetchWithTimeout(`${KOLIBRI_BASE}/api/auth/session/current/`); + const seed = await fetchWithTimeout(`${KOLIBRI_BASE}/api/auth/session/current/`, + { headers: { ...agent } }); const setCookies = seed.headers.getSetCookie(); cookie = mergeCookies('', setCookies); csrfToken = cookieValue(setCookies, CSRF_COOKIE); @@ -135,6 +145,7 @@ export async function login( res = await fetchWithTimeout(`${KOLIBRI_BASE}/api/auth/session/`, { method: 'POST', headers: { + ...agent, 'Content-Type': 'application/json', Cookie: cookie, [CSRF_HEADER]: csrfToken,