diff --git a/docs/waves/backlog.md b/docs/waves/backlog.md index df784c5..07c162e 100644 --- a/docs/waves/backlog.md +++ b/docs/waves/backlog.md @@ -623,3 +623,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. diff --git a/docs/waves/wave-10.md b/docs/waves/wave-10.md index 0404b4c..d0b0155 100644 --- a/docs/waves/wave-10.md +++ b/docs/waves/wave-10.md @@ -104,7 +104,7 @@ misconfigured deploy is indistinguishable from a forgotten password at the login - [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" -- [ ] error-49 sub-codes appear in the log and nowhere in any response +- [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 --- @@ -142,7 +142,7 @@ is criterion 4 and must keep working. - [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`) - [x] the migration's `downgrade()` recreates the column nullable, not `NOT NULL` — there are no hashes to put back — verified on SQLite and Postgres -- [ ] `token_version` still invalidates sessions, exercised by a role change +- [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` @@ -189,7 +189,7 @@ creating it is exactly the kind of event that record exists for. - [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 -- [ ] the new account appears in the Admin console user list so access can be granted +- [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 @@ -282,7 +282,7 @@ Keep the role-granting controls exactly as they are. That is criterion 4. - [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 `` or handler remains for a removed view -- [ ] granting admin to an existing user still works from the console +- [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 diff --git a/tests/ldap_auth_check.py b/tests/ldap_auth_check.py index bcccb98..8034e49 100644 --- a/tests/ldap_auth_check.py +++ b/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,10 +63,31 @@ 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 @@ -301,6 +324,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: