fix: keep is_admin from settings.json for hash_dir users (etherpad#8110)

[JohnMcLear]

Root cause

A site can list a user in settings.json with is_admin but no hash, keeping the password in a hash_dir hash file:

"users": { "hashadmin": { "is_admin": true } },
"ep_hash_auth": { "hash_dir": "/var/etherpad/users" }

authenticate() then takes the hash_dir branch, and that branch ended with:

settings.users[username] = {username, is_admin: adm, displayname};

i.e. it replaced the configured user object with a fresh one, deriving is_admin only from the .adm file or the global hash_adm default. The is_admin: true the administrator had written in settings.json was silently discarded.

Symptom: Basic auth succeeds (200), the session user exists, but /admin answers 403.

Fix

Resolve the admin flag most-specific-first:

1. an explicit .adm file (hash_adm_ext) — including one containing false, so a deliberate demotion still wins;
2. otherwise is_admin already configured for that user in settings.json;
3. otherwise the site-wide hash_adm default.

.adm keeps absolute precedence, so nothing that worked before changes. README documents the precedence.

Evidence

Unit tests (static/tests/backend/specs/authenticate.js)

Six new cases in is_admin declared in settings.json (etherpad#8110). Before the fix (plugin main, core with #8155 merged):

  18 passing (78ms)
  3 failing

  1) ep_hash_auth authenticate
       is_admin declared in settings.json (etherpad#8110)
         keeps is_admin: true from settings.json when .adm is missing:
      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
      false !== true

  2) ... keeps is_admin: true across repeated logins:
      false !== true

  3) ... keeps is_admin: false from settings.json when .adm is missing:
      true !== false

After the fix, the whole plugin backend suite is green:

      ✔ keeps is_admin: true from settings.json when .adm is missing
      ✔ keeps is_admin: true across repeated logins
      ✔ keeps is_admin: false from settings.json when .adm is missing
      ✔ lets an explicit .adm "false" demote a settings.json admin
      ✔ lets an explicit .adm "true" promote a settings.json non-admin
      ✔ still falls back to hash_adm when settings.json has no is_admin
  30 passing (58ms)

End to end against Etherpad core

Real server (core with ether/etherpad#8155 merged), bcrypt .hash in hash_dir, no .adm file, "users": {"hashadmin": {"is_admin": true}}, POST /admin-auth/:

RESULT[nofix]:          correct-creds=403 wrong-creds=401
RESULT[fixed]:          correct-creds=200 wrong-creds=401
RESULT[fixed-admfalse]: correct-creds=403 wrong-creds=401

The third run adds .adm containing false and confirms explicit demotion still overrides settings.json.

Follow-up: fail closed on an unreadable .adm (Qodo review)

The first commit treated every fs.readFile error on .adm as "file absent", so a .adm containing false that lost read permission would be overridden by is_admin: true in settings.json. Commit 764f687 restricts the fallback to ENOENT; any other error logs a warning and denies admin. Covered by denies admin when .adm exists but cannot be read (uses a directory at the .adm path, so EISDIR is deterministic regardless of uid — a chmod 000 test would pass vacuously under root). Fails on the previous commit, passes now. Full suite: 31 passing.

Context

• Fixes the remaining half of authenticate plugin hook not invoked for /admin-auth/ endpoint, leading to ep_hash_auth not working etherpad#8110.
• The core half — plugin ep_* settings blocks not being visible through require(), which made ep_hash_auth fall back to its default hash_dir and find no hash file at all — shipped in fix(settings): expose plugin ep_* config blocks to CJS require (#8110) etherpad#8155.

Notes

• pnpm run lint is currently broken on main for an unrelated reason (typescript@7.0.2 vs @typescript-eslint@7 / ts-api-utils: "Failed to load plugin '@typescript-eslint' … Cannot read properties of undefined (reading 'Intrinsic')"). Linting with typescript@5.9.3 pinned, the changed files report 0 errors; the only 2 errors in the repo are pre-existing mocha/no-synchronous-tests in static/tests/backend/specs/no_scrypt.js. Not touched here.

🤖 Generated with Claude Code

https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

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

PR Summary by Qodo

Preserve settings.json admin flags for hash_dir users

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

AI Description

• Preserve per-user settings.json admin flags when hash_dir users lack .adm files.
• Keep explicit .adm values authoritative, including false demotions.
• Document and regression-test the complete admin precedence order.

Diagram

graph TD
  A["Hash-dir login"] --> B["Read admin file"] --> C{"Admin file exists?"}
  C -- Yes --> D["Use file flag"] --> H["Session user"]
  C -- No --> E{"User flag set?"}
  E -- Yes --> F["Use user flag"] --> H
  E -- No --> G["Use global default"] --> H

Loading

High-Level Assessment

The targeted precedence fallback is the best approach: it preserves explicit .adm behavior, respects both true and false settings.json values, and limits behavioral change to the missing-file path. Broadly merging configured user objects or introducing a separate resolver would add unnecessary scope for this localized fix.

Files changed (3) +99 / -2

Bug fix (1) +15 / -1

ep_hash_auth.jsPreserve configured admin flags during hash_dir authentication +15/-1

Preserve configured admin flags during hash_dir authentication

• When the per-user admin file is absent, authentication now retains an explicitly configured settings.users[username].is_admin value before falling back to hash_adm. Existing admin-file values, including false, keep highest precedence.

ep_hash_auth.js

Tests (1) +77 / -0

authenticate.jsCover all hash_dir administrator precedence cases +77/-0

Cover all hash_dir administrator precedence cases

• Adds six regression cases covering configured true and false values, repeated logins, explicit admin-file promotion and demotion, and global-default fallback. These tests ensure hash_dir authentication no longer discards settings.json administrator roles.

static/tests/backend/specs/authenticate.js

Documentation (1) +7 / -1

README.mdDocument hash_dir administrator flag precedence +7/-1

Document hash_dir administrator flag precedence

• Explains that an explicit hash_adm_ext file overrides the user's settings.json is_admin value, which in turn overrides the global hash_adm default. Clarifies that explicit false values remain authoritative.

README.md

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

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Unreadable demotion files grant admin ✓ Resolved 🐞 Bug ⛨ Security

Description

authenticate() treats every error from reading the per-user admin file as absence and falls back
to the configured is_admin value. If an existing demotion file becomes unreadable while
settings.json grants that user admin access, authentication stores is_admin: true in the session
despite the file's documented precedence.

Code

ep_hash_auth.js[R169-170]

+                    adm = configuredUser && configuredUser.is_admin !== undefined
+                      ? configuredUser.is_admin : hash_adm;

Evidence

The callback receives every filesystem failure through err, and the added branch copies the
configured administrator flag for all of them before storing it in the session. The documentation
says an existing admin file, including one containing false, has absolute precedence, so treating
permission and I/O failures as nonexistence can reverse an intended demotion.

ep_hash_auth.js[152-170]
ep_hash_auth.js[172-178]
README.md[38-44]

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 hash-directory authentication path treats all `.adm` read failures as a missing file. An unreadable explicit demotion file can therefore fall through to `settings.json` and grant administrator access.
## Fix Focus Areas
- ep_hash_auth.js[168-170]
- static/tests/backend/specs/authenticate.js[194-268]
## Recommended Fix
Use the settings/default fallback only when the admin file is genuinely absent, such as `ENOENT`. For other read errors, fail closed by rejecting authentication or assigning non-admin status, log the error, and add a regression test for an unreadable or otherwise failing `.adm` read.

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

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