diff --git a/.env.example b/.env.example index 8bbc1919..a016c471 100644 --- a/.env.example +++ b/.env.example @@ -161,16 +161,42 @@ FALKORDB_URL=redis://localhost:6379/0 # REQUIRED - change to your FalkorDB URL # OAUTH_BASE_URL=http://localhost:5000 # ----------------------------- -# Email Configuration (optional - for sending invitation emails) -# ----------------------------- +# Email Configuration +# ----------------------------- +# Signup with email/password sends a six-digit confirmation code, and the +# account is only created when that code is typed back into the signup form. +# With APP_ENV=development and no MAIL_SERVER the message is written to the +# application log instead of being sent, which is enough for local development +# -- copy the code out of the log. Anywhere else an unconfigured process +# refuses the send rather than logging the code, so signup fails loudly instead +# of leaving users waiting for mail nobody sent. +# +# Any provider works: Mailgun, SendGrid, Resend, SES and Postmark all expose an +# SMTP endpoint. # MAIL_SERVER=smtp.mailgun.org -# MAIL_PORT=587 -# MAIL_USE_TLS=True -# MAIL_USERNAME=your_mail_username +# MAIL_PORT=587 # 465 selects implicit TLS (SMTPS) +# MAIL_USE_TLS=True # STARTTLS on non-465 ports +# MAIL_USERNAME=your_mail_username # omit to send unauthenticated # MAIL_PASSWORD=your_mail_password # MAIL_DEFAULT_SENDER=noreply@yourdomain.com +# MAIL_TIMEOUT_SECONDS=10 + +# Write messages to this directory as .eml files instead of sending them. Used +# by the Playwright suite to read the verification code back. Takes precedence +# over MAIL_SERVER, so a machine with a real relay configured can still run the +# tests without mailing anyone. +# MAIL_OUTBOX_DIR=e2e/.mail + +# Email/password auth is on by default when no OAuth provider is configured. # EMAIL_AUTH_ENABLED=false +# Confirmation code lifetime, wrong guesses allowed per code, and per-address +# send limits. +# EMAIL_VERIFICATION_TTL_MINUTES=15 +# EMAIL_VERIFICATION_MAX_ATTEMPTS=5 +# EMAIL_VERIFICATION_RESEND_SECONDS=60 +# EMAIL_VERIFICATION_MAX_SENDS=5 + # ----------------------------- # Frontend / analytics (optional) # ----------------------------- diff --git a/.github/wordlist.txt b/.github/wordlist.txt index 9dce194d..393bfb34 100644 --- a/.github/wordlist.txt +++ b/.github/wordlist.txt @@ -124,3 +124,8 @@ SDK Dependabot PyPI pypi +signup +SMTP +outbox +PendingSignup +unconfigured diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index db1f904b..bca72d3c 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -135,6 +135,7 @@ jobs: FASTAPI_DEBUG=False FALKORDB_URL=redis://localhost:6379 DISABLE_MCP=true + MAIL_OUTBOX_DIR=e2e/.mail EOF # Start the FastAPI application @@ -169,6 +170,9 @@ jobs: FASTAPI_DEBUG: False FALKORDB_URL: redis://localhost:6379 DISABLE_MCP: true + # Signup mails a confirmation code and the account is only created when + # it is typed back, so the suite has to be able to read the message. + MAIL_OUTBOX_DIR: e2e/.mail # Azure OpenAI API keys - required for database schema analysis AZURE_API_KEY: ${{ secrets.AZURE_API_KEY }} AZURE_API_BASE: ${{ secrets.AZURE_API_BASE }} diff --git a/.gitignore b/.gitignore index 2360cdc8..3ef769ff 100644 --- a/.gitignore +++ b/.gitignore @@ -39,6 +39,7 @@ demo_tokens.py /playwright/.cache/ /playwright/.auth/ e2e/.auth/ +e2e/.mail/ # Build artifacts clients/python/queryweaver_client.egg-info/ clients/ts/dist/ diff --git a/AGENTS.md b/AGENTS.md index e99d9abe..a5d7e41a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,6 +132,8 @@ Optional overrides: `COMPLETION_MODEL`, `EMBEDDING_MODEL` (must match provider), Authentication is deliberately split three ways — a signed session cookie for the browser login (no FalkorDB dependency, see `api/auth/browser_session.py`), FalkorDB-backed API tokens for programmatic clients, and per-request data-source credentials. `validate_user` in `api/auth/user_management.py` owns the precedence between them. +Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a six-digit code, and `POST /signup/email/verify` is what creates the `User` and establishes the session. A code rather than a link, so that confirmation has to come back through the session that submitted the form — an emailed link can be opened by a victim who never signed up, which would create an account under a password the sender chose. Holding the code is not on its own enough: every submission also mints a ticket that stays in the submitting browser's session (`remember_signup_ticket` in `api/auth/browser_session.py`) and is matched, hashed, inside the same query that spends the code, so a code read out of another person's inbox cannot be redeemed. Wrong guesses are charged against a per-record attempt budget that destroys the pending signup once it runs out; that budget, not the six digits, is what makes a short code safe. The send budget is likewise per record and resets when the record expires, so exhausting it cannot lock an address out permanently. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Every branch of `POST /signup/email` answers the same 202, so nothing about a pending signup is observable from outside. Mail goes through `api/mail.py`, which picks a transport from the environment: a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the code back), SMTP (`MAIL_SERVER`), or — only when `APP_ENV=development` — the console; an unconfigured process anywhere else fails the send rather than logging the code. + See `.env.example` for the full list. ## CI/CD diff --git a/Makefile b/Makefile index b79bc5ce..0bccaf65 100644 --- a/Makefile +++ b/Makefile @@ -76,6 +76,7 @@ clean: ## Clean up test artifacts rm -rf test-results/ rm -rf playwright-report/ rm -rf e2e/.auth/ + rm -rf e2e/.mail/ rm -rf __pycache__/ rm -rf dist/ rm -rf *.egg-info/ diff --git a/README.md b/README.md index cc407cc0..3e7e7e46 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,46 @@ the browser, so there is none to revoke — tokens are created explicitly from t tokens API and revoked there. (A legacy `api_token` cookie left over from an older release is cleared and revoked on logout too.) +#### Email signup is verified before the account exists + +Signing up with an email address and password does not create an account. The +submitted details are parked, a six-digit confirmation code is mailed to the +address, and the account — and the session — come into being only when that code +is typed back into the signup form. So an address the registrant does not +control never becomes an account at all, and there is no half-real user for the +rest of the system to reason about. + +A code rather than an emailed link, because the code has to come back to the +session that submitted the form. A link can be opened by anyone who receives it: +a stranger could submit your address with a password of their choosing, and your +single click would create an account they knew the password to. Nobody can be +signed up by someone else here, because the person who fills in the form is the +only one who ever holds both halves. + +The code is single-use, expires after 15 minutes and tolerates only a handful of +wrong guesses before the pending signup is discarded — a short code is only safe +while the number of attempts is small. It is also only redeemable in the browser +that submitted the form: each submission mints a ticket that stays in that +browser's session, and a code presented without its ticket is refused. Entering +it signs the browser in directly: the password was chosen minutes earlier, and +asking for it again would prove nothing. A code can be re-sent from the same +screen, subject to a per-address rate limit; the send budget is per pending +signup, so it starts over once the pending signup expires and an address can +always be signed up again later. Typing a code that has expired is not one of +the wrong guesses and does not discard anything — the pending signup is left +where it is so the same screen can send a fresh code. + +In development, a message with no mail server configured is written to the +application log instead of being sent, so the flow can be completed by copying +the code out of the log. This needs `APP_ENV=development` — anywhere else an +unconfigured process refuses the send rather than logging the code and +reporting success. Set `MAIL_SERVER` (plus `MAIL_PORT`, `MAIL_USERNAME`, +`MAIL_PASSWORD`, `MAIL_DEFAULT_SENDER`) to send for real; any provider with an +SMTP endpoint works. `EMAIL_VERIFICATION_TTL_MINUTES`, +`EMAIL_VERIFICATION_MAX_ATTEMPTS`, `EMAIL_VERIFICATION_RESEND_SECONDS` and +`EMAIL_VERIFICATION_MAX_SENDS` tune the lifetime and the limits. See +`.env.example` for the full list. + The trade-off of a signed session cookie is that it cannot be revoked from the server before it expires: the TTL bounds the damage, and rotating `FASTAPI_SECRET_KEY` invalidates every browser login at once. API tokens keep diff --git a/api/auth/browser_session.py b/api/auth/browser_session.py index 7ae8a81b..03a4a1eb 100644 --- a/api/auth/browser_session.py +++ b/api/auth/browser_session.py @@ -30,6 +30,11 @@ # Key under which the login payload lives inside the Starlette session dict. SESSION_KEY = "browser_login" +# Key under which a signup awaiting its mailed code parks its ticket. Separate +# from the login: it is held before any account exists, and clearing one must +# not clear the other. +SIGNUP_TICKET_KEY = "signup_ticket" + # Bumped whenever the payload shape changes, so old cookies are ignored rather # than misread. SESSION_VERSION = 1 @@ -176,3 +181,40 @@ def mark_provisioned(request: Request) -> None: payload["provisioned"] = True # Reassign so Starlette re-serialises the mutated payload. store[SESSION_KEY] = payload + + +def remember_signup_ticket(request: Request, *, email: str, ticket: str) -> None: + """Hold the ticket for a signup this browser just started. + + Not a login -- there is no account yet. It is the browser's half of the + pending signup, and it lives here rather than in the graph because that is + exactly what it has to prove: that the caller redeeming the mailed code is + the same browser that submitted the password being redeemed. One at a time + is enough; a second signup in the same browser replaces the first. + + Storing it in a signed-but-readable cookie is fine. It is a capability of + the browser it was handed to, so its owner reading it learns nothing they + did not already have, and it is worthless without the code that was mailed. + """ + store = _session_store(request) + if store is None: + logging.error("Cannot hold a signup ticket: SessionMiddleware is not installed") + return + store[SIGNUP_TICKET_KEY] = {"email": email, "ticket": ticket} + + +def read_signup_ticket(request: Request, *, email: str) -> Optional[str]: + """Return this browser's ticket for ``email``, or ``None``.""" + store = _session_store(request) + payload = store.get(SIGNUP_TICKET_KEY) if store else None + if not isinstance(payload, dict) or payload.get("email") != email: + return None + ticket = payload.get("ticket") + return ticket if isinstance(ticket, str) and ticket else None + + +def forget_signup_ticket(request: Request) -> None: + """Drop any held signup ticket. The code it belonged to is spent.""" + store = _session_store(request) + if store is not None: + store.pop(SIGNUP_TICKET_KEY, None) diff --git a/api/auth/email_verification.py b/api/auth/email_verification.py new file mode 100644 index 00000000..c0de85c0 --- /dev/null +++ b/api/auth/email_verification.py @@ -0,0 +1,558 @@ +"""Signup email verification -- the holding pen for accounts that do not exist yet. + +Signing up does not create a user. It records the submitted details on a +``PendingSignup`` node and mails a six-digit code; typing that code back into +the browser that signed up is what creates the ``User`` and ``Identity`` and +logs it in. An address that is never confirmed therefore never becomes an +account at all -- nothing to count, nothing to log in as, nothing to clean up +beyond an expiring node. + +That ordering is what makes the guarantee cheap. There is no ``email_verified`` +flag to check at every call site and no half-real user for the org graph, +analytics or quota logic to trip over, because an unverified address is simply +absent from the account graph. + +A code rather than a link, deliberately. A link can be opened by whoever +receives it, which is the wrong person in the case that matters: submit someone +else's address with a password of your choosing, and their click would create an +account they do not control the password to. A code has to be carried back to +the session that submitted the form, and the person who submitted it never sees +the mail. It also means no URL for a mail scanner to fetch and silently burn. + +The code alone is not quite enough, because two people can submit the same +address. Whoever writes last owns the stored password, and the code that goes +out is mailed to the address, not to the submitter -- so a stranger could +re-submit someone else's pending signup with a password of their own and let the +owner redeem it for them. Each submission therefore also mints a ticket that +stays in the submitting browser's session, and a code is only redeemable by the +browser holding the ticket that was issued with it. Re-submission then costs the +first submitter their pending signup, which is a nuisance they can undo by +signing up again, rather than an account under someone else's password. + +The code is short, so its secrecy cannot rest on entropy -- a million +possibilities is nothing to a script. What bounds it is the attempt limit: a +handful of wrong guesses destroys the pending signup outright, and the code +expires in minutes. Only the SHA-256 is stored, which keeps a casual reader of +the graph from lifting a live code, but the attempt limit is the actual defence. + +Codes are single-use (consuming one deletes the node) and expiring, and sends +are rate-limited per address so the endpoint cannot be used to mail-bomb a third +party. +""" + +import hashlib +import html +import logging +import os +import secrets +import time +from dataclasses import dataclass +from typing import Optional, Tuple + +from api.config import ORGANIZATIONS_GRAPH +from api.extensions import db +from api.mail import send_mail + +# Digits in a verification code. Six is what people expect to retype; the +# attempt limit below is what makes it safe, not the length. +CODE_DIGITS = 6 + +# Long enough to walk to the other device and back, short enough that a code +# left in an inbox is not still live tomorrow. +DEFAULT_TTL_MINUTES = 15 + +# Wrong guesses allowed before the pending signup is destroyed. With six digits +# this is the whole defence against grinding the code, so it is deliberately +# small: five wrong guesses out of a million is not a meaningful head start. +DEFAULT_MAX_ATTEMPTS = 5 + +# Minimum gap between two sends to one address. +DEFAULT_RESEND_INTERVAL_SECONDS = 60 + +# Total sends allowed for one pending signup, resend included. Bounds how much +# mail a single submitted address can generate. +DEFAULT_MAX_SENDS = 5 + +# Outcomes of redeeming a code, kept as constants so routes and tests agree on +# the spelling. +RESULT_OK = "ok" +RESULT_INVALID = "invalid" +RESULT_EXPIRED = "expired" + + +@dataclass(frozen=True) +class PendingSignup: + """The details captured at signup, replayed when the code is redeemed.""" + + email: str + first_name: str + last_name: str + password_hash: str + + @property + def full_name(self) -> str: + """Display name, matching the format the signup route stored.""" + return f"{self.first_name} {self.last_name}".strip() + + +@dataclass(frozen=True) +class CodeIssue: + """The result of asking for a verification code. + + ``code`` is the only time the raw value exists outside the mail, and + ``ticket`` the only time that value exists outside the caller's session; the + graph keeps just their hashes. Nothing here says *why* a code was refused: + the routes answer refusals and successes identically, so the reason would + only be a way to ask the graph questions about other people's addresses. + + ``previous`` is the record exactly as it stood before this send, so + ``revert_verification_send`` can put it back if the mail never goes out. It + is the whole property map rather than the handful of fields that seemed + interesting: issuing a code rewrites the name, the password hash and the + ticket as well, and an undo that restores only some of those leaves a + record nobody submitted -- someone else's code against this caller's + password. An empty map means the record did not exist until this call. + """ + + code: Optional[str] = None + first_name: Optional[str] = None + ticket: Optional[str] = None + previous: Optional[dict] = None + + @property + def issued(self) -> bool: + """Whether a code was actually produced.""" + return self.code is not None + + @property + def displaced(self) -> bool: + """Whether a live code was overwritten, and so is there to put back.""" + return bool(self.previous and self.previous.get("code_hash")) + + +def _positive_int_env(name: str, default: int) -> int: + """Read a positive integer setting, falling back on anything unusable.""" + raw = os.getenv(name) + if not raw: + return default + try: + value = int(raw) + except ValueError: + value = 0 + if value > 0: + return value + logging.warning("Invalid %s value %r, using %s", name, raw, default) + return default + + +def code_ttl_seconds() -> int: + """How long a verification code stays valid.""" + return _positive_int_env("EMAIL_VERIFICATION_TTL_MINUTES", DEFAULT_TTL_MINUTES) * 60 + + +def max_attempts() -> int: + """Wrong guesses a pending signup survives.""" + return _positive_int_env("EMAIL_VERIFICATION_MAX_ATTEMPTS", DEFAULT_MAX_ATTEMPTS) + + +def resend_interval_seconds() -> int: + """Minimum gap between two sends to the same address.""" + return _positive_int_env( + "EMAIL_VERIFICATION_RESEND_SECONDS", DEFAULT_RESEND_INTERVAL_SECONDS + ) + + +def max_sends() -> int: + """Total sends allowed for a single pending signup.""" + return _positive_int_env("EMAIL_VERIFICATION_MAX_SENDS", DEFAULT_MAX_SENDS) + + +def _now_ms() -> int: + """Wall-clock milliseconds, matching the units Cypher's ``timestamp()`` uses.""" + return int(time.time() * 1000) + + +def generate_code() -> str: + """A fresh zero-padded verification code.""" + return f"{secrets.randbelow(10 ** CODE_DIGITS):0{CODE_DIGITS}d}" + + +def generate_ticket() -> str: + """A fresh signup ticket. + + Unlike the code this one is never typed by anybody, so it is as long as it + wants to be -- and it has to be, because it is the half of the pair that + never leaves the browser and therefore has no attempt limit behind it. + """ + return secrets.token_urlsafe(32) + + +def hash_code(code: str) -> str: + """Hash a raw code for storage and lookup.""" + return hashlib.sha256(code.encode("utf-8")).hexdigest() + + +def _graph(): + """The Organizations graph, where identity records live.""" + return db.select_graph(ORGANIZATIONS_GRAPH) + + +# The guard the two issuing queries share. It rides along in the write itself +# rather than being checked first: a single Cypher query is atomic and writes to +# one graph are serialised, so checking separately would let two concurrent +# requests both pass the check and both send while the counter advanced once. +# +# ``stale`` is what keeps the send budget from turning into a permanent lock on +# an address. Nothing deletes a pending signup that is simply never confirmed, +# so without it a stranger could spend an address's five sends, leave the +# counter pinned at its limit for good, and the real owner would never be able +# to sign up. Once the code the counter was rationing has expired the record is +# spent anyway, and the next submission starts a fresh budget on it. +_SEND_GUARD = """ + WITH p, (p.expires_at IS NULL OR p.expires_at < $now) AS stale + WHERE (stale OR p.send_count < $max_sends) + AND (p.last_sent_at IS NULL OR $now - p.last_sent_at >= $interval_ms) +""" + +# Counts this send, starting over when the record it lands on had expired. The +# interval still applies on that path, so an expired record is not a way to send +# faster -- only a way to keep sending at all. +_COUNT_SEND = "p.send_count = CASE WHEN stale THEN 1 ELSE p.send_count + 1 END" + +# Creates the pending signup if it is new, then replaces its details and issues +# a code -- but only for a node the guard lets through. A node created by this +# very query has no code and no expiry, so it counts as stale and always passes. +_START_SIGNUP = ( + """ + MERGE (p:PendingSignup {email: $email}) + ON CREATE SET p.created_at = $now, p.send_count = 0 +""" + + _SEND_GUARD + + """ + WITH p, stale, properties(p) AS previous + SET p.code_hash = $code_hash, + p.ticket_hash = $ticket_hash, + p.first_name = $first_name, + p.last_name = $last_name, + p.password_hash = $password_hash, + p.expires_at = $expires_at, + p.attempts = 0, + p.last_sent_at = $now, + """ + + _COUNT_SEND + + """ + RETURN previous +""" +) + +# Refresh only: never MERGE, so the resend endpoint cannot conjure a pending +# signup for an address nobody submitted. The ticket is deliberately left alone +# -- a resend is another copy of the same signup, not a new one, and rotating it +# would lock out the browser that is waiting for the code. +_REFRESH_SIGNUP = ( + """ + MATCH (p:PendingSignup {email: $email}) +""" + + _SEND_GUARD + + """ + WITH p, stale, properties(p) AS previous + SET p.code_hash = $code_hash, + p.expires_at = $expires_at, + p.attempts = 0, + p.last_sent_at = $now, + """ + + _COUNT_SEND + + """ + RETURN p.first_name AS first_name, previous +""" +) + +# Undoes one send, by putting the record back exactly as it was rather than +# by naming the fields to restore -- ``SET p = $previous`` also drops properties +# the send added, and cannot fall behind a change to what issuing a code writes. +# ``last_sent_at`` is then pushed back to now: the send budget is refunded with +# the rest of the snapshot, but retries still stay one per interval, which is +# what stops a broken transport from being hammered. Matching on the hash this +# send wrote makes the whole thing a no-op if another request has since issued a +# code of its own. +_REVERT_SEND = """ + MATCH (p:PendingSignup {email: $email}) + WHERE p.code_hash = $code_hash + SET p = $previous + SET p.last_sent_at = $now +""" + + +def _throttle_params(now: int) -> dict: + """The parameters ``_SEND_GUARD`` reads.""" + return { + "now": now, + "max_sends": max_sends(), + "interval_ms": resend_interval_seconds() * 1000, + } + + +async def start_pending_signup( + email: str, first_name: str, last_name: str, password_hash: str +) -> CodeIssue: + """Record a signup awaiting verification and return its code and ticket. + + Re-submitting the form for an address that is already pending replaces the + stored details and invalidates the previous code *and* ticket, so only the + browser behind the most recent attempt can complete it. The send counter + deliberately survives that replace: otherwise resubmitting would reset the + rate limit and defeat it. + """ + now = _now_ms() + code = generate_code() + ticket = generate_ticket() + result = await _graph().query( + _START_SIGNUP, + { + "email": email, + "code_hash": hash_code(code), + "ticket_hash": hash_code(ticket), + "first_name": first_name, + "last_name": last_name, + "password_hash": password_hash, + "expires_at": now + code_ttl_seconds() * 1000, + **_throttle_params(now), + }, + ) + if not result.result_set: + # Sent to a moment ago, or out of sends while the current code is still + # live. Which one is not reported: the route answers a refusal exactly + # like a success, so telling them apart here would only be a way to ask + # whether a stranger's address has a signup in flight. + return CodeIssue() + + return CodeIssue( + code=code, + first_name=first_name, + ticket=ticket, + previous=result.result_set[0][0], + ) + + +async def refresh_pending_signup(email: str) -> CodeIssue: + """Issue a fresh code for an existing pending signup. + + Only ever refreshes; it will not create a pending signup, so the resend + endpoint cannot be used to send mail to an address nobody submitted. A new + code comes with a fresh attempt budget -- the old one belonged to a code + that no longer works. + """ + now = _now_ms() + code = generate_code() + result = await _graph().query( + _REFRESH_SIGNUP, + { + "email": email, + "code_hash": hash_code(code), + "expires_at": now + code_ttl_seconds() * 1000, + **_throttle_params(now), + }, + ) + if not result.result_set: + # No pending signup, one that has used up its sends, or one that was + # sent to a moment ago. Also covers losing a race with a verification + # that just consumed the record. Indistinguishable on purpose. + return CodeIssue() + + first_name, previous = result.result_set[0] + return CodeIssue(code=code, first_name=first_name, previous=previous) + + +async def revert_verification_send(email: str, issue: CodeIssue) -> None: + """Give back a send whose mail never left. Best-effort; never fatal. + + Puts the record back exactly as it stood, so a transport failure costs the + user neither their send budget nor the code they may already be holding. + Only for a send that displaced a live code -- there is nothing to restore + a record to if this call is what created it, and ``discard_pending_signup`` + is the right way to undo that. + """ + if not issue.issued or not issue.displaced: + return + try: + await _graph().query( + _REVERT_SEND, + { + "email": email, + "code_hash": hash_code(issue.code), + "previous": issue.previous, + "now": _now_ms(), + }, + ) + except Exception as e: # pylint: disable=broad-exception-caught + logging.warning("Could not revert a failed verification send: %s", e) + + +# Redeems a code. The attempt guard rides inside the write for the same reason +# the send guard does: checking first would let concurrent guesses all pass a +# check that only one increment ever answered for. +# +# The delete is conditional because an expired record is still worth something: +# the send guard lets a resend revive it, and deleting it here would strand a +# user who typed the right code a minute late -- their resend would quietly do +# nothing, because refreshing only ever MATCHes. A NULL ``expires_at`` fails the +# comparison and so is treated as not live, which is the safe way round. +_CONSUME_CODE = """ + MATCH (p:PendingSignup {email: $email}) + WHERE p.code_hash = $code_hash + AND p.ticket_hash = $ticket_hash + AND p.attempts < $max_attempts + WITH p, + p.expires_at >= $now AS live, + p.first_name AS first_name, + p.last_name AS last_name, + p.password_hash AS password_hash + FOREACH (_ IN CASE WHEN live THEN [1] ELSE [] END | DELETE p) + RETURN live, first_name, last_name, password_hash +""" + +# Charges a wrong guess, and destroys the signup once the budget is gone. A +# six-digit code only stays secret while the number of guesses stays small. +_CHARGE_ATTEMPT = """ + MATCH (p:PendingSignup {email: $email}) + SET p.attempts = p.attempts + 1 + WITH p, p.attempts AS attempts + WHERE attempts >= $max_attempts + DELETE p + RETURN attempts +""" + + +async def consume_pending_signup( + email: str, code: str, ticket: str +) -> Tuple[Optional[PendingSignup], str]: + """Redeem a verification code exactly once. + + Returns ``(pending, RESULT_OK)`` when the code was live. The node is deleted + in the same query that reads it, so a replayed code finds nothing -- that, + not a flag, is what makes it single-use. + + Both halves are required. The code proves the caller reads the address's + mail; the ticket proves they are the browser that submitted the password + being redeemed. Without the ticket, anyone could re-submit a stranger's + pending signup under a password of their own and let the address's owner + confirm it into an account the submitter can log into. + + A wrong guess is charged against the record's attempt budget, and running + that budget out deletes the pending signup. That, rather than the length of + the code, is what makes six digits enough. + + Lookup is by code *hash*, an exact match on a stored value, so there is no + secret-dependent comparison here for timing to leak. + """ + if not email or not code or not ticket: + return None, RESULT_INVALID + + result = await _graph().query( + _CONSUME_CODE, + { + "email": email, + "code_hash": hash_code(code), + "ticket_hash": hash_code(ticket), + "max_attempts": max_attempts(), + "now": _now_ms(), + }, + ) + if not result.result_set: + await _charge_failed_attempt(email) + return None, RESULT_INVALID + + live, first_name, last_name, password_hash = result.result_set[0] + + # The record survives an expired code, so the user can ask for a fresh one + # from the screen they are already on. No attempt is charged either: the + # code was right, and running the budget out here would delete the very + # record the resend needs. + if not live: + return None, RESULT_EXPIRED + + if not password_hash: + logging.error("Discarding a malformed pending signup record") + return None, RESULT_INVALID + + return ( + PendingSignup( + email=email, + first_name=first_name or "", + last_name=last_name or "", + password_hash=password_hash, + ), + RESULT_OK, + ) + + +async def _charge_failed_attempt(email: str) -> None: + """Bill a wrong guess. Best-effort; a failure must not become a free retry.""" + try: + result = await _graph().query( + _CHARGE_ATTEMPT, {"email": email, "max_attempts": max_attempts()} + ) + if result.result_set: + logging.warning("Pending signup discarded after too many wrong codes") + except Exception as e: # pylint: disable=broad-exception-caught + logging.warning("Could not record a failed verification attempt: %s", e) + + +async def discard_pending_signup(email: str) -> None: + """Drop any pending signup for an address. Best-effort; never fatal. + + Called once an account exists for the address by some other route, so a + stale code cannot later be redeemed against it. + """ + try: + await _graph().query( + "MATCH (p:PendingSignup {email: $email}) DELETE p", {"email": email} + ) + except Exception as e: # pylint: disable=broad-exception-caught + logging.warning("Could not discard pending signup: %s", e) + + +def _greeting(first_name: Optional[str]) -> str: + """``Hi Ada`` when a name is known, plain ``Hi`` when it is not.""" + name = (first_name or "").strip() + return f"Hi {name}," if name else "Hi," + + +async def send_verification_code( + email: str, first_name: Optional[str], code: str +) -> bool: + """Mail the verification code. Returns whether it was handed to a transport.""" + minutes = code_ttl_seconds() // 60 + greeting = _greeting(first_name) + + text_body = ( + f"{greeting}\n\n" + "Your QueryWeaver confirmation code is:\n\n" + f" {code}\n\n" + f"Type it into the tab where you signed up. It works once and expires in " + f"{minutes} minutes. Your account is not created until you enter it.\n\n" + "If you did not sign up for QueryWeaver, ignore this message -- no " + "account exists for this address and none will be created. Never share " + "this code with anyone.\n" + ) + + html_body = ( + "" + f"

{html.escape(greeting)}

" + "

Your QueryWeaver confirmation code is:

" + f'

' + f"{html.escape(code)}

" + f"

Type it into the tab where you signed up. It works once and expires " + f"in {minutes} minutes. Your account is not created until you enter it.

" + "

If you did not sign up for QueryWeaver, ignore this message — " + "no account exists for this address and none will be created. Never " + "share this code with anyone.

" + "" + ) + + return await send_mail( + to=email, + subject="Your QueryWeaver confirmation code", + text_body=text_body, + html_body=html_body, + ) diff --git a/api/auth/user_management.py b/api/auth/user_management.py index c63763e9..97d3633d 100644 --- a/api/auth/user_management.py +++ b/api/auth/user_management.py @@ -132,6 +132,8 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d provider: str, api_token: Optional[str], picture: str | None = None, + *, + password_hash: Optional[str] = None, ) -> tuple[bool, Optional[IdentityInfo]]: """ Check if identity exists in Organizations graph, create if not. @@ -142,6 +144,10 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d minting a programmatic token — used when repairing an existing browser login that was established while the graph was unreachable. + ``password_hash`` is stored on the identity as it is created, in the same + write. A second write would be a window in which the account exists but + cannot be logged into, and the address is already taken for signup. + Returns (is_new_identity, user_info). ``user_info`` is ``None`` when the records could not be persisted, so callers should test that, not the flag. Raises :class:`AuthBackendUnavailableError` when the graph is unreachable, @@ -162,7 +168,10 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d organizations_graph = db.select_graph(ORGANIZATIONS_GRAPH) first_name, last_name = _extract_name_parts(name) - merge_query = _build_user_merge_query(include_token=api_token is not None) + merge_query = _build_user_merge_query( + include_token=api_token is not None, + include_password=password_hash is not None, + ) query_params = _build_query_params( provider, provider_user_id, @@ -172,6 +181,7 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d first_name=first_name, last_name=last_name, api_token=api_token, + password_hash=password_hash, ) result = await organizations_graph.query(merge_query, query_params) @@ -458,11 +468,14 @@ def _extract_name_parts(name: str) -> tuple: return first_name, last_name -def _build_user_merge_query(include_token: bool = True) -> str: +def _build_user_merge_query(include_token: bool = True, include_password: bool = False) -> str: """Build the Cypher query for user/identity merge operations. ``include_token`` drops the Token MERGE so an identity can be persisted - without issuing a programmatic credential. + without issuing a programmatic credential. ``include_password`` stores a + password on the identity as it is created; it is deliberately absent from + the ON MATCH branch, so this can never overwrite the password of an + identity that already exists. """ token_clause = ( """ @@ -476,6 +489,7 @@ def _build_user_merge_query(include_token: bool = True) -> str: if include_token else "" ) + password_clause = ",\n identity.password_hash = $password_hash" if include_password else "" return """ // First, ensure user exists (merge by email) MERGE (user:User {email: $email}) @@ -491,7 +505,7 @@ def _build_user_merge_query(include_token: bool = True) -> str: identity.name = $name, identity.picture = $picture, identity.created_at = timestamp(), - identity.last_login = timestamp() + identity.last_login = timestamp()""" + password_clause + """ ON MATCH SET identity.email = $email, identity.name = $name, @@ -518,7 +532,8 @@ def _build_query_params( # pylint: disable=too-many-arguments picture: str | None = None, first_name: str, last_name: str, - api_token: Optional[str] + api_token: Optional[str], + password_hash: Optional[str] = None, ) -> dict: """Build query parameters for the database operation.""" return { @@ -530,6 +545,7 @@ def _build_query_params( # pylint: disable=too-many-arguments "first_name": first_name, "last_name": last_name, "api_token": api_token, + "password_hash": password_hash, } diff --git a/api/mail.py b/api/mail.py new file mode 100644 index 00000000..80eca43d --- /dev/null +++ b/api/mail.py @@ -0,0 +1,261 @@ +"""Outbound mail. + +QueryWeaver sends one kind of message today -- the signup confirmation code -- +so this module stays small: pick a transport from the environment, hand it a +built message, and report whether it left the process. + +Three transports: + +* ``console`` writes the message to the log instead of sending it, so local + development completes the signup flow without a mail server. It is what an + unconfigured process falls back to, which is why it is confined to + ``APP_ENV=development``: reached by omission anywhere else, it would turn a + deployment that forgot ``MAIL_SERVER`` into one that logs every confirmation + code and tells the user it sent them. Outside development the send simply + fails, which the signup routes surface and roll back. +* ``file`` writes each message to ``MAIL_OUTBOX_DIR`` as an ``.eml``. The + Playwright suite reads the confirmation code back out of it, which is what + keeps the end-to-end signup test exercising the real flow instead of a + test-only shortcut through the backend. It takes precedence over a configured + relay: nobody sets this variable by accident, and a test run that quietly + mails real addresses is a worse failure than one that quietly does not. +* ``smtp`` talks to any relay. Every hosted provider -- Mailgun, SendGrid, + Resend, SES, Postmark -- exposes an SMTP endpoint, so this covers them all + without binding the project to a vendor SDK. + +The relay is selected by whether ``MAIL_SERVER`` is set rather than by a +separate switch, so a half-configured relay cannot be selected by accident. + +``smtplib`` is synchronous, so sends run on a worker thread: a relay that takes +its time must not stall the event loop for every other request. +""" + +import asyncio +import logging +import os +import secrets +import smtplib +import ssl +import time +from email.message import EmailMessage +from pathlib import Path +from typing import Optional + +# Implicit-TLS port. Everything else is assumed to be plain SMTP that may be +# upgraded with STARTTLS. +SMTPS_PORT = 465 + +DEFAULT_SMTP_PORT = 587 +DEFAULT_TIMEOUT_SECONDS = 10.0 +FALLBACK_SENDER = "no-reply@queryweaver.local" + + +def _env_flag(name: str, default: bool) -> bool: + """Read a boolean environment variable, falling back on anything unrecognised.""" + raw = os.getenv(name) + if raw is None: + return default + value = raw.strip().lower() + if value in ("true", "1", "yes", "on"): + return True + if value in ("false", "0", "no", "off"): + return False + logging.warning("Invalid %s value %r, using %s", name, raw, default) + return default + + +def _smtp_timeout() -> float: + """Socket timeout for the relay, from ``MAIL_TIMEOUT_SECONDS``.""" + raw = os.getenv("MAIL_TIMEOUT_SECONDS") + if not raw: + return DEFAULT_TIMEOUT_SECONDS + try: + timeout = float(raw) + except ValueError: + timeout = 0.0 + if timeout > 0: + return timeout + logging.warning("Invalid MAIL_TIMEOUT_SECONDS value %r, ignoring", raw) + return DEFAULT_TIMEOUT_SECONDS + + +def _smtp_port() -> int: + """Relay port, from ``MAIL_PORT``.""" + raw = os.getenv("MAIL_PORT") + if raw: + try: + return int(raw) + except ValueError: + logging.warning("Invalid MAIL_PORT value %r, ignoring", raw) + return DEFAULT_SMTP_PORT + + +def is_smtp_configured() -> bool: + """Whether a relay is configured, from ``MAIL_SERVER``.""" + return bool(os.getenv("MAIL_SERVER", "").strip()) + + +def outbox_dir() -> str: + """Directory for the file transport, from ``MAIL_OUTBOX_DIR``. Empty when unset.""" + return os.getenv("MAIL_OUTBOX_DIR", "").strip() + + +def console_transport_allowed() -> bool: + """Whether mail may be logged instead of sent. + + Fail secure, the same way the session cookie's ``Secure`` flag does: the + console transport is what a laptop with no relay falls back to, and it + writes the confirmation code -- a credential -- into the log. Since it is + reached by *omitting* configuration rather than choosing it, a deployment + that forgets ``MAIL_SERVER`` would otherwise swallow every code silently and + tell the user their mail was on its way. Only an explicit + ``APP_ENV=development`` opts in. + """ + app_env = os.getenv("APP_ENV") + return app_env is not None and app_env.strip().lower() == "development" + + +def transport_name() -> str: + """Name of the active transport, for logs and diagnostics.""" + if outbox_dir(): + return "file" + if is_smtp_configured(): + return "smtp" + return "console" if console_transport_allowed() else "none" + + +def default_sender() -> str: + """The ``From`` address.""" + return ( + os.getenv("MAIL_DEFAULT_SENDER", "").strip() + or os.getenv("MAIL_USERNAME", "").strip() + or FALLBACK_SENDER + ) + + +def _build_message(*, to: str, subject: str, text_body: str, html_body: Optional[str]) -> EmailMessage: + """Assemble the message. Raises ``ValueError`` for an unusable recipient.""" + # Header injection guard. Callers validate the address first, but a header + # split turns one verification mail into mail to anybody, so it is checked + # again at the point the header is actually written. + if not to or any(char in to for char in "\r\n"): + raise ValueError("Recipient address contains a line break") + + message = EmailMessage() + message["From"] = default_sender() + message["To"] = to + message["Subject"] = subject + message.set_content(text_body) + if html_body: + message.add_alternative(html_body, subtype="html") + return message + + +def _send_via_smtp(message: EmailMessage) -> None: + """Hand the message to the relay. Blocking; call it on a worker thread.""" + host = os.getenv("MAIL_SERVER", "").strip() + port = _smtp_port() + username = os.getenv("MAIL_USERNAME", "").strip() + password = os.getenv("MAIL_PASSWORD", "") + timeout = _smtp_timeout() + + if port == SMTPS_PORT: + context = ssl.create_default_context() + with smtplib.SMTP_SSL(host, port, timeout=timeout, context=context) as client: + if username: + client.login(username, password) + client.send_message(message) + return + + with smtplib.SMTP(host, port, timeout=timeout) as client: + if _env_flag("MAIL_USE_TLS", True): + client.starttls(context=ssl.create_default_context()) + # The pre-STARTTLS greeting is unauthenticated, so the capability + # list has to be re-read over the encrypted channel. + client.ehlo() + if username: + client.login(username, password) + client.send_message(message) + + +def _sanitize_for_log(value: str) -> str: + """Flatten a value so it cannot forge log entries of its own.""" + # ``.replace('\n', ...)`` must be the outermost call for CodeQL's + # log-injection sanitizer to recognise it. The body is deliberately + # rendered on one line rather than dropped: the console transport exists so + # a developer can copy the confirmation code out of the log. + return str(value).replace("\r", " ").replace("\n", " ") + + +def _log_to_console(message: EmailMessage, text_body: str) -> None: + """Write the message to the log in place of sending it.""" + logging.info( + "[mail:console] No MAIL_SERVER configured, so this message was not sent. " + "To: %s | Subject: %s | %s", + _sanitize_for_log(message["To"]), + _sanitize_for_log(message["Subject"]), + _sanitize_for_log(text_body), + ) + + +def _write_to_outbox(message: EmailMessage, directory: str) -> bool: + """Drop the message into the outbox directory as an ``.eml`` file.""" + try: + path = Path(directory) + path.mkdir(parents=True, exist_ok=True) + # Timestamp first so readers can pick the newest by name; the random + # suffix keeps two sends in the same millisecond from colliding. + name = f"{int(time.time() * 1000)}-{secrets.token_hex(4)}.eml" + (path / name).write_bytes(message.as_bytes()) + return True + except OSError as e: + logging.error("Could not write mail to the outbox: %s", e) + return False + + +async def send_mail( + *, + to: str, + subject: str, + text_body: str, + html_body: Optional[str] = None, +) -> bool: + """Send one message. Returns ``False`` instead of raising when it could not go. + + Callers are signup paths, where a failed send must not lose the work already + done or leak relay detail to the client, so every failure is logged here and + reported as a plain boolean. + """ + try: + message = _build_message( + to=to, subject=subject, text_body=text_body, html_body=html_body + ) + except ValueError as e: + logging.error("Refusing to send mail: %s", e) + return False + + directory = outbox_dir() + if directory: + return _write_to_outbox(message, directory) + + if not is_smtp_configured(): + if not console_transport_allowed(): + # No relay, and not a development run. Refusing is the honest + # answer: the caller rolls the signup back and tells the user to + # retry, instead of the code being logged and never delivered. + logging.error( + "No MAIL_SERVER is configured, so this message cannot be sent. " + "Set MAIL_SERVER, or APP_ENV=development to log mail instead." + ) + return False + _log_to_console(message, text_body) + return True + + try: + await asyncio.to_thread(_send_via_smtp, message) + return True + except (smtplib.SMTPException, OSError, ssl.SSLError) as e: + # Deliberately does not log the message body: it carries the + # confirmation code, which is a credential. + logging.error("Could not send mail via SMTP: %s", e) + return False diff --git a/api/routes/auth.py b/api/routes/auth.py index e665fcf1..6c6f0d0c 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -23,9 +23,23 @@ from api.auth.browser_session import ( clear_browser_session, establish_browser_session, + forget_signup_ticket, is_provisioned, mark_provisioned, read_browser_session, + read_signup_ticket, + remember_signup_ticket, +) +from api.auth.email_verification import ( + RESULT_OK, + code_ttl_seconds, + consume_pending_signup, + discard_pending_signup, + refresh_pending_signup, + resend_interval_seconds, + revert_verification_send, + send_verification_code, + start_pending_signup, ) from api.auth.user_management import delete_user_token, ensure_user_in_organizations, validate_user from api.config import ORGANIZATIONS_GRAPH @@ -106,6 +120,15 @@ class EmailSignupRequest(BaseModel): email: str password: str +class EmailResendRequest(BaseModel): + """Request to re-send a signup verification code.""" + email: str + +class EmailVerifyRequest(BaseModel): + """The confirmation code, typed back in by the browser that signed up.""" + email: str + code: str + # ---- Password utilities ---- def _hash_password(password: str) -> str: """Hash a password using PBKDF2 with a random salt.""" @@ -143,47 +166,12 @@ def _sanitize_for_log(value: str) -> str: def _validate_email(email: str) -> bool: """Basic email validation.""" - pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$' + # ``\Z`` rather than ``$``: ``$`` also matches immediately before a trailing + # newline, which would let "victim@example.com\nBcc: ..." through and split + # the headers of the verification mail this address is about to receive. + pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}\Z' return re.match(pattern, email) is not None -async def _set_mail_hash(email: str, password_hash: str) -> bool: - """Set email hash for the user in the database.""" - # Sanitized up front so the error path below can log it too. - safe_email = _sanitize_for_log(email) - try: - organizations_graph = db.select_graph(ORGANIZATIONS_GRAPH) - - # Create new email identity and user - create_query = """ - MERGE (i:Identity { - provider_user_id: $email, - email: $email - }) - SET i.password_hash = $password_hash - RETURN i - """ - - result = await organizations_graph.query(create_query, { - "email": email, - "password_hash": password_hash, - }) - - if result.result_set: - return True - else: - logging.error("Failed to set email hash for user: %s", safe_email) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error" - ) - - except Exception as e: - logging.error("Error setting email hash for user %s: %s", safe_email, e) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error" - ) - async def _email_account_exists(email: str) -> bool: """Return True if an account already exists for the given email (any provider). @@ -302,6 +290,27 @@ async def _complete_login(request: Request, provider: str, user_data: dict) -> N ) +def _signup_accepted(email: str) -> JSONResponse: + """The one answer ``/signup/email`` gives once the form itself is valid. + + 202, not 201: the account does not exist yet, and will not until the code + comes back. It reads the same whether a code went out or the address was + refused one for being asked too often -- a 429 here would say that a signup + for this address is already in flight, which is the same question + ``/signup/email/resend`` deliberately refuses to answer. + """ + return JSONResponse({ + "success": True, + "pending": True, + "email": email, + "message": "Check your inbox for the confirmation code.", + "codeTtlSeconds": code_ttl_seconds(), + # So the resend button comes back exactly when another request + # would be honoured, whatever the deployment configured. + "retryAfterSeconds": resend_interval_seconds(), + }, status_code=status.HTTP_202_ACCEPTED) + + # ---- Email Authentication Routes ---- @auth_router.post("/signup/email") async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSONResponse: @@ -347,74 +356,265 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO # (CVE-2026-10130, authentication bypass via signup token issuance). if await _email_account_exists(email): logging.info("Signup attempt for existing account: %s", _sanitize_for_log(email)) + # An account exists, so any code still outstanding for this address + # must not stay redeemable against it. + await discard_pending_signup(email) return JSONResponse( {"success": False, "error": "An account with this email already exists"}, status_code=status.HTTP_409_CONFLICT ) - # ``api_token=None``: signup logs the browser in with the session cookie - # and never returns a token, so minting one here would only leave an - # orphan Token node behind. - is_new_identity, user_info = await ensure_user_in_organizations(email, email, - f"{first_name} {last_name}", "email", None) - - if not (is_new_identity and user_info and user_info.get("new_identity")): - # Creation failed (e.g. DB error) or raced with a concurrent signup. - logging.error("Failed to create new user during signup: %s", + # Nothing is created yet. The details are parked on a PendingSignup node + # and only become a User when the mailed code is typed back in, so an + # address the registrant does not control never turns into an account. + password_hash = _hash_password(password) + issue = await start_pending_signup(email, first_name, last_name, password_hash) + + if not issue.issued: + # Refused for asking too often. Answered exactly like a success so + # the state of a stranger's signup stays private; the caller can + # still use the code they were sent a moment ago. + logging.info("Verification code not issued for %s", _sanitize_for_log(email)) + return _signup_accepted(email) + + if not await send_verification_code(email, first_name, issue.code): + if issue.displaced: + # This address already had a live code before the failed send. + # Put it back rather than deleting the record: the user may be + # holding that code, and it is the only one that ever arrived. + await revert_verification_send(email, issue) + else: + # Nothing was displaced, so the record is dead weight that would + # only burn the rate limit on the retry. + await discard_pending_signup(email) + logging.error("Could not send the verification email for %s", _sanitize_for_log(email)) return JSONResponse( - {"success": False, "error": "Registration failed"}, - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR + {"success": False, + "error": "Could not send the verification email - please retry"}, + status_code=status.HTTP_503_SERVICE_UNAVAILABLE ) - logging.info("New user created: %s", _sanitize_for_log(email)) + # The other half of the code. It stays in this browser's session, so the + # code is only redeemable here -- someone else re-submitting this + # address cannot have their password confirmed by its owner. + remember_signup_ticket(request, email=email, ticket=issue.ticket) - # Hash password - password_hash = _hash_password(password) + logging.info("Verification code sent for pending signup: %s", + _sanitize_for_log(email)) - # Set email hash - await _set_mail_hash(email, password_hash) + return _signup_accepted(email) - logging.info("User registration successful: %s", _sanitize_for_log(email)) + except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: + # Same reasoning as /login/email: an unreachable store is not a rejected + # registration, and answering 500 tells the caller to give up on + # something a retry would fix. + logging.error("Auth store unreachable during signup: %s", e) + return JSONResponse( + {"success": False, + "error": "Authentication service temporarily unavailable - please retry"}, + status_code=status.HTTP_503_SERVICE_UNAVAILABLE + ) + except Exception as e: + logging.error("Signup error: %s", e) + return JSONResponse( + {"success": False, "error": "Registration failed"}, + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR + ) + +@auth_router.post("/signup/email/verify") +async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSONResponse: + """Redeem a signup confirmation code. + + This is where the account is actually created. Producing the code is the + proof that the registrant controls the address, so creating the ``User`` + here -- rather than at signup and flagging it afterwards -- is what keeps an + unconfirmed address from existing as an account at all. + + A code rather than a link because the code has to come back to the session + that submitted the form. Mail a link instead and anyone can be signed up by + a stranger who picked their password: the victim's click would be enough to + create the account, and the stranger would know the password to it. The + person who fills in the form is the only one who sees both halves. + + Both halves are checked. The ticket held in this browser's session is the + other one, and without it the same attack survives the change to codes: + re-submit a stranger's pending signup with a password of your own, and the + code that reaches them redeems *your* password. A code is only good in the + browser it was issued to. + + Every refusal reads the same. Whether the address has a signup pending, a + wrong code, an expired one or nothing at all is not something an + unauthenticated caller gets to learn by guessing. + """ + if not _is_email_auth_enabled(): + return JSONResponse( + {"success": False, "error": "Email authentication is not enabled"}, + status_code=status.HTTP_403_FORBIDDEN + ) + + email = verify_data.email.strip().lower() if verify_data.email else "" + code = verify_data.code.strip() if verify_data.code else "" + + rejected = JSONResponse( + {"success": False, + "error": "That code is not valid or has expired. " + "Request a new one and try again."}, + status_code=status.HTTP_400_BAD_REQUEST + ) + + if not _validate_email(email) or not code: + return rejected + + ticket = read_signup_ticket(request, email=email) + if not ticket: + # No signup was started in this browser for this address, so there is + # nothing here to confirm -- whatever code the caller has, it belongs to + # somebody else's session. + logging.info("Verification without a signup ticket for %s", + _sanitize_for_log(email)) + return rejected + + try: + pending, result = await consume_pending_signup(email, code, ticket) + if result != RESULT_OK or pending is None: + logging.info("Verification refused for %s: %s", + _sanitize_for_log(email), result) + return rejected + + # The code is spent from here on, whatever happens next, so the ticket + # that went with it has nothing left to unlock. + forget_signup_ticket(request) + + # The address may have acquired an account by another route (Google, + # GitHub) while the code sat unused. The code is spent either way. + if await _email_account_exists(pending.email): + logging.info("Verification code for an address that now has an account: %s", + _sanitize_for_log(pending.email)) + return JSONResponse( + {"success": False, + "error": "This address already has an account. Sign in instead."}, + status_code=status.HTTP_409_CONFLICT + ) + + # ``api_token=None``: the browser is credentialed by the session cookie, + # so minting a token here would only leave an orphan Token node. The + # password goes in with the identity: a second write could fail and + # leave an account that cannot be logged into and cannot be signed up + # for again. + is_new_identity, user_info = await ensure_user_in_organizations( + pending.email, pending.email, pending.full_name, "email", None, + password_hash=pending.password_hash, + ) + if not (is_new_identity and user_info and user_info.get("new_identity")): + logging.error("Could not create the verified account for %s", + _sanitize_for_log(pending.email)) + return JSONResponse( + {"success": False, "error": "Could not create your account"}, + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR + ) if not establish_browser_session( request, - email=email, - name=f"{first_name} {last_name}", + email=pending.email, + name=pending.full_name, provider="email", - provider_user_id=email, + provider_user_id=pending.email, provisioned=True, ): - # The account exists but the browser has no credential, so reporting - # success would leave the user staring at a logged-out page. - logging.error("Could not establish a browser session after email signup") + # Unlike the old signup path this is recoverable: the account is + # real, so the user can simply log in. + logging.error("Verified %s but could not establish a browser session", + _sanitize_for_log(pending.email)) return JSONResponse( - {"success": False, "error": "Registration failed"}, + {"success": False, + "error": "Your account is ready, but we could not sign you in. " + "Please log in."}, status_code=status.HTTP_500_INTERNAL_SERVER_ERROR ) - response = JSONResponse({ + logging.info("Email verified and account created: %s", + _sanitize_for_log(pending.email)) + return JSONResponse({ "success": True, - }, status_code=201) - return response + "email": pending.email, + "name": pending.full_name, + }) except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: - # Same reasoning as /login/email: an unreachable store is not a rejected - # registration, and answering 500 tells the caller to give up on - # something a retry would fix. - logging.error("Auth store unreachable during signup: %s", e) + logging.error("Auth store unreachable during email verification: %s", e) return JSONResponse( {"success": False, "error": "Authentication service temporarily unavailable - please retry"}, status_code=status.HTTP_503_SERVICE_UNAVAILABLE ) except Exception as e: - logging.error("Signup error: %s", e) + logging.error("Email verification error: %s", e) return JSONResponse( - {"success": False, "error": "Registration failed"}, + {"success": False, "error": "Could not confirm your email address"}, status_code=status.HTTP_500_INTERNAL_SERVER_ERROR ) + +@auth_router.post("/signup/email/resend") +async def resend_verification_email( + request: Request, resend_data: EmailResendRequest +) -> JSONResponse: + """Re-send a signup verification code. + + Always answers the same way once the address is well-formed. Whether it has + a signup awaiting confirmation, an account already, or nothing at all is not + something an unauthenticated caller gets to learn from this endpoint -- and + that includes the rate-limit state, which would otherwise answer the same + question a beat later. + """ + if not _is_email_auth_enabled(): + return JSONResponse( + {"success": False, "error": "Email authentication is not enabled"}, + status_code=status.HTTP_403_FORBIDDEN + ) + + email = resend_data.email.strip().lower() if resend_data.email else "" + if not _validate_email(email): + return JSONResponse( + {"success": False, "error": "Invalid email format"}, + status_code=status.HTTP_400_BAD_REQUEST + ) + + accepted = JSONResponse( + {"success": True, + "message": "If that address is waiting to be confirmed, a new code is on its way.", + "retryAfterSeconds": resend_interval_seconds()}, + status_code=status.HTTP_202_ACCEPTED + ) + + try: + issue = await refresh_pending_signup(email) + except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: + logging.error("Auth store unreachable during verification resend: %s", e) + return JSONResponse( + {"success": False, + "error": "Authentication service temporarily unavailable - please retry"}, + status_code=status.HTTP_503_SERVICE_UNAVAILABLE + ) + + if not issue.issued: + # Unknown address, throttled and exhausted are indistinguishable here by + # design, and are not told apart anywhere else either: knowing which one + # it was is the answer this endpoint exists to withhold. + logging.info("Verification resend not issued for %s", _sanitize_for_log(email)) + return accepted + + if not await send_verification_code(email, issue.first_name, issue.code): + # Nothing left the building, so hand the send back and put the code the + # user may already be holding back in force. + await revert_verification_send(email, issue) + logging.error("Could not re-send the verification email for %s", + _sanitize_for_log(email)) + + return accepted + + @auth_router.post("/login/email") async def email_login(request: Request, login_data: EmailLoginRequest) -> JSONResponse: """Handle email/password user login.""" @@ -750,6 +950,7 @@ async def auth_status(request: Request) -> JSONResponse: response = JSONResponse( content={ "authenticated": True, + "providers": _get_auth_config(), "user": { # Falls back to the email so the id is always a usable string # for clients, even for database-backed API tokens. @@ -767,7 +968,7 @@ async def auth_status(request: Request) -> JSONResponse: # Not authenticated - return 200 with authenticated: false # This is NOT an error - unauthenticated users can still use the app return JSONResponse( - content={"authenticated": False}, + content={"authenticated": False, "providers": _get_auth_config()}, status_code=200 ) diff --git a/app/src/components/modals/LoginModal.tsx b/app/src/components/modals/LoginModal.tsx index 65c8a389..7462fb7e 100644 --- a/app/src/components/modals/LoginModal.tsx +++ b/app/src/components/modals/LoginModal.tsx @@ -1,5 +1,11 @@ +import { useEffect, useState } from "react"; import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogDescription } from "@/components/ui/dialog"; import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; +import { Label } from "@/components/ui/label"; +import { useToast } from "@/components/ui/use-toast"; +import { useAuth } from "@/contexts/AuthContext"; +import { AuthService } from "@/services/auth"; import { buildApiUrl, API_CONFIG } from "@/config/api"; interface LoginModalProps { @@ -8,7 +14,58 @@ interface LoginModalProps { canClose?: boolean; // Whether user can close the modal (false for required login) } +const MIN_PASSWORD_LENGTH = 8; + +// Only a fallback: the backend reports its own resend interval, and that is +// what the button waits for whenever it is available. +const RESEND_COOLDOWN_SECONDS = 60; + +// Likewise a fallback for the code lifetime the backend reports. +const CODE_TTL_MINUTES = 15; + +const CODE_LENGTH = 6; + +const emptyForm = { firstName: "", lastName: "", email: "", password: "" }; + const LoginModal = ({ open, onOpenChange, canClose = true }: LoginModalProps) => { + const { providers, refreshAuth } = useAuth(); + const { toast } = useToast(); + + const [mode, setMode] = useState<"login" | "signup">("login"); + const [form, setForm] = useState(emptyForm); + const [error, setError] = useState(null); + const [submitting, setSubmitting] = useState(false); + // Set once a signup has been accepted. While it holds an address the modal + // asks for the mailed code instead of showing the form -- there is no + // account yet, so there is nothing else to offer. + const [awaitingEmail, setAwaitingEmail] = useState(null); + const [code, setCode] = useState(""); + const [verifying, setVerifying] = useState(false); + const [codeTtlMinutes, setCodeTtlMinutes] = useState(CODE_TTL_MINUTES); + const [cooldown, setCooldown] = useState(0); + const [resending, setResending] = useState(false); + + const emailEnabled = providers?.email_auth_enabled ?? false; + // Default to showing the OAuth buttons: a backend that does not report + // providers at all would otherwise leave the user with no way in. + const googleEnabled = providers?.google_auth_enabled ?? true; + const githubEnabled = providers?.github_auth_enabled ?? true; + const showOAuth = googleEnabled || githubEnabled; + + useEffect(() => { + if (cooldown <= 0) return; + const timer = setTimeout(() => setCooldown((seconds) => seconds - 1), 1000); + return () => clearTimeout(timer); + }, [cooldown]); + + // Reopening should not drop the user back into a stale error. + useEffect(() => { + if (!open) { + setError(null); + setSubmitting(false); + } + }, [open]); + const handleGoogleLogin = () => { window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GOOGLE); }; @@ -17,6 +74,303 @@ const LoginModal = ({ open, onOpenChange, canClose = true }: LoginModalProps) => window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GITHUB); }; + const switchMode = (next: "login" | "signup") => { + setMode(next); + setError(null); + }; + + const update = + (field: keyof typeof emptyForm) => (event: React.ChangeEvent) => { + setForm((current) => ({ ...current, [field]: event.target.value })); + }; + + const handleSubmit = async (event: React.FormEvent) => { + event.preventDefault(); + setError(null); + + if (mode === "signup" && form.password.length < MIN_PASSWORD_LENGTH) { + setError(`Password must be at least ${MIN_PASSWORD_LENGTH} characters long`); + return; + } + + setSubmitting(true); + try { + if (mode === "login") { + const result = await AuthService.loginWithEmail(form.email.trim(), form.password); + if (!result.success) { + setError(result.error ?? "Could not sign you in."); + return; + } + await refreshAuth(); + setForm(emptyForm); + onOpenChange(false); + return; + } + + const result = await AuthService.signupWithEmail({ + firstName: form.firstName.trim(), + lastName: form.lastName.trim(), + email: form.email.trim(), + password: form.password, + }); + + if (!result.success) { + setError(result.error ?? "Could not create your account."); + return; + } + + // Deliberately no refreshAuth(): signing up does not sign you in. The + // account is created when the mailed code is handed back, and that is + // what establishes the session. + setAwaitingEmail(result.email ?? form.email.trim()); + setCooldown(result.retryAfterSeconds ?? RESEND_COOLDOWN_SECONDS); + if (result.codeTtlSeconds) { + setCodeTtlMinutes(Math.max(1, Math.round(result.codeTtlSeconds / 60))); + } + setCode(""); + setForm(emptyForm); + } catch { + setError("Could not reach the server. Please try again."); + } finally { + setSubmitting(false); + } + }; + + const handleVerify = async (event: React.FormEvent) => { + event.preventDefault(); + if (!awaitingEmail || verifying) return; + setError(null); + setVerifying(true); + try { + const result = await AuthService.verifyEmail(awaitingEmail, code.trim()); + if (!result.success) { + setError(result.error ?? "Could not confirm your email address."); + setCode(""); + return; + } + // Confirming is what created the account and signed the browser in. + await refreshAuth(); + setAwaitingEmail(null); + setCode(""); + setMode("login"); + onOpenChange(false); + toast({ + title: "Email confirmed", + description: "Your account is ready and you are signed in.", + }); + } catch { + setError("Could not reach the server. Please try again."); + } finally { + setVerifying(false); + } + }; + + const handleResend = async () => { + if (!awaitingEmail || cooldown > 0 || resending) return; + setResending(true); + setError(null); + try { + const result = await AuthService.resendVerification(awaitingEmail); + // Only a request the backend actually took starts the clock. Refusing to + // retry after a failed one would lock the user out over an email that + // was never sent. + if (result.success) { + setCooldown(result.retryAfterSeconds ?? RESEND_COOLDOWN_SECONDS); + setCode(""); + } + toast({ + title: result.success ? "Email sent" : "Could not send the email", + description: result.success + ? result.message ?? "Check your inbox for the confirmation code." + : result.error, + variant: result.success ? undefined : "destructive", + }); + } catch { + toast({ + title: "Could not send the email", + description: "Could not reach the server. Please try again.", + variant: "destructive", + }); + } finally { + setResending(false); + } + }; + + const backToSignIn = () => { + setAwaitingEmail(null); + setCode(""); + setMode("login"); + setError(null); + }; + + const renderAwaitingVerification = () => ( +
+

+ We sent a {CODE_LENGTH}-digit confirmation code to{" "} + {awaitingEmail}. Enter it + below to finish creating your account — you will be signed in straight away. +

+

+ The code works once and expires after {codeTtlMinutes} minutes. Until you enter it, no + account exists. +

+ +
+ + + setCode(event.target.value.replace(/\D/g, "").slice(0, CODE_LENGTH)) + } + inputMode="numeric" + autoComplete="one-time-code" + pattern={`\\d{${CODE_LENGTH}}`} + placeholder={"0".repeat(CODE_LENGTH)} + className="text-center text-lg tracking-[0.4em]" + required + autoFocus + data-testid="verification-code" + /> +
+ + {error && ( +

+ {error} +

+ )} + + + + +
+ ); + + const renderEmailForm = () => ( +
+ {mode === "signup" && ( +
+
+ + +
+
+ + +
+
+ )} + +
+ + +
+ +
+ + + {mode === "signup" && ( +

+ At least {MIN_PASSWORD_LENGTH} characters. +

+ )} +
+ + {error && ( +

+ {error} +

+ )} + + + +

+ {mode === "login" ? ( + <> + Don't have an account?{" "} + + + ) : ( + <> + Already have an account?{" "} + + + )} +

+
+ ); + return ( > - Welcome to QueryWeaver + {awaitingEmail ? "Check your inbox" : "Welcome to QueryWeaver"} - Sign in to access your databases and start querying + {awaitingEmail + ? "Enter the code we emailed you to create your account" + : "Sign in to access your databases and start querying"} -
- - - -
+ {awaitingEmail ? ( + renderAwaitingVerification() + ) : ( + <> + {showOAuth && ( +
+ {googleEnabled && ( + + )} + + {githubEnabled && ( + + )} +
+ )} + + {emailEnabled && showOAuth && ( +
+
+ +
+
+ or +
+
+ )} + + {emailEnabled &&
{renderEmailForm()}
} + + )} - {canClose && ( + {canClose && !awaitingEmail && (

By signing in, you agree to our Terms of Service and Privacy Policy

diff --git a/app/src/config/api.ts b/app/src/config/api.ts index cc5fc6ab..33461990 100644 --- a/app/src/config/api.ts +++ b/app/src/config/api.ts @@ -18,6 +18,10 @@ export const API_CONFIG = { AUTH_STATUS: '/auth-status', LOGIN_GOOGLE: '/login/google', LOGIN_GITHUB: '/login/github', + LOGIN_EMAIL: '/login/email', + SIGNUP_EMAIL: '/signup/email', + VERIFY_EMAIL: '/signup/email/verify', + RESEND_VERIFICATION: '/signup/email/resend', LOGOUT: '/logout', // Graph/Database management diff --git a/app/src/contexts/AuthContext.tsx b/app/src/contexts/AuthContext.tsx index 81a52306..032bb449 100644 --- a/app/src/contexts/AuthContext.tsx +++ b/app/src/contexts/AuthContext.tsx @@ -1,6 +1,6 @@ import React, { createContext, useContext, useState, useEffect, useRef } from 'react'; import { AuthService } from '@/services/auth'; -import type { User } from '@/types/api'; +import type { AuthProviders, User } from '@/types/api'; // How long to wait before re-checking while the backend cannot answer. The // login survives the outage, so the session comes back on its own once the @@ -10,6 +10,8 @@ const UNAVAILABLE_RETRY_MS = 15000; interface AuthContextType { user: User | null; isAuthenticated: boolean; + /** Which sign-in methods the backend offers; null until the first check lands. */ + providers: AuthProviders | null; /** The check could not be made, as opposed to answering "not logged in". */ isUnavailable: boolean; isLoading: boolean; @@ -25,6 +27,7 @@ const AuthContext = createContext(undefined); export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const [user, setUser] = useState(null); + const [providers, setProviders] = useState(null); const [isUnavailable, setIsUnavailable] = useState(false); const [isLoading, setIsLoading] = useState(true); const retryTimer = useRef | null>(null); @@ -37,7 +40,15 @@ export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children try { setIsLoading(true); const status = await AuthService.checkAuthStatus(); - setUser(status.user || null); + // A backend that could not answer has not said the session ended, so an + // outage must not log the user out. Same reasoning for the providers: + // keep the last known ones so the sign-in form does not vanish mid-outage. + if (!status.unavailable) { + setUser(status.user || null); + } + if (status.providers) { + setProviders(status.providers); + } setIsUnavailable(!!status.unavailable); if (status.unavailable) { retryTimer.current = setTimeout(checkAuth, UNAVAILABLE_RETRY_MS); @@ -73,6 +84,7 @@ export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children const value: AuthContextType = { user, isAuthenticated: !!user, + providers, isUnavailable, isLoading, login: { diff --git a/app/src/services/auth.ts b/app/src/services/auth.ts index 2f9e490f..e5a982b5 100644 --- a/app/src/services/auth.ts +++ b/app/src/services/auth.ts @@ -1,6 +1,6 @@ import { API_CONFIG, buildApiUrl } from '@/config/api'; import { csrfHeaders } from '@/lib/csrf'; -import type { AuthStatus, User } from '@/types/api'; +import type { AuthStatus, SignupResult, User } from '@/types/api'; /** * Authentication Service @@ -94,6 +94,110 @@ export class AuthService { } } + /** + * Sign up with an email address and password. + * + * A successful call creates nothing: the backend holds the details and mails + * a confirmation code, and the account comes into existence when that code is + * typed back in here. So there is no session to refresh yet. + */ + static async signupWithEmail(details: { + firstName: string; + lastName: string; + email: string; + password: string; + }): Promise { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.SIGNUP_EMAIL), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify(details), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { + success: false, + error: data.error || 'Could not create your account. Please try again.', + retryAfterSeconds: data.retryAfterSeconds, + }; + } + return data as SignupResult; + } + + /** + * Log in with an email address and password. + */ + static async loginWithEmail(email: string, password: string): Promise<{ success: boolean; error?: string }> { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_EMAIL), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify({ email, password }), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { success: false, error: data.error || 'Could not sign you in. Please try again.' }; + } + return { success: true }; + } + + /** + * Ask for another copy of the signup confirmation code. + * + * The backend answers identically whether or not the address is waiting to be + * confirmed, so the caller cannot use this to probe for accounts -- and + * neither can the UI report anything more specific than "sent". + */ + static async resendVerification( + email: string + ): Promise<{ success: boolean; message?: string; error?: string; retryAfterSeconds?: number }> { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.RESEND_VERIFICATION), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify({ email }), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { + success: false, + error: data.error || 'Could not send the email. Please try again.', + retryAfterSeconds: data.retryAfterSeconds, + }; + } + return { success: true, message: data.message, retryAfterSeconds: data.retryAfterSeconds }; + } + + /** + * Hand back the confirmation code that was mailed. + * + * This is what creates the account, so a success means the caller is now + * logged in and the session should be refreshed. + */ + static async verifyEmail( + email: string, + code: string + ): Promise<{ success: boolean; error?: string }> { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.VERIFY_EMAIL), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify({ email, code }), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { + success: false, + error: data.error || 'Could not confirm your email address. Please try again.', + }; + } + return { success: true }; + } + /** * Logout current user */ diff --git a/app/src/types/api.ts b/app/src/types/api.ts index 3798ae46..43df0f9c 100644 --- a/app/src/types/api.ts +++ b/app/src/types/api.ts @@ -11,9 +11,18 @@ export interface User { } // Authentication types + +/** Which sign-in methods the backend has configured. */ +export interface AuthProviders { + email_auth_enabled: boolean; + google_auth_enabled: boolean; + github_auth_enabled: boolean; +} + export interface AuthStatus { authenticated: boolean; user?: User; + providers?: AuthProviders; /** * The backend could not check, rather than checking and saying no. Callers * should offer a retry instead of sending the user back to the login screen. @@ -21,6 +30,17 @@ export interface AuthStatus { unavailable?: boolean; } +/** Reply to a signup: no account exists yet, only a mailed code. */ +export interface SignupResult { + success: boolean; + pending?: boolean; + email?: string; + message?: string; + error?: string; + retryAfterSeconds?: number; + codeTtlSeconds?: number; +} + // Graph/Database types export interface Graph { id: string; diff --git a/e2e/logic/api/apiResponses.ts b/e2e/logic/api/apiResponses.ts index 611230c0..364bac16 100644 --- a/e2e/logic/api/apiResponses.ts +++ b/e2e/logic/api/apiResponses.ts @@ -13,6 +13,11 @@ export interface User { export interface AuthStatusResponse { authenticated: boolean; user?: User; + providers?: { + email_auth_enabled: boolean; + google_auth_enabled: boolean; + github_auth_enabled: boolean; + }; } export interface LoginResponse { @@ -20,8 +25,12 @@ export interface LoginResponse { error?: string; } +/** Signup creates nothing: it mails a code and reports that it is waiting. */ export interface SignupResponse { success: boolean; + pending?: boolean; + email?: string; + message?: string; error?: string; } diff --git a/e2e/logic/api/mailbox.ts b/e2e/logic/api/mailbox.ts new file mode 100644 index 00000000..4d870c85 --- /dev/null +++ b/e2e/logic/api/mailbox.ts @@ -0,0 +1,123 @@ +import fs from 'fs'; +import path from 'path'; +import { APIRequestContext } from '@playwright/test'; +import { postRequest } from '../../infra/api/apiRequests'; +import { getBaseUrl } from '../../config/urls'; + +/** + * Reading the signup confirmation code out of the mail outbox. + * + * Signup no longer logs anybody in: it mails a code, and handing that code back + * is what creates the account and starts the session. The suite therefore has + * to go through the mail. The backend's file transport (`MAIL_OUTBOX_DIR`) + * writes each message to disk, so the tests exercise the real flow rather than + * a test-only shortcut that would prove nothing about it. + */ + +const OUTBOX_DIR = process.env.MAIL_OUTBOX_DIR || 'e2e/.mail'; + +// The mail is written while the signup request is still in flight, so a short +// poll covers the gap without making the setup slow when it is already there. +const WAIT_TIMEOUT_MS = 10_000; +const POLL_INTERVAL_MS = 200; + +const CODE_LENGTH = 6; + +interface Message { + file: string; + contents: string; + mtimeMs: number; +} + +function readOutbox(): Message[] { + if (!fs.existsSync(OUTBOX_DIR)) return []; + return fs + .readdirSync(OUTBOX_DIR) + .filter((name) => name.endsWith('.eml')) + .map((name) => { + const file = path.join(OUTBOX_DIR, name); + return { + file, + contents: fs.readFileSync(file, 'utf8'), + mtimeMs: fs.statSync(file).mtimeMs, + }; + }); +} + +/** + * Undo quoted-printable encoding. + * + * Python's email builder wraps long lines with a trailing `=` and escapes bytes + * as `=XX`, either of which can land in the middle of the digits we are after. + */ +function decodeQuotedPrintable(contents: string): string { + return contents + .replace(/=\r?\n/g, '') + .replace(/=([0-9A-Fa-f]{2})/g, (_, hex) => String.fromCharCode(parseInt(hex, 16))); +} + +/** + * Pull the code out of the message body. + * + * The plain-text part indents it on a line of its own, which is specific enough + * to keep the match away from the digits in headers and timestamps. + */ +function extractCode(contents: string): string | null { + const match = decodeQuotedPrintable(contents).match( + new RegExp(`^\\s+(\\d{${CODE_LENGTH}})\\s*$`, 'm') + ); + return match ? match[1] : null; +} + +/** + * Wait for the confirmation code most recently mailed to `email`. + */ +export async function waitForVerificationCode(email: string): Promise { + const deadline = Date.now() + WAIT_TIMEOUT_MS; + + while (Date.now() < deadline) { + const candidates = readOutbox() + .filter((message) => message.contents.includes(email)) + .sort((a, b) => b.mtimeMs - a.mtimeMs); + + for (const candidate of candidates) { + const code = extractCode(candidate.contents); + // Consume it, so a later signup for the same address cannot match this + // message and submit a code that has already been spent. + if (code) { + fs.rmSync(candidate.file, { force: true }); + return code; + } + } + + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + } + + throw new Error( + `No verification email for ${email} appeared in ${OUTBOX_DIR}. ` + + 'Is the server running with MAIL_OUTBOX_DIR set?' + ); +} + +/** + * Hand the mailed code back, which creates the account and signs the request + * context in. + */ +export async function completeVerification( + email: string, + requestContext: APIRequestContext +): Promise { + const code = await waitForVerificationCode(email); + const response = await postRequest( + `${getBaseUrl()}/signup/email/verify`, + { email, code }, + requestContext + ); + + const data = await response.json().catch(() => ({})); + if (!data.success) { + throw new Error( + `Verification for ${email} failed: ${data.error || response.status()}` + ); + } +} diff --git a/e2e/logic/pom/userProfile.ts b/e2e/logic/pom/userProfile.ts index 314c456f..b5792573 100644 --- a/e2e/logic/pom/userProfile.ts +++ b/e2e/logic/pom/userProfile.ts @@ -47,6 +47,10 @@ export class UserProfile extends BasePage { return this.page.getByTestId("github-login-btn"); } + private get emailAuthForm(): Locator { + return this.page.getByTestId("email-auth-form"); + } + // ==================== TOKENS MODAL LOCATORS ==================== private get generateTokenBtn(): Locator { @@ -277,6 +281,10 @@ export class UserProfile extends BasePage { } } + async isEmailAuthFormVisible(): Promise { + return await waitForElementToBeVisible(this.emailAuthForm); + } + async isGenerateTokenBtnVisible(): Promise { return await waitForElementToBeVisible(this.generateTokenBtn); } diff --git a/e2e/tests/auth.setup.ts b/e2e/tests/auth.setup.ts index fcbe1778..9e47bb3a 100644 --- a/e2e/tests/auth.setup.ts +++ b/e2e/tests/auth.setup.ts @@ -1,116 +1,63 @@ -import { test as setup } from '@playwright/test'; +import { test as setup, Page } from '@playwright/test'; import apiCalls from '../logic/api/apiCalls'; +import { completeVerification } from '../logic/api/mailbox'; import { getTestUser, getTestUser2, getTestUser3 } from '../config/urls'; const authFile = 'e2e/.auth/user.json'; const authFile2 = 'e2e/.auth/user2.json'; const authFile3 = 'e2e/.auth/user3.json'; -setup('authenticate users', async ({ page }) => { - const api = new apiCalls(); - - // Authenticate user 1 - const { email, password } = getTestUser(); +/** + * Sign the context in as one test user, creating the account if it is missing. + * + * Signup does not log anybody in any more: it mails a confirmation code, and + * handing that code back is what creates the account and starts the session. So + * the create path goes through the mail outbox rather than trusting the signup + * response, which is also what keeps this setup honest about the real flow. + */ +async function authenticateUser( + api: apiCalls, + page: Page, + user: { email: string; password: string }, + firstName: string, + lastName: string, + storagePath: string, + label: string +): Promise { + const { email, password } = user; try { - // Try to login first - let response = await api.loginWithEmail( - email, - password, - page.request - ); + const response = await api.loginWithEmail(email, password, page.request); - // If login fails, try to create the user - if (!response.success) { + if (!response.success) { const signupResponse = await api.signupWithEmail( - 'Test', - 'User', + firstName, + lastName, email, password, page.request ); if (!signupResponse.success) { - throw new Error(`Failed to create test user 1: ${signupResponse.error || 'Unknown error'}`); + throw new Error( + `Failed to create ${label}: ${signupResponse.error || 'Unknown error'}` + ); } - } - } catch (error) { - const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 1. \n Error: ${errorMessage}` - ); - } - - // Save authentication state for user 1 - await page.context().storageState({ path: authFile }); - // Authenticate user 2 - const user2 = getTestUser2(); - - try { - // Try to login first - let response = await api.loginWithEmail( - user2.email, - user2.password, - page.request - ); - - // If login fails, try to create the user - if (!response.success) { - const signupResponse = await api.signupWithEmail( - 'Test2', - 'User2', - user2.email, - user2.password, - page.request - ); - - if (!signupResponse.success) { - throw new Error(`Failed to create test user 2: ${signupResponse.error || 'Unknown error'}`); - } - } + await completeVerification(email, page.request); + } } catch (error) { const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 2. \n Error: ${errorMessage}` - ); + throw new Error(`Authentication failed for ${label}. \n Error: ${errorMessage}`); } - // Save authentication state for user 2 - await page.context().storageState({ path: authFile2 }); + await page.context().storageState({ path: storagePath }); +} - // Authenticate user 3 - const user3 = getTestUser3(); - - try { - // Try to login first - let response = await api.loginWithEmail( - user3.email, - user3.password, - page.request - ); - - // If login fails, try to create the user - if (!response.success) { - const signupResponse = await api.signupWithEmail( - 'Test3', - 'User3', - user3.email, - user3.password, - page.request - ); - - if (!signupResponse.success) { - throw new Error(`Failed to create test user 3: ${signupResponse.error || 'Unknown error'}`); - } - } - } catch (error) { - const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 3. \n Error: ${errorMessage}` - ); - } +setup('authenticate users', async ({ page }) => { + const api = new apiCalls(); - // Save authentication state for user 3 - await page.context().storageState({ path: authFile3 }); + await authenticateUser(api, page, getTestUser(), 'Test', 'User', authFile, 'test user 1'); + await authenticateUser(api, page, getTestUser2(), 'Test2', 'User2', authFile2, 'test user 2'); + await authenticateUser(api, page, getTestUser3(), 'Test3', 'User3', authFile3, 'test user 3'); }); diff --git a/e2e/tests/userProfile.spec.ts b/e2e/tests/userProfile.spec.ts index 00f4b985..6628c672 100644 --- a/e2e/tests/userProfile.spec.ts +++ b/e2e/tests/userProfile.spec.ts @@ -81,12 +81,15 @@ test.describe('User Profile Tests', () => { const isUserMenuVisible = await userProfile.isUserMenuVisible(); expect(isUserMenuVisible).toBeFalsy(); - // Verify welcome screen with login options is shown + // Verify welcome screen with login options is shown. Which options appear + // depends on what the deployment configured, so any one of them counts. + // Checked first because it waits, giving the modal time to render. + const isEmailFormVisible = await userProfile.isEmailAuthFormVisible(); const isGoogleLoginVisible = await userProfile.isGoogleLoginBtnVisible(); const isGithubLoginVisible = await userProfile.isGithubLoginBtnVisible(); // At least one login option should be visible - expect(isGoogleLoginVisible || isGithubLoginVisible).toBeTruthy(); + expect(isEmailFormVisible || isGoogleLoginVisible || isGithubLoginVisible).toBeTruthy(); }); test('generate token and copy token', async () => { diff --git a/tests/test_auth_status.py b/tests/test_auth_status.py index 2cb72d76..11f4329d 100644 --- a/tests/test_auth_status.py +++ b/tests/test_auth_status.py @@ -82,7 +82,15 @@ async def test_anonymous_visitor_is_not_an_error(self): response = await auth_status(FakeRequest()) assert response.status_code == 200 - assert _body(response) == {"authenticated": False} + body = _body(response) + assert body["authenticated"] is False + # The sign-in screen is rendered from this, so an anonymous visitor is + # exactly who needs to know which methods are on offer. + assert set(body["providers"]) == { + "email_auth_enabled", + "google_auth_enabled", + "github_auth_enabled", + } @pytest.mark.asyncio async def test_unreachable_auth_store_yields_503(self): diff --git a/tests/test_email_signup.py b/tests/test_email_signup.py index d461aad9..eca3501d 100644 --- a/tests/test_email_signup.py +++ b/tests/test_email_signup.py @@ -1,27 +1,65 @@ -"""Tests for the email signup endpoint, focused on the authentication-bypass fix. +"""Tests for the email signup, verification and resend endpoints. + +Two properties are pinned here. Regression coverage for CVE-2026-10130: signing up with an email that already belongs to an account (under any provider) must NOT issue a session token, which would otherwise allow taking over that account without knowing its password. + +And the property that replaces it: signup creates nothing at all. It parks the +details and mails a code, and only handing that code back creates the account +and signs the browser in. So an address the registrant does not control never +becomes an account -- and because the code has to come back to the session that +submitted the form, a stranger cannot get someone else to finish a signup they +never started. """ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from api.auth.browser_session import SESSION_KEY, SIGNUP_TICKET_KEY +from api.auth.email_verification import ( + RESULT_EXPIRED, + RESULT_INVALID, + RESULT_OK, + CodeIssue, + PendingSignup, +) +from api.auth.user_management import _build_user_merge_query from api.core.errors import AuthBackendUnavailableError -from api.routes.auth import EmailSignupRequest, _email_account_exists, email_signup +from api.routes.auth import ( + EmailResendRequest, + EmailSignupRequest, + EmailVerifyRequest, + _email_account_exists, + email_signup, + resend_verification_email, + verify_email, +) pytestmark = [pytest.mark.unit, pytest.mark.auth] -def _mock_request(): - """Build a minimal mock Request for the signup handler.""" +def _mock_request(signup_ticket="ticket", ticket_email="new@example.com"): + """Build a minimal mock Request for the signup handler. + + Redeeming a code takes the ticket the browser was handed when it submitted + the form, so the default request carries one for the address the + verification tests use. Pass ``signup_ticket=None`` for a browser that never + started a signup. + """ request = MagicMock() # The transport helpers read these; default to a plain http request. request.headers.get.return_value = None request.url.scheme = "http" + request.base_url = "http://testserver/" request.session = {} + if signup_ticket: + request.session[SIGNUP_TICKET_KEY] = { + "email": ticket_email, + "ticket": signup_ticket, + } return request @@ -34,6 +72,19 @@ def _signup_data(email="victim@example.com"): ) +def _pending(email="new@example.com"): + return PendingSignup( + email=email, + first_name="Ada", + last_name="Lovelace", + password_hash="00" * 32, + ) + + +def _verify_data(email="new@example.com", code="123456"): + return EmailVerifyRequest(email=email, code=code) + + def _set_cookie_header(response): return response.headers.get("set-cookie", "") or "" @@ -42,25 +93,26 @@ class TestEmailSignupExistingAccount: """An existing account must never be handed a session token via signup.""" @pytest.mark.asyncio - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_existing_account_is_rejected_without_token( - self, _enabled, mock_exists, mock_set_hash, mock_ensure + self, _enabled, mock_exists, mock_start, mock_discard ): mock_exists.return_value = True response = await email_signup(_mock_request(), _signup_data()) assert response.status_code == 409 - body = response.body.decode() - assert "already exists" in body + assert "already exists" in response.body.decode() # No session token must be issued for an existing account. assert "api_token=" not in _set_cookie_header(response) - # The account/token graph mutation and password write must not run. - mock_ensure.assert_not_called() - mock_set_hash.assert_not_called() + # Nothing may be parked either: a code mailed now would be redeemable + # against an account that already exists. + mock_start.assert_not_called() + # Any code still outstanding for the address is revoked. + mock_discard.assert_awaited_once() @pytest.mark.asyncio @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @@ -75,16 +127,14 @@ async def test_existence_check_failure_fails_closed(self, _enabled, mock_exists) assert "api_token=" not in _set_cookie_header(response) @pytest.mark.asyncio - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_an_outage_during_creation_is_retryable_not_a_bug( - self, _enabled, mock_exists, mock_ensure - ): + async def test_an_outage_is_retryable_not_a_bug(self, _enabled, mock_exists, mock_start): # 500 would tell the caller the registration is broken, when in fact # nothing was decided and retrying is the right move. mock_exists.return_value = False - mock_ensure.side_effect = AuthBackendUnavailableError("down") + mock_start.side_effect = AuthBackendUnavailableError("down") response = await email_signup(_mock_request(), _signup_data("new@example.com")) @@ -92,57 +142,480 @@ async def test_an_outage_during_creation_is_retryable_not_a_bug( assert "api_token=" not in _set_cookie_header(response) -class TestEmailSignupNewAccount: - """A genuinely new account should be created and logged in.""" +class TestEmailSignupPending: + """Signup mails a code and creates nothing.""" @pytest.mark.asyncio @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_new_account_is_created_and_logged_in( - self, _enabled, mock_exists, mock_set_hash, mock_ensure + async def test_signup_mails_a_code_and_creates_nothing( + self, _enabled, mock_exists, mock_start, mock_send, mock_ensure ): mock_exists.return_value = False - mock_ensure.return_value = (True, {"new_identity": True}) + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = True - request = _mock_request() + request = _mock_request(signup_ticket=None) response = await email_signup(request, _signup_data("new@example.com")) - assert response.status_code == 201 - # The signed session is the credential; the token never reaches the browser. + # 202, not 201: nothing has been created. + assert response.status_code == 202 + body = response.body.decode() + assert '"pending":true' in body.replace(" ", "") assert "api_token=" not in _set_cookie_header(response) - assert request.session, "signup must establish the browser session" + # The whole point: no account and no login until the code comes back. + # The session holds the signup's ticket and nothing else. + assert SESSION_KEY not in request.session + mock_ensure.assert_not_called() + + # The mail carries the raw code, which exists nowhere else. + assert mock_send.await_args.args[2] == "123456" + # And the code must not be echoed to the caller: the point of mailing it + # is that only whoever reads the inbox learns it. + assert "123456" not in body + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_the_ticket_stays_in_the_browser_that_signed_up( + self, _enabled, mock_exists, mock_start, mock_send + ): + # It is the half of the pair that never goes in the mail: whoever else + # submits this address, only this browser can redeem a code for it. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = True + + request = _mock_request(signup_ticket=None) + response = await email_signup(request, _signup_data("new@example.com")) + + assert request.session[SIGNUP_TICKET_KEY] == { + "email": "new@example.com", + "ticket": "ticket-abc", + } + # Not in the reply, and not in the mail either: it is the cookie. + assert "ticket-abc" not in response.body.decode() + assert "ticket-abc" not in mock_send.await_args.args + + @pytest.mark.asyncio + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_undeliverable_mail_is_reported_and_rolled_back( + self, _enabled, mock_exists, mock_start, mock_send, mock_discard + ): + # Answering 202 here would leave the user waiting for a mail that was + # never sent, and the dead record would burn the rate limit on retry. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = False + + response = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) + + assert response.status_code == 503 + mock_discard.assert_awaited_once() + + @pytest.mark.asyncio + @patch("api.routes.auth.revert_verification_send", new_callable=AsyncMock) + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_failed_send_does_not_destroy_a_code_already_delivered( + self, _enabled, mock_exists, mock_start, mock_send, mock_discard, mock_revert + ): + # Re-submitting the form displaces a code that did arrive. If the new + # mail then fails, deleting the record would take the working code with + # it and leave the user with nothing to type. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", + first_name="Mallory", + ticket="ticket-abc", + previous={"code_hash": "old-hash", "expires_at": 4242, "attempts": 1}, + ) + mock_send.return_value = False + + response = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) + + assert response.status_code == 503 + mock_revert.assert_awaited_once() + mock_discard.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_refused_send_reads_exactly_like_a_sent_one( + self, _enabled, mock_exists, mock_start, mock_send + ): + # A 429 here would say "this address has a signup in flight", which is + # the question /signup/email/resend refuses to answer. Answering it on + # the way in would give it away just the same. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = True + sent = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) + + mock_start.return_value = CodeIssue() + refused = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) + + assert sent.status_code == refused.status_code == 202 + assert sent.body == refused.body + assert mock_send.await_count == 1 + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_refused_send_hands_out_no_ticket( + self, _enabled, mock_exists, mock_start, mock_send + ): + # There is no code to go with one, and overwriting the ticket already + # held would lock this browser out of the signup it did start. + mock_exists.return_value = False + mock_start.return_value = CodeIssue() + + request = _mock_request(signup_ticket="held", ticket_email="new@example.com") + await email_signup(request, _signup_data("new@example.com")) + + assert request.session[SIGNUP_TICKET_KEY]["ticket"] == "held" + mock_send.assert_not_called() + + +class TestVerifyEmail: + """Handing the code back is what creates the account and the session.""" + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_valid_code_creates_the_account_and_logs_in( + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_session + ): + pending = _pending() + mock_consume.return_value = (pending, RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (True, {"new_identity": True}) + + response = await verify_email(_mock_request(), _verify_data()) + + assert response.status_code == 200 mock_ensure.assert_awaited_once() - mock_set_hash.assert_awaited_once() + # No API token is minted: the session cookie is the browser credential. + assert mock_ensure.await_args.args[-1] is None + # The password is written with the identity, not after it: a separate + # write could fail and leave an account nobody can log into. + assert mock_ensure.await_args.kwargs["password_hash"] == pending.password_hash + assert mock_session.call_args.kwargs["provisioned"] is True + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_the_spent_ticket_is_dropped( + self, _enabled, mock_consume, mock_exists, mock_ensure, _session + ): + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (True, {"new_identity": True}) + + request = _mock_request() + await verify_email(request, _verify_data()) + + assert SIGNUP_TICKET_KEY not in request.session + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_code_alone_confirms_nothing( + self, _enabled, mock_consume, mock_ensure + ): + # The attack the ticket exists for: submit someone else's address with a + # password of your own, and the code that lands in their inbox would + # otherwise create an account you know the password to. A browser that + # did not start this signup cannot finish it, whatever it read. + response = await verify_email(_mock_request(signup_ticket=None), _verify_data()) + + assert response.status_code == 400 + mock_consume.assert_not_called() + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_ticket_for_another_address_does_not_travel( + self, _enabled, mock_consume + ): + # Signing up for one address must not confer the right to confirm a + # pending signup for a different one. + response = await verify_email( + _mock_request(ticket_email="other@example.com"), _verify_data() + ) + + assert response.status_code == 400 + mock_consume.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_the_code_is_matched_against_the_address_that_was_submitted( + self, _enabled, mock_consume + ): + # A code that redeemed against any pending signup would be a code worth + # guessing against every one of them at once. + mock_consume.return_value = (None, RESULT_INVALID) + await verify_email( + _mock_request(ticket_email="ada@example.com"), + _verify_data(email="Ada@Example.com "), + ) -class TestEmailSignupCreationFailure: - """If account creation does not yield a new identity, no token is issued.""" + assert mock_consume.await_args.args == ("ada@example.com", "123456", "ticket") @pytest.mark.asyncio - @patch("api.routes.auth.delete_user_token", new_callable=AsyncMock) @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_wrong_expired_and_replayed_codes_read_identically( + self, _enabled, mock_consume, mock_ensure + ): + # Telling them apart would let a guesser learn when they had found a + # live signup, and let anyone probe which addresses are mid-signup. + mock_consume.return_value = (None, RESULT_INVALID) + wrong = await verify_email(_mock_request(), _verify_data()) + + mock_consume.return_value = (None, RESULT_EXPIRED) + expired = await verify_email(_mock_request(), _verify_data()) + + assert wrong.status_code == expired.status_code == 400 + assert wrong.body == expired.body + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_an_empty_code_never_reaches_the_store( + self, _enabled, mock_consume, mock_ensure + ): + response = await verify_email(_mock_request(), _verify_data(code=" ")) + + assert response.status_code == 400 + mock_consume.assert_not_called() + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_code_for_an_address_that_gained_an_account_is_refused( + self, _enabled, mock_consume, mock_exists, mock_ensure + ): + # Signed up by email, then logged in with Google before confirming. The + # code must not log anyone into that account. + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = True + + response = await verify_email(_mock_request(), _verify_data()) + + assert response.status_code == 409 + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=False) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_non_new_identity_is_rejected_and_no_token_minted( - self, _enabled, mock_exists, mock_set_hash, mock_ensure, mock_delete + async def test_unset_session_is_reported_as_a_failure( + self, _enabled, mock_consume, mock_exists, mock_ensure, _session ): - # Passes the pre-check, but creation reports the identity already existed - # (e.g. a concurrent signup race). No token must leak. + # Silently reporting success would leave the user logged out with no + # explanation; the account is real, so logging in still works. + mock_consume.return_value = (_pending(), RESULT_OK) mock_exists.return_value = False - mock_ensure.return_value = (False, {"new_identity": False}) + mock_ensure.return_value = (True, {"new_identity": True}) - response = await email_signup(_mock_request(), _signup_data("race@example.com")) + response = await verify_email(_mock_request(), _verify_data()) assert response.status_code == 500 - assert "api_token=" not in _set_cookie_header(response) - mock_set_hash.assert_not_called() - # Signup asks for the records without a token, so there is nothing to - # revoke on the failure path. - assert mock_ensure.await_args.args[-1] is None - mock_delete.assert_not_awaited() + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_failed_account_write_does_not_log_anyone_in( + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_session + ): + # The account and its password are one write, so a failure leaves + # nothing behind and the address is still free to sign up again. + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (False, None) + + response = await verify_email(_mock_request(), _verify_data()) + + assert response.status_code == 500 + mock_session.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_backend_outage_is_retryable(self, _enabled, mock_consume): + mock_consume.side_effect = AuthBackendUnavailableError("down") + + response = await verify_email(_mock_request(), _verify_data()) + + assert response.status_code == 503 + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=False) + async def test_disabled_email_auth_redeems_nothing(self, _enabled, mock_consume): + response = await verify_email(_mock_request(), _verify_data()) + + assert response.status_code == 403 + mock_consume.assert_not_called() + + +class TestResendVerification: + """The resend endpoint must not become an account-existence oracle.""" + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_pending_address_gets_a_fresh_code(self, _enabled, mock_refresh, mock_send): + mock_refresh.return_value = CodeIssue(code="654321", first_name="Ada") + mock_send.return_value = True + + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert response.status_code == 202 + mock_send.assert_awaited_once() + assert "654321" not in response.body.decode() + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unknown_and_throttled_answer_exactly_like_a_success( + self, _enabled, mock_refresh, mock_send + ): + # Same status and same body either way, or the endpoint would tell an + # anonymous caller which addresses are mid-signup. The store does not + # even report which refusal it was, so there is nothing to leak. + mock_refresh.return_value = CodeIssue(code="654321", first_name="Ada") + mock_send.return_value = True + issued = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + mock_refresh.return_value = CodeIssue() + unknown = await resend_verification_email( + _mock_request(), EmailResendRequest(email="nobody@example.com") + ) + + refused = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert issued.status_code == unknown.status_code == refused.status_code == 202 + assert issued.body == unknown.body == refused.body + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unknown_address_is_never_mailed(self, _enabled, mock_refresh, mock_send): + mock_refresh.return_value = CodeIssue() + + await resend_verification_email( + _mock_request(), EmailResendRequest(email="nobody@example.com") + ) + + mock_send.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_malformed_address_is_rejected(self, _enabled, mock_refresh): + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="not-an-email") + ) + + assert response.status_code == 400 + mock_refresh.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_backend_outage_is_retryable(self, _enabled, mock_refresh): + mock_refresh.side_effect = AuthBackendUnavailableError("down") + + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert response.status_code == 503 + + +class TestPasswordIsWrittenWithTheIdentity: + """The password clause on the identity merge. + + Verification consumes the code before it creates the account, so there is + no second chance: an account written without its password could neither be + logged into nor signed up for again. + """ + + def test_no_password_is_written_unless_one_is_given(self): + assert "password_hash" not in _build_user_merge_query() + + def test_the_password_is_set_as_the_identity_is_created(self): + query = _build_user_merge_query(include_token=False, include_password=True) + on_create, on_match = query.split("ON MATCH SET", 1) + + assert "identity.password_hash = $password_hash" in on_create + # Never on the ON MATCH branch: an identity that already exists keeps + # the password it already has. + assert "password_hash" not in on_match class TestEmailAccountExistsResultHandling: @@ -184,24 +657,3 @@ async def test_query_error_propagates_to_fail_closed(self): with patch("api.routes.auth.db.select_graph", return_value=graph): with pytest.raises(RuntimeError): await _email_account_exists("err@example.com") - - -class TestEmailSignupSessionFailure: - """A created account with no browser credential is not a successful signup.""" - - @pytest.mark.asyncio - @patch("api.routes.auth.establish_browser_session", return_value=False) - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) - @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) - @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_unset_session_is_reported_as_a_failure( - self, _enabled, mock_exists, _set_hash, mock_ensure, _establish - ): - # Reporting 201 here would leave the user staring at a logged-out page. - mock_exists.return_value = False - mock_ensure.return_value = (True, {"new_identity": True}) - - response = await email_signup(_mock_request(), _signup_data("new@example.com")) - - assert response.status_code == 500 diff --git a/tests/test_email_verification.py b/tests/test_email_verification.py new file mode 100644 index 00000000..cb47066a --- /dev/null +++ b/tests/test_email_verification.py @@ -0,0 +1,604 @@ +"""Tests for the pending-signup store that backs email verification. + +The store is what makes "no account until the code comes back" true, so the +properties pinned here are the ones the guarantee rests on: the raw code is +never stored, a code is redeemable exactly once, wrong guesses are finite, and +the send limits cannot be reset by asking again. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from api.auth import email_verification as ev + +pytestmark = [pytest.mark.unit, pytest.mark.auth] + + +def _result(rows): + return MagicMock(result_set=rows) + + +class _FakeGraph: + """Records the queries a helper runs and replays canned result sets.""" + + def __init__(self, results): + self._results = list(results) + self.calls = [] + self.query = AsyncMock(side_effect=self._query) + + async def _query(self, cypher, params=None): + self.calls.append((cypher, params or {})) + return self._results.pop(0) if self._results else _result([]) + + +def _patch_graph(graph): + return patch("api.auth.email_verification._graph", return_value=graph) + + +class TestStartPendingSignup: + """Parking a signup, and the limits on how much mail it can generate.""" + + @staticmethod + def _issued(**previous): + """The row the issuing query returns: the record as it stood before.""" + return _result([[previous]]) + + @pytest.mark.asyncio + async def test_only_the_code_hash_is_stored(self): + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + _, params = graph.calls[-1] + assert params["code_hash"] == ev.hash_code(issue.code) + # A graph snapshot must not yield a usable code. + assert issue.code not in params.values() + + @pytest.mark.asyncio + async def test_only_the_ticket_hash_is_stored(self): + # Same reasoning as the code: the ticket is the other half of the pair, + # so a reader of the graph must not be able to lift a usable one. + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + _, params = graph.calls[-1] + assert params["ticket_hash"] == ev.hash_code(issue.ticket) + assert issue.ticket not in params.values() + + @pytest.mark.asyncio + async def test_every_signup_gets_its_own_ticket(self): + # The ticket is what stops a second submission for the same address + # from having its password confirmed by the address's owner. + graph = _FakeGraph([self._issued(), self._issued(code_hash="old-hash")]) + with _patch_graph(graph): + first = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + second = await ev.start_pending_signup( + "new@example.com", "Mal", "Lory", "other-hash" + ) + + assert first.ticket and second.ticket + assert first.ticket != second.ticket + + @pytest.mark.asyncio + async def test_the_code_is_six_digits(self): + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + # Including the leading zeros: a code that sometimes arrives five + # digits long is a code the user cannot type into a fixed-width field. + assert len(issue.code) == ev.CODE_DIGITS + assert issue.code.isdigit() + + @pytest.mark.asyncio + async def test_the_limit_is_enforced_inside_the_write(self): + # Checking first and writing after would let two concurrent requests + # both pass the check and both send while the counter advanced once. + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + assert len(graph.calls) == 1 + cypher, params = graph.calls[0] + assert "$max_sends" in cypher and "$interval_ms" in cypher + assert params["max_sends"] == ev.max_sends() + + @pytest.mark.asyncio + async def test_resubmitting_cannot_reset_the_send_limit(self): + # Otherwise the rate limit is decorative: re-post the form and the + # counter starts over. Only a record this query creates starts at zero. + graph = _FakeGraph([self._issued(code_hash="old-hash", send_count=1)]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + cypher, params = graph.calls[-1] + assert "ELSE p.send_count + 1" in cypher + assert "ON CREATE SET p.created_at = $now, p.send_count = 0" in cypher + assert "send_count" not in params + + @pytest.mark.asyncio + async def test_an_expired_record_starts_a_fresh_budget(self): + # Nothing deletes a signup that is never confirmed, so a spent send + # budget would otherwise lock an address out of the product for good -- + # five submissions by a stranger and the real owner can never sign up. + graph = _FakeGraph([self._issued(code_hash="old-hash", expires_at=1)]) + with _patch_graph(graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + cypher, _ = graph.calls[-1] + assert "(p.expires_at IS NULL OR p.expires_at < $now) AS stale" in cypher + assert "WHERE (stale OR p.send_count < $max_sends)" in cypher + assert "p.send_count = CASE WHEN stale THEN 1" in cypher + + @pytest.mark.asyncio + async def test_expiry_does_not_lift_the_interval(self): + # An expired record is a way to keep sending, not a way to send faster. + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + cypher, _ = graph.calls[-1] + interval = "AND (p.last_sent_at IS NULL OR $now - p.last_sent_at >= $interval_ms)" + assert interval in cypher + assert "stale OR p.last_sent_at" not in cypher + + @pytest.mark.asyncio + async def test_the_whole_displaced_record_comes_back_for_reverting(self): + # Not just the code: issuing rewrites the name, the password hash and + # the ticket too, and an undo that restores a subset would leave a + # record nobody submitted -- one person's code against another's + # password. The snapshot is also how the caller tells "this address + # already had a live code" from "this record is one I just created". + previous = { + "code_hash": "old-hash", + "ticket_hash": "old-ticket", + "password_hash": "someone-elses-password", + "expires_at": 4242, + "attempts": 3, + } + graph = _FakeGraph([self._issued(**previous)]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.previous == previous + assert issue.displaced + + @pytest.mark.asyncio + async def test_a_record_this_call_created_displaced_nothing(self): + graph = _FakeGraph([self._issued(email="new@example.com", send_count=0)]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + assert not issue.displaced + + @pytest.mark.asyncio + async def test_a_refused_send_says_nothing_about_why(self, monkeypatch): + # The route answers a refusal exactly like a success, so asking the + # graph why would only be a way to probe for a stranger's signup. + monkeypatch.setenv("EMAIL_VERIFICATION_RESEND_SECONDS", "60") + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert not issue.issued + assert issue == ev.CodeIssue() + assert len(graph.calls) == 1 + + @pytest.mark.asyncio + async def test_send_budget_is_finite(self, monkeypatch): + # Bounds how much mail one submitted address can aim at a third party. + monkeypatch.setenv("EMAIL_VERIFICATION_MAX_SENDS", "2") + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert not issue.issued + _, params = graph.calls[-1] + assert params["max_sends"] == 2 + + +class TestRefreshPendingSignup: + """Resending refreshes an existing signup and never invents one.""" + + @pytest.mark.asyncio + async def test_unknown_address_is_not_created(self): + # A MERGE here would turn the resend endpoint into a way to mail an + # address nobody ever submitted. + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("nobody@example.com") + + assert not issue.issued + assert all("MERGE" not in cypher for cypher, _ in graph.calls) + + @pytest.mark.asyncio + async def test_a_resend_keeps_the_ticket(self): + # A resend is another copy of the same signup. Minting a new ticket + # would lock out the browser that is sitting on the code entry screen. + graph = _FakeGraph([_result([["Ada", {"code_hash": "old-hash"}]])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + cypher, params = graph.calls[-1] + assert "ticket_hash" not in cypher + assert "ticket_hash" not in params + assert issue.ticket is None + + @pytest.mark.asyncio + async def test_refresh_replaces_the_previous_code(self): + previous = {"code_hash": "old-hash", "ticket_hash": "kept", "attempts": 3} + graph = _FakeGraph([_result([["Ada", previous]])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + assert issue.issued + assert issue.first_name == "Ada" + write_cypher, params = graph.calls[-1] + assert "MATCH" in write_cypher and "MERGE" not in write_cypher + assert params["code_hash"] == ev.hash_code(issue.code) + assert "ELSE p.send_count + 1" in write_cypher + # A fresh code deserves a fresh budget of guesses. + assert "p.attempts = 0" in write_cypher + # Kept so a send that never reaches a transport can be undone. + assert issue.previous == previous + + @pytest.mark.asyncio + async def test_losing_a_race_with_verification_is_not_an_error(self): + # The record was consumed between the write and this call. + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + assert not issue.issued + + +class TestRevertVerificationSend: + """Undoing a send whose mail never reached a transport.""" + + @staticmethod + def _issued(): + return ev.CodeIssue( + code="123456", + first_name="Ada", + previous={ + "code_hash": "old-hash", + "ticket_hash": "old-ticket", + "password_hash": "someone-elses-password", + "expires_at": 4242, + "attempts": 3, + "send_count": 1, + }, + ) + + @pytest.mark.asyncio + async def test_the_record_is_put_back_exactly_as_it_was(self): + # A transport failure must cost the user neither their send budget nor + # the code they may already be holding. Restoring the whole map rather + # than named fields is also what keeps the undo honest: the send + # rewrote the password hash and the ticket too. + graph = _FakeGraph([]) + with _patch_graph(graph): + await ev.revert_verification_send("pending@example.com", self._issued()) + + cypher, params = graph.calls[-1] + assert "SET p = $previous" in cypher + assert params["previous"] == self._issued().previous + + @pytest.mark.asyncio + async def test_a_send_that_displaced_nothing_is_not_reverted(self): + # There is no record to restore this one to. Reverting anyway would + # write back the bare node the MERGE created, leaving a husk behind; + # discarding it is the caller's job. + graph = _FakeGraph([]) + with _patch_graph(graph): + await ev.revert_verification_send( + "new@example.com", + ev.CodeIssue(code="123456", previous={"email": "new@example.com"}), + ) + + assert graph.calls == [] + + @pytest.mark.asyncio + async def test_a_code_issued_since_is_left_alone(self): + # The revert only matches the hash it wrote, so it cannot clobber a + # send that succeeded in the meantime. + graph = _FakeGraph([]) + with _patch_graph(graph): + await ev.revert_verification_send("pending@example.com", self._issued()) + + cypher, params = graph.calls[-1] + assert "WHERE p.code_hash = $code_hash" in cypher + assert params["code_hash"] == ev.hash_code("123456") + + @pytest.mark.asyncio + async def test_the_clock_is_not_rolled_back(self): + # The snapshot would restore the old last_sent_at along with everything + # else, and a broken transport could then be retried without limit. + graph = _FakeGraph([]) + with _patch_graph(graph): + await ev.revert_verification_send("pending@example.com", self._issued()) + + cypher, params = graph.calls[-1] + assert "SET p.last_sent_at = $now" in cypher + assert params["now"] >= params["previous"].get("last_sent_at", 0) + + @pytest.mark.asyncio + async def test_nothing_to_undo_when_nothing_was_issued(self): + graph = _FakeGraph([]) + with _patch_graph(graph): + await ev.revert_verification_send( + "pending@example.com", ev.CodeIssue() + ) + + assert graph.calls == [] + + @pytest.mark.asyncio + async def test_a_failing_revert_is_swallowed(self): + # Best-effort: the caller is already on its error path. + graph = _FakeGraph([]) + graph.query = AsyncMock(side_effect=RuntimeError("down")) + with _patch_graph(graph): + await ev.revert_verification_send("pending@example.com", self._issued()) + + +class TestConsumePendingSignup: + """Redeeming a code, and the budget of wrong guesses.""" + + @staticmethod + def _row(live): + return [[live, "Ada", "Lovelace", "hash"]] + + @pytest.mark.asyncio + async def test_empty_code_never_reaches_the_database(self): + graph = _FakeGraph([]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + assert not graph.calls + + @pytest.mark.asyncio + async def test_a_code_without_a_ticket_never_reaches_the_database(self): + # The pair is the credential. Half of it is not a partial answer, it is + # a request from a browser that never submitted this signup. + graph = _FakeGraph([]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + assert not graph.calls + + @pytest.mark.asyncio + async def test_live_code_returns_the_details_and_deletes_the_record(self): + graph = _FakeGraph([_result(self._row(True))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "ticket" + ) + + assert result == ev.RESULT_OK + assert pending.email == "new@example.com" + assert pending.full_name == "Ada Lovelace" + cypher, params = graph.calls[0] + # Single-use is structural: the read and the delete are one query, so a + # replay cannot find the node no matter how the caller behaves. + assert "DELETE p" in cypher + assert params["code_hash"] == ev.hash_code("123456") + + @pytest.mark.asyncio + async def test_the_ticket_is_matched_in_the_same_query(self): + # Otherwise re-submitting a stranger's pending signup with a password + # of your own gets it confirmed by the address's owner. + graph = _FakeGraph([_result(self._row(True))]) + with _patch_graph(graph): + await ev.consume_pending_signup("new@example.com", "123456", "ticket") + + cypher, params = graph.calls[0] + assert "p.ticket_hash = $ticket_hash" in cypher + assert params["ticket_hash"] == ev.hash_code("ticket") + + @pytest.mark.asyncio + async def test_the_right_code_with_the_wrong_ticket_is_refused(self): + # The graph matches nothing, exactly as it would for a wrong code. + graph = _FakeGraph([_result([]), _result([])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "someone-elses-ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + + @pytest.mark.asyncio + async def test_the_attempt_limit_is_enforced_inside_the_write(self): + # Reading the counter first would let a burst of concurrent guesses all + # pass a check that only one increment ever answered for. + graph = _FakeGraph([_result(self._row(True))]) + with _patch_graph(graph): + await ev.consume_pending_signup("new@example.com", "123456", "ticket") + + cypher, params = graph.calls[0] + assert "p.attempts < $max_attempts" in cypher + assert params["max_attempts"] == ev.max_attempts() + + @pytest.mark.asyncio + async def test_a_wrong_code_is_charged_for(self): + # Six digits are only enough while the number of guesses is small. + graph = _FakeGraph([_result([]), _result([])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "000000", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + charge_cypher, _ = graph.calls[-1] + assert "p.attempts = p.attempts + 1" in charge_cypher + + @pytest.mark.asyncio + async def test_running_out_of_guesses_destroys_the_signup(self): + # Guessing has to end somewhere, and ending it by deleting the record + # costs the attacker their target while the user just signs up again. + graph = _FakeGraph([_result([]), _result([[5]])]) + with _patch_graph(graph): + await ev.consume_pending_signup("new@example.com", "000000", "ticket") + + charge_cypher, params = graph.calls[-1] + assert "attempts >= $max_attempts" in charge_cypher + assert "DELETE p" in charge_cypher + assert params["max_attempts"] == ev.max_attempts() + + @pytest.mark.asyncio + async def test_a_failing_charge_is_not_a_free_retry_signal(self): + # The guess still fails; only the bookkeeping is best-effort. + graph = _FakeGraph([_result([])]) + graph.query = AsyncMock( + side_effect=[_result([]), RuntimeError("db down")] + ) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "000000", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + + @pytest.mark.asyncio + async def test_replayed_code_finds_nothing(self): + graph = _FakeGraph([_result([]), _result([])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + + @pytest.mark.asyncio + async def test_an_expired_code_leaves_the_record_to_be_resent(self): + # Deleting it would strand a user who typed the right code a minute + # late: refreshing only ever MATCHes, so the resend button on the screen + # they are looking at would quietly do nothing. + graph = _FakeGraph([_result(self._row(False))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_EXPIRED + cypher, params = graph.calls[0] + assert "FOREACH (_ IN CASE WHEN live THEN [1] ELSE [] END | DELETE p)" in cypher + assert params["now"] > 0 + # The code was right, so no guess is charged -- and charging one could + # run the budget out and delete the record the resend needs. + assert len(graph.calls) == 1 + + @pytest.mark.asyncio + async def test_record_without_an_expiry_is_not_treated_as_eternal(self): + # A missing expiry must fail closed, not read as "never expires". The + # comparison in the query yields NULL, which is not true. + graph = _FakeGraph([_result(self._row(None))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_EXPIRED + + @pytest.mark.asyncio + async def test_record_missing_a_password_is_rejected(self): + graph = _FakeGraph([_result([[True, "Ada", "Lovelace", None]])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + + +class TestDiscardPendingSignup: + """Discarding is best-effort: it must never be the thing that fails a request.""" + + @pytest.mark.asyncio + async def test_a_failing_delete_is_swallowed(self): + graph = MagicMock() + graph.query = AsyncMock(side_effect=RuntimeError("db down")) + with _patch_graph(graph): + await ev.discard_pending_signup("new@example.com") + + +class TestSendVerificationCode: + """The mail itself.""" + + @pytest.mark.asyncio + async def test_code_is_included_in_both_bodies(self): + with patch("api.auth.email_verification.send_mail", + new_callable=AsyncMock) as mock_send: + mock_send.return_value = True + sent = await ev.send_verification_code("new@example.com", "Ada", "123456") + + assert sent is True + kwargs = mock_send.await_args.kwargs + assert kwargs["to"] == "new@example.com" + assert "123456" in kwargs["text_body"] + assert "123456" in kwargs["html_body"] + + @pytest.mark.asyncio + async def test_no_link_is_offered(self): + # The point of a code is that nothing in the mail can be acted on by + # someone who did not fill in the form -- including a scanner that + # fetches every URL it sees. + with patch("api.auth.email_verification.send_mail", + new_callable=AsyncMock) as mock_send: + mock_send.return_value = True + await ev.send_verification_code("new@example.com", "Ada", "123456") + + kwargs = mock_send.await_args.kwargs + assert "http" not in kwargs["text_body"] + assert "alert(1)", "123456" + ) + + html_body = mock_send.await_args.kwargs["html_body"] + assert "