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); +});