Skip to content

Commit abff812

Browse files
JohnMcLearclaude
andauthored
test(admin): cover saveSettings round-trip + restart persistence (#7819) (#7820)
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) <noreply@anthropic.com>
1 parent 271eb6a commit abff812

2 files changed

Lines changed: 280 additions & 0 deletions

File tree

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
'use strict';
2+
3+
import {strict as assert} from 'assert';
4+
import setCookieParser from 'set-cookie-parser';
5+
import * as fs from 'fs';
6+
import * as os from 'os';
7+
import * as path from 'path';
8+
9+
const io = require('socket.io-client');
10+
const common = require('../../common');
11+
const settings = require('../../../../node/utils/Settings');
12+
13+
// Mirrors the adminSocket helper in adminSettingsResolved.ts. Lifted here
14+
// because the suite owns its own settings.settingsFilename stub and we
15+
// don't want either suite's setup/teardown to step on the other.
16+
const adminSocket = async () => {
17+
settings.users = settings.users || {};
18+
settings.users['test-admin'] = {password: 'test-admin-password', is_admin: true};
19+
const savedRequireAuthentication = settings.requireAuthentication;
20+
settings.requireAuthentication = true;
21+
let res: any;
22+
try {
23+
res = await (common.agent as any)
24+
.get('/admin/')
25+
.auth('test-admin', 'test-admin-password');
26+
} finally {
27+
settings.requireAuthentication = savedRequireAuthentication;
28+
}
29+
const resCookies = setCookieParser.parse(res, {map: true});
30+
const reqCookieHdr = Object.entries(resCookies)
31+
.map(([name, cookie]: [string, any]) =>
32+
`${name}=${encodeURIComponent(cookie.value)}`)
33+
.join('; ');
34+
const socket = io(`${common.baseUrl}/settings`, {
35+
forceNew: true,
36+
query: {cookie: reqCookieHdr},
37+
});
38+
await new Promise<void>((res, rej) => {
39+
const onErr = (err: any) => { socket.off('connect', onConn); rej(err); };
40+
const onConn = () => { socket.off('connect_error', onErr); res(); };
41+
socket.once('connect', onConn);
42+
socket.once('connect_error', onErr);
43+
});
44+
return socket;
45+
};
46+
47+
const PROBE_BUDGET_MS = 15000;
48+
const adminSocketWithProbe = async (budgetMs: number): Promise<{
49+
ok: true; socket: any;
50+
} | {ok: false; reason: string;}> => {
51+
const deadline = Date.now() + budgetMs;
52+
let socket: any;
53+
try {
54+
socket = await Promise.race([
55+
adminSocket(),
56+
new Promise<never>((_, rej) =>
57+
setTimeout(() => rej(new Error('adminSocket connect timed out')),
58+
Math.max(0, deadline - Date.now()))),
59+
]);
60+
} catch (err: any) {
61+
return {ok: false, reason: String(err && err.message || err)};
62+
}
63+
const remaining = Math.max(0, deadline - Date.now());
64+
const replied = new Promise<true>((res) => socket.once('settings', () => res(true)));
65+
socket.emit('load', null);
66+
const probed = await Promise.race([
67+
replied,
68+
new Promise<false>((res) => setTimeout(() => res(false), remaining)),
69+
]);
70+
if (!probed) {
71+
socket.disconnect();
72+
return {ok: false, reason: `no \`settings\` reply within ${budgetMs}ms`};
73+
}
74+
return {ok: true, socket};
75+
};
76+
77+
const ask = (socket: any, evt: string, payload: any, replyEvt: string) =>
78+
new Promise<any>((res) => {
79+
socket.once(replyEvt, res);
80+
socket.emit(evt, payload);
81+
});
82+
83+
const save = (socket: any, payload: string) =>
84+
new Promise<{status: string; detail?: any}>((res, rej) => {
85+
const timeout = setTimeout(
86+
() => rej(new Error('saveSettings: no saveprogress within 5s')), 5000);
87+
socket.once('saveprogress', (status: string, detail: any) => {
88+
clearTimeout(timeout);
89+
res({status, detail});
90+
});
91+
socket.emit('saveSettings', payload);
92+
});
93+
94+
// Regression coverage for issue #7819 / the broader observation that the
95+
// admin saveSettings socket has zero backend coverage. The goal here is
96+
// narrow: prove that whatever raw string the admin SPA emits ends up on
97+
// disk byte-for-byte at settings.settingsFilename, and the subsequent
98+
// `load` reply reflects the new file contents. We do NOT exercise
99+
// runtime reload — that's reloadSettings()' job and is covered elsewhere.
100+
describe(__filename, function () {
101+
let socket: any;
102+
let savedUsers: any;
103+
let savedRequireAuthentication: boolean;
104+
let savedSettingsFilename: any;
105+
let tmpSettingsPath: string | null = null;
106+
let baselineContents: string;
107+
let setupCompleted = false;
108+
109+
before(async function () {
110+
this.timeout(60000);
111+
await common.init();
112+
113+
savedSettingsFilename = settings.settingsFilename;
114+
tmpSettingsPath = path.join(os.tmpdir(),
115+
`etherpad-7819-settings-${process.pid}.json`);
116+
// Realistic baseline: keys you'd find in a stock settings.json.
117+
// Saved with two-space indent so we can later assert formatting is
118+
// preserved through the write path.
119+
baselineContents = JSON.stringify({
120+
title: 'Etherpad',
121+
ip: '0.0.0.0',
122+
port: 9001,
123+
users: {admin: {password: 'changeme1', is_admin: true}},
124+
}, null, 2) + '\n';
125+
fs.writeFileSync(tmpSettingsPath, baselineContents);
126+
settings.settingsFilename = tmpSettingsPath;
127+
128+
savedUsers = settings.users;
129+
savedRequireAuthentication = settings.requireAuthentication;
130+
setupCompleted = true;
131+
132+
const probe = await adminSocketWithProbe(PROBE_BUDGET_MS);
133+
if (!probe.ok) {
134+
console.warn(
135+
`[adminSettingsSave] admin socket probe failed (${probe.reason}); ` +
136+
'skipping suite — likely an authenticate-hook plugin rejecting test creds.');
137+
this.skip();
138+
return;
139+
}
140+
socket = probe.socket;
141+
});
142+
143+
after(function () {
144+
if (socket) socket.disconnect();
145+
if (!setupCompleted) return;
146+
settings.settingsFilename = savedSettingsFilename;
147+
if (tmpSettingsPath) {
148+
try { fs.unlinkSync(tmpSettingsPath); } catch { /* best effort */ }
149+
}
150+
if (settings.users) delete settings.users['test-admin'];
151+
settings.users = savedUsers;
152+
settings.requireAuthentication = savedRequireAuthentication;
153+
});
154+
155+
// Reset to baseline between tests so each it() is independent — earlier
156+
// suites in the same mocha run can leave behind state via shared sockets.
157+
beforeEach(function () {
158+
if (!tmpSettingsPath) this.skip();
159+
fs.writeFileSync(tmpSettingsPath!, baselineContents);
160+
});
161+
162+
it('saveSettings writes the payload byte-for-byte to settings.settingsFilename',
163+
async function () {
164+
const payload = JSON.stringify({title: 'EtherpadWrittenViaSocket'}, null, 2);
165+
const ack = await save(socket, payload);
166+
assert.equal(ack.status, 'saved', 'saveprogress should be "saved"');
167+
const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
168+
assert.equal(onDisk, payload,
169+
'on-disk contents must equal the raw payload (no transform)');
170+
});
171+
172+
// The shape that triggered #7819: take an existing settings.json and add
173+
// one new top-level block (a plugin config). The block must persist on
174+
// disk verbatim and reappear in the next `load` reply.
175+
it('augmenting existing JSON with a new top-level plugin block round-trips',
176+
async function () {
177+
const augmented = JSON.stringify({
178+
title: 'Etherpad',
179+
ip: '0.0.0.0',
180+
port: 9001,
181+
ep_oauth: {
182+
clientID: 'Iv1.testclient',
183+
clientSecret: 'testsecret',
184+
callbackURL: 'https://etherpad.example.com/auth/callback',
185+
},
186+
users: {admin: {password: 'changeme1', is_admin: true}},
187+
}, null, 2);
188+
189+
const ack = await save(socket, augmented);
190+
assert.equal(ack.status, 'saved');
191+
192+
const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
193+
assert.equal(onDisk, augmented,
194+
'augmented JSON must be on disk verbatim');
195+
196+
// load() now reads the file we just wrote — `results` is the raw
197+
// string, so it must contain the plugin block we added.
198+
const reply: any = await ask(socket, 'load', null, 'settings');
199+
assert.equal(reply.results, augmented,
200+
'load.results must equal the file we just saved');
201+
assert.ok(reply.results.includes('"ep_oauth"'),
202+
'plugin block must be present in subsequent load');
203+
});
204+
205+
// /* */ comments are legal in the admin editor (jsonc-parser tolerates
206+
// them; the SPA's isJSONClean strips them before validation). The save
207+
// path must not normalize or strip them — the SPA test
208+
// 'preserves /* */ comments after save round-trip' covers the UI side;
209+
// this one covers the socket-level guarantee.
210+
it('preserves /* */ comments in the written file', async function () {
211+
const withComment =
212+
'/* persisted-marker-7819 */\n' +
213+
JSON.stringify({title: 'Etherpad'}, null, 2);
214+
const ack = await save(socket, withComment);
215+
assert.equal(ack.status, 'saved');
216+
const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
217+
assert.ok(onDisk.includes('persisted-marker-7819'),
218+
'comment must survive the write path');
219+
});
220+
});

