DEPLOYMENT: a pre-deploy backup sequence for the D13 auth change

Asked whether there is an easy way to take a full backup before deploying,
since this touches auth. There is - the backup sidecar is already running and
scripts/db-backup.sh takes a one-shot dump - but three things about THIS deploy
were not written down anywhere.

Timing. The container starts with `alembic upgrade head && exec gunicorn`, so
the migration runs seconds after redeploy and there is no window afterwards. The
dump has to be taken before, not after.

Retention. The scheduled job prunes to the newest BACKUP_KEEP (14) files
matching wpsuite-*.sql.gz*, so on a daily cadence a pre-deploy dump is deleted
in a fortnight - exactly when a slow-burning problem would surface. Copying it
to a name outside the glob protects it.

Rollback is not just the database. The new code has no password_hash in its
model and the old code requires it, so restoring without also rolling the code
back leaves schema and application disagreeing. Recorded as both steps in
order, with a note to capture the current commit FIRST, since that is easy to
forget and impossible to reconstruct afterwards.

Also flagged what this particular dump is: the last copy of every password hash
that will ever exist. BACKUP_ENC_PASSPHRASE must be set before taking it - the
script warns and writes plaintext otherwise - and its retention deserves a
deliberate decision rather than the default fortnight, because bcrypt is not
plaintext but is crackable offline given a copy and time.

And a step to prove the dump is readable before deploying, because an untested
dump is not a backup.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

DEPLOYMENT.md

@@ -91,13 +91,34 @@ ignored whenever the three `POSTGRES_*` values are present.
 
 Generate a strong password with `openssl rand -base64 32`.
 
