Fix: recover an admin session that expired mid-edit

CHANGELOG.md

@@ -36,6 +36,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- A session that expires while you are working in Admin now raises the sign-in
+  box over the page, and the interrupted save completes once you are back in,
+  instead of failing as "Unauthorised" until the page is reloaded.
+
 - Saving an app or widget no longer reports "Updated" when the write failed; the
   editor stays open with the form intact.
 

ui/js/admin-auth.js

@@ -21,6 +21,21 @@ export async function checkAuth(onLogin) {
   }
 }
 
+/** Show the sign-in box over whatever is on screen, and resolve once the person
+    is back in.
+
+    The box is a fixed overlay, so the admin page underneath is untouched: a
+    half-filled editor is still there, and still filled in, when it closes. That
+    is the point of it, since the alternative on an expired session was a reload
+    that threw the work away.
+
+    @returns {Promise<boolean>} */
+export function requireLogin() {
+  return new Promise(resolve => {
+    showLoginScreen(() => resolve(true));
+  });
+}
+
 function showLoginScreen(onLogin) {
   const s = el('login-screen');
   const btn = inpById('login-btn');

ui/js/admin-logic.js

@@ -126,6 +126,21 @@ export function shouldWritePassword({ enabled, newPassword }) {
   return !!enabled && !!(newPassword || '').length;
 }
 
+/* Signing in is itself two requests that answer with 401, and recovering from
+   those would put a sign-in box on top of a sign-in box: the first is how the
+   page asks whether anyone is signed in at all, and the second is how a wrong
+   password is reported. Every other 401 is a session that expired under someone
+   who is still working, which is what there is to recover from. */
+const NO_REAUTH = new Set(['/api/auth/check', '/api/auth/login']);
+
+/** Whether this failed request should send the user back through sign-in and
+    then be tried again.
+
+    @param {string} path @param {number} status @returns {boolean} */
+export function recoversSession(path, status) {
+  return status === 401 && !NO_REAUTH.has(String(path || '').split('?')[0]);
+}
+
 /* Why a settings save cannot go ahead. Codes rather than catalog keys: the
    message text is chosen at the call site, where the key can be written as a
    literal and traced back from the catalog. */

ui/js/admin-shared.js

@@ -1,5 +1,5 @@
 /* Stateless helpers shared by the admin modules. Mutable state stays out. */
-import { nextActiveIndex } from '/js/admin-logic.js?v=056a11e9';
+import { nextActiveIndex, recoversSession } from '/js/admin-logic.js?v=056a11e9';
 import { el, qa, q } from '/js/utils.js?v=84d58686';
 import { t } from '/js/i18n.js?v=133a7aac';
 
@@ -28,20 +28,50 @@ function tagged(status, body) {
   if (body && body.detail && typeof body.detail === 'object') e.detail = body.detail;
   return e;
 }
-export const ag = async p => {
+/* Set by the admin entry point rather than imported, because the sign-in screen
+   imports this module: importing it back would be a cycle, and one of the two
+   would be half-initialised at the moment it was needed. */
+/** @type {(() => Promise<boolean>) | null} */
+let _reauth = null;
+
+/** @param {() => Promise<boolean>} fn */
+export function setReauthHandler(fn) {
+  _reauth = fn;
+}
+
+/* One sign-in however many requests fail at once. A settings save makes three
+   writes in a row, and without this each would raise its own sign-in box over
+   the last. */
+/** @type {Promise<boolean> | null} */
+let _signingIn = null;
+function reauthenticate() {
+  if (!_reauth) return Promise.resolve(false);
+  if (!_signingIn) {
+    _signingIn = Promise.resolve(_reauth()).finally(() => {
+      _signingIn = null;
+    });
+  }
+  return _signingIn;
+}
+
+/* Retried once and only once: a second 401 after a successful sign-in is the
+   server refusing the request itself, and trying again would loop. */
+export const ag = async (p, recover = true) => {
   const r = await fetch(API + p, { cache: 'no-store' });
+  if (recover && recoversSession(p, r.status) && (await reauthenticate())) return ag(p, false);
   if (!r.ok) {
     const d = r.status === 401 ? null : await r.json().catch(() => null);
     throw tagged(r.status, d || (r.status === 401 ? { error: 'Unauthorised', kind: 'auth' } : null));
   }
   return r.json();
 };
-export const ap = async (p, b) => {
+export const ap = async (p, b, recover = true) => {
   const r = await fetch(API + p, {
     method: 'POST',
     headers: { 'Content-Type': 'application/json' },
     body: JSON.stringify(b),
   });
+  if (recover && recoversSession(p, r.status) && (await reauthenticate())) return ap(p, b, false);
   if (!r.ok) {
     const d = await r.json().catch(() => null);
     throw tagged(r.status, d || (r.status === 401 ? { error: 'Unauthorised', kind: 'auth' } : null));

ui/js/admin.js

@@ -10,8 +10,8 @@ import {
   snapshotItems,
   saveWithRevert,
 } from '/js/admin-save-logic.js?v=77cac1d1';
-import { API, toast, ag, ap, initInlineEdit } from '/js/admin-shared.js?v=bb36dbb3';
-import { checkAuth, wirePasswordStrength } from '/js/admin-auth.js?v=dd849d4c';
+import { API, toast, ag, ap, initInlineEdit, setReauthHandler } from '/js/admin-shared.js?v=bb36dbb3';
+import { checkAuth, requireLogin, wirePasswordStrength } from '/js/admin-auth.js?v=dd849d4c';
 import { state } from '/js/admin-state.js?v=3f9ad806';
 import { buildWidgetForm } from '/js/admin-widget-form.js?v=653071f0';
 import { buildAppForm, buildFolderForm, serializeKvRows } from '/js/admin-app-form.js?v=b46e5b19';
@@ -1484,6 +1484,12 @@ initBgType();
 initLogLevel();
 initLanguage();
 
+/* An expired session used to surface as "Save failed: Unauthorised" on every
+   attempt, with no way back to signing in short of a reload that discarded
+   whatever was being edited. Registered here rather than imported by the API
+   helpers, which the sign-in screen itself depends on. */
+setReauthHandler(requireLogin);
+
 checkAuth(load).then(ok => {
   if (!ok) return;
   load().catch(e => {

ui/test/admin-logic.test.mjs

@@ -15,6 +15,7 @@ import {
   refusedNoticeKey,
   settingsSaveBlocker,
   clearsStoredPassword,
+  recoversSession,
   BLOCK,
 } from '../js/admin-logic.js';
 /* The real strength check, so these assert the rule the save actually applies. */
@@ -457,3 +458,30 @@ test('switching protection off with no password stored has nothing to lose', ()
 test('leaving protection on never clears a password', () => {
   assert.equal(clearsStoredPassword({ enabled: true, passwordSet: true }), false);
 });
+
+/* ── recoversSession ──────────────────────────────────────────────────────── */
+
+test('a 401 on an ordinary write is a session to recover', () => {
+  assert.equal(recoversSession('/api/config', 401), true);
+  assert.equal(recoversSession('/api/auth/toggle', 401), true);
+});
+
+test('the two sign-in requests are answers, not sessions to recover', () => {
+  assert.equal(recoversSession('/api/auth/check', 401), false);
+  assert.equal(recoversSession('/api/auth/login', 401), false);
+});
+
+test('a query string does not hide the path', () => {
+  assert.equal(recoversSession('/api/auth/check?x=1', 401), false);
+});
+
+test('only a 401 means the session, not any other failure', () => {
+  /* An upstream service answering 401 arrives as a 502 carrying that status in
+     its detail, so a failing badge never raises a sign-in box. */
+  for (const s of [200, 400, 403, 500, 502]) assert.equal(recoversSession('/api/config', s), false, String(s));
+});
+
+test('recoversSession tolerates a missing path', () => {
+  assert.equal(recoversSession(undefined, 401), true);
+  assert.equal(recoversSession(null, 200), false);
+});

ui/test/admin-reauth.test.mjs

@@ -0,0 +1,136 @@
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import { register } from 'node:module';
+
+/* admin-shared imports its peers by their served paths, so map them first. */
+register('./js-root-hooks.mjs', import.meta.url);
+
+/* toast writes into an element; nothing here asserts on it. */
+globalThis.document = { getElementById: () => null, querySelectorAll: () => [], addEventListener() {} };
+
+const { ag, ap, setReauthHandler } = await import('../js/admin-shared.js');
+
+/* A session that expired under someone who was still working. The first request
+   answers 401, and whatever the queue does next decides whether their work
+   survives. */
+function fakeFetch(plan) {
+  const calls = [];
+  globalThis.fetch = async (url, opts) => {
+    calls.push({ url, method: opts?.method || 'GET' });
+    const status = plan.shift();
+    return {
+      ok: status < 400,
+      status,
+      /* A 401 comes back from the auth gate with no JSON body, which is the
+         case the helpers' fallback message exists for. */
+      json: async () => {
+        if (status >= 400) throw new Error('not json');
+        return { ok: true };
+      },
+    };
+  };
+  return calls;
+}
+
+test('a 401 signs back in and sends the same request again', async () => {
+  const calls = fakeFetch([401, 200]);
+  let signIns = 0;
+  setReauthHandler(async () => {
+    signIns++;
+    return true;
+  });
+
+  const out = await ap('/api/config', { items: [] });
+
+  assert.equal(signIns, 1, 'the sign-in box is raised once');
+  assert.equal(calls.length, 2, 'the save is retried rather than abandoned');
+  assert.deepEqual(
+    calls.map(c => c.method),
+    ['POST', 'POST'],
+    'the retry is the same write, not a read',
+  );
+  assert.deepEqual(out, { ok: true });
+});
+
+test('a second 401 after signing in gives up rather than looping', async () => {
+  fakeFetch([401, 401]);
+  let signIns = 0;
+  setReauthHandler(async () => {
+    signIns++;
+    return true;
+  });
+
+  await assert.rejects(ap('/api/config', {}), /Unauthorised/);
+  assert.equal(signIns, 1, 'the retry does not raise a second sign-in box');
+});
+
+test('declining to sign in reports the failure instead of hanging', async () => {
+  fakeFetch([401]);
+  setReauthHandler(async () => false);
+
+  await assert.rejects(ag('/api/config'), /Unauthorised/);
+});
+
+/* A settings save makes three writes in a row. Without one sign-in shared
+   between them, each would raise its own box over the last. */
+test('requests failing together share one sign-in', async () => {
+  fakeFetch([401, 401, 401, 200, 200, 200]);
+  let signIns = 0;
+  let release;
+  const gate = new Promise(r => {
+    release = r;
+  });
+  setReauthHandler(async () => {
+    signIns++;
+    await gate;
+    return true;
+  });
+
+  const all = Promise.all([ap('/api/config', {}), ap('/api/auth/toggle', {}), ag('/api/widgets')]);
+  await new Promise(r => setImmediate(r));
+  release();
+  await all;
+
+  assert.equal(signIns, 1, 'three failures, one sign-in');
+});
+
+/* Signing in is itself two requests that answer 401. Recovering from those
+   would put a sign-in box on top of the sign-in box. */
+test('the sign-in requests themselves never trigger a sign-in', async () => {
+  fakeFetch([401, 401]);
+  let signIns = 0;
+  setReauthHandler(async () => {
+    signIns++;
+    return true;
+  });
+
+  await assert.rejects(ag('/api/auth/check'), /Unauthorised/);
+  await assert.rejects(ap('/api/auth/login', { password: 'x' }), /Unauthorised/);
+  assert.equal(signIns, 0);
+});
+
+test('a later save signs in again, rather than the first one being the only chance', async () => {
+  fakeFetch([401, 200, 401, 200]);
+  let signIns = 0;
+  setReauthHandler(async () => {
+    signIns++;
+    return true;
+  });
+
+  await ap('/api/config', {});
+  await ap('/api/config', {});
+  assert.equal(signIns, 2, 'the shared sign-in is cleared once it resolves');
+});
+
+test('a request that succeeds never asks anyone to sign in', async () => {
+  const calls = fakeFetch([200]);
+  let signIns = 0;
+  setReauthHandler(async () => {
+    signIns++;
+    return true;
+  });
+
+  await ag('/api/config');
+  assert.equal(signIns, 0);
+  assert.equal(calls.length, 1);
+});