{ui.esc(_WORDING[purpose]["done"])} Add this server to Claude as a custom +connector:
{ui.esc(base_url)}/mcp
""" return ui.auth_page("All set", body) def setup_invalid_html() -> str: + """One page for every failure — see the module note on why it says nothing specific.""" body = """""" return ui.auth_page("Invalid link", body) @@ -114,55 +221,141 @@ async def _form(request: Request) -> dict[str, str]: return {k: (v if isinstance(v, str) else "") for k, v in data.items()} -def _pending_user(username: str) -> dict[str, Any] | None: - """The pending user a token names, or None (unknown / already claimed / no store).""" +def _user(username: str) -> dict[str, Any] | None: + """The account a token names, or None (unknown, or no store).""" store = _open_store() if store is None: return None try: - user = user_store.get_user(store, username) + return user_store.get_user(store, username) finally: store.close() - return user if (user is not None and is_pending(user)) else None + + +def _actionable(token: str) -> tuple[str, str, str | None] | None: + """`(username, purpose, expected_hash)` for a token that may act on that account right now. + + The one place the two purposes are matched against the state of the row, so a link can only ever + do the job it was minted for: + + - `setup` requires a **pending** account — nothing to overwrite. + - `reset` requires a **claimed, active, password-based** account whose credential is still the + one the token was minted against. Together the two mean a reset token cannot act on a pending + account, nor a setup token on a claimed one. + + **Every condition here mirrors `user_store.reset_password`'s WHERE, and the mirroring is the + point.** When it did not, two things broke at once and neither was obvious. A token for an account + that had since bound an identity provider got a **200 and a password form**, because only the + write refused — so somebody chose a password, submitted it and was told the link was invalid, + which is the exact split the switched-off case already calls unacceptable. And because such a + token could never spend (the write refuses, so the credential never changes, so the marker still + matches), it stayed actionable for its whole three-day life while every POST paid for an argon2 + hash: an unauthenticated, unrate-limited, unbounded CPU lever, measured at 45x a rejected token. + Refusing here is what makes both of those a flat 400 that hashes nothing. + + `expected_hash` is handed back so the UPDATE can be guarded on it. Checking the marker here and + writing afterwards is check-then-act across two connections; see `reset_password` for why that + loses the single-use property it appears to provide. + """ + # Through the verifiers rather than re-deriving from `_decode`: each purpose's claim validation + # then exists once. When this decoded for itself, `verify_reset_token`'s `isinstance` check on + # `cred` was skipped on the live path — so the type-checked function was dead code and the public + # path was the one missing the check, which is the drift `_decode` exists to prevent. + setup_for = verify_setup_token(token) + if setup_for is not None: + user = _user(setup_for) + return (setup_for, _SETUP_PURPOSE, None) if user and is_pending(user) else None + + reset = verify_reset_token(token) + if reset is None: + return None + username, cred = reset + user = _user(username) + if user is None: + return None + stored = user.get("password_hash") + if ( + not stored + or user.get("oidc_provider") is not None + or user.get("status") != user_store.ACTIVE_STATUS + ): + return None + # `compare_digest` rather than `==` for the usual reason a secret-derived value gets it: this is + # the check that decides whether a link is spent, and it is reachable by anyone holding it. + if not compare_digest(user_store.credential_fingerprint(user), cred): + return None + return (username, _RESET_PURPOSE, stored) async def claim(request: Request) -> Response: - """GET → the set-password page for a valid link to a still-pending user; POST → set the password. - Every failure (bad token, already-claimed, weak password, lost race) is a generic page — no - credential overwrite of a claimed account, no enumeration. Both the token check and the pending - re-read run BEFORE any argon2 hash, so an attacker can't drive hashing without a valid (signed, - admin-minted) token for a still-pending user. This endpoint is **not** rate-limited in-process — - rely on the deployment's proxy/LB for that (a documented gap, like the other public endpoints).""" + """GET → the choose-a-password page for a link that can still act; POST → write the password. + + Every failure (bad token, wrong state for the purpose, a spent reset link, a weak password, a lost + race) is the same generic page — no credential overwrite a token did not authorise, and no + enumeration. This endpoint is **not** rate-limited in-process — rely on the deployment's proxy/LB + for that (a documented gap, like the other public endpoints). + """ if request.method == "GET": - username = verify_setup_token(request.query_params.get("token", "")) - if username is None or _pending_user(username) is None: + token = request.query_params.get("token", "") + actionable = _actionable(token) + if actionable is None: return HTMLResponse(setup_invalid_html(), status_code=400) - return HTMLResponse(setup_page_html(request.query_params.get("token", ""))) + return HTMLResponse(claim_page_html(token, actionable[1])) form = await _form(request) token = form.get("token", "") - username = verify_setup_token(token) - if username is None or _pending_user(username) is None: + actionable = _actionable(token) + if actionable is None: return HTMLResponse(setup_invalid_html(), status_code=400) + username, purpose, expected_hash = actionable password = form.get("password", "") if not _MIN_PASSWORD_LEN <= len(password) <= _MAX_PASSWORD_LEN: return HTMLResponse( - setup_page_html(token, error=f"Use at least {_MIN_PASSWORD_LEN} characters."), + claim_page_html(token, purpose, error=f"Use at least {_MIN_PASSWORD_LEN} characters."), status_code=400, ) + # Off the event loop. Argon2 is deliberately slow and memory-hard, and this handler is `async` on a + # public endpoint — hashing inline stalls every other request in flight for the duration, which is + # the reason `oauth_server`'s own credential check offloads. Everything blocking goes together, so + # the store work rides the same worker rather than crossing back and forth. + changed = await run_blocking(_write_password, username, purpose, password, expected_hash) + if not changed: + return HTMLResponse(setup_invalid_html(), status_code=400) + from mcp_http import public_base_url + + return HTMLResponse(claim_done_html(public_base_url(), purpose)) + + +def _write_password(username: str, purpose: str, password: str, expected_hash: str | None) -> int: + """Set the password and, for a reset, revoke that principal's sessions. Rows changed, 0 if refused. + + **One transaction, and it has to be.** The password moving and the old sessions dying are one + event: committed separately, a revocation that failed after the write would leave the credential + changed, every old refresh token live, and the caller looking at a 500 — a reset in name only, on + a link that is now spent so they cannot retry. Committed together, either both happen or neither + does. `revoke_refresh_tokens_for` is also called only when the write actually moved a row, so a + lost race never signs somebody out on behalf of a change that did not happen. + + Synchronous by design — it is called through `run_blocking`; see the caller. + """ store = _open_store() if store is None: - return HTMLResponse(setup_invalid_html(), status_code=400) + return 0 try: - # Guarded: the UPDATE only fires while still pending — a concurrent claim makes it a no-op. - changed = user_store.claim_pending_password(store, username, password) + if purpose == _SETUP_PURPOSE: + # Guarded: the UPDATE only fires while still pending — a concurrent claim makes it a no-op. + return user_store.claim_pending_password(store, username, password) + # Likewise guarded, on different conditions (`user_store.reset_password` spells them out) — + # including the credential it was minted against, which is what makes a replay a no-op here. + changed = user_store.reset_password( + store, username, password, expected_hash or "", commit=False + ) + if changed: + oauth_server.revoke_refresh_tokens_for(store, username, commit=False) + store.commit() + return changed finally: store.close() - if not changed: - return HTMLResponse(setup_invalid_html(), status_code=400) - from mcp_http import public_base_url - - return HTMLResponse(setup_done_html(public_base_url())) def routes() -> list: diff --git a/packages/agami-core/src/user_store.py b/packages/agami-core/src/user_store.py index 5d95e77f..e42f9c10 100644 --- a/packages/agami-core/src/user_store.py +++ b/packages/agami-core/src/user_store.py @@ -12,6 +12,7 @@ from __future__ import annotations +import hashlib import os from datetime import datetime, timezone from typing import Any @@ -21,7 +22,13 @@ from ports import Principal from store import Store -_ACTIVE = "active" +#: The status a live account holds. **Public, because two modules must agree on it**: `onboarding` +#: refuses a reset for a switched-off account before drawing the page, and `reset_password`'s WHERE +#: refuses the write. Those two checks exist deliberately as separate layers, and a copy of this +#: string in each is exactly how layers drift apart — which is the defect this whole flow was fixed +#: for once already. `_ACTIVE` stays as the in-module alias so nothing below has to change. +ACTIVE_STATUS = "active" +_ACTIVE = ACTIVE_STATUS # The OIDC provider keys a deploy may pin the admin to. Mirrors `oidc._PROVIDERS`, duplicated here on # purpose: `oidc` is the one egress module (httpx), and `user_store` must stay import-light + egress-free. @@ -127,6 +134,82 @@ def claim_pending_password(store: Store, username: str, password: str) -> int: return cur.rowcount +def reset_password( + store: Store, username: str, password: str, expected_hash: str, *, commit: bool = True +) -> int: + """Set the password of an account that **already has one** — the administrator-initiated reset. + + Deliberately the opposite of `claim_pending_password` above, which fires only while an account is + still pending. This one overwrites a live credential, so every condition that makes that safe is + in the WHERE rather than trusted to the caller: a guard a route forgets to apply is a guard that + is not there, and this UPDATE is one route away from a public page. + + - `oidc_provider IS NULL` — an SSO identity never grows a password. Without it, a reset would add + a second way into an account whose deployment decided there is exactly one, and the person + whose password it is would have no idea it existed. + - `status = ?` (active) — a reset cannot resurrect a switched-off account. The account being off + is the security decision; letting a link undo it silently would make the link the stronger one. + - `password_hash = ?` (`expected_hash`) — **this is what actually makes a reset link single-use.** + Checking the credential marker in the handler and then writing is check-then-act across two + connections: two simultaneous posts of the same link both read the old hash, both pass, and both + UPDATE, so whoever commits last owns the account while the other is told they succeeded. As a + condition of the UPDATE the loser matches no row, gets 0, and is shown the same invalid page as + any other spent link. `claim_pending_password` above is single-use by exactly this shape + (`password_hash IS NULL`); this is that argument applied to a credential that already exists. + + `commit=False` leaves the transaction open so the caller can land the revocation of that + principal's sessions in the same one — a password that has moved with sessions still renewing on + the old one is a reset in name only, and the two must not be separately committable. + + Returns the row count: 0 means refused, and the caller must not report success. It cannot tell + you WHICH condition refused, on purpose — the page this is reached from is public and answers + every failure identically. + """ + cur = store.execute( + "UPDATE users SET password_hash = ? WHERE username = ? AND password_hash = ? " + "AND password_hash IS NOT NULL AND oidc_provider IS NULL AND status = ?", + (hash_password(password), username, expected_hash, _ACTIVE), + ) + if commit: + store.commit() + return cur.rowcount + + +def credential_fingerprint(user: dict[str, Any]) -> str: + """A short, one-way marker of the credential an account holds right now. + + This is what makes a reset link **single-use**, and it needs its own mechanism because the one + that makes a *setup* link single-use does not apply: a setup token dies because claiming flips the + account out of pending, and a reset leaves the account exactly as claimed as it was. So the token + carries this marker, the claim page re-computes it, and setting a new password changes the stored + hash — which retires every link minted against the old one, including the one just used. + + **A hash of the hash, never the hash itself.** A setup link's payload is base64url and readable by + whoever holds it (`onboarding`'s module note says so), so putting `password_hash` in a token would + hand an argon2 digest to anyone the link is forwarded to. Truncated because this is a change + detector, not a credential: it needs to differ when the hash differs, and it is compared only + against a value re-derived from the same row. + + Argon2 salts every hash, so re-setting the *same* password still produces a different digest and + still retires the old link. `authenticate`'s opportunistic rehash changes the stored hash too, so + a cost-parameter bump silently spends outstanding reset links on the owner's next sign-in — which + is the safe direction and worth knowing before it looks like a bug. + + **Raises for an account with no password**, rather than fingerprinting the empty string. That + fallback would have produced `sha256("")` — one publicly computable constant, identical for every + passwordless row in every deployment — so the marker would have been a marker of nothing and the + single-use binding would have been vacuous exactly where the account is most exposed. There is no + legitimate caller: a reset is for an account that HAS a credential, and a pending one is the setup + link's job. + """ + stored = user.get("password_hash") + if not stored: + raise ValueError( + "this account has no password to fingerprint; a reset does not apply to it" + ) + return hashlib.sha256(stored.encode()).hexdigest()[:16] + + def get_user_by_email(store: Store, email: str) -> dict[str, Any] | None: """Look up a user by email — the onboarded-only lookup OIDC uses. Case-insensitive (emails are stored lowercased) and one-to-one (the email index is UNIQUE).""" diff --git a/tests/test_onboarding.py b/tests/test_onboarding.py index dbbd746b..3ca71c59 100644 --- a/tests/test_onboarding.py +++ b/tests/test_onboarding.py @@ -25,6 +25,7 @@ import jwt # noqa: E402 import mcp_http # noqa: E402 +import oauth_server # noqa: E402 import onboarding # noqa: E402 import user_store # noqa: E402 from starlette.testclient import TestClient # noqa: E402 @@ -36,6 +37,21 @@ ADMIN_PW = "admin-password-localtest" PENDING = "newbie@example.com" +# The password a refused attempt sends. A whole English phrase rather than a credential-shaped string +# — the convention `tests/e2e/test_member_onboarding.py` already states for its own passphrase — and +# here it is load-bearing rather than stylistic: the credential-shaped version tripped the secret scan +# in CI on four lines. It only has to clear agami-core's eight-character floor and never be accepted. +# **Every password in the reset tests is an English phrase, not a credential-shaped string**, and +# that is a requirement rather than a style: the credential-shaped versions tripped two independent +# secret scanners in CI. They are synthetic either way — what changes is whether a scanner can tell. +# The convention (and the reason) is already stated in `tests/e2e/test_member_onboarding.py`. Each +# only has to clear agami-core's eight-character floor. +REFUSED_ATTEMPT = "the password this request must not be allowed to set" +CHOSEN_AT_RESET = "the passphrase chosen when the reset link was used" +REPLAYED_ATTEMPT = "the passphrase a replayed link would try to set" +SET_OUT_OF_BAND = "the passphrase set by some route other than the link" +ALREADY_HELD = "the passphrase this account already had" + @pytest.fixture def env(tmp_path, monkeypatch): @@ -46,8 +62,12 @@ def env(tmp_path, monkeypatch): monkeypatch.setenv("AGAMI_ADMIN_USERNAME", ADMIN_USER) monkeypatch.setenv("AGAMI_ADMIN_PASSWORD", ADMIN_PW) # A password deployment — no OIDC configured. - for var in ("AGAMI_OIDC_GOOGLE_CLIENT_ID", "AGAMI_OIDC_GOOGLE_CLIENT_SECRET", - "AGAMI_OIDC_MICROSOFT_CLIENT_ID", "AGAMI_OIDC_MICROSOFT_CLIENT_SECRET"): + for var in ( + "AGAMI_OIDC_GOOGLE_CLIENT_ID", + "AGAMI_OIDC_GOOGLE_CLIENT_SECRET", + "AGAMI_OIDC_MICROSOFT_CLIENT_ID", + "AGAMI_OIDC_MICROSOFT_CLIENT_SECRET", + ): monkeypatch.delenv(var, raising=False) s = Store.connect(db_url) s.run_migrations() @@ -73,16 +93,27 @@ def test_setup_token_round_trips(env): def test_setup_token_rejects_forged_expired_and_wrong_purpose(env): assert onboarding.verify_setup_token("not-a-jwt") is None # signed with a different key → bad signature - forged = jwt.encode({"sub": PENDING, "purpose": "setup", "exp": 9_999_999_999}, "y" * 40, algorithm="HS256") + forged = jwt.encode( + {"sub": PENDING, "purpose": "setup", "exp": 9_999_999_999}, "y" * 40, algorithm="HS256" + ) assert onboarding.verify_setup_token(forged) is None # expired expired = jwt.encode( - {"sub": PENDING, "purpose": "setup", "exp": int(datetime(2000, 1, 1, tzinfo=timezone.utc).timestamp())}, - SECRET, algorithm="HS256", + { + "sub": PENDING, + "purpose": "setup", + "exp": int(datetime(2000, 1, 1, tzinfo=timezone.utc).timestamp()), + }, + SECRET, + algorithm="HS256", ) assert onboarding.verify_setup_token(expired) is None # right key, wrong purpose (e.g. a bearer/admin-session token replayed as a setup link) - wrong = jwt.encode({"sub": PENDING, "purpose": "admin_session", "exp": 9_999_999_999}, SECRET, algorithm="HS256") + wrong = jwt.encode( + {"sub": PENDING, "purpose": "admin_session", "exp": 9_999_999_999}, + SECRET, + algorithm="HS256", + ) assert onboarding.verify_setup_token(wrong) is None @@ -103,17 +134,21 @@ def test_claim_link_is_single_use(client, env): token = onboarding.mint_setup_token(PENDING) client.post("/claim", data={"token": token, "password": "teammate-pw-123"}) # replay: the user is no longer pending → generic invalid page, password unchanged - r = client.post("/claim", data={"token": token, "password": "attacker-pw-999"}) + r = client.post("/claim", data={"token": token, "password": REPLAYED_ATTEMPT}) assert r.status_code == 400 and "isn't valid" in r.text s = Store.connect(env) - assert user_store.authenticate(s, PENDING, "teammate-pw-123") is not None # original still works - assert user_store.authenticate(s, PENDING, "attacker-pw-999") is None + assert ( + user_store.authenticate(s, PENDING, "teammate-pw-123") is not None + ) # original still works + assert user_store.authenticate(s, PENDING, REPLAYED_ATTEMPT) is None s.close() def test_claim_rejects_a_bad_token(client): assert client.get("/claim", params={"token": "nope"}, follow_redirects=False).status_code == 400 - assert client.post("/claim", data={"token": "nope", "password": "whatever-123"}).status_code == 400 + assert ( + client.post("/claim", data={"token": "nope", "password": "whatever-123"}).status_code == 400 + ) def test_claim_rejects_a_short_password(client, env): @@ -131,6 +166,337 @@ def test_claim_for_an_already_password_user_is_refused(client, env): assert client.get("/claim", params={"token": token}).status_code == 400 +# --- the RESET link ---------------------------------------------------------- +# +# A reset link overwrites a credential somebody is using today, which the setup link never does. So +# these press on the four things that keep that safe: it works only on the account state it was minted +# for, it is spent by use, it can never reach an SSO or a switched-off account, and it ends the +# sessions running on the old password. + + +def _reset_token(env, username: str) -> str: + """A reset link's token, minted the way the console mints one — against the credential the account + holds at this moment.""" + s = Store.connect(env) + try: + return onboarding.mint_reset_token( + username, user_store.credential_fingerprint(user_store.get_user(s, username)) + ) + finally: + s.close() + + +def test_reset_token_and_setup_token_are_not_interchangeable(env): + # The property the two purposes exist for. A setup token names a pending account; a reset token + # names a claimed one; neither verifier accepts the other's token, so neither link can do the + # other's job even before the store is consulted. + setup, reset = onboarding.mint_setup_token(PENDING), _reset_token(env, ADMIN_USER) + assert onboarding.verify_setup_token(reset) is None + assert onboarding.verify_reset_token(setup) is None + assert onboarding.verify_reset_token(reset)[0] == ADMIN_USER + + +def test_reset_link_sets_a_new_password_for_a_claimed_user(client, env): + token = _reset_token(env, ADMIN_USER) + # Worded for somebody who HAS an account — being told to "finish setting up" would read as though + # theirs had been wiped. + assert "Choose a new password" in client.get("/claim", params={"token": token}).text + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 200 + ) + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + assert user_store.authenticate(s, ADMIN_USER, ADMIN_PW) is None # the old one is gone + s.close() + + +def test_reset_link_is_single_use(client, env): + # Nothing about the ROW changes state here the way a claim flips `pending`, so the only thing + # retiring this link is the credential marker in the token. If that check regressed, this replay + # would succeed and an administrator's old link would stay live forever. + token = _reset_token(env, ADMIN_USER) + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}) + replay = client.post("/claim", data={"token": token, "password": REPLAYED_ATTEMPT}) + assert replay.status_code == 400 and "isn't valid" in replay.text + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + assert user_store.authenticate(s, ADMIN_USER, REPLAYED_ATTEMPT) is None + s.close() + + +def test_a_reset_link_dies_when_the_password_moves_by_any_other_route(client, env): + # The marker is bound to the credential, not to this request — so a link minted before somebody + # changed their password elsewhere is already spent when it arrives. + token = _reset_token(env, ADMIN_USER) + s = Store.connect(env) + user_store.reset_password( + s, ADMIN_USER, SET_OUT_OF_BAND, user_store.get_user(s, ADMIN_USER)["password_hash"] + ) + s.close() + assert client.get("/claim", params={"token": token}).status_code == 400 + + +def test_a_reset_token_cannot_act_on_a_pending_account(client, env): + # The mirror of `test_claim_for_an_already_password_user_is_refused`, and the reason the purposes + # are matched against the row's state rather than trusted from the token. The marker is hand-made: + # there is no legitimate way to mint one for a pending account, which is the next test. + forged_for_pending = onboarding.mint_reset_token(PENDING, "deadbeefdeadbeef") + assert client.get("/claim", params={"token": forged_for_pending}).status_code == 400 + assert ( + client.post( + "/claim", data={"token": forged_for_pending, "password": REFUSED_ATTEMPT} + ).status_code + == 400 + ) + + +def test_a_credential_marker_cannot_be_taken_for_an_account_with_no_password(env): + """The marker for a passwordless row would otherwise be `sha256("")` — one constant, identical for + every such row in every deployment, so the binding that makes a reset link single-use would be a + binding to a publicly known value exactly where the account is least protected. Raising means there + is no way to mint one at all, rather than a way that quietly means nothing.""" + s = Store.connect(env) + try: + with pytest.raises(ValueError): + user_store.credential_fingerprint(user_store.get_user(s, PENDING)) + # ...while a claimed account gives one, and it changes when the credential does. + before = user_store.credential_fingerprint(user_store.get_user(s, ADMIN_USER)) + user_store.reset_password( + s, ADMIN_USER, CHOSEN_AT_RESET, user_store.get_user(s, ADMIN_USER)["password_hash"] + ) + assert user_store.credential_fingerprint(user_store.get_user(s, ADMIN_USER)) != before + finally: + s.close() + + +def test_a_reset_never_gives_an_sso_identity_a_password(client, env): + # An SSO account has no password by design. A reset that added one would be a second way in that + # the deployment never chose and the account holder would never know about. + s = Store.connect(env) + user_store.create_user( + s, + username="sso@example.com", + email="sso@example.com", + password=None, + oidc_provider="google", + oidc_subject="sub-123", + ) + s.close() + # Hand-made marker again — the account has no credential to mint one against. + token = onboarding.mint_reset_token("sso@example.com", "deadbeefdeadbeef") + # The GET matters as much as the POST: drawing the form and refusing the submit is the split this + # whole resolver exists to avoid. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.get_user(s, "sso@example.com")["password_hash"] is None + s.close() + + +def test_a_reset_is_refused_once_an_account_has_bound_an_identity_provider(client, env): + """The case that ISOLATES the `oidc_provider IS NULL` guard, and the reachable one. + + The test above does not: that account has no password either, so `password_hash IS NOT NULL` + refuses it first and the OIDC guard is never consulted — removing the guard leaves that test + green. This is the state where only the guard stands in the way: somebody who signed in with a + password and has since bound a provider (`bind_oidc_subject`, on their first OIDC login), so the + row carries BOTH. Their way in is now the provider, and a reset would quietly re-arm the password + that is no longer how they get in. + """ + s = Store.connect(env) + user_store.create_user( + s, username="both@example.com", email="both@example.com", password=ALREADY_HELD + ) + user_store.bind_oidc_subject(s, "both@example.com", "sub-456") + s.execute( + "UPDATE users SET oidc_provider = ? WHERE username = ?", ("google", "both@example.com") + ) + s.commit() + token = onboarding.mint_reset_token( + "both@example.com", + user_store.credential_fingerprint(user_store.get_user(s, "both@example.com")), + ) + s.close() + # 400 on the GET too. When it was 200 the page was drawn for an account the write would refuse, so + # somebody chose a password, submitted it and was told the link was invalid — and, because such a + # token can never spend, every attempt paid for an argon2 hash on a public endpoint. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.authenticate(s, "both@example.com", REFUSED_ATTEMPT) is None + s.close() + + +def test_a_reset_cannot_revive_a_switched_off_account(client, env): + # Switching an account off is a security decision; a link that undid it would be the stronger of + # the two, and it is held by whoever was forwarded it. + token = _reset_token(env, ADMIN_USER) + s = Store.connect(env) + user_store.set_status(s, ADMIN_USER, "disabled") + s.close() + # The GET matters on its own: without the handler's own check the page would be DRAWN for a + # switched-off account and only the write would refuse, so somebody would choose a password, submit + # it, and be told the link is invalid. Refusing here is the difference between the two layers. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is None + s.close() + + +def test_reset_password_refuses_in_its_own_where_clause(env): + """`user_store.reset_password`'s guards, exercised directly rather than through a page. + + **Not redundant with the tests above, and finding that out is why this exists.** `_actionable` + checks the same conditions first, so through `/claim` these UPDATEs are never reached with a row + that should be refused — deleting a condition from the WHERE left every page test green. A guard + nothing can fail is a guard nothing is holding, and this one is the last line before a live + credential is overwritten. + """ + s = Store.connect(env) + try: + # Pending: no password to replace. This is `claim_pending_password`'s job, not this one's. + assert user_store.reset_password(s, PENDING, CHOSEN_AT_RESET, "whatever") == 0 + # Bound to an identity provider: their way in is the provider now. + user_store.create_user( + s, username="bound@example.com", email="bound@example.com", password=ALREADY_HELD + ) + s.execute( + "UPDATE users SET oidc_provider = ? WHERE username = ?", ("google", "bound@example.com") + ) + s.commit() + bound = user_store.get_user(s, "bound@example.com")["password_hash"] + assert user_store.reset_password(s, "bound@example.com", CHOSEN_AT_RESET, bound) == 0 + # Switched off: the account being off outranks the link. + user_store.create_user( + s, username="off@example.com", email="off@example.com", password=ALREADY_HELD + ) + user_store.set_status(s, "off@example.com", "disabled") + off = user_store.get_user(s, "off@example.com")["password_hash"] + assert user_store.reset_password(s, "off@example.com", CHOSEN_AT_RESET, off) == 0 + # ...and the one it is for. `stale` is captured first so the replay below can present it. + stale = user_store.get_user(s, ADMIN_USER)["password_hash"] + assert user_store.reset_password(s, ADMIN_USER, CHOSEN_AT_RESET, stale) == 1 + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + # **The race.** A second post of the same link presents the hash it read before the first + # one landed. Without `password_hash = ?` in the WHERE both writes succeed and the later + # one owns the account, while the person who legitimately reset is told it worked. + assert user_store.reset_password(s, ADMIN_USER, REPLAYED_ATTEMPT, stale) == 0 + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + finally: + s.close() + + +def test_a_reset_revokes_the_sessions_running_on_the_old_password(client, env): + # Otherwise the reset is one in name only: whoever was signed in keeps renewing indefinitely on a + # credential that has been taken away from them. + s = Store.connect(env) + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-1", "fam-1", "client-1", ADMIN_USER, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + # A second principal's live token, to prove the revocation is scoped to one person. + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-2", "fam-2", "client-1", PENDING, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + s.commit() + s.close() + token = _reset_token(env, ADMIN_USER) + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 200 + ) + s = Store.connect(env) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-1",))[0][ + "revoked" + ] + == 1 + ) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-2",))[0][ + "revoked" + ] + == 0 + ) + # The count is the function's only output; asserting it is what stops it silently becoming 0. + assert oauth_server.revoke_refresh_tokens_for(s, PENDING) == 1 + s.close() + + +def test_a_failed_reset_leaves_every_session_alone(client, env, monkeypatch): + """The ordering: revocation happens only for a write that actually moved a row. + + **The obvious version of this test does not test it.** Posting a short password returns on the + length check, before the store is opened at all — so the guard could be deleted and the test would + still pass, which is what review found. The write has to be REACHED and refused, so the UPDATE is + forced to match nothing: that is the lost-race outcome, where the credential moved between the + check and the write. + """ + s = Store.connect(env) + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-3", "fam-3", "client-1", ADMIN_USER, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + s.commit() + s.close() + token = _reset_token(env, ADMIN_USER) + monkeypatch.setattr(user_store, "reset_password", lambda *a, **k: 0) + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 400 + ) + s = Store.connect(env) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-3",))[0][ + "revoked" + ] + == 0 + ) + s.close() + + +def test_claim_refuses_a_token_whose_purpose_is_not_one_of_the_two(client, env): + """Wrong-purpose rejection, on the LIVE path. + + It was covered only against `verify_setup_token`, which the handler stopped calling when + `_actionable` began decoding for itself — so the property still held and nothing was holding it: + review mutated the purpose comparison so that an `admin_session` cookie JWT would act as a setup + link, and every test stayed green. These post the tokens that actually exist in this deployment + signed with the same secret, plus one carrying no purpose at all. + """ + for purpose in ("admin_session", "", None, 7): + claims = {"sub": PENDING, "exp": 9_999_999_999} + if purpose is not None: + claims["purpose"] = purpose + token = jwt.encode(claims, SECRET, algorithm="HS256") + assert client.get("/claim", params={"token": token}).status_code == 400, purpose + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code + == 400 + ), purpose + + +def test_a_reset_token_with_a_malformed_marker_is_refused_rather_than_crashing(client, env): + """A `cred` that is not a string reached `compare_digest` and raised, which is a 500 — the one + response distinguishable from the uniform generic page, and therefore an oracle. The handler now + goes through `verify_reset_token`, whose type check is what keeps this a 400.""" + for cred in (None, 7, ["x"]): + claims = {"sub": ADMIN_USER, "purpose": "reset", "exp": 9_999_999_999} + if cred is not None: + claims["cred"] = cred + token = jwt.encode(claims, SECRET, algorithm="HS256") + assert client.get("/claim", params={"token": token}).status_code == 400, cred + + # --- the admin roster setup link --------------------------------------------- @@ -142,7 +508,7 @@ def test_admin_roster_shows_a_setup_link_for_pending_users(client): _login(client) html = client.get("/admin").text assert "Setup link" in html - m = re.search(r'/claim\?token=([\w.\-]+)', html) + m = re.search(r"/claim\?token=([\w.\-]+)", html) assert m and onboarding.verify_setup_token(m.group(1)) == PENDING # the admin's own (password) row offers no setup link admin_row = next(r for r in html.split("