Skip to content

Commit d9dabe3

Browse files
JohnMcLearclaude
andauthored
feat(admin): explain env-var substitution in /settings, surface auth errors (#7819) (#7826)
* feat(admin): explain env-var substitution in /settings, surface auth errors (#7819) Three small, env-var-only UX improvements driven by issue #7819, where a Docker operator saved an ep_oauth block in the admin /settings raw view and reported it "disappeared" — but the underlying confusion was that settings.json on disk is a *template*, not the effective config. None of these changes is visible to installs that don't use ${VAR} placeholders. * Banner above the editor explaining the template/env-substitution model, only rendered when the loaded file contains a ${VAR} placeholder. Tells the operator that the file is not env-substituted in place and that the Effective tab shows the live values. * Effective tab in the mode toggle, read-only, also gated on ${VAR}. The backend was already emitting redacted runtime settings as `resolved` alongside every `load`; the SPA now exposes them so an operator can verify what Etherpad is actually using. * admin_auth_error event from the /settings socket handler. The handler previously silently returned when the connecting session wasn't admin, which made misrouted Traefik+SSO auth look like "save did nothing" with no error path in the UI. Emit a dedicated event before dropping the socket so the SPA can show a clear toast. Tests: - src/tests/backend/specs/admin/adminSettingsAuthError.ts — new spec for the auth_error/disconnect contract. - src/tests/frontend-new/admin-spec/adminsettings.spec.ts — new Playwright test asserting the banner + Effective tab only appear after a ${VAR} is added to settings.json, and that the Effective view is read-only + shows [REDACTED] for secrets. No behaviour change for installs without ${VAR} placeholders — banner, Effective tab, and auth-error contract are all the same as before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(admin): drop fragile pre-condition + add reconnect-loop guard (#7819) CI's admin-UI workflow seeds settings.json by copying settings.json.template verbatim, which contains ~30 \${VAR} placeholders. The new Playwright test asserted "banner not present before adding placeholder" — true on a fresh dev machine, false in CI. Drop that assertion: the negative path is covered by the SettingsPage ENV_VAR_PATTERN regex itself; what matters at the UI level is the positive path (banner + Effective tab render correctly when placeholders are present), which this test still exercises. Also: the server's admin_auth_error path calls socket.disconnect(), which the SPA's existing disconnect handler interprets as "io server disconnect" and immediately reconnects — creating a reject/reconnect loop. Track an authErrored flag and suppress the reconnect once an auth_error has been received. Reset on successful connect, so a legitimate re-auth path still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 40f0a40 commit d9dabe3

9 files changed

Lines changed: 266 additions & 42 deletions

File tree

admin/src/App.css

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,37 @@ textarea.settings:focus {
3434
border-top: 1px solid #ddd;
3535
}
3636

37+
/* --- env-var banner --- */
38+
/* Shown only when settings.json contains ${VAR} placeholders. The
39+
typical reader is a Docker/K8s operator who has just been surprised
40+
by env-var substitution semantics, so the copy must explain rather
41+
than warn — visual weight matches a note, not an error. */
42+
.settings-envvar-banner {
43+
display: flex;
44+
align-items: flex-start;
45+
gap: 12px;
46+
padding: 12px 16px;
47+
margin-bottom: 16px;
48+
background: #f5f7ff;
49+
border: 1px solid #c7d2fe;
50+
border-radius: 6px;
51+
color: #1e293b;
52+
}
53+
.settings-envvar-banner svg {
54+
flex-shrink: 0;
55+
color: #4f46e5;
56+
margin-top: 2px;
57+
}
58+
.settings-envvar-banner strong {
59+
display: block;
60+
margin-bottom: 4px;
61+
}
62+
.settings-envvar-banner p {
63+
margin: 0;
64+
font-size: 13px;
65+
line-height: 1.5;
66+
}
67+
3768
/* --- mode toggle --- */
3869
.settings-mode-toggle {
3970
display: inline-flex;

admin/src/App.tsx

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,19 @@ export const App = () => {
3232

3333
const settingSocket = connect(`${WS_URL}/settings`, {transports: ['websocket']});
3434
const pluginsSocket = connect(`${WS_URL}/pluginfw/installer`, {transports: ['websocket']})
35+
// When the server explicitly rejects us for not being admin, we must
36+
// NOT reconnect on the subsequent `disconnect` event — otherwise the
37+
// socket cycles forever (connect → admin_auth_error → server
38+
// disconnect → SPA auto-reconnect → …). The flag is reset on each
39+
// successful connect.
40+
let authErrored = false;
3541

3642
pluginsSocket.on('connect', () => {
3743
useStore.getState().setPluginsSocket(pluginsSocket);
3844
});
3945

4046
settingSocket.on('connect', () => {
47+
authErrored = false;
4148
useStore.getState().setSettingsSocket(settingSocket);
4249
useStore.getState().setShowLoading(false)
4350
settingSocket.emit('load');
@@ -46,7 +53,7 @@ export const App = () => {
4653

4754
settingSocket.on('disconnect', (reason) => {
4855
useStore.getState().setShowLoading(true)
49-
if (reason === 'io server disconnect') settingSocket.connect();
56+
if (reason === 'io server disconnect' && !authErrored) settingSocket.connect();
5057
});
5158

5259
settingSocket.on('settings', (settings: any) => {
@@ -75,6 +82,22 @@ export const App = () => {
7582
const detail = payload?.message ?? '';
7683
setToastState({open: true, title: t('admin_settings.toast.save_failed') + (detail ? ` (${detail})` : ''), success: false});
7784
}
85+
});
86+
87+
// Backend emits this when the connecting socket does not have an
88+
// admin session. Previously the server just dropped silently, so the
89+
// SPA would sit on a loading screen with no clue. Surface it AND
90+
// suppress the automatic reconnect — without this flag the SPA would
91+
// immediately reconnect to a socket that will reject it again.
92+
settingSocket.on('admin_auth_error', (payload?: {message?: string}) => {
93+
authErrored = true;
94+
const {setToastState} = useStore.getState();
95+
setToastState({
96+
open: true,
97+
title: payload?.message || t('admin_settings.toast.auth_error'),
98+
success: false,
99+
});
100+
useStore.getState().setShowLoading(false);
78101
})
79102

80103
return () => {

admin/src/components/settings/ModeToggle.tsx

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
import { Trans, useTranslation } from 'react-i18next';
22

3-
export type Mode = 'form' | 'raw';
3+
export type Mode = 'form' | 'raw' | 'effective';
44

55
type Props = {
66
mode: Mode;
77
onChange: (mode: Mode) => void;
8+
// When false, the Effective tab is hidden. We hide it for installs that
9+
// aren't using env-var substitution at all — there's no useful difference
10+
// between the raw file and the effective in-memory config for them.
11+
showEffective?: boolean;
812
};
913

10-
export const ModeToggle = ({ mode, onChange }: Props) => {
14+
export const ModeToggle = ({ mode, onChange, showEffective = false }: Props) => {
1115
const { t } = useTranslation();
1216
return (
1317
<div className="settings-mode-toggle" role="tablist" aria-label={t('admin_settings.mode.aria_label')}>
@@ -31,6 +35,19 @@ export const ModeToggle = ({ mode, onChange }: Props) => {
3135
>
3236
<Trans i18nKey="admin_settings.mode.raw" />
3337
</button>
38+
{showEffective && (
39+
<button
40+
type="button"
41+
role="tab"
42+
aria-selected={mode === 'effective'}
43+
data-testid="mode-toggle-effective"
44+
className={mode === 'effective' ? 'active' : ''}
45+
onClick={() => onChange('effective')}
46+
title={t('admin_settings.mode.effective_tooltip')}
47+
>
48+
<Trans i18nKey="admin_settings.mode.effective" />
49+
</button>
50+
)}
3451
</div>
3552
);
3653
};

admin/src/pages/SettingsPage.tsx

Lines changed: 89 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,42 @@
1-
import React, { useState } from 'react';
1+
import React, { useEffect, useMemo, useState } from 'react';
22
import { useStore } from '../store/store';
33
import { isJSONClean, cleanComments } from '../utils/utils';
44
import { Trans, useTranslation } from 'react-i18next';
55
import { IconButton } from '../components/IconButton';
6-
import { RotateCw, Save, AlignLeft, ShieldCheck } from 'lucide-react';
6+
import { RotateCw, Save, AlignLeft, ShieldCheck, Info } from 'lucide-react';
77
import { FormView } from '../components/settings/FormView';
88
import { ModeToggle, type Mode } from '../components/settings/ModeToggle';
99

1010
const TAB_INDENT = ' ';
1111

12+
// Heuristic: `${VAR}` or `${VAR:default}` in the file means the operator is
13+
// running with env-var substitution (overwhelmingly Docker / Kubernetes).
14+
// We use this to gate the Docker-aware UX (the explanatory banner and the
15+
// Effective-config tab) so non-container installs see the existing UI
16+
// unchanged. Conservative on purpose — false negatives just keep the old
17+
// behaviour.
18+
const ENV_VAR_PATTERN = /\$\{[A-Za-z_][A-Za-z0-9_]*(?::[^}]*)?\}/;
19+
1220
export const SettingsPage = () => {
1321
const { t } = useTranslation();
1422
const settingsSocket = useStore(state => state.settingsSocket);
1523
const settings = useStore(state => state.settings) ?? '';
24+
const resolved = useStore(state => state.resolved);
25+
26+
const usesEnvVars = useMemo(() => ENV_VAR_PATTERN.test(settings), [settings]);
1627

1728
const [mode, setMode] = useState<Mode>('form');
1829
const [exposeExperimental] = useState(false);
1930

31+
// The Effective tab is only meaningful when there is a `resolved`
32+
// payload AND the file uses substitution. Falling back to Raw on
33+
// either condition keeps the toggle honest if the user opens this
34+
// page against an older server.
35+
const canShowEffective = usesEnvVars && resolved != null;
36+
useEffect(() => {
37+
if (mode === 'effective' && !canShowEffective) setMode('raw');
38+
}, [mode, canShowEffective]);
39+
2040
// Tab in textarea inserts two spaces instead of moving focus; rAF restores caret position after React re-renders.
2141
const handleKeyDown = (e: React.KeyboardEvent<HTMLTextAreaElement>) => {
2242
if (e.key !== 'Tab') return;
@@ -57,49 +77,80 @@ export const SettingsPage = () => {
5777
settingsSocket.emit('saveSettings', settings);
5878
};
5979

80+
const effectiveJson = useMemo(() => {
81+
if (resolved == null) return '';
82+
try { return JSON.stringify(resolved, null, 2); } catch { return ''; }
83+
}, [resolved]);
84+
6085
return (
6186
<div className="settings-page">
6287
<h1><Trans i18nKey="admin_settings.current" /></h1>
6388

64-
<ModeToggle mode={mode} onChange={setMode} />
65-
66-
{mode === 'form'
67-
? <FormView onSwitchToRaw={() => setMode('raw')} />
68-
: (
69-
<textarea
70-
value={settings}
71-
className="settings"
72-
data-testid="settings-raw-textarea"
73-
spellCheck={false}
74-
onKeyDown={handleKeyDown}
75-
onChange={v => useStore.getState().setSettings(v.target.value)}
76-
/>
77-
)
78-
}
89+
{usesEnvVars && (
90+
<div
91+
className="settings-envvar-banner"
92+
role="note"
93+
data-testid="settings-envvar-banner"
94+
>
95+
<Info size={18} aria-hidden="true" />
96+
<div>
97+
<strong><Trans i18nKey="admin_settings.envvar_banner.title" /></strong>
98+
<p><Trans i18nKey="admin_settings.envvar_banner.body" /></p>
99+
</div>
100+
</div>
101+
)}
79102

80-
<div className="settings-button-bar">
81-
<IconButton
82-
className="settingsButton"
83-
data-testid="save-settings-button"
84-
icon={<Save />}
85-
title={<Trans i18nKey="admin_settings.current_save.value" />}
86-
onClick={handleSave}
103+
<ModeToggle mode={mode} onChange={setMode} showEffective={canShowEffective} />
104+
105+
{mode === 'form' && <FormView onSwitchToRaw={() => setMode('raw')} />}
106+
{mode === 'raw' && (
107+
<textarea
108+
value={settings}
109+
className="settings"
110+
data-testid="settings-raw-textarea"
111+
spellCheck={false}
112+
onKeyDown={handleKeyDown}
113+
onChange={v => useStore.getState().setSettings(v.target.value)}
87114
/>
88-
<IconButton
89-
className="settingsButton"
90-
data-testid="test-settings-button"
91-
icon={<ShieldCheck />}
92-
title={<Trans i18nKey="admin_settings.current_test.value" />}
93-
onClick={testJSON}
115+
)}
116+
{mode === 'effective' && (
117+
<textarea
118+
value={effectiveJson}
119+
className="settings"
120+
data-testid="settings-effective-textarea"
121+
spellCheck={false}
122+
readOnly
123+
aria-readonly="true"
94124
/>
95-
{exposeExperimental && (
96-
<IconButton
97-
className="settingsButton"
98-
data-testid="prettify-settings-button"
99-
icon={<AlignLeft />}
100-
title={<Trans i18nKey="admin_settings.current_prettify.value" />}
101-
onClick={prettifyJSON}
102-
/>
125+
)}
126+
127+
<div className="settings-button-bar">
128+
{mode !== 'effective' && (
129+
<>
130+
<IconButton
131+
className="settingsButton"
132+
data-testid="save-settings-button"
133+
icon={<Save />}
134+
title={<Trans i18nKey="admin_settings.current_save.value" />}
135+
onClick={handleSave}
136+
/>
137+
<IconButton
138+
className="settingsButton"
139+
data-testid="test-settings-button"
140+
icon={<ShieldCheck />}
141+
title={<Trans i18nKey="admin_settings.current_test.value" />}
142+
onClick={testJSON}
143+
/>
144+
{exposeExperimental && (
145+
<IconButton
146+
className="settingsButton"
147+
data-testid="prettify-settings-button"
148+
icon={<AlignLeft />}
149+
title={<Trans i18nKey="admin_settings.current_prettify.value" />}
150+
onClick={prettifyJSON}
151+
/>
152+
)}
153+
</>
103154
)}
104155
<IconButton
105156
className="settingsButton"

src/locales/en.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,12 @@
137137
"admin_settings.prettify_confirm": "Prettifying will remove all comments. Continue?",
138138
"admin_settings.mode.form": "Form",
139139
"admin_settings.mode.raw": "Raw",
140+
"admin_settings.mode.effective": "Effective",
141+
"admin_settings.mode.effective_tooltip": "Read-only view of the values Etherpad is actually using right now, after environment-variable substitution. Secrets are redacted.",
140142
"admin_settings.mode.aria_label": "Editor mode",
143+
"admin_settings.envvar_banner.title": "This file is a template, not the live config.",
144+
"admin_settings.envvar_banner.body": "Placeholders like ${VAR:default} are substituted into memory at startup; they are never written back to this file. Edit env vars in your environment (Docker compose, systemd, .env) to change the resolved value, or replace the placeholder here with a literal. Switch to the Effective tab to see what Etherpad is using right now.",
145+
"admin_settings.toast.auth_error": "You are not authenticated as admin. Please log in again.",
141146
"admin_settings.section.general": "General",
142147
"admin_settings.parse_error.title": "Cannot parse settings.json",
143148
"admin_settings.parse_error.cta": "Switch to raw to edit",

src/node/hooks/express/adminsettings.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,17 @@ exports.socketio = (hookName: string, {io}: any) => {
5050
io.of('/settings').on('connection', (socket: any) => {
5151
// @ts-ignore
5252
const {session: {user: {is_admin: isAdmin} = {}} = {}} = socket.conn.request;
53-
if (!isAdmin) return;
53+
if (!isAdmin) {
54+
// Previously this branch silently returned, so a non-admin client
55+
// (e.g. a misrouted Traefik / OIDC session that didn't carry the
56+
// admin cookie) would connect, emit load/save, and get nothing
57+
// back — no error toast, no way to tell the save was ignored. Emit
58+
// a dedicated event the SPA can surface, then drop the socket.
59+
socket.emit('admin_auth_error',
60+
{message: 'Not authenticated as admin. Re-authenticate and retry.'});
61+
socket.disconnect(true);
62+
return;
63+
}
5464

5565
socket.on('load', async (query: string): Promise<any> => {
5666
let data;
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
// Regression coverage for #7819 follow-up: non-admin sockets used to be
4+
// silently dropped, which made misconfigured admin auth (e.g. Traefik +
5+
// SSO + cross-origin iframe losing the cookie) look like "save didn't
6+
// work" with no error path. The connection handler now emits a dedicated
7+
// `admin_auth_error` event before disconnecting so the SPA can surface it.
8+
9+
import {strict as assert} from 'assert';
10+
11+
const io = require('socket.io-client');
12+
const common = require('../../common');
13+
14+
const connectAnonymous = (): any =>
15+
io(`${common.baseUrl}/settings`, {
16+
forceNew: true,
17+
// Deliberately omit cookie / auth — this mirrors a socket from a
18+
// session that resolves through express-session but lacks is_admin.
19+
transports: ['websocket'],
20+
});
21+
22+
describe(__filename, function () {
23+
this.timeout(15000);
24+
25+
before(async function () {
26+
await common.init();
27+
});
28+
29+
it('emits admin_auth_error and disconnects when not authenticated as admin',
30+
async function () {
31+
const socket = connectAnonymous();
32+
const result = await new Promise<{event: 'auth_error' | 'disconnect' | 'timeout'; payload?: any}>((res) => {
33+
const timeout = setTimeout(
34+
() => { try { socket.disconnect(); } catch {} res({event: 'timeout'}); },
35+
8000);
36+
socket.once('admin_auth_error', (payload: any) => {
37+
clearTimeout(timeout);
38+
res({event: 'auth_error', payload});
39+
});
40+
socket.once('disconnect', () => {
41+
clearTimeout(timeout);
42+
res({event: 'disconnect'});
43+
});
44+
});
45+
try { socket.disconnect(); } catch {}
46+
// Either order is acceptable (auth_error then disconnect, or just
47+
// disconnect if the event arrives after the close handshake) — but
48+
// a timeout is a regression: the silent-no-op path is back.
49+
assert.notEqual(result.event, 'timeout',
50+
'admin_auth_error or disconnect must arrive within 8s for non-admin socket');
51+
if (result.event === 'auth_error') {
52+
assert.ok(result.payload && typeof result.payload.message === 'string',
53+
`admin_auth_error must carry a message; got ${JSON.stringify(result.payload)}`);
54+
}
55+
});
56+
});

src/tests/backend/specs/admin/adminSettingsSave.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,4 +217,5 @@ describe(__filename, function () {
217217
assert.ok(onDisk.includes('persisted-marker-7819'),
218218
'comment must survive the write path');
219219
});
220+
220221
});

0 commit comments

Comments
 (0)