diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index c198bce0..c1343dd0 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -143,12 +143,12 @@ def _activate_user_by_email(db: Session, email: str) -> None: S1 justified that by saying `password_login` refuses an unverified account BEFORE it looks at `is_active`, so a pre-emptor could not sign in "approved or not". A review of the finished epic showed that argument only holds while the address stays unverified — and S6's approval mail - hands the real owner a confirmation link precisely so they will end that state. What actually - guards this now is `_register_account`: while an address is unverified the NEWEST registration - replaces the password and kills the outstanding verify tokens, so the real owner takes the - account back by registering, and a pre-emptor's password is only ever as good as the owner's - silence. The residual (an owner who confirms without ever registering or resetting) is logged - for E12 beside SEC-15. The revoke also stays on the Google-adoption path + hands the real owner a confirmation link precisely so they will end that state. Nothing replaced + the revoke, and that is deliberate: round 2 of the same review tried "the newest registration + owns an unverified account" and reverted it, because the rule is symmetric — an attacker can + always register last — and it handed anonymous callers a write onto a real account's credential. + The owner's recourse is password-reset, which proves the mailbox, rewrites the hash and bumps + the session epoch. The residual is logged as SEC-16. The revoke also stays on the Google-adoption path (auth.revoke_preset_password_if_unverified), where ownership is actually attested.""" # Canonical, with the literal spelling preferred when both exist: approving # `npeter83.temu@gmail.com` must activate the account that registered as `npeter83temu@gmail.com`, @@ -299,10 +299,6 @@ def send_invitation( # but a button the admin pressed must not answer with a success toast for a mail that was # never sent. raise HTTPException(status_code=409, detail={"code": "invite_send", "reason": "email_off"}) - # Same as the other two paths that decide for an address (`approve_invite`, `add_invite`): an - # approved row means this mailbox may come in, so finish it. Without this the mail can promise - # "your account is ready — just sign in" to someone `password_login` will refuse for `is_active`. - _activate_user_by_email(db, inv.email) link, has_account, hours = _welcome_mail_parts(db, inv.email) # An explicit pick from the dialog is recorded, so a later mail to this address matches the one # the admin already sent. Only the explicit pick — writing the fallback back would turn a column @@ -317,10 +313,21 @@ def send_invitation( # exists to answer "did I already write to them?", and the UI dims the button and warns about a # second mail on the strength of it. Stamping it before a send that then failed makes the one # question it answers a lie, and talks the admin out of the retry that would fix it. + # + # One thing a failed send does leave behind: the confirmation token `_welcome_mail_parts` minted + # to render the link, which commits inside `_issue_token`. Accepted rather than chased — the + # token is single-use, expires on its own, and was mailed nowhere; deferring it would mean + # rewriting a helper three callers share so the handler owns its commit. if not email_mod.send_access_invitation(inv.email, lang, link, has_account, hours): raise HTTPException( status_code=422, detail={"code": "invite_send", "reason": "send_failed"} ) + # Only after the mail is actually out. Same as the other two paths that decide for an address + # (`approve_invite`, `add_invite`): an approved row means this mailbox may come in, so finish it, + # or the mail can promise "your account is ready — just sign in" to someone `password_login` will + # refuse for `is_active`. Doing it BEFORE the send would mean a request the UI reports as failed + # had quietly activated an account behind the admin's back. + _activate_user_by_email(db, inv.email) inv.invited_at = datetime.now(timezone.utc) audit.record( db, diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 528f2c93..ec3a25ea 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -13,7 +13,7 @@ from authlib.integrations.starlette_client import OAuth, OAuthError from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request from fastapi.responses import JSONResponse, RedirectResponse from pydantic import BaseModel -from sqlalchemy import delete, select, update +from sqlalchemy import select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -235,12 +235,15 @@ def upsert_pending_invite(db: Session, email: str, lang: str | None = None) -> I .scalars() .all() ) - # An approved row wins, so a denied duplicate can't mask an approval; then the exact spelling; - # then the oldest. Without the middle rule an unordered scan could hand back a DENIED sibling - # while a pending row already existed, and the denied branch below would reopen it — re-notifying - # every admin and putting one person in the queue twice. + # An approved row wins, so a denied duplicate can't mask an approval. Then a row that is not + # DENIED, preferring the exact spelling — a denied sibling must never be picked while a pending + # row exists, or the reopen branch below would fire and put one person in the admin's queue + # twice. Only when every row is denied does the spelling decide which one reopens. + undecided = [r for r in rows if r.status != "denied"] inv = ( next((r for r in rows if r.status == "approved"), None) + or next((r for r in undecided if r.email == email), None) + or next(iter(undecided), None) or next((r for r in rows if r.email == email), None) or next(iter(rows), None) ) @@ -837,29 +840,21 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non # ON THE ACCOUNT, and this is exactly what the public /auth/verify/resend already does # for the same input — so it is not a new capability, just the end of a dead end. An # account that is already confirmed still gets nothing: there is nothing to confirm. + # + # And a re-send is ALL it does: the submitted password is discarded. E9.5's closing + # review tried "while the address is unverified, the newest registration owns it" + # (replace the hash, kill the outstanding verify tokens) as a fix for address + # pre-emption, and round 2 showed it was worse than the problem — DO NOT REPEAT IT. + # The rule is symmetric, and an attacker can always be last: it hands an unauthenticated + # caller the power to overwrite a real registrant's chosen password and delete the + # confirmation link already in their inbox, and on a SEC-10-stripped row (password_hash + # NULL, password_revoked_at set) it would install a password where the whole point was + # that there is none. The owner's real recourse already exists and needs no new + # primitive: password-reset accepts this account, proves the mailbox, rewrites the hash + # and bumps the session epoch. The residual is logged as SEC-16. user = existing - if not user.email_verified and not user.is_demo: - # While the address is UNVERIFIED, the newest registration owns the account. - # - # Anyone can post any address here, so the first registration for a stranger's - # mailbox used to own it permanently: the password stayed, approval activated the - # account (E9.5 S1 stopped revoking it), and the confirmation link the approval mail - # carries goes to the real owner — whose click would open sign-in for the squatter's - # password. Letting the later registration replace the hash makes that pre-emption - # worthless the moment the real owner registers, and it costs a legitimate registrant - # nothing: re-registering an address you already registered is you, choosing a - # password again. Every verify token issued to the previous holder dies with it, so a - # link already in flight cannot confirm the account for them either. - user.password_hash = hash_password(password) - user.password_revoked_at = None - db.execute( - delete(AuthToken).where( - AuthToken.user_id == user.id, AuthToken.kind == "verify" - ) - ) - db.commit() - if email_mod.email_enabled(): - send_verification_link(db, user) + if not user.email_verified and not user.is_demo and email_mod.email_enabled(): + send_verification_link(db, user) return # Without working SMTP we can't deliver a verification link, so email ownership can't gate # sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so @@ -1179,11 +1174,13 @@ def revoke_preset_password_if_unverified(user: User) -> None: S1 also claimed the revoke was REDUNDANT there, because `password_login` refuses an unverified account before it reads `is_active`. That part was wrong, and the epic's own review caught it: the gate holds only while the address is unverified, and S6's approval mail exists to get the real - owner to end that state. The guard that actually replaced it lives in `_register_account` — while - an address is unverified, the newest registration replaces the password and deletes the - outstanding verify tokens, so the owner reclaims the account by registering. Do not re-add a - caller here without re-reading that: two revokes racing would put "your password was cleared" - back in front of every legitimate registrant. + owner to end that state — at which point a pre-emptor's password works. Nothing replaced the + revoke, and that is deliberate. The review tried "the newest registration owns an unverified + account" and reverted it: the rule is symmetric, so an attacker can simply register last, and it + would have handed anonymous callers a write onto a real account's credential. What the owner + actually has is password-reset, which proves the mailbox, rewrites the hash and bumps the epoch. + The residual is logged as SEC-16; re-adding a caller here would put "your password was cleared" + back in front of every legitimate registrant, which is the whole reason S1 removed it. `password_revoked_at` is what makes "the owner re-sets one via password-reset" actually true. The revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and diff --git a/backend/tests/test_review_round.py b/backend/tests/test_review_round.py index 97cc9320..9d7c81a0 100644 --- a/backend/tests/test_review_round.py +++ b/backend/tests/test_review_round.py @@ -87,68 +87,92 @@ def test_a_weeks_link_is_described_in_days_not_hours(wire): assert "24 óra" in wire[1][2] -# --- the pre-emption guard: while unverified, the newest registration owns the account ------------- +# --- what a second registration may and may not do ------------------------------------------------- -def test_registering_again_takes_the_account_back_from_a_pre_emptor(db, client, smtp_on, wire): - """The hole E9.5 S1's argument left open, and the guard that replaces it. +def test_a_later_registration_never_touches_an_existing_password(db, client, smtp_on, wire): + """The rule E9.5's review round 2 put back, having tried the opposite and found it worse. - S1 stopped admin approval revoking a preset password, reasoning that `password_login` refuses an - unverified account before it reads `is_active`. True — but S6's approval mail exists to get the - real owner to confirm the address, and that click opens the gate for whatever password is on the - row. So the owner must be able to take the row back, and re-registering is how. + "While the address is unverified, the newest registration owns it" looked like a fix for address + pre-emption. It is symmetric — an attacker can always be last — so it closed nothing, and it + handed an unauthenticated caller the power to overwrite a real registrant's chosen password and + delete the confirmation link already sitting in their inbox. On a SEC-10-stripped row it would + have installed a password where the whole point is that there is none. Reverted; the owner's + recourse is password-reset, which proves the mailbox. Residual logged as SEC-16. """ - client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"}) - db.expire_all() - user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() - squatted = user.password_hash - assert squatted is not None - client.post( - "/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"} + "/auth/register", json={"email": "victim@example.com", "password": "the first password"} ) - db.expire_all() user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() - assert user.password_hash != squatted - # One account, not two — the canonical pre-check still holds. - assert ( - len(db.execute(select(User).where(User.email_canonical == "victim@example.com")).all()) == 1 - ) - - -def test_the_pre_emptors_verify_link_dies_with_their_password(db, client, smtp_on, wire): - client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"}) - db.expire_all() - user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() - first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() - assert len(first) == 1 - - client.post( - "/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"} - ) - - db.expire_all() - live = db.execute(select(AuthToken).where(AuthToken.user_id == user.id)).scalars().all() - # Exactly one token, and not the one already in the pre-emptor's hands: otherwise they could - # confirm the address themselves and re-open the gate for a password that is no longer theirs. - assert len(live) == 1 and live[0].id != first[0] - - -def test_a_confirmed_account_is_not_re_opened_by_a_later_registration( - db, client, smtp_on, make_user -): - # The guard is scoped to the UNVERIFIED window. Once the address is confirmed the account is its - # owner's, and a stranger posting the form must not touch the password. - user = make_user(email="done@example.com", password=_PW) # already verified before = user.password_hash - client.post("/auth/register", json={"email": "done@example.com", "password": "not yours"}) + client.post( + "/auth/register", json={"email": "victim@example.com", "password": "the second password"} + ) db.expire_all() assert db.get(User, user.id).password_hash == before +def test_a_later_registration_leaves_a_link_already_in_flight_alive(db, client, smtp_on, wire): + client.post( + "/auth/register", json={"email": "victim@example.com", "password": "the first password"} + ) + db.expire_all() + user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one() + first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() + + client.post( + "/auth/register", json={"email": "victim@example.com", "password": "the second password"} + ) + + db.expire_all() + live = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all() + # The re-send adds a link; it must not invalidate the one the owner may already be holding. + assert set(first) <= set(live) + + +def test_a_stripped_account_keeps_its_recovery_stamp(db, client, smtp_on, make_user): + """Prod carries accounts SEC-10 left with no password and a `password_revoked_at` stamp. That + stamp is the only reason password-reset still accepts them; an anonymous POST must not be able to + clear it, nor to put a password on a row whose whole state is "there is none".""" + stripped = make_user( + email="stripped@example.com", + password=None, + email_verified=False, + is_active=True, + password_revoked_at=datetime.now(timezone.utc), + ) + + client.post( + "/auth/register", json={"email": "stripped@example.com", "password": "an attacker password"} + ) + + db.expire_all() + row = db.get(User, stripped.id) + assert row.password_hash is None and row.password_revoked_at is not None + + +def test_a_denied_sibling_is_not_reopened_while_a_pending_row_exists(db, client): + # Canonical siblings exist on prod. Picking the denied one reopens it and re-notifies every + # admin, putting one person in the queue twice — and the exact-spelling tie-break made that + # deterministic rather than merely possible. + db.add_all( + [ + Invite(email="pair.one@gmail.com", status="pending"), + Invite(email="pairone@gmail.com", status="denied"), + ] + ) + db.commit() + + client.post("/auth/request-access", json={"email": "pairone@gmail.com"}) + + db.expire_all() + rows = {i.email: i.status for i in db.execute(select(Invite)).scalars().all()} + assert rows == {"pair.one@gmail.com": "pending", "pairone@gmail.com": "denied"} + + # --- the env bootstrap answers the same question as the gate --------------------------------------