src/tests/frontend-new/admin-spec/adminsettings.spec.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,66 @@ test.describe('admin settings',()=> {
124124
await expect(settings).not.toHaveValue('', {timeout: 30000});
125125
});
126126

127+
// Regression for https://github.com/ether/etherpad/issues/7819.
128+
// The pre-existing 'restart works' test only proves the server comes
129+
// back up; it never sets a custom value first, so a deployment that
130+
// resets settings.json on restart (Docker layer wipe, init-container
131+
// template render, snap reseed bug) would pass it. This test mirrors
132+
// the user's actual workflow: open Raw, add a new top-level plugin
133+
// block, save, restart, confirm the block is still there.
134+
test('#7819 custom top-level block survives server restart', async ({page}) => {
135+
await page.goto('http://localhost:9001/admin/settings');
136+
await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
137+
await page.getByTestId('mode-toggle-raw').click();
138+
const raw = page.getByTestId('settings-raw-textarea');
139+
await expect(raw).toBeVisible({timeout: 10000});
140+
const original = await raw.inputValue();
141+
expect(original.length).toBeGreaterThan(0);
142+
143+
// Inject `"ep_oauth": {…},` right after the opening brace. This matches
144+
// what a user adding a plugin config block would type — we keep every
145+
// other key intact, no schema, just a new top-level object.
146+
const augmented = original.replace(
147+
/^(\s*\{)/,
148+
'$1"ep_oauth":{"clientID":"persist-marker-7819","clientSecret":"x",' +
149+
'"callbackURL":"http://x/cb"},',
150+
);
151+
expect(augmented).not.toEqual(original);
152+
expect(augmented).toContain('persist-marker-7819');
153+
154+
await raw.fill(augmented);
155+
await saveSettings(page);
156+
157+
await restartEtherpad(page);
158+
await loginToAdmin(page, 'admin', 'changeme1');
159+
await page.goto('http://localhost:9001/admin/settings');
160+
await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
161+
await page.getByTestId('mode-toggle-raw').click();
162+
const rawAfter = page.getByTestId('settings-raw-textarea');
163+
await expect(rawAfter).toBeVisible({timeout: 10000});
164+
await expect(rawAfter).not.toHaveValue('', {timeout: 30000});
165+
166+
const afterRestart = await rawAfter.inputValue();
167+
expect(afterRestart).toContain('"ep_oauth"');
168+
expect(afterRestart).toContain('persist-marker-7819');
169+
170+
// The Form view must also surface the new block as its own section,
171+
// since FormView renders every top-level object/array as a Section
172+
// (admin/src/components/settings/FormView.tsx). humanize('ep_oauth')
173+
// becomes 'Ep oauth'.
174+
await page.getByTestId('mode-toggle-form').click();
175+
await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
176+
await expect(page.locator('[data-testid="settings-form-view"]'))
177+
.toContainText(/Ep oauth/i);
178+
179+
// Restore — DO NOT rely on the next test's cleanup; the file is
180+
// shared across the serial run and a custom ep_oauth block could
181+
// poison anything that asserts the exact length of settings.
182+
await page.getByTestId('mode-toggle-raw').click();
183+
await rawAfter.fill(original);
184+
await saveSettings(page);
185+
});
186+
127187
test('form view derives label + help text from key comment', async ({page}) => {
128188
await page.goto('http://localhost:9001/admin/settings');
129189
await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});

0 commit comments

Comments
 (0)