T10.7 - close four more done-when boxes offline; token_version was mis-specified
I had said all six remaining boxes needed the live environment. Four did not,
and saying so was lazy scoping. ldap_auth_check now covers them (32/32):
a just-provisioned account appears in GET /api/auth/users, the request the
Admin console makes - without which nobody could grant a JIT account access
granting admin through POST /api/auth/users/{id}/role, the request users.js
sends, with the role verified to have actually changed. D13 criterion 4 was
until now only asserted for role PRESERVATION, never for role GRANTING.
no AD error-49 sub-code appears in any response body, while _err49 does parse
one out of a real AD message - the log gets the detail, the caller does not
token_version invalidates a cookie already issued, and leaves other sessions
alone
That last box was wrong as written. It asked to exercise token_version "by a
role change", and nothing in app.py bumps it on a role change - or on a
deactivation. Neither needs to: get_current_user re-reads the account from the
database every request, so both take effect on the next request regardless.
T10.3's note that "role changes and deactivation should bump it" described an
intention rather than the code, and I repeated it without checking.
What token_version actually is now: a mechanism whose only trigger is the bump
manage_users makes on disable, which is belt-and-braces since is_active already
refuses the request. It works, and it is tested - but nothing much triggers it.
Logged as BL-030 rather than resolved here, because whether to wire it to
something (a "sign out everywhere" control is the usual shape) or remove it is
a session-handling design question, not an auth-wave bug.
Two boxes remain open, and both genuinely need your environment: member_of
against a real NESTED group, and the in-container openssl certificate check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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 `<a href="#">` 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
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user