test(docker): admin save persists across container restart (#7819)

[JohnMcLear]

Summary

Follow-up to #7820. Adds the Docker-container layer of regression coverage for #7819, which the OP confirmed they're hitting on the official Docker image.

Two layers:

1. src/tests/container/specs/api/adminSettings_7819.ts — mocha spec that runs inside pnpm run test-container against the live Docker image. Authenticates against /admin, opens the /settings socket, saves an augmented JSON with an ep_oauth-shaped top-level block, and asserts the next load reply contains the marker. Intentionally leaves the marker on disk so the workflow can verify file persistence.
2. .github/workflows/docker.yml — build-test job now passes ADMIN_PASSWORD=changeme1 so the spec can authenticate, then after test-container it docker execs into the container and greps for the marker (proves the socket-driven save actually wrote to /opt/etherpad-lite/settings.json), docker restarts, waits for the health probe, and re-greps (proves an in-place restart doesn't reset the file — the failure mode a docker-compose user with restart: always would see on host reboot).

A recreate (docker rm + docker run) would wipe the file because settings.json lives in the image layer, not on a volume — that's expected behavior in the official Dockerfile and explicitly out of scope here.

Why not in #7820

#7820 was merged before this could be added. The two halves are independent: #7820 covers the in-process admin flow + Etherpad restart; this covers the Docker layer + container restart.

Test plan

• pnpm run ts-check (passes)
• CI's Docker / build-test job exercises the full chain (image build → container start → save via socket → grep marker on disk → docker restart → re-grep)

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Add Docker regression test for admin settings persistence across restart

🧪 Tests

Walkthroughs

Description

• Add Docker container regression test for admin settings persistence
• New mocha spec validates socket-driven save writes to disk correctly
• Verify settings survive in-place container restart via docker restart
• Extend test-container timeout and discovery to support new TypeScript spec

Diagram

flowchart LR
  A["New adminSettings_7819.ts spec"] -->|authenticates| B["Admin /settings socket"]
  B -->|saves marker| C["settings.json on disk"]
  D["docker.yml workflow"] -->|runs spec| A
  D -->|greps marker| C
  D -->|docker restart| E["Container restart"]
  E -->|greps marker again| C
  C -->|marker persists| F["Regression test passes"]

Loading

File Changes

1. src/tests/container/specs/api/adminSettings_7819.ts 🧪 Tests +114/-0

New Docker container spec for settings persistence

• New mocha spec that authenticates to admin /settings socket with credentials from ADMIN_PASSWORD
 env var
• Loads baseline settings, injects ep_oauth marker block via textual splice, saves via socket
• Verifies marker persists in next load reply, proving file write succeeded
• Intentionally leaves marker on disk for workflow verification of actual file persistence

src/tests/container/specs/api/adminSettings_7819.ts

---

2. .github/workflows/docker.yml 🧪 Tests +35/-1

Add workflow steps for settings persistence verification

• Pass ADMIN_PASSWORD=changeme1 environment variable to docker run so settings.json.docker
 provisions admin user
• Add docker exec grep checks after test-container to verify marker reached
 /opt/etherpad-lite/settings.json on disk
• Add docker restart sequence with health probe wait, then re-grep to prove marker survives
 container restart
• Includes detailed error messages and container logs on failure for debugging

.github/workflows/docker.yml

---

3. src/package.json ⚙️ Configuration changes +1/-1

Extend test-container timeout and TypeScript discovery

• Increase test-container timeout from 5000ms to 30000ms to accommodate socket connect and save
 round-trip
• Add ts extension to mocha discovery so new TypeScript spec adminSettings_7819.ts is picked up

src/package.json

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Admin auth endpoint wrong ✓ Resolved 🐞 Bug ≡ Correctness

Description

adminCookieHeader() requests GET /admin/ with Basic Auth, but when requireAuthentication is false
(Docker default) that route is authorized without running the authentication flow, so no admin
session cookie is created and the /settings socket never registers handlers. The test will
hang/timeout on load() because adminsettings.ts ignores non-admin socket connections.

Code

src/tests/container/specs/api/adminSettings_7819.ts[R23-30]

+const adminCookieHeader = async (): Promise<string> => {
+  const res: any = await supertest(BASE_URL)
+      .get('/admin/')
+      .auth(ADMIN_USER, ADMIN_PASSWORD);
+  const cookies = setCookieParser.parse(res, {map: true}) as Record<string, any>;
+  return Object.entries(cookies)
+      .map(([name, cookie]) => `${name}=${encodeURIComponent(cookie.value)}`)
+      .join('; ');

Evidence

The access-control middleware only forces authentication for paths starting with /admin-auth; if
requireAuthentication is false and the request is not /admin-auth/*, authorize() succeeds
without authenticating and therefore does not establish a session cookie. The /settings socket
handler then immediately returns for non-admin sessions, so the test never receives a settings
event.

src/tests/container/specs/api/adminSettings_7819.ts[23-31]
settings.json.docker[470-476]
src/node/hooks/express/webaccess.ts[59-66]
src/node/hooks/express/webaccess.ts[128-132]
src/node/hooks/express/webaccess.ts[143-148]
src/node/hooks/express/openapi-admin.ts[30-38]
src/node/hooks/express/adminsettings.ts[49-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The container spec tries to establish an admin session by requesting `GET /admin/` with Basic Auth, but `/admin/` does not force authentication when `requireAuthentication` is false (the default in `settings.json.docker`). As a result, no admin session cookie is set and the `/settings` namespace handlers never get attached (because the server checks `session.user.is_admin`).
### Issue Context
`/admin-auth/` is the dedicated endpoint for establishing/verifying an admin session cookie.
### Fix Focus Areas
- src/tests/container/specs/api/adminSettings_7819.ts[23-31]
- src/node/hooks/express/openapi-admin.ts[30-38]
- src/node/hooks/express/webaccess.ts[59-66]
- src/node/hooks/express/webaccess.ts[128-132]
### Suggested fix
- Change the request in `adminCookieHeader()` to hit `/admin-auth/` (GET is used elsewhere in tests; POST is also acceptable) with Basic Auth.
- Assert that the response indicates success (e.g., HTTP 200) and that at least one cookie is returned; otherwise fail fast with a clear error message.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Brace splice misses comments ✓ Resolved 🐞 Bug ≡ Correctness

Description

The marker injection uses originalRaw.replace(/^(\s*\{)/, ...), which only matches if the file
begins with { after whitespace. In the Docker image, settings.json is copied from
settings.json.docker, which starts with comment blocks, so the splice becomes a no-op and the test
fails its own assert.notEqual().

Code

src/tests/container/specs/api/adminSettings_7819.ts[R93-98]

+    const augmented = originalRaw.replace(
+        /^(\s*\{)/,
+        `$1"ep_oauth":{"clientID":"${MARKER}","clientSecret":"x",` +
+        '"callbackURL":"http://x/cb"},',
+    );
+    assert.notEqual(augmented, originalRaw, 'splice should have changed the string');

Evidence

The Docker image copies settings.json.docker directly to settings.json, and
settings.json.docker begins with comment blocks, so the raw file content loaded via the admin
settings socket does not start with {. Because the test’s regex is anchored to the beginning of
the string, the replace does nothing.

src/tests/container/specs/api/adminSettings_7819.ts[89-99]
settings.json.docker[1-12]
Dockerfile[46-47]
Dockerfile[126-128]
src/node/hooks/express/adminsettings.ts[55-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test tries to inject a top-level `ep_oauth` block by replacing only an opening `{` at the very start of the file. Docker’s `settings.json` begins with comment blocks (copied verbatim from `settings.json.docker`), so the regex never matches and the injection does not happen.
### Issue Context
The `/settings` socket `load` path emits the *raw* `settings.json` file contents from disk, including comments.
### Fix Focus Areas
- src/tests/container/specs/api/adminSettings_7819.ts[89-99]
- settings.json.docker[1-12]
- Dockerfile[46-47]
- Dockerfile[126-128]
- src/node/hooks/express/adminsettings.ts[55-71]
### Suggested fix
- Replace the anchored regex with something that finds the opening brace of the actual JSON object even when there are leading comments.
- Example approach: match the first `{` that appears at the start of a line: `/(^|\n)(\s*\{)/` and inject after `$2`.
- Avoid a naive `indexOf('{')` because `{` can appear inside comment examples.
- Keep the existing `assert.notEqual(...)` to ensure the injection actually happened.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Restart health loop unchecked 🐞 Bug ☼ Reliability

Description

After docker restart, the workflow polls health status up to 60 times but does not fail if the
container never becomes healthy; it will proceed to the grep step even if status remains
starting. This can produce false-green CI runs and weakens the intended regression coverage.

Code

.github/workflows/docker.yml[R102-111]

+          docker restart test >/dev/null
+          for i in $(seq 1 60); do
+            status=$(docker container inspect -f '{{.State.Health.Status}}' test 2>/dev/null) || { docker logs test; exit 1; }
+            case ${status} in
+              healthy) break;;
+              starting) sleep 2;;
+              *) docker logs test; exit 1;;
+            esac
+          done
+          docker exec test grep -q persist-marker-7819 /opt/etherpad-lite/settings.json || {

Evidence

The new loop ends after 60 iterations without checking whether healthy was ever reached, unlike
the existing #7718 loop that explicitly asserts success via an ok flag.

.github/workflows/docker.yml[99-115]
.github/workflows/docker.yml[137-152]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The post-restart health wait loop breaks on `healthy` but has no post-loop assertion, so exhausting all retries without reaching `healthy` still allows the job to continue.
### Issue Context
The same workflow already has a robust pattern (the #7718 regression step) that sets an `ok` flag and asserts it after the loop.
### Fix Focus Areas
- .github/workflows/docker.yml[99-115]
- .github/workflows/docker.yml[137-152]
### Suggested fix
- Introduce an `ok=0` flag before the loop.
- Set `ok=1` when status becomes `healthy`.
- After the loop, if `ok != 1`, print `docker logs test` and `exit 1`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---