-> **Portainer note:** for a Git-based stack these go in the stack's
-> **Environment variables** section (Portainer doesn't read a local `.env`).
-> Set `POSTGRES_DB` / `POSTGRES_USER` / `POSTGRES_PASSWORD` / `AUTH_SECRET_KEY` /
-> `BACKUP_ENC_PASSPHRASE` (and `SMTP_PASSWORD`, if you enable email) there.
+> **Portainer note:** for a Git-based stack the stack's **Environment variables**
+> section is not merely an alternative to `.env` — it is the ONLY route, because
+> Portainer does not read a local `.env` at all. Every value the compose file
+> references as `${VAR}` has to be set there or it arrives empty.
+>
+> The full list, and what an empty one costs you:
+>
+> | Variable | Required? | If unset |
+> |---|---|---|
+> | `POSTGRES_DB` / `POSTGRES_USER` / `POSTGRES_PASSWORD` | **yes** | the stack will not start |
+> | `AUTH_SECRET_KEY` | **yes** | compose fails fast; the API refuses to start |
+> | `LDAP_REQUIRED_GROUP` | **effectively yes** | no group gate — **every account in the domain may sign in**. Silent: sign-in works, so nothing looks wrong. |
+> | `BACKUP_ENC_PASSPHRASE` | before real data | dumps are written unencrypted |
+> | `SMTP_PASSWORD` | only with email on | notifications are recorded and never sent |
+> | `MICRON_DB_URL` | optional | the asset picker degrades to manual entry |
+>
+> Paste values raw — it is a form field, not a shell, so no surrounding quotes.
+> Quotes are not stripped and become part of the value: a quoted
+> `LDAP_REQUIRED_GROUP` will not resolve, and a quoted `MICRON_DB_URL` will not
+> parse.
+>
+> **`MICRON_DB_URL` must be URL-encoded** (`@` → `%40`, `#` → `%23`, `/` → `%2F`)
+> because it is a full connection URL. `LDAP_REQUIRED_GROUP` must NOT be encoded —
+> it is an LDAP distinguished name, and its spaces and commas are legal as they are.
 
-These are the only credentials in the system, and they never appear in the
-compose file or in git.
+`POSTGRES_PASSWORD`, `AUTH_SECRET_KEY`, `BACKUP_ENC_PASSPHRASE`, `SMTP_PASSWORD` and
+the password inside `MICRON_DB_URL` are the only credentials in the system, and none
+of them appears in the compose file or in git.
 
 ## 3. Point your reverse proxy at the nginx container
 
@@ -303,6 +324,56 @@ docker compose up -d --build webserver   # front-end change (html/) — rebuild
 docker compose up -d --build api         # backend change (server/)
 ```
 
+## Before deploying the D13 auth change — take a backup first
+
+**Timing is the whole point.** The container's start command is
+`alembic upgrade head && exec gunicorn`, so migrations run *seconds after you
+redeploy*. Migration `b7e4f1a20c93` drops `users.password_hash`. There is no window
+afterwards: back up **before** you redeploy, not after.
+
+```bash
+# 1. Record what you are rolling back TO. Do this first; it is easy to forget
+#    and impossible to reconstruct under pressure.
+git -C /path/to/repo rev-parse --short HEAD
+
+# 2. One-shot dump, using the sidecar that is already running.
+docker compose exec backup sh /scripts/db-backup.sh
+#    -> ./backups/wpsuite-<UTC timestamp>.sql.gz[.enc] on the host
+
+# 3. Take it OUT of the rotation. The scheduled job prunes to the newest
+#    BACKUP_KEEP (default 14) files matching wpsuite-*.sql.gz*, so on a daily
+#    cadence this dump is deleted in a fortnight. A prefix that does not match
+#    the glob is enough to protect it.
+docker compose exec backup sh -c   'cd /backups && cp "$(ls -1t wpsuite-*.sql.gz* | head -1)" "pre-d13-$(ls -1t wpsuite-*.sql.gz* | head -1)"'
+
+# 4. Prove it is readable BEFORE you deploy. An untested dump is not a backup.
+docker compose exec backup sh -c   'openssl enc -d -aes-256-cbc -pbkdf2 -pass env:BACKUP_ENC_PASSPHRASE      -in /backups/pre-d13-*.sql.gz.enc | gunzip -c | grep -c "INSERT INTO public.users"'
+#    (drop the openssl stage for an unencrypted .sql.gz)
+```
+
+### Two things about this particular dump
+
+**It is the last copy of every password hash that will ever exist.** After the
+migration the column is gone; this file is where those bcrypt hashes live from then
+on. Make sure `BACKUP_ENC_PASSPHRASE` is set before step 2 — the script warns loudly
+if it is not, and writes plaintext — and decide deliberately how long to keep the
+file. bcrypt is not plaintext, but it is crackable offline given time and a copy.
+
+**Restoring the database is not, by itself, a rollback.** The new code has no
+`password_hash` in its model and the old code requires it, so a restore without a
+matching code rollback leaves you with a schema and an application that disagree. A
+real rollback is both, in this order:
+
+```bash
+# redeploy the commit from step 1 (Portainer: point the stack back and rebuild)
+docker compose exec backup sh /scripts/db-restore.sh /backups/pre-d13-wpsuite-<ts>.sql.gz.enc
+```
+
+`db-restore.sh` dumps are taken with `--clean --if-exists`, so restoring **drops and
+recreates** objects before loading. It overwrites whatever is currently there.
+
+---
+
 ## Backups & retention
 
 A **`backup` sidecar** (in `docker-compose.yml`) runs `pg_dump` on a schedule and

docs/waves/backlog.md

@@ -601,8 +601,12 @@ deliberately deferred.
   anyone who has ever used the asset picker — the API is genuinely configured, returns
   real Micron tags, and three checks fail. Nothing is wrong with the app; the test's
   premise is violated by the environment it runs in.
-- **Fix:** pop `MICRON_DB_URL` from the env for that server, exactly as `start_server`
-  now pops `LDAP_REQUIRED_GROUP` for the same reason (`T10.7`).
+- **Fix:** SET `MICRON_DB_URL` empty for that server — do not pop it. `server/db.py`
+  calls `load_dotenv()` at import, and python-dotenv only skips keys already present in
+  `os.environ`, so a *popped* variable is restored from the developer's `.env` inside the
+  subprocess and the test runs against the real catalog anyway. An empty string counts as
+  present and therefore wins. `start_server` does exactly this for `LDAP_REQUIRED_GROUP`
+  (`T10.7`), after the pop-based version was caught doing the wrong thing.
 - **Why not now:** it is not this wave's defect and the fix belongs with whoever owns
   the asset picker's tests. Recorded so the failure is not mistaken for D13 fallout.
 - **Suggested wave or follow-up:** next housekeeping pass.
@@ -623,3 +627,25 @@ deliberately deferred.
 - **Why not now:** drive-by fixes to the token system are what `CLAUDE.md` forbids, and
   this one needs the C4 exception interpreted rather than guessed.
 - **Suggested wave or follow-up:** next housekeeping pass, with `C4` re-read first.
+
+### BL-030 — `token_version` is now near-vestigial; decide whether it earns its place
+
+- **Found during:** `T10.7` (D13), closing the done-when that assumed a role change bumps it
+- **Where:** `server/models.py` (`User.token_version`), `server/auth.py` (`create_token`,
+  `get_current_user`), `server/manage_users.py` (`_set_active`)
+- **What:** `token_version` existed to invalidate live sessions when a password changed.
+  D13 removed passwords, and **nothing in `app.py` bumps it any more** — not
+  `set_user_role`, not `set_user_active`. Nor do they need to: `get_current_user` loads
+  the account from the database on every request, so a role change or a deactivation
+  takes effect on the next request regardless. `T10.3`'s note that "role changes and
+  deactivation should bump it" describes an intention, not the code.
+- **Its one remaining trigger** is the bump added to `manage_users._set_active` in
+  `T10.9`, which is belt-and-braces rather than load-bearing — `is_active` alone already
+  refuses the request.
+- **The question:** is there still a case for invalidating a live cookie *without* also
+  disabling the account? If yes, wire it to something (a "sign out everywhere" control is
+  the usual shape) and say so. If no, the column, the claim, and the check are three
+  places carrying a mechanism nothing triggers.
+- **Why not now:** it is a design question about session handling, not an auth-wave bug,
+  and the mechanism works correctly — it is exercised in `ldap_auth_check`.
+- **Suggested wave or follow-up:** next housekeeping pass.

docs/waves/wave-10.md

@@ -12,6 +12,14 @@ with their Windows password instead of an app password, the "Forgot password?" l
 disappears, and admins stop issuing passwords. Everything else is invisible — which means
 the done-when checks are the only evidence the wave worked.
 
+> **BEFORE PRODUCTION: `LDAP_REQUIRED_GROUP` must be set.** It is empty by default,
+> and empty means *no group gate* — every account in `prime.local` may sign in. The
+> successful live sign-in on Aug 24 was made without it, so it proved the bind, the
+> certificate chain and JIT provisioning, but **not** the group check: that path never
+> executed. The group gate is covered against the fake directory in `ldap_auth_check`
+> and has never run against the real directory. The startup line says which state you
+> are in — `required group: (none configured — every domain account may sign in)`.
+
 **Build order is task order** for once. `T10.1` is a standalone module with no callers and
 should merge first; nothing else can be tested until it exists.
 
@@ -51,15 +59,15 @@ across resolved addresses, because round-robin will hand out a rebooting DC's ad
 
 **Done when:**
 
-- [ ] `ldap3` is pinned to an exact version in `requirements.txt`
-- [ ] an empty or whitespace-only password returns failure **without calling `bind()`**
-- [ ] an empty username returns failure without calling `bind()`
-- [ ] `Tls` is constructed with `validate=ssl.CERT_REQUIRED` and an explicit `ca_certs_file`
-- [ ] no code path sets `CERT_NONE`, and none falls back to the system trust store
-- [ ] `member_of` returns true for an account in a **nested** child of the required group
-- [ ] a bind against `192.168.3.37` (raw IP) fails hostname validation rather than silently passing
+- [x] `ldap3` is pinned to an exact version in `requirements.txt`
+- [x] an empty or whitespace-only password returns failure **without calling `bind()`** — proven with `Connection` nulled, so any call to `bind()` would raise
+- [x] an empty username returns failure without calling `bind()`
+- [x] `Tls` is constructed with `validate=ssl.CERT_REQUIRED` and an explicit `ca_certs_file`
+- [x] no code path sets `CERT_NONE`, and none falls back to the system trust store — asserted by an AST walk in `ldap_auth_check`, not a grep: the docstring names it to explain the ban
+- [~] `member_of` returns true for an account in a **nested** child of the required group — **partially closed Aug 24.** The transitive matching rule was exercised against the live directory for a DIRECT member (`CN=Prime Employees`, 2,003 members) and returned a match, so the rule and the filter are correct on this estate. A genuinely NESTED case (member of a group that is a member of the required group) still has no test, because no such account was to hand. `member_of` now also warns loudly if the transitive query returns nothing where a direct check succeeds, which is how a broken nested lookup would announce itself rather than refusing real members silently.
+- [x] a bind against `192.168.3.37` (raw IP) fails hostname validation rather than silently passing — `selftest()` to the raw IP returns `untrusted`; to `prime.local`, `0 (ok)`
 - [ ] `docker compose exec api openssl s_client -connect prime.local:636 -CAfile $LDAP_CA_FILE` reports `Verify return code: 0 (ok)`
-- [ ] the module imports cleanly with no LDAP env set (unconfigured is a first-class state, as with `MICRON_DB_URL`)
+- [x] the module imports cleanly with no LDAP env set (unconfigured is a first-class state, as with `MICRON_DB_URL`)
 
 ---
 
@@ -89,14 +97,15 @@ misconfigured deploy is indistinguishable from a forgotten password at the login
 
 **Done when:**
 
-- [ ] a correct domain password signs in and sets the session cookie
-- [ ] a wrong password is refused with the generic message
-- [ ] a blank password is refused (guards `T10.1` from the caller's side too)
-- [ ] a user not in the required group is refused even though the bind succeeded
-- [ ] `is_active = false` locally still refuses, independent of the directory
-- [ ] the local throttle trips below the domain lockout threshold and stops calling the DC
-- [ ] no response body distinguishes "no such user" from "wrong password"
-- [ ] error-49 sub-codes appear in the log and nowhere in any response
+- [x] the API logs one line at startup saying whether LDAP is configured and which group gates sign-in — configuration only, so an unreachable DC cannot hang startup. This is what `DEPLOYMENT.md`, `DEPLOY-login-portal.md` and `server/README.md` all send people to first; it was specified in this task on Aug 21 and not actually written until Aug 24.
+- [x] a correct domain password signs in and sets the session cookie — confirmed against the live domain Aug 24, and in `ldap_auth_check`
+- [x] a wrong password is refused with the generic message
+- [x] a blank password is refused (guards `T10.1` from the caller's side too)
+- [x] a user not in the required group is refused even though the bind succeeded
+- [x] `is_active = false` locally still refuses, independent of the directory — a disabled account is refused 403 even though the bind succeeds
+- [x] the local throttle trips below the domain lockout threshold and stops calling the DC — proved by presenting a CORRECT password once the budget is spent: a 429 for a credential that would otherwise work is only possible if the throttle runs before the directory is consulted
+- [x] no response body distinguishes "no such user" from "wrong password"
+- [x] error-49 sub-codes appear in the log and nowhere in any response — asserted both ways: `_err49` parses a real AD message, and no sub-code appears in any response body
 
 ---
 
@@ -131,12 +140,12 @@ is criterion 4 and must keep working.
 
 **Done when:**
 
-- [ ] `grep -rn "password_hash\|hash_password\|verify_password\|password_problem" server/` returns nothing outside the migration
+- [x] `grep -rn "password_hash\|hash_password\|verify_password\|password_problem" server/` returns nothing outside the migration — only the migration and one docstring naming the dropped column
 - [x] `alembic upgrade head` then `downgrade -1` round-trips on SQLite and on Postgres (16.15, the compose image — Aug 24; needed a pre-existing T8.6 migration bug fixed first, see `495d87d`)
-- [ ] the migration's `downgrade()` recreates the column nullable, not `NOT NULL` — there are no hashes to put back
-- [ ] `token_version` still invalidates sessions, exercised by a role change
-- [ ] `manage_users.py list`, `disable`, `enable` still work; `reset-password` is gone
-- [ ] `python -m server.manage_users create-admin <u>` creates an admin with no password prompt
+- [x] the migration's `downgrade()` recreates the column nullable, not `NOT NULL` — there are no hashes to put back — verified on SQLite and Postgres
+- [x] `token_version` still invalidates an already-issued session — but **not** via a role change, which was the wrong premise. Nothing in `app.py` bumps it any more: `get_current_user` re-reads the account every request, so `role` and `is_active` changes take effect immediately without it. Its one remaining trigger is `manage_users` on disable. Tested by bumping it directly: the old cookie 401s and every other session is untouched. See `BL-030`.
+- [x] `manage_users.py list`, `disable`, `enable` still work; `reset-password` is gone
+- [x] no CLI command prompts for a password — superseded by `T10.9`, which removed `create-admin` and `create` outright; the first admin is now bootstrapped by signing in and then `promote`
 
 ---
 
@@ -176,15 +185,15 @@ creating it is exactly the kind of event that record exists for.
 
 **Done when:**
 
-- [ ] an unknown username with a valid bind and group membership gets a `users` row at `project_user`
-- [ ] `full_name` and `email` are populated from the directory on creation
-- [ ] an existing `admin` signing in is still `admin` afterwards — asserted, not assumed
-- [ ] an existing account with a locally-set `full_name` does not have it overwritten
-- [ ] a JIT account has NO `ProjectMember` rows and sees no projects
-- [ ] the new account appears in the Admin console user list so access can be granted
-- [ ] each JIT creation writes an `AuditLog` row
-- [ ] a failed bind creates **no** row
-- [ ] a bind that succeeds but fails the group check creates **no** row
+- [x] an unknown username with a valid bind and group membership gets a `users` row at `project_user`
+- [x] `full_name` and `email` are populated from the directory on creation
+- [x] an existing `admin` signing in is still `admin` afterwards — asserted, not assumed — asserted in `ldap_auth_check` against a real server
+- [x] an existing account with a locally-set `full_name` does not have it overwritten
+- [x] a JIT account has NO `ProjectMember` rows and sees no projects
+- [x] the new account appears in the Admin console user list so access can be granted — asserted through `GET /api/auth/users`, the request the console makes
+- [x] each JIT creation writes an `AuditLog` row
+- [x] a failed bind creates **no** row
+- [x] a bind that succeeds but fails the group check creates **no** row
 
 ---
 
@@ -267,16 +276,16 @@ Keep the role-granting controls exactly as they are. That is criterion 4.
 
 **Done when:**
 
-- [ ] `grep -rn "forgot\|reset-password\|new-password" html/` returns nothing but prose
-- [ ] "Forgot password?" opens `https://primecontrols.okta.com/` in a new tab
-- [ ] `#forgot-link` has NO click handler (a `preventDefault()` would swallow the navigation)
-- [ ] a 503 from the login endpoint says sign-in is unavailable, not that the password is wrong
-- [ ] the sign-in form still submits, and a failure still announces through `role="alert"` (`login.html` already does this correctly — do not regress it)
-- [ ] the password field says which password to enter
-- [ ] no dead `<a href="#">` or handler remains for a removed view
-- [ ] granting admin to an existing user still works from the console
-- [ ] exercised at 390px and at 1440px, screenshots in the PR
-- [ ] no raw hex added to any stylesheet (the token rule)
+- [x] `grep -rn "forgot\|reset-password\|new-password" html/` returns nothing but prose
+- [x] "Forgot password?" opens `https://primecontrols.okta.com/` in a new tab — by inspection of the markup: `target="_blank"` with `rel="noopener noreferrer"`
+- [x] `#forgot-link` has NO click handler (a `preventDefault()` would swallow the navigation)
+- [x] a 503 from the login endpoint says sign-in is unavailable, not that the password is wrong
+- [x] the sign-in form still submits, and a failure still announces through `role="alert"` (`login.html` already does this correctly — do not regress it) — `url_state_check` drives the real form end to end; the `role="alert"` region is untouched
+- [x] the password field says which password to enter — see the screenshots
+- [x] no dead `<a href="#">` or handler remains for a removed view
+- [x] granting admin to an existing user still works from the console — asserted through `POST /api/auth/users/{id}/role`, the request `users.js` sends, and the role really changes
+- [x] exercised at 390px and at 1440px, screenshots in the PR — `docs/reference/baseline/before-wave10/` and `after-wave10/`, captured Aug 24 with `tests/baseline_shots.py`. "Before" comes from a detached worktree at `main` (`a8e28bf`) so each half was shot against its own server.
+- [x] no raw hex added to any stylesheet (the token rule) — no CSS was added at all — the hint reuses the `.hint` class the page already had
 
 ---
 

server/README.md

@@ -138,6 +138,37 @@ previously did not.
 
 ## Local dev
 
+> ### A SQLite database created before D13 will reject new sign-ins
+>
+> `Base.metadata.create_all()` creates missing tables; it never alters existing ones.
+> So a `wpsuite.db` built before D13 still has `users.password_hash` declared
+> `NOT NULL` with no default, while the current model has no such column — and an
+> INSERT that omits it is rejected:
+>
+> ```
+> IntegrityError: NOT NULL constraint failed: users.password_hash
+> ```
+>
+> Accounts already in the file keep working, so **you** can sign in and nothing looks
+> wrong. It breaks the moment a *new* person signs in, because provisioning them is an
+> INSERT — and it surfaces as an HTTP **500**, not a 401, so it reads as a server fault
+> rather than anything to do with the schema.
+>
+> Such a database also has no `alembic_version` table, so `alembic upgrade head` would
+> try to replay the baseline against tables that already exist. Stamp it first:
+>
+> ```bash
+> python -m alembic -c server/alembic.ini stamp a1b8c6d4e2f9   # the revision before the drop
+> python -m alembic -c server/alembic.ini upgrade head         # runs only the drop
+> ```
+>
+> That keeps whatever is in the file. Deleting the database also works and
+> `create_all()` rebuilds a correct schema, but it throws away your test data.
+>
+> Production is unaffected: it runs Postgres and the container applies migrations at
+> start, so the column is dropped properly there.
+
+
 ```bash
 cd server
 python -m venv .venv && . .venv/bin/activate      # Windows: .venv\Scripts\activate

server/app.py

@@ -10,6 +10,7 @@ Interactive docs: http://<host>/api/docs
 """
 import base64
 import logging
+from contextlib import asynccontextmanager
 import os
 import re
 import uuid
@@ -44,7 +45,30 @@ if engine.dialect.name == "sqlite":
 # Interactive docs are handy in dev but hand an attacker the full API map in prod,
 # so enable them only on the SQLite dev fallback (production runs on Postgres).
 _docs_enabled = engine.dialect.name == "sqlite"
+
+
+@asynccontextmanager
+async def _lifespan(_app):
+    """Say, once, whether anyone can sign in at all.
+
+    D13 removed the local password path and left no break-glass, so a broken LDAP
+    configuration and a forgotten password look identical at the login box. This
+    line is what tells an operator which one they have, and DEPLOYMENT.md,
+    DEPLOY-login-portal.md and server/README.md all send people here first:
+
+        docker compose logs api | grep -i "LDAP auth"
+
+    Configuration only — it opens no connection and binds nothing, so startup stays
+    fast and cannot be made to hang by an unreachable domain controller. Use
+    `ldap_auth.selftest()` for a reachability check; it validates the certificate
+    without binding, so it cannot contribute to a lockout either.
+    """
+    log.info("%s", ldap_auth.describe())
+    yield
+
+
 app = FastAPI(
+    lifespan=_lifespan,
     title="Work Package Suite API",
     docs_url="/api/docs" if _docs_enabled else None,
     redoc_url=None,
@@ -821,7 +845,11 @@ def login(body: LoginIn, request: Request, response: Response, db: Session = Dep
                             detail="Sign-in is temporarily unavailable. Contact IT.")
 
     if not result.ok:
-        log.info("sign-in refused for %r (%s: %s)", sam, result.reason, result.detail)
+        # WARNING, not INFO: this is the line an operator needs when someone
+        # cannot sign in, and nothing configures the root logger — under plain
+        # uvicorn an INFO record from wpsuite.* goes nowhere, so the reason was
+        # invisible in exactly the situation it exists for.
+        log.warning("sign-in refused for %r (%s: %s)", sam, result.reason, result.detail)
         _record_bind_failure(sam)
         if user:
             user.failed_attempts = (user.failed_attempts or 0) + 1

server/ldap_auth.py

@@ -117,6 +117,7 @@ BAD_CREDENTIALS = "bad_credentials"
 NOT_IN_GROUP = "not_in_group"
 NO_DIRECTORY_ENTRY = "no_directory_entry"
 GROUP_NOT_FOUND = "group_not_found"
+GROUP_CHECK_FAILED = "group_check_failed"
 
 # AD returns these as `data <code>` inside an error-49 message. Kept for the log
 # only: telling an unauthenticated caller "your password expired" confirms the
@@ -156,7 +157,8 @@ class LdapResult:
         """True when the failure is ours, not the user's. With no local password
         fallback (D13) these must be logged as errors and surfaced to an operator —
         otherwise a broken deploy is indistinguishable from a forgotten password."""
-        return self.reason in (UNCONFIGURED, UNREACHABLE, UNTRUSTED, GROUP_NOT_FOUND)
+        return self.reason in (UNCONFIGURED, UNREACHABLE, UNTRUSTED, GROUP_NOT_FOUND,
+                               GROUP_CHECK_FAILED)
 
 
 def base_dn(domain: Optional[str] = None) -> str:
@@ -268,17 +270,57 @@ def _resolve_group_dn(conn, group: str) -> Optional[str]:
 
 
 def member_of(conn, sam: str, group: str) -> bool:
-    """Nested-group-aware membership test for an already-bound connection."""
+    """Is `sam` in `group`, counting nested membership?
+
+    THE RETURN VALUE OF conn.search() IS NOT OPTIONAL READING. The connection is
+    built with raise_exceptions=False, so a search that FAILS returns False and
+    leaves conn.entries empty — which is byte-for-byte indistinguishable from "no
+    match" if you only look at conn.entries. An earlier version of this function did
+    exactly that, and every failure of the extensible-match filter presented to the
+    user as "you are not in the group" while they plainly were.
+
+    Two searches, in order, and the second one exists to catch the first being wrong:
+
+      1. AD's transitive matching rule (LDAP_MATCHING_RULE_IN_CHAIN). This is the
+         correct query — it walks nested groups, which plain memberOf does not.
+      2. If that matches nothing, a plain memberOf equality check for DIRECT
+         membership.
+
+    If (2) matches after (1) did not, the person IS a member and is let in — refusing
+    a real member is the worse error — but it is logged as a WARNING, because it means
+    the transitive rule is returning nothing and NESTED membership is silently not
+    working on this connection. That needs a human; it must not pass unnoticed.
+    """
     dn = _resolve_group_dn(conn, group)
     if not dn:
         log.error("required group %r does not resolve in %s — refusing the sign-in. "
                   "This is a configuration fault, not a bad password.", group, base_dn())
         raise LookupError(GROUP_NOT_FOUND)
-    filt = (f"(&(sAMAccountName={escape_filter_chars(sam)})"
-            f"(memberOf:{NESTED_MEMBER_RULE}:={escape_filter_chars(dn)}))")
-    conn.search(base_dn(), filt, search_scope=SUBTREE,
-                attributes=["sAMAccountName"], size_limit=1)
-    return bool(conn.entries)
+
+    esc_sam, esc_dn = escape_filter_chars(sam), escape_filter_chars(dn)
+
+    def _search(filt: str, label: str):
+        ok = conn.search(base_dn(), filt, search_scope=SUBTREE,
+                         attributes=["sAMAccountName"], size_limit=1)
+        if not ok:
+            log.error("the %s membership search FAILED (not 'no match') for %r: %s | "
+                      "filter=%s", label, sam, conn.result, filt)
+            raise LookupError(GROUP_CHECK_FAILED)
+        return bool(conn.entries)
+
+    if _search(f"(&(sAMAccountName={esc_sam})(memberOf:{NESTED_MEMBER_RULE}:={esc_dn}))",
+               "nested"):
+        return True
+
+    if _search(f"(&(sAMAccountName={esc_sam})(memberOf={esc_dn}))", "direct"):
+        log.warning(
+            "%r IS a direct member of %r, but AD's transitive matching rule "
+            "(%s) returned nothing for them. Allowing the sign-in — refusing a real "
+            "member is worse — but NESTED group membership is not working on this "
+            "connection and needs investigating.", sam, dn, NESTED_MEMBER_RULE)
+        return True
+
+    return False
 
 
 def verify(username: str, password: str, required_group: Optional[str] = None) -> LdapResult:
@@ -330,7 +372,7 @@ def verify(username: str, password: str, required_group: Optional[str] = None) -
             )
             if not conn.bind():
                 detail = _err49(conn.result)
-                log.info("bind refused for %r: %s", sam, detail)
+                log.warning("bind refused for %r: %s", sam, detail)
                 return LdapResult(False, BAD_CREDENTIALS, detail)
 
             # Bound as the user. AD lets an account read its own object, so no
@@ -351,11 +393,12 @@ def verify(username: str, password: str, required_group: Optional[str] = None) -
             if group:
                 try:
                     if not member_of(conn, sam, group):
-                        log.info("bind succeeded for %r but the account is not in %r",
-                                 sam, group)
+                        log.warning("bind succeeded for %r but the account is NOT in %r",
+                                    sam, group)
                         return LdapResult(False, NOT_IN_GROUP, f"not in {group}")
-                except LookupError:
-                    return LdapResult(False, GROUP_NOT_FOUND, f"group {group!r} not found")
+                except LookupError as exc:
+                    reason = str(exc) or GROUP_NOT_FOUND
+                    return LdapResult(False, reason, f"group {group!r}: {reason}")
 
             return LdapResult(
                 True, OK,

tests/browser_check.py

@@ -172,9 +172,12 @@ def start_server(port, db_path):
     env["DATABASE_URL"] = "sqlite:///" + db_path.replace("\\", "/")
     env.setdefault("AUTH_SECRET_KEY", "browser-check-secret-not-for-production")
     env["WP_LDAP_FAKE_DIRECTORY"] = FAKE_DIRECTORY
-    # No required group: the fake grants "WP-Suite-Users" to everyone, and a test
-    # asserting the group gate belongs in ldap_auth_check where it can be explicit.
-    env.pop("LDAP_REQUIRED_GROUP", None)
+    # SET empty, never pop: server/db.py calls load_dotenv() at import and
+    # python-dotenv only skips keys already present in os.environ, so a popped
+    # variable comes back from the developer's .env inside the subprocess. An empty
+    # string is "present" and therefore wins. The fake grants "WP-Suite-Users" to
+    # everyone; a test asserting the group gate belongs in ldap_auth_check.
+    env["LDAP_REQUIRED_GROUP"] = ""
     proc = subprocess.Popen(
         [sys.executable, "-m", "uvicorn", "server.app:app", "--host", "127.0.0.1",
          "--port", str(port), "--log-level", "warning"],

tests/ldap_auth_check.py

@@ -39,6 +39,8 @@ from cdp import free_port                                            # noqa: E40
 _PASS, _FAIL = [], []
 PW = "CorrectHorseBattery9"
 GROUP = "WP-Suite-Users"
+SECRET = "ldap-auth-check-not-for-production"
+os.environ.setdefault("AUTH_SECRET_KEY", SECRET)
 ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 
 
@@ -61,15 +63,38 @@ def post(base, path, payload):
             return e.code, {}
 
 
+def get(base, path, token):
+    req = urllib.request.Request(base + path, headers={"Cookie": f"wp_session={token}"})
+    try:
+        with urllib.request.urlopen(req, timeout=10) as r:
+            return r.status, json.loads(r.read().decode() or "{}")
+    except urllib.error.HTTPError as e:
+        return e.code, {}
+
+
+def post_as(base, path, token, payload):
+    req = urllib.request.Request(base + path, method="POST",
+                                 data=json.dumps(payload).encode(),
+                                 headers={"Content-Type": "application/json",
+                                          "Cookie": f"wp_session={token}"})
+    try:
+        with urllib.request.urlopen(req, timeout=10) as r:
+            return r.status, json.loads(r.read().decode() or "{}")
+    except urllib.error.HTTPError as e:
+        return e.code, {}
+
+
 def start(port, db_path, fake, required_group=""):
     env = dict(os.environ)
     env["DATABASE_URL"] = "sqlite:///" + db_path.replace("\\", "/")
-    env["AUTH_SECRET_KEY"] = "ldap-auth-check-not-for-production"
+    env["AUTH_SECRET_KEY"] = SECRET
     env["WP_LDAP_FAKE_DIRECTORY"] = json.dumps(fake)
-    if required_group:
-        env["LDAP_REQUIRED_GROUP"] = required_group
-    else:
-        env.pop("LDAP_REQUIRED_GROUP", None)
+    # SET it empty, never pop it. server/db.py calls load_dotenv() at import, and
+    # python-dotenv only skips a key that is already present in os.environ — so a
+    # popped variable is helpfully restored from the developer's .env inside the
+    # subprocess, and the test silently runs against the real required group.
+    # An empty string counts as present, so it wins.
+    env["LDAP_REQUIRED_GROUP"] = required_group or ""
     proc = subprocess.Popen(
         [sys.executable, "-m", "uvicorn", "server.app:app", "--host", "127.0.0.1",
          "--port", str(port), "--log-level", "warning"],
@@ -214,6 +239,55 @@ def main():
         except subprocess.TimeoutExpired:
             pass
 
+    print("")
+    print("4. local state overrides the directory, and the throttle protects it")
+    db_fd3, db3 = tempfile.mkstemp(suffix=".db"); os.close(db_fd3)
+    import sqlalchemy as _sa0
+    from server.db import Base as _B0
+    from server import models as _m0                                   # noqa: F401
+    eng0 = _sa0.create_engine("sqlite:///" + db3.replace("\\", "/"))
+    _B0.metadata.create_all(bind=eng0)
+    with eng0.begin() as con:
+        con.execute(_sa0.text(
+            "insert into users (id,username,email,full_name,role,is_active,"
+            "failed_attempts,token_version,project_role,locale,timezone,"
+            "auto_add_projects,auto_add_role,created_at,updated_at) values "
+            "('user_root','root','','Root','project_user',0,0,0,'','','',0,'',"
+            "datetime('now'),datetime('now'))"))          # is_active = 0
+    eng0.dispose()
+    port3 = free_port()
+    server3 = start(port3, db3, fake, required_group=GROUP)
+    if server3 is None:
+        print("the third test server would not start.")
+        return 2
+    b3 = f"http://127.0.0.1:{port3}"
+    try:
+        st, _ = post(b3, "/api/auth/login", {"username": "root", "password": PW})
+        chk("a disabled local account is refused even though the bind succeeds",
+            st == 403, st)
+
+        # The throttle. AUTH_MAX_ATTEMPTS is 2, and its whole purpose is that failures
+        # are real domain binds counting against the AD lockout policy — so it has to
+        # stop CALLING the directory, not merely refuse. Proving that: burn the budget
+        # with wrong passwords, then present the CORRECT one. A 429 for a credential
+        # that would otherwise succeed is only possible if the throttle runs before the
+        # directory is consulted.
+        codes = [post(b3, "/api/auth/login",
+                      {"username": "outsider", "password": "wrong"})[0] for _ in range(3)]
+        chk("the attempt budget is spent and the next try is throttled",
+            codes[-1] == 429, codes)
+        st, _ = post(b3, "/api/auth/login", {"username": "outsider", "password": PW})
+        chk("...and a CORRECT password is still refused while throttled, proving the "
+            "directory is never reached", st == 429, st)
+        chk("...while another account is unaffected (the budget is per-username)",
+            post(b3, "/api/auth/login", {"username": "root", "password": PW})[0] == 403, "")
+    finally:
+        server3.kill()
+        try:
+            server3.wait(timeout=10)
+        except subprocess.TimeoutExpired:
+            pass
+
     print("\n3. an existing admin survives the switch")
     db_fd2, db2 = tempfile.mkstemp(suffix=".db"); os.close(db_fd2)
     # Build the schema with a NEW engine bound to this file — see users_in().
@@ -252,6 +326,84 @@ def main():
         except subprocess.TimeoutExpired:
             pass
 
+    print("")
+    print("5. the console's own routes, and what token_version actually does")
+    db_fd4, db4 = tempfile.mkstemp(suffix=".db"); os.close(db_fd4)
+    import sqlalchemy as _sa1
+    from server.db import Base as _B1
+    from server import models as _m1, auth as _auth                    # noqa: F401
+    url4 = "sqlite:///" + db4.replace("\\", "/")
+    eng1 = _sa1.create_engine(url4)
+    _B1.metadata.create_all(bind=eng1)
+    with eng1.begin() as con:
+        for uid, un, role in [("user_boss", "root", "admin"),
+                              ("user_pat", "pat", "project_user")]:
+            con.execute(_sa1.text(
+                "insert into users (id,username,email,full_name,role,is_active,"
+                "failed_attempts,token_version,project_role,locale,timezone,"
+                "auto_add_projects,auto_add_role,created_at,updated_at) values "
+                f"('{uid}','{un}','','{un}','{role}',1,0,0,'','','',0,'',"
+                "datetime('now'),datetime('now'))"))
+
+    class _U:
+        pass
+    def _tok(uid, un, role):
+        u = _U(); u.id = uid; u.username = un; u.role = role; u.token_version = 0
+        return _auth.create_token(u)
+    boss_tok = _tok("user_boss", "root", "admin")
+    pat_tok = _tok("user_pat", "pat", "project_user")
+
+    port4 = free_port()
+    server4 = start(port4, db4, fake, required_group="")
+    if server4 is None:
+        print("the fourth test server would not start.")
+        return 2
+    b4 = f"http://127.0.0.1:{port4}"
+    try:
+        st, body = get(b4, "/api/auth/users", boss_tok)
+        chk("an admin can list accounts", st == 200, st)
+
+        post(b4, "/api/auth/login", {"username": "outsider", "password": PW})
+        st, body = get(b4, "/api/auth/users", boss_tok)
+        rows = body if isinstance(body, list) else (body.get("users") or body.get("items") or [])
+        names = [u.get("username") for u in rows if isinstance(u, dict)]
+        chk("a just-provisioned account appears in the Admin console list",
+            "outsider" in names, names)
+
+        # Exactly the request users.html sends — D13 criterion 4.
+        st, _ = post_as(b4, "/api/auth/users/user_pat/role", boss_tok, {"role": "admin"})
+        chk("granting admin through the console route succeeds", st == 200, st)
+        chk("...and the role really changed", users_in(db4).get("pat") == "admin", users_in(db4))
+
+        # token_version. NOTHING in app.py bumps it any more: is_active and role are
+        # re-read from the database every request, so both take effect at once without
+        # it. What it still does is invalidate an ALREADY-ISSUED cookie, which is what
+        # manage_users does on disable. Bump it directly and prove the effect.
+        eng2 = _sa1.create_engine(url4)
+        with eng2.begin() as con:
+            con.execute(_sa1.text("update users set token_version = 1 where id='user_pat'"))
+        eng2.dispose()
+        st, _ = get(b4, "/api/auth/me", pat_tok)
+        chk("bumping token_version invalidates a cookie already issued", st == 401, st)
+        st, _ = get(b4, "/api/auth/me", boss_tok)
+        chk("...and leaves every other session alone", st == 200, st)
+
+        st, body = post(b4, "/api/auth/login", {"username": "root", "password": "wrong"})
+        blob = json.dumps(body).lower()
+        chk("no AD sub-code leaks into a response body",
+            not any(c in blob for c in ("52e", "525", "532", "533", "775", "data ")), body)
+        from server import ldap_auth as _la
+        chk("...though _err49 does parse one when AD sends it",
+            "52e" in _la._err49({"message": "80090308: LdapErr: DSID-0C0903A9, comment: "
+                                            "AcceptSecurityContext error, data 52e, v4563"}))
+    finally:
+        eng1.dispose()
+        server4.kill()
+        try:
+            server4.wait(timeout=10)
+        except subprocess.TimeoutExpired:
+            pass
+
     print("\n" + "-" * 54)
     print(f"{len(_PASS)}/{len(_PASS) + len(_FAIL)} checks passed.")
     for f in _FAIL: