From fd19ae9d9e6e864c7376301b0ec70e1ce80ce05f Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:26:46 -0400 Subject: [PATCH] Fix: recover an admin session that expired mid-edit An expired session surfaced as "Save failed: Unauthorised" on every attempt, with no route back to signing in. The only way out was reloading the page, which discarded whatever was in the editor at the time, and nothing on screen said that was what had happened. Intercept a 401 in ag and ap, the two helpers every admin read and write goes through, raise the sign-in overlay, and reissue the request once the session is back. The overlay is fixed positioning over the page, so a half-filled editor survives the interruption untouched. The rule is recoversSession: only a 401, and never from the two requests that signing in is made of, which would put a sign-in box on top of a sign-in box. An upstream service answering 401 arrives as a 502 carrying that status in its detail, so a failing badge does not raise one. One sign-in is shared between requests that fail together, since a settings save makes three writes in a row, and a retry is never itself retried. --- CHANGELOG.md | 4 + ui/js/admin-auth.js | 15 ++++ ui/js/admin-logic.js | 15 ++++ ui/js/admin-shared.js | 36 ++++++++- ui/js/admin.js | 10 ++- ui/test/admin-logic.test.mjs | 28 +++++++ ui/test/admin-reauth.test.mjs | 136 ++++++++++++++++++++++++++++++++++ 7 files changed, 239 insertions(+), 5 deletions(-) create mode 100644 ui/test/admin-reauth.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 32d6abef..4430892a 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/ui/js/admin-auth.js b/ui/js/admin-auth.js index 7844d768..05fbf3a1 100644 --- a/ui/js/admin-auth.js +++ b/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} */ +export function requireLogin() { + return new Promise(resolve => { + showLoginScreen(() => resolve(true)); + }); +} + function showLoginScreen(onLogin) { const s = el('login-screen'); const btn = inpById('login-btn'); diff --git a/ui/js/admin-logic.js b/ui/js/admin-logic.js index 4d5bf883..d6d2ca5a 100644 --- a/ui/js/admin-logic.js +++ b/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. */ diff --git a/ui/js/admin-shared.js b/ui/js/admin-shared.js index 27f6a372..f88f0916 100644 --- a/ui/js/admin-shared.js +++ b/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) | null} */ +let _reauth = null; + +/** @param {() => Promise} 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 | 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)); diff --git a/ui/js/admin.js b/ui/js/admin.js index 08f969ec..338e622a 100644 --- a/ui/js/admin.js +++ b/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 => { diff --git a/ui/test/admin-logic.test.mjs b/ui/test/admin-logic.test.mjs index 0a754562..e6b261f1 100644 --- a/ui/test/admin-logic.test.mjs +++ b/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); +}); diff --git a/ui/test/admin-reauth.test.mjs b/ui/test/admin-reauth.test.mjs new file mode 100644 index 00000000..617770e6 --- /dev/null +++ b/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); +});