feat(users): admin can toggle admin status and delete any user but self

[benders]

Closes #274.

What

• New PUT /api/admin/hub/users/:id/admin (owner-only), body { isAdmin: boolean } → 204.
• Changed DELETE /api/admin/hub/users/:id — drops the blanket Cannot delete admin users refusal.

An admin may now promote, demote, and delete any account except their own.

Case	Result
target is self	400 on both endpoints
target is __system__	400 on both endpoints
unknown id	404
non-boolean isAdmin	400
caller is not an admin	403 (requireOwner)

No last-admin guard, deliberately. Self is never a valid target for either operation, so the acting admin always survives — a count-based guard would be unreachable. Recorded in docs/pitfalls.md so it does not get "fixed" later.

The latent 500 this surfaced

instances.owner_id is NOT NULL REFERENCES users(id) with no ON DELETE CASCADE, and every admission site picks the owner with an unordered SELECT id FROM users LIMIT 1. So an arbitrary user — including a non-admin guest — can end up holding instance rows, and deleting them threw SQLITE_CONSTRAINT → 500.

This is not hypothetical: on the live hub a non-admin guest owned two peer instance rows. The handler now reassigns owned rows to the acting admin inside the delete transaction.

That column turns out to be read by nothing at all — not the UI, not any API response, not any authz check. #275 tracks dropping it along with this workaround.

Frontend

Row actions become icon+text pills matching PeersSection, replacing icon-only buttons that could not distinguish state ("is an admin") from action ("click to make admin"):

alice  [admin]     [ Password ]  [ Revoke admin ]  [ Remove ]
bob                [ Password ]  [ Make admin   ]  [ Remove ]
you    [admin]     [ Password ]

Delete is behind a window.confirm (same pattern as PeersSection / CacheSection) that names the user and warns when the target is an admin. Mutation errors render inline.

Notes

• Neither operation revokes outstanding tokens. A demoted user's JWT stays valid until expiry (≤15 min), but requireOwner / requireAuth re-read the users row per request, so a demotion 403s and a deletion 401s on the next call. Documented in docs/authentication.md.
• Extracts SYSTEM_USERNAME to hub/src/db/system-user.ts, replacing three scattered '__system__' literals.

Testing

• 15 new backend cases in hub/test/admin-routes.test.ts (promote/demote round-trip incl. real access changes via login, all guards, owner_id reassignment, cascade of stars/play events).
• 9 new frontend cases in UsersSection.test.tsx (both toggle directions, self-row hiding, confirm accept/decline, admin-specific confirm wording, error surfacing).
• pnpm verify: typecheck + lint clean, 839 hub + 175 frontend tests pass.
• pnpm test:federation: 84 subsonic-compat tests green across hub-a/b/c plus tombstone-gossip and re-admission flows.
• Pre-existing unrelated failure: dlna-ssdp.integration.test.ts (3 tests), reproduced identically on main — the known flake in flaky: dlna-ssdp integration test 'M-SEARCH with ssdp:all' intermittently sees zero replies #268.
• Deployed to the live hub and verified: healthy, navidrome: ok, no application-level warnings, data intact.

🤖 Generated with Claude Code

[benders]

FROM @claude:

Reviewed this branch against origin/main — backend endpoints, frontend rework, docs. Brief for what holds up: the instances.owner_id reassignment inside the delete transaction is correct, the cascade set (playlists, user_stars, play_events) is complete and foreign_keys = ON is set in hub/src/db/client.ts:363, users.updated_at exists, the SYSTEM_USERNAME extraction is a real de-duplication, sameSite: "lax" plus a JSON body makes CSRF on the new PUT a non-issue, and there is no INSERT INTO users path outside the three known ones.

Six findings.

---

1. Deleting the POUTINE_OWNER_USERNAME account is unrecoverable and blanks DLNA — hub/src/routes/admin.ts:349

Dropping the is_admin === 1 refusal makes the config-seeded owner row a legal delete target, and nothing puts it back:

• seedOwner (hub/src/server.ts:115-145) matches by username first. Row gone → existingByName undefined → falls through to the realUsers.count > 0 check → returns. Other real users exist, so it never re-inserts.
• README.md:98 points recovery at ./reset-password.sh, which errors if the user does not exist.
• createHubSubsonicCaller with no asUser authenticates as owner u+p (hub/src/services/hub-subsonic-caller.ts:99-107). The DLNA browse caller (hub/src/server.ts:549) always takes that branch, since dlnaPseudoUser is normally unset. lookupAndVerify fails → Subsonic error 40 → hub/src/routes/dlna.ts:150 catches it and serves <DIDL-Lite/>. The whole DLNA library goes blank with a single warn line as the only signal.
• The same username is baked into DIDL res@uri as u= (dlna.ts:148), so even cached object IDs 401 on stream.

Demotion via the new endpoint is equally permanent — seedOwner only re-asserts is_admin = 1 when password_enc is empty.

Cheapest fix: refuse both operations when user.username === config.poutineOwnerUsername, same shape as the existing SYSTEM_USERNAME guard.

2. POST /users has no SYSTEM_USERNAME guard — hub/src/routes/admin.ts:237

