From 5f47b6af64738b518cfa3b1a8a49d06442970528 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 19 May 2026 12:04:31 +0100 Subject: [PATCH] test(admin): cover saveSettings round-trip + restart persistence (#7819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin saveSettings socket had zero direct backend coverage and the e2e 'restart works' test only checked the page renders after restart — neither catches a deployment that resets settings.json on restart, nor the user-visible workflow that triggered #7819 (add a top-level plugin block via Raw, save, watch it disappear). Adds three backend specs for the saveSettings socket: - payload is written byte-for-byte to settings.settingsFilename - augmenting existing JSON with a new top-level block round-trips through the next load reply - /* */ comments survive the write path Adds one e2e spec mirroring the #7819 workflow: open Raw, prepend an ep_oauth-shaped top-level block, save, restartEtherpad(), re-login, confirm the block is still in Raw and surfaces as its own Form-view section ('Ep oauth' from humanize()). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../backend/specs/admin/adminSettingsSave.ts | 220 ++++++++++++++++++ .../admin-spec/adminsettings.spec.ts | 60 +++++ 2 files changed, 280 insertions(+) create mode 100644 src/tests/backend/specs/admin/adminSettingsSave.ts diff --git a/src/tests/backend/specs/admin/adminSettingsSave.ts b/src/tests/backend/specs/admin/adminSettingsSave.ts new file mode 100644 index 00000000000..cab307352b9 --- /dev/null +++ b/src/tests/backend/specs/admin/adminSettingsSave.ts @@ -0,0 +1,220 @@ +'use strict'; + +import {strict as assert} from 'assert'; +import setCookieParser from 'set-cookie-parser'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +const io = require('socket.io-client'); +const common = require('../../common'); +const settings = require('../../../../node/utils/Settings'); + +// Mirrors the adminSocket helper in adminSettingsResolved.ts. Lifted here +// because the suite owns its own settings.settingsFilename stub and we +// don't want either suite's setup/teardown to step on the other. +const adminSocket = async () => { + settings.users = settings.users || {}; + settings.users['test-admin'] = {password: 'test-admin-password', is_admin: true}; + const savedRequireAuthentication = settings.requireAuthentication; + settings.requireAuthentication = true; + let res: any; + try { + res = await (common.agent as any) + .get('/admin/') + .auth('test-admin', 'test-admin-password'); + } finally { + settings.requireAuthentication = savedRequireAuthentication; + } + const resCookies = setCookieParser.parse(res, {map: true}); + const reqCookieHdr = Object.entries(resCookies) + .map(([name, cookie]: [string, any]) => + `${name}=${encodeURIComponent(cookie.value)}`) + .join('; '); + const socket = io(`${common.baseUrl}/settings`, { + forceNew: true, + query: {cookie: reqCookieHdr}, + }); + await new Promise((res, rej) => { + const onErr = (err: any) => { socket.off('connect', onConn); rej(err); }; + const onConn = () => { socket.off('connect_error', onErr); res(); }; + socket.once('connect', onConn); + socket.once('connect_error', onErr); + }); + return socket; +}; + +const PROBE_BUDGET_MS = 15000; +const adminSocketWithProbe = async (budgetMs: number): Promise<{ + ok: true; socket: any; +} | {ok: false; reason: string;}> => { + const deadline = Date.now() + budgetMs; + let socket: any; + try { + socket = await Promise.race([ + adminSocket(), + new Promise((_, rej) => + setTimeout(() => rej(new Error('adminSocket connect timed out')), + Math.max(0, deadline - Date.now()))), + ]); + } catch (err: any) { + return {ok: false, reason: String(err && err.message || err)}; + } + const remaining = Math.max(0, deadline - Date.now()); + const replied = new Promise((res) => socket.once('settings', () => res(true))); + socket.emit('load', null); + const probed = await Promise.race([ + replied, + new Promise((res) => setTimeout(() => res(false), remaining)), + ]); + if (!probed) { + socket.disconnect(); + return {ok: false, reason: `no \`settings\` reply within ${budgetMs}ms`}; + } + return {ok: true, socket}; +}; + +const ask = (socket: any, evt: string, payload: any, replyEvt: string) => + new Promise((res) => { + socket.once(replyEvt, res); + socket.emit(evt, payload); + }); + +const save = (socket: any, payload: string) => + new Promise<{status: string; detail?: any}>((res, rej) => { + const timeout = setTimeout( + () => rej(new Error('saveSettings: no saveprogress within 5s')), 5000); + socket.once('saveprogress', (status: string, detail: any) => { + clearTimeout(timeout); + res({status, detail}); + }); + socket.emit('saveSettings', payload); + }); + +// Regression coverage for issue #7819 / the broader observation that the +// admin saveSettings socket has zero backend coverage. The goal here is +// narrow: prove that whatever raw string the admin SPA emits ends up on +// disk byte-for-byte at settings.settingsFilename, and the subsequent +// `load` reply reflects the new file contents. We do NOT exercise +// runtime reload — that's reloadSettings()' job and is covered elsewhere. +describe(__filename, function () { + let socket: any; + let savedUsers: any; + let savedRequireAuthentication: boolean; + let savedSettingsFilename: any; + let tmpSettingsPath: string | null = null; + let baselineContents: string; + let setupCompleted = false; + + before(async function () { + this.timeout(60000); + await common.init(); + + savedSettingsFilename = settings.settingsFilename; + tmpSettingsPath = path.join(os.tmpdir(), + `etherpad-7819-settings-${process.pid}.json`); + // Realistic baseline: keys you'd find in a stock settings.json. + // Saved with two-space indent so we can later assert formatting is + // preserved through the write path. + baselineContents = JSON.stringify({ + title: 'Etherpad', + ip: '0.0.0.0', + port: 9001, + users: {admin: {password: 'changeme1', is_admin: true}}, + }, null, 2) + '\n'; + fs.writeFileSync(tmpSettingsPath, baselineContents); + settings.settingsFilename = tmpSettingsPath; + + savedUsers = settings.users; + savedRequireAuthentication = settings.requireAuthentication; + setupCompleted = true; + + const probe = await adminSocketWithProbe(PROBE_BUDGET_MS); + if (!probe.ok) { + console.warn( + `[adminSettingsSave] admin socket probe failed (${probe.reason}); ` + + 'skipping suite — likely an authenticate-hook plugin rejecting test creds.'); + this.skip(); + return; + } + socket = probe.socket; + }); + + after(function () { + if (socket) socket.disconnect(); + if (!setupCompleted) return; + settings.settingsFilename = savedSettingsFilename; + if (tmpSettingsPath) { + try { fs.unlinkSync(tmpSettingsPath); } catch { /* best effort */ } + } + if (settings.users) delete settings.users['test-admin']; + settings.users = savedUsers; + settings.requireAuthentication = savedRequireAuthentication; + }); + + // Reset to baseline between tests so each it() is independent — earlier + // suites in the same mocha run can leave behind state via shared sockets. + beforeEach(function () { + if (!tmpSettingsPath) this.skip(); + fs.writeFileSync(tmpSettingsPath!, baselineContents); + }); + + it('saveSettings writes the payload byte-for-byte to settings.settingsFilename', + async function () { + const payload = JSON.stringify({title: 'EtherpadWrittenViaSocket'}, null, 2); + const ack = await save(socket, payload); + assert.equal(ack.status, 'saved', 'saveprogress should be "saved"'); + const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8'); + assert.equal(onDisk, payload, + 'on-disk contents must equal the raw payload (no transform)'); + }); + + // The shape that triggered #7819: take an existing settings.json and add + // one new top-level block (a plugin config). The block must persist on + // disk verbatim and reappear in the next `load` reply. + it('augmenting existing JSON with a new top-level plugin block round-trips', + async function () { + const augmented = JSON.stringify({ + title: 'Etherpad', + ip: '0.0.0.0', + port: 9001, + ep_oauth: { + clientID: 'Iv1.testclient', + clientSecret: 'testsecret', + callbackURL: 'https://etherpad.example.com/auth/callback', + }, + users: {admin: {password: 'changeme1', is_admin: true}}, + }, null, 2); + + const ack = await save(socket, augmented); + assert.equal(ack.status, 'saved'); + + const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8'); + assert.equal(onDisk, augmented, + 'augmented JSON must be on disk verbatim'); + + // load() now reads the file we just wrote — `results` is the raw + // string, so it must contain the plugin block we added. + const reply: any = await ask(socket, 'load', null, 'settings'); + assert.equal(reply.results, augmented, + 'load.results must equal the file we just saved'); + assert.ok(reply.results.includes('"ep_oauth"'), + 'plugin block must be present in subsequent load'); + }); + + // /* */ comments are legal in the admin editor (jsonc-parser tolerates + // them; the SPA's isJSONClean strips them before validation). The save + // path must not normalize or strip them — the SPA test + // 'preserves /* */ comments after save round-trip' covers the UI side; + // this one covers the socket-level guarantee. + it('preserves /* */ comments in the written file', async function () { + const withComment = + '/* persisted-marker-7819 */\n' + + JSON.stringify({title: 'Etherpad'}, null, 2); + const ack = await save(socket, withComment); + assert.equal(ack.status, 'saved'); + const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8'); + assert.ok(onDisk.includes('persisted-marker-7819'), + 'comment must survive the write path'); + }); +}); diff --git a/src/tests/frontend-new/admin-spec/adminsettings.spec.ts b/src/tests/frontend-new/admin-spec/adminsettings.spec.ts index 46fcd3f47a0..6216c9eaa91 100644 --- a/src/tests/frontend-new/admin-spec/adminsettings.spec.ts +++ b/src/tests/frontend-new/admin-spec/adminsettings.spec.ts @@ -124,6 +124,66 @@ test.describe('admin settings',()=> { await expect(settings).not.toHaveValue('', {timeout: 30000}); }); + // Regression for https://github.com/ether/etherpad/issues/7819. + // The pre-existing 'restart works' test only proves the server comes + // back up; it never sets a custom value first, so a deployment that + // resets settings.json on restart (Docker layer wipe, init-container + // template render, snap reseed bug) would pass it. This test mirrors + // the user's actual workflow: open Raw, add a new top-level plugin + // block, save, restart, confirm the block is still there. + test('#7819 custom top-level block survives server restart', async ({page}) => { + await page.goto('http://localhost:9001/admin/settings'); + await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000}); + await page.getByTestId('mode-toggle-raw').click(); + const raw = page.getByTestId('settings-raw-textarea'); + await expect(raw).toBeVisible({timeout: 10000}); + const original = await raw.inputValue(); + expect(original.length).toBeGreaterThan(0); + + // Inject `"ep_oauth": {…},` right after the opening brace. This matches + // what a user adding a plugin config block would type — we keep every + // other key intact, no schema, just a new top-level object. + const augmented = original.replace( + /^(\s*\{)/, + '$1"ep_oauth":{"clientID":"persist-marker-7819","clientSecret":"x",' + + '"callbackURL":"http://x/cb"},', + ); + expect(augmented).not.toEqual(original); + expect(augmented).toContain('persist-marker-7819'); + + await raw.fill(augmented); + await saveSettings(page); + + await restartEtherpad(page); + await loginToAdmin(page, 'admin', 'changeme1'); + await page.goto('http://localhost:9001/admin/settings'); + await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000}); + await page.getByTestId('mode-toggle-raw').click(); + const rawAfter = page.getByTestId('settings-raw-textarea'); + await expect(rawAfter).toBeVisible({timeout: 10000}); + await expect(rawAfter).not.toHaveValue('', {timeout: 30000}); + + const afterRestart = await rawAfter.inputValue(); + expect(afterRestart).toContain('"ep_oauth"'); + expect(afterRestart).toContain('persist-marker-7819'); + + // The Form view must also surface the new block as its own section, + // since FormView renders every top-level object/array as a Section + // (admin/src/components/settings/FormView.tsx). humanize('ep_oauth') + // becomes 'Ep oauth'. + await page.getByTestId('mode-toggle-form').click(); + await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000}); + await expect(page.locator('[data-testid="settings-form-view"]')) + .toContainText(/Ep oauth/i); + + // Restore — DO NOT rely on the next test's cleanup; the file is + // shared across the serial run and a custom ep_oauth block could + // poison anything that asserts the exact length of settings. + await page.getByTestId('mode-toggle-raw').click(); + await rawAfter.fill(original); + await saveSettings(page); + }); + test('form view derives label + help text from key comment', async ({page}) => { await page.goto('http://localhost:9001/admin/settings'); await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});