This PR promotes __system__ from a seed-time placeholder to a load-bearing security check, but applies it only on the two new paths. POST /users still accepts it as a username. On any hub where the placeholder was never seeded — i.e. every hub with real users — an admin can create an account named __system__ that:

• is hidden from GET /users (line 205), so it never appears in the admin UI;
• is rejected by DELETE /users/:id (349) and PUT /users/:id/admin (315), so it cannot be removed through the API;
• logs in normally, and is excluded from seedOwner's real-user count.

A persistent invisible account is a bad thing to leave reachable. Add the guard to POST /users. PUT /users/:id/password (271) is missing it too, which is the difference between "placeholder with an empty password_enc" and "hidden usable login".

3. The "no last-admin guard needed" claim is wrong — hub/src/routes/admin.ts:302, docs/pitfalls.md:86

The id === request.userId check is per-request. Two admins demoting or deleting each other concurrently both pass their own guard, and the hub lands at zero admins — no requireOwner route is reachable after that, so recovery is manual SQL.

Low likelihood, but docs/pitfalls.md states it as unreachable ("A count-based guard would only add a race with no case that reaches it"), which inverts the actual situation: the guard is what removes the race. The same claim appears in docs/authentication.md:70. Either add a SELECT COUNT(*) ... WHERE is_admin = 1 inside a transaction with the write, or reword both docs to say the window is accepted rather than nonexistent.

4. Stale error text persists and masks later errors — frontend/src/features/hub-admin/UsersSection.tsx:222

deleteMutation.error ?? adminMutation.error: react-query keeps error set until the next mutate() on that mutation. A failed delete leaves its message rendered under the row indefinitely, and every subsequent admin-toggle failure is short-circuited away by the still-set delete error. Reset the sibling mutation in each onMutate, or select the error from whichever mutation has the more recent submittedAt.

5. Peer-reassignment warning gated on the wrong condition — frontend/src/features/hub-admin/UsersSection.tsx:218

deleteConfirmMessage only mentions "any peer records they own are reassigned to you" when user.isAdmin. Non-admins own instances rows — the PR description's own motivating example is a non-admin guest owning two on the live hub. Guest deletions therefore silently re-home peer records with no warning. The owner_id condition and the isAdmin condition are unrelated; the frontend cannot know which users own rows, so either always include the sentence or have GET /users report an ownsInstances flag.

6. Duplicated guard block — hub/src/routes/admin.ts:296-320 / 334-350

Both handlers repeat the same sequence: self check → SELECT ... FROM users WHERE id = ? → 404 → SYSTEM_USERNAME → 400. Findings 1 and 2 each add another clause to both copies. Worth a loadMutableTarget(db, id, actingUserId) helper returning either the row or a { code, error } — it makes the guard set one thing to audit instead of two to keep in sync.

---

Documentation

docs/authentication.md:67-76 and the two new docs/pitfalls.md rows are otherwise accurate and correctly cross-referenced. Nothing else in docs/ or README.md still claims admins are undeletable, and the #275 follow-up for the vestigial owner_id column is recorded in both places.

[benders]

FROM @claude:

All six findings from the review above are addressed in d6e67ec.

#	Finding	Fix
1	Owner account deletable / demotable, unrecoverable, blanks DLNA	loadTargetUser({ protectOwner }) refuses the POUTINE_OWNER_USERNAME row on demote and delete. Promotion stays allowed — that is the recovery path if the row ever ends up non-admin
2	POST /users could claim __system__	Name reserved in POST /users (400); PUT /users/:id/password picks up the placeholder guard via the shared helper
3	Last-admin race, and the docs called it unreachable	otherAdminCount() runs in the handler body, synchronously with the write, so the second request of a racing pair observes the first. Both demote and delete refuse a zero-admin outcome
4	Stale delete error masked later toggle errors	Error comes from whichever mutation has the newer submittedAt, not delete ?? admin
5	Peer-reassignment warning gated on isAdmin	Warning is unconditional; only the noun still varies on isAdmin
6	Guard block duplicated across handlers	Extracted loadTargetUser(request, id, opts). Fixes 1–3 each added a clause to one place instead of two

One note on the tests. I first wrote the last-admin guard as two racing app.inject calls. Then I deleted the guard and re-ran them — both still passed, so they proved nothing. app.inject serializes, and the real window is between requireOwner's preHandler read and the handler, which cannot be widened without changing production code. They are replaced by a direct assertion on otherAdminCount against the exact DB state the race produces; the helper is exported for that test with a comment explaining why.

Docs. docs/pitfalls.md lost the row claiming "a count-based guard would only add a race with no case that reaches it" — that inverted the situation, the guard is what removes the race — and gained three accurate rows (owner account, reserved __system__, preHandler window). docs/authentication.md replaces the prose claim with a guard table plus an explicit note on the preHandler read.

Gate.

• pnpm verify: typecheck + lint clean, 846 hub (+7) and 177 frontend (+2) tests pass.
• pnpm --filter frontend build: passes — pnpm verify does not actually typecheck the frontend.
• pnpm test:federation: 84 subsonic-compat tests green across hub-a/b/c, exit 0.
• Unchanged pre-existing failure: dlna-ssdp.integration.test.ts (3 tests, flaky: dlna-ssdp integration test 'M-SEARCH with ssdp:all' intermittently sees zero replies #268), untouched by this diff.