diff --git a/backend/accounts/models.py b/backend/accounts/models.py index 56ed510..debea78 100644 --- a/backend/accounts/models.py +++ b/backend/accounts/models.py @@ -24,7 +24,7 @@ from sqlalchemy import ( UniqueConstraint, false, ) -from sqlalchemy.orm import Mapped, mapped_column +from sqlalchemy.orm import Mapped, mapped_column, relationship from accounts.db import Base @@ -41,12 +41,60 @@ class User(SQLAlchemyBaseUserTableUUID, Base): # opt-in); the user can mute it while staying linked. Same migration caveat. discord_dm: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, server_default=false()) - # True when the account was created by "Sign in with Discord" and so has no - # password its owner has ever seen (hashed_password is NOT NULL, so the row - # carries a generated one nobody knows). Load-bearing: unlinking Discord from - # such an account would remove its only way in, so discord_link.py refuses. - discord_only: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, - server_default=false()) + # True when the account was created by signing in with an OAuth provider and so + # has no password its owner has ever seen (hashed_password is NOT NULL, so the + # row carries a generated one nobody knows). Load-bearing: unlinking the *last* + # linked provider from such an account would remove its only way in, so + # accounts/oauth.py refuses that. Was `discord_only` before Google was added. + oauth_only: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, + server_default=false()) + + # selectin, not the lazy default: these are read while serialising /users/me on + # the async engine, where a lazy load raises MissingGreenlet rather than + # quietly issuing a query. One extra SELECT per user load is the price. + oauth_accounts: Mapped[list["OAuthAccount"]] = relationship( + "OAuthAccount", lazy="selectin", cascade="all, delete-orphan", + ) + + @property + def oauth_providers(self) -> list[str]: + """Linked provider names — read straight onto UserRead by from_attributes.""" + return sorted(a.provider for a in self.oauth_accounts) + + +class OAuthAccount(Base): + """One external identity — a provider plus that provider's own user id — bound + to a Thermograph account. This is what "sign in with X" resolves against. + + Keyed on ``subject``, never email: the provider's id for an account is stable, + while an email address can be changed or reassigned. Email is stored only for + support and debugging, and is deliberately not used for lookup. + + Note ``User.discord_id`` is *not* redundant with a discord row here. That column + is a delivery address — notify.py DMs it — and survives on its own terms; this + table is purely about authentication. + """ + __tablename__ = "oauth_account" + + id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) + user_id: Mapped[uuid.UUID] = mapped_column( + GUID, ForeignKey("user.id", ondelete="CASCADE"), nullable=False + ) + provider: Mapped[str] = mapped_column(String(32), nullable=False) + # "sub" for Google, "id" for Discord. + subject: Mapped[str] = mapped_column(String(64), nullable=False) + email: Mapped[str | None] = mapped_column(String(320), nullable=True) + created_at: Mapped[float] = mapped_column(Float, nullable=False, default=time.time) + + __table_args__ = ( + # A provider account signs into exactly one Thermograph account — this is + # the constraint that stops one Google login resolving two ways. + UniqueConstraint("provider", "subject", name="uq_oauth_provider_subject"), + # And an account holds at most one identity per provider, so "connect + # Google" is idempotent rather than accumulating rows. + UniqueConstraint("user_id", "provider", name="uq_oauth_user_provider"), + Index("idx_oauth_user", "user_id"), + ) class AccessToken(SQLAlchemyBaseAccessTokenTableUUID, Base): diff --git a/backend/accounts/oauth.py b/backend/accounts/oauth.py new file mode 100644 index 0000000..75e82ad --- /dev/null +++ b/backend/accounts/oauth.py @@ -0,0 +1,528 @@ +"""Sign in with an external provider, and connect one to an existing account. + +One engine, two providers (Discord, Google). Adding a third means adding a +``Provider`` entry and nothing else — the flow, the signed state, the account +resolution and the session issuance are all provider-agnostic on purpose. That is +not tidiness for its own sake: account resolution is the security-critical part, +and a second hand-rolled copy is where a subtle divergence becomes a takeover. + +Two flows over one authorization-code grant, told apart by the signed `state`: + + link — /oauth/{provider}/link/start a signed-in user attaches an identity + login — /oauth/{provider}/login/start an anonymous visitor authenticates + +`state` is signed with the app's auth secret (stdlib hmac, no new dependency) and +carries the provider, the purpose, and — for a link — the Thermograph user id. All +three are inside the HMAC, so a state cannot be replayed, bound to a different +account, replayed as the other flow, or replayed against a different provider. + +How a login resolves to an account, in order: + + 1. An ``oauth_account`` row for (provider, subject) — the durable key, no email + needed and immune to the user changing their address at the provider. + 2. Otherwise the provider's email, but **only if it reports it verified**. That + flag is the sole evidence the person owns the address; matching an existing + Thermograph account on an unverified one would hand that account to whoever + typed the address into Discord or Google. Unverified is refused outright. + 3. No account for that email: create one, flagged ``oauth_only`` because it has + no password its owner has ever seen. + +**Legacy routes.** Discord shipped first, under /discord/*, and its callback URL is +registered in Discord's developer portal — so /discord/link/callback keeps working +forever, and the /discord/* aliases stay because the frontend deploys independently +of this service and an older one must keep working against a newer backend. +""" +from __future__ import annotations + +import base64 +import hashlib +import hmac +import json +import os +import time +import urllib.parse +from dataclasses import dataclass, field +from typing import Callable + +import httpx +from fastapi import APIRouter, Depends, HTTPException, Request +from fastapi.responses import RedirectResponse +from sqlalchemy import delete, func, select, update +from sqlalchemy.ext.asyncio import AsyncSession + +from accounts.db import get_async_session +from accounts.models import OAuthAccount, User +from accounts.users import ( + SECRET, + attach_session, + current_active_user, + current_user_optional, + get_access_token_db, + get_user_manager, +) +from core import audit + +router = APIRouter(tags=["oauth"]) + +BASE = os.environ.get("THERMOGRAPH_BASE", "/thermograph").rstrip("/") + +# How long a start->callback round trip may take before the signed state expires. +_STATE_TTL = 600 +_TIMEOUT = httpx.Timeout(10.0) + + +# --- providers ---------------------------------------------------------------- +def _parse_discord(p: dict) -> tuple[str, str, bool]: + return (str(p.get("id") or ""), + str(p.get("email") or "").strip().lower(), + p.get("verified") is True) + + +def _parse_google(p: dict) -> tuple[str, str, bool]: + # OpenID Connect userinfo. `email_verified` arrives as a real bool from the + # JSON endpoint, but Google has historically also serialised it as the string + # "true" in some responses, so accept both rather than silently reading a + # verified address as unverified and refusing every Google login. + verified = p.get("email_verified") + return (str(p.get("sub") or ""), + str(p.get("email") or "").strip().lower(), + verified is True or str(verified).lower() == "true") + + +@dataclass(frozen=True) +class Provider: + name: str + label: str + authorize_url: str + token_url: str + userinfo_url: str + # Linking needs only an identity; signing in also needs the address to resolve + # or create an account by, which is a separate scope the user is prompted for. + scope_link: str + scope_login: str + # Where this provider's callback lives. Discord's is grandfathered to the path + # already registered in its portal; anything new gets the canonical one. + callback_path: str + parse: Callable[[dict], tuple[str, str, bool]] + # Extra authorize-URL params. Google needs prompt=consent to re-issue consent, + # and Discord takes the same key, so this stays per-provider rather than shared. + extra_authorize: dict = field(default_factory=dict) + + +PROVIDERS: dict[str, Provider] = { + "discord": Provider( + name="discord", + label="Discord", + authorize_url="https://discord.com/api/oauth2/authorize", + token_url="https://discord.com/api/oauth2/token", + userinfo_url="https://discord.com/api/users/@me", + scope_link="identify", + scope_login="identify email", + callback_path="/api/v2/discord/link/callback", + parse=_parse_discord, + extra_authorize={"prompt": "consent"}, + ), + "google": Provider( + name="google", + label="Google", + authorize_url="https://accounts.google.com/o/oauth2/v2/auth", + token_url="https://oauth2.googleapis.com/token", + userinfo_url="https://openidconnect.googleapis.com/v1/userinfo", + # Google has no identity-only scope worth the name: openid alone yields a + # subject but no address, and we want the address recorded on the link too. + scope_link="openid email", + scope_login="openid email", + callback_path="/api/v2/oauth/google/callback", + parse=_parse_google, + # access_type=online: there is no offline work to do on the user's behalf, + # so asking for a refresh token would be collecting a credential we would + # never use. include_granted_scopes keeps incremental consent sane. + extra_authorize={"prompt": "consent", "access_type": "online", + "include_granted_scopes": "true"}, + ), +} + +# Credentials, read once at import. A dict rather than per-provider constants so a +# test can enable or disable one provider without touching the others. +CREDENTIALS: dict[str, tuple[str, str]] = { + "discord": (os.environ.get("THERMOGRAPH_DISCORD_APP_ID", "").strip(), + os.environ.get("THERMOGRAPH_DISCORD_CLIENT_SECRET", "").strip()), + "google": (os.environ.get("THERMOGRAPH_GOOGLE_CLIENT_ID", "").strip(), + os.environ.get("THERMOGRAPH_GOOGLE_CLIENT_SECRET", "").strip()), +} + + +def enabled(provider: str) -> bool: + """Whether this provider is configured. An unconfigured provider surfaces no UI + and every one of its routes bounces, so a half-set environment is inert rather + than broken.""" + cid, secret = CREDENTIALS.get(provider, ("", "")) + return bool(cid and secret) + + +def _provider_or_404(name: str) -> Provider: + p = PROVIDERS.get(name) + if p is None: + raise HTTPException(status_code=404, detail="Unknown provider.") + return p + + +# --- signed state ------------------------------------------------------------ +def _b64(raw: bytes) -> str: + return base64.urlsafe_b64encode(raw).decode().rstrip("=") + + +def _unb64(s: str) -> bytes: + return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4)) + + +def _sign_state(uid: str | None, purpose: str, provider: str) -> str: + payload = _b64(json.dumps( + {"u": uid or "", "t": int(time.time()), "p": purpose, "pr": provider} + ).encode()) + mac = hmac.new(SECRET.encode(), payload.encode(), hashlib.sha256).hexdigest()[:32] + return f"{payload}.{mac}" + + +def _verify_state(state: str, purpose: str, provider: str) -> str | None: + """The Thermograph user id this state was minted for, or None if it's missing, + tampered, expired, or was minted for a different purpose or provider. + + A login state has no user id yet, so it verifies as ``""`` — falsy, but distinct + from the None that means reject. Callers must test against None, not + truthiness, or a valid login state reads as a failure. + """ + try: + payload, mac = state.split(".", 1) + except (ValueError, AttributeError): + return None + expect = hmac.new(SECRET.encode(), payload.encode(), hashlib.sha256).hexdigest()[:32] + if not hmac.compare_digest(mac, expect): + return None + try: + data = json.loads(_unb64(payload)) + except (ValueError, json.JSONDecodeError): + return None + if time.time() - float(data.get("t", 0)) > _STATE_TTL: + return None + if data.get("p") != purpose: + return None + # States minted before the provider field existed are Discord's; they age out + # in _STATE_TTL, so this fallback only spans a deploy. + if data.get("pr", "discord") != provider: + return None + return data.get("u") or "" + + +# --- redirects --------------------------------------------------------------- +def _redirect_uri(request: Request, provider: Provider) -> str: + """The callback URL, matched to how the app is actually reached (scheme/host + + base path). Must equal the redirect registered in the provider's console.""" + proto = request.headers.get("x-forwarded-proto") or request.url.scheme + host = request.headers.get("host") or request.url.netloc + return f"{proto}://{host}{BASE}{provider.callback_path}" + + +def _authorize_redirect(request: Request, provider: Provider, scope: str, + state: str) -> RedirectResponse: + cid, _ = CREDENTIALS[provider.name] + params = { + "client_id": cid, + "redirect_uri": _redirect_uri(request, provider), + "response_type": "code", + "scope": scope, + "state": state, + **provider.extra_authorize, + } + return RedirectResponse( + url=f"{provider.authorize_url}?{urllib.parse.urlencode(params)}", status_code=303) + + +def _account_redirect(status: str, provider: str) -> RedirectResponse: + """Back to the alerts page, which surfaces the outcome as a toast; 303 so the + browser issues a GET after the callback. + + Emits `discord=` as well for Discord, because the frontend deploys + independently of this service and the one in production reads that parameter. + Drop it once no deployed frontend predates `oauth=`.""" + params = {"oauth": status, "provider": provider} + if provider == "discord": + params["discord"] = status + return RedirectResponse(url=f"{BASE}/alerts?{urllib.parse.urlencode(params)}", + status_code=303) + + +# --- provider API ------------------------------------------------------------- +async def _fetch_profile(request: Request, provider: Provider, code: str) -> dict | None: + """Trade the authorization code for the provider's user object, or None if any + step fails. Callers treat None as a graceful error, never a crash.""" + cid, secret = CREDENTIALS[provider.name] + try: + async with httpx.AsyncClient(timeout=_TIMEOUT) as client: + tok = await client.post(provider.token_url, data={ + "client_id": cid, + "client_secret": secret, + "grant_type": "authorization_code", + "code": code, + "redirect_uri": _redirect_uri(request, provider), + }, headers={"Content-Type": "application/x-www-form-urlencoded"}) + if tok.status_code >= 300: + return None + access = tok.json().get("access_token") + if not access: + return None + me = await client.get(provider.userinfo_url, + headers={"Authorization": f"Bearer {access}"}) + if me.status_code >= 300: + return None + profile = me.json() + except Exception: # noqa: BLE001 - any network/parse failure is a graceful error + return None + return profile if isinstance(profile, dict) else None + + +# --- persistence -------------------------------------------------------------- +async def _account_for(session: AsyncSession, provider: str, + subject: str) -> OAuthAccount | None: + return (await session.execute( + select(OAuthAccount).where(OAuthAccount.provider == provider, + OAuthAccount.subject == subject) + )).scalar_one_or_none() + + +async def _link_row(session: AsyncSession, user_id, provider: str, subject: str, + email: str | None) -> None: + """Bind (provider, subject) to a user, plus the Discord-only side effect. + + discord_id is a *delivery* address, not an identity — notify.py DMs it — so the + Discord link has to write it as well as the oauth_account row. Turning DMs on + here mirrors the original behaviour: linking is an active opt-in, and the user + can mute it (POST /discord/dm) while staying linked. + """ + session.add(OAuthAccount(user_id=user_id, provider=provider, subject=subject, + email=email or None)) + if provider == "discord": + await session.execute( + update(User).where(User.id == user_id) + .values(discord_id=subject, discord_dm=True)) + await session.commit() + + +async def _create_user(user_manager, email: str) -> User: + """Create an account for an identity that matched none of ours. + + Deliberately goes around ``UserManager.create()``: there is no password to + validate, and its ``on_after_register`` mails a confirmation link for an address + the provider has already verified. The generated password exists only because + ``hashed_password`` is NOT NULL — nobody ever sees it, which is what + ``oauth_only`` records. + """ + password = user_manager.password_helper.generate() + return await user_manager.user_db.create({ + "email": email, + "hashed_password": user_manager.password_helper.hash(password), + "is_active": True, + "is_superuser": False, + "is_verified": True, + "oauth_only": True, + }) + + +# --- routes ------------------------------------------------------------------- +@router.get("/oauth/config") +async def oauth_config(): + """Which providers this server can actually complete a flow with. The sign-in + modal and account menu render from this, so an unconfigured provider shows no + dead-end UI and configuring one later makes it appear on its own.""" + return {"providers": [ + {"name": p.name, "label": p.label, "enabled": enabled(p.name)} + for p in PROVIDERS.values() + ]} + + +@router.get("/oauth/{provider}/link/start") +async def link_start(provider: str, request: Request, user=Depends(current_active_user)): + p = _provider_or_404(provider) + if not enabled(provider): + return _account_redirect("unavailable", provider) + return _authorize_redirect(request, p, p.scope_link, + _sign_state(str(user.id), "link", provider)) + + +@router.get("/oauth/{provider}/login/start") +async def login_start(provider: str, request: Request, + user=Depends(current_user_optional)): + """Sign in with a provider. Unauthenticated by design — this is how you get a + session, not something you do with one.""" + p = _provider_or_404(provider) + if not enabled(provider): + return _account_redirect("unavailable", provider) + # Someone already signed in who lands here wants to connect, not to be switched + # into whichever account the external identity resolves to. + if user is not None: + return _authorize_redirect(request, p, p.scope_link, + _sign_state(str(user.id), "link", provider)) + return _authorize_redirect(request, p, p.scope_login, + _sign_state(None, "login", provider)) + + +async def _callback(provider: str, request: Request, user, session, user_manager, + access_token_db) -> RedirectResponse: + p = _provider_or_404(provider) + if not enabled(provider): + return _account_redirect("unavailable", provider) + # User declined on the provider's screen, or the state doesn't check out. + code = request.query_params.get("code") + state = request.query_params.get("state", "") + if not code: + return _account_redirect("cancelled", provider) + + # Which flow minted this state? Purpose and provider are both inside the HMAC, + # so a state only verifies under the flow and provider it was signed for. + uid = _verify_state(state, "link", provider) + if uid is not None: + return await _finish_link(p, request, code, uid, user, session) + if _verify_state(state, "login", provider) is not None: + return await _finish_login(p, request, code, session, user_manager, + access_token_db) + return _account_redirect("error", provider) + + +@router.get("/oauth/{provider}/callback") +async def oauth_callback( + provider: str, + request: Request, + user=Depends(current_user_optional), + session: AsyncSession = Depends(get_async_session), + user_manager=Depends(get_user_manager), + access_token_db=Depends(get_access_token_db), +): + return await _callback(provider, request, user, session, user_manager, + access_token_db) + + +async def _finish_link(p: Provider, request: Request, code: str, uid: str, user, + session: AsyncSession) -> RedirectResponse: + # Linking always acts on the person actually logged in, and only on the one the + # state was minted for. + if user is None or str(user.id) != uid: + return _account_redirect("error", p.name) + profile = await _fetch_profile(request, p, code) + if profile is None: + return _account_redirect("error", p.name) + subject, email, _ = p.parse(profile) + if not subject: + return _account_redirect("error", p.name) + # (provider, subject) is UNIQUE, so without this the insert would raise an + # IntegrityError and 500 instead of explaining itself. + owner = await _account_for(session, p.name, subject) + if owner is not None: + return _account_redirect("linked" if owner.user_id == user.id else "taken", p.name) + # And (user_id, provider) is UNIQUE: this account already has a different + # identity from this provider attached. + existing = (await session.execute( + select(OAuthAccount).where(OAuthAccount.user_id == user.id, + OAuthAccount.provider == p.name) + )).scalar_one_or_none() + if existing is not None: + return _account_redirect("already", p.name) + await _link_row(session, user.id, p.name, subject, email) + return _account_redirect("linked", p.name) + + +async def _finish_login(p: Provider, request: Request, code: str, + session: AsyncSession, user_manager, + access_token_db) -> RedirectResponse: + """Resolve the external identity to an account and sign in as it. + + Deliberately ignores any session already present. A login state carries no user + id — it can't, there is no user yet — so it is replayable against whoever + happens to be signed in. Treating it as a link would therefore be a + forced-linking takeover: an attacker who gets a victim to open this callback + with a code from the *attacker's* provider account would attach their identity + to the victim's account, then sign in as them. Only link/start, whose state is + bound to a specific user id, may link. The worst a replayed login state can do + is sign someone into the attacker's own account. + """ + profile = await _fetch_profile(request, p, code) + if profile is None: + return _account_redirect("error", p.name) + subject, email, verified = p.parse(profile) + if not subject: + return _account_redirect("error", p.name) + + created = False + acct = await _account_for(session, p.name, subject) + if acct is not None: + user = await user_manager.user_db.get(acct.user_id) + if user is None: + return _account_redirect("error", p.name) + else: + # Nothing carries this identity, so fall back to the email — the only other + # identity the provider gives us, and only when it vouches for it. + if not email: + return _account_redirect("noemail", p.name) + if not verified: + return _account_redirect("unverified", p.name) + user = await user_manager.user_db.get_by_email(email) + if user is None: + user = await _create_user(user_manager, email) + await _link_row(session, user.id, p.name, subject, email) + audit.log_activity("auth.register", {"user_id": str(user.id), "via": p.name}) + created = True + elif not user.is_active: + # Checked before the link, not just before the session: a deactivated + # account must not quietly acquire an identity on the way out. + return _account_redirect("inactive", p.name) + else: + # That address belongs to an account already tied to a different + # identity from this provider; connecting would silently move it. + existing = (await session.execute( + select(OAuthAccount).where(OAuthAccount.user_id == user.id, + OAuthAccount.provider == p.name) + )).scalar_one_or_none() + if existing is not None: + return _account_redirect("mismatch", p.name) + await _link_row(session, user.id, p.name, subject, email) + if not user.is_active: + return _account_redirect("inactive", p.name) + response = _account_redirect("created" if created else "signedin", p.name) + return await attach_session(response, user, user_manager, access_token_db, request) + + +async def _unlink(provider: str, user, session: AsyncSession) -> None: + p = _provider_or_404(provider) + linked = (await session.execute( + select(func.count()).select_from(OAuthAccount) + .where(OAuthAccount.user_id == user.id) + )).scalar_one() + mine = await session.execute( + select(OAuthAccount).where(OAuthAccount.user_id == user.id, + OAuthAccount.provider == p.name)) + if mine.scalar_one_or_none() is None: + return # already unlinked; idempotent + if user.oauth_only and linked <= 1: + # No password anyone has ever seen, and this is the last way in. No + # reset-password route is mounted to recover from it either. + raise HTTPException( + status_code=409, + detail=f"This account was created with {p.label} and has no password, " + "so unlinking would leave no way to sign in.", + ) + await session.execute( + delete(OAuthAccount).where(OAuthAccount.user_id == user.id, + OAuthAccount.provider == p.name)) + if p.name == "discord": + await session.execute( + update(User).where(User.id == user.id) + .values(discord_id=None, discord_dm=False)) + await session.commit() + + +@router.post("/oauth/{provider}/unlink", status_code=204) +async def unlink( + provider: str, + user=Depends(current_active_user), + session: AsyncSession = Depends(get_async_session), +): + await _unlink(provider, user, session) diff --git a/backend/accounts/schemas.py b/backend/accounts/schemas.py index 7385240..43f77c0 100644 --- a/backend/accounts/schemas.py +++ b/backend/accounts/schemas.py @@ -8,7 +8,7 @@ import uuid from typing import Literal from fastapi_users import schemas -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, Field, computed_field, field_validator from data import grading @@ -26,12 +26,24 @@ BOOKMARK_IMPORT_MAX_ITEMS = 200 class UserRead(schemas.BaseUser[uuid.UUID]): display_name: str | None = None # Present so the frontend can show linked/unlinked state; set via the OAuth - # flow (discord_link.py), not editable through the profile. + # flow (accounts/oauth.py), not editable through the profile. discord_id: str | None = None discord_dm: bool = False - # True when Discord is the account's only credential, so the UI can explain - # why "Unlink Discord" is refused rather than just failing. - discord_only: bool = False + # True when an OAuth provider is the account's only credential, so the UI can + # explain why unlinking the last one is refused rather than just failing. + oauth_only: bool = False + # Provider names currently linked, e.g. ["discord", "google"] — what the + # account menu renders its connected-accounts section from. + oauth_providers: list[str] = [] + + # Superseded by oauth_only. Kept because frontend and backend deploy + # independently: the frontend in production reads this field, and dropping it + # would break its account menu the moment this backend shipped ahead. Remove + # once no deployed frontend predates oauth_only. + @computed_field + @property + def discord_only(self) -> bool: + return self.oauth_only class UserCreate(schemas.BaseUserCreate): diff --git a/backend/alembic/versions/0005_oauth_accounts.py b/backend/alembic/versions/0005_oauth_accounts.py new file mode 100644 index 0000000..1b20b73 --- /dev/null +++ b/backend/alembic/versions/0005_oauth_accounts.py @@ -0,0 +1,104 @@ +"""oauth_account table; user.discord_only -> user.oauth_only + +Generalises "sign in with Discord" to any provider (see accounts/oauth.py). Three +steps, each conditional so the revision is safe on a database created before or +after the ORM gained these definitions — revision 0001 builds the schema with +``Base.metadata.create_all`` off *live* model metadata, so a freshly created +database already has both the table and the renamed column, and an unconditional +DDL would fail there with duplicate-object errors. + +The backfill is the load-bearing step. Discord identities used to live in +``user.discord_id``, and account resolution now keys on ``oauth_account``; without +copying the existing rows across, every already-linked user would silently stop +being recognised at login and would get a *second* account created on their next +sign-in. ``user.discord_id`` is deliberately left in place — it is the DM delivery +address notify.py reads, not merely an identity. + +Revision ID: 0005_oauth_accounts +Revises: 0004_bookmarks +Create Date: 2026-07-26 +""" +import time + +import sqlalchemy as sa +from alembic import op +# The same UUID type the ORM uses for user.id — a plain sa.Uuid renders differently +# on SQLite and would not match the column create_all produces. +from fastapi_users_db_sqlalchemy.generics import GUID + +revision = "0005_oauth_accounts" +down_revision = "0004_bookmarks" +branch_labels = None +depends_on = None + + +def _columns(table: str) -> set[str]: + return {c["name"] for c in sa.inspect(op.get_bind()).get_columns(table)} + + +def _tables() -> set[str]: + return set(sa.inspect(op.get_bind()).get_table_names()) + + +def upgrade() -> None: + cols = _columns("user") + + # 1. Rename the flag. It no longer means "Discord created this account", it + # means "no password its owner has ever seen, whichever provider made it". + if "oauth_only" in cols: + # A database built by 0001 already has oauth_only (create_all reads *current* + # metadata), and 0003 then re-added an empty discord_only beside it, because + # 0003 tests for the name it knew. Drop that vestige, or a fresh database + # carries a column an upgraded one does not — schema drift that would only + # surface much later. Safe: on this path the column was created empty + # moments ago and nothing has ever read it. + if "discord_only" in cols: + op.drop_column("user", "discord_only") + elif "discord_only" in cols: + # The real upgrade path: a deployed database carrying live values. Renamed, + # never dropped, so nothing is lost. + op.alter_column("user", "discord_only", new_column_name="oauth_only") + else: + # Database predates even discord_only (never deployed with it). + op.add_column("user", sa.Column("oauth_only", sa.Boolean(), nullable=False, + server_default=sa.false())) + + # 2. The identity table. + if "oauth_account" not in _tables(): + op.create_table( + "oauth_account", + sa.Column("id", sa.Integer(), primary_key=True, autoincrement=True), + sa.Column("user_id", GUID(), sa.ForeignKey("user.id", ondelete="CASCADE"), + nullable=False), + sa.Column("provider", sa.String(length=32), nullable=False), + sa.Column("subject", sa.String(length=64), nullable=False), + sa.Column("email", sa.String(length=320), nullable=True), + sa.Column("created_at", sa.Float(), nullable=False), + sa.UniqueConstraint("provider", "subject", name="uq_oauth_provider_subject"), + sa.UniqueConstraint("user_id", "provider", name="uq_oauth_user_provider"), + ) + op.create_index("idx_oauth_user", "oauth_account", ["user_id"]) + + # 3. Backfill every existing Discord link. Guarded by NOT EXISTS so re-running + # (or running after create_all already produced the table) cannot violate + # either unique constraint. + op.execute(sa.text(""" + INSERT INTO oauth_account (user_id, provider, subject, email, created_at) + SELECT u.id, 'discord', u.discord_id, u.email, :now + FROM "user" u + WHERE u.discord_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM oauth_account o + WHERE o.provider = 'discord' + AND (o.subject = u.discord_id OR o.user_id = u.id) + ) + """).bindparams(now=time.time())) + + +def downgrade() -> None: + if "oauth_account" in _tables(): + op.drop_index("idx_oauth_user", table_name="oauth_account") + op.drop_table("oauth_account") + cols = _columns("user") + if "oauth_only" in cols and "discord_only" not in cols: + op.alter_column("user", "oauth_only", new_column_name="discord_only") diff --git a/backend/notifications/discord_link.py b/backend/notifications/discord_link.py index 272d81c..dcb710e 100644 --- a/backend/notifications/discord_link.py +++ b/backend/notifications/discord_link.py @@ -1,243 +1,54 @@ -"""Discord as an identity: sign in with it, and connect it to an account. +"""Discord's legacy /discord/* auth routes, plus the DM opt-in toggle. -Two flows over one authorization-code grant, told apart by the signed `state`: +The OAuth flow itself moved to ``accounts/oauth.py`` when Google was added — this +module holds no auth logic, only the paths that have to keep answering: - link — /discord/link/start an already-signed-in user attaches a Discord - account to the session they're in - login — /discord/login/start an anonymous visitor authenticates, creating or - resolving a Thermograph account from the Discord - identity +* **/discord/link/callback is registered in Discord's developer portal.** Changing + it would need an operator to edit the portal before any Discord login worked, so + it stays exactly where it is and forwards to the shared handler. +* The other /discord/* routes are what the currently-deployed frontend calls. + Frontend and backend deploy independently (see the root CLAUDE.md), so a newer + backend must keep serving an older frontend. They can go once no deployed + frontend predates /oauth/*. -**Both come back to /discord/link/callback.** Discord only honours redirect URIs -pre-registered in the developer portal, so a second callback path would mean an -operator has to add one there before login could work anywhere. Sharing the -registered one keeps this deployable with no portal change; the flows separate on -the state's purpose, which is inside the HMAC and so can't be flipped by hand. - -`state` is signed with the app's auth secret (stdlib hmac, no new dependency) and -carries the purpose plus, for a link, the Thermograph user id — so a callback can't -be replayed, bound to a different account, or replayed *as the other flow*. - -How a login resolves to an account, in order: - - 1. A user already carrying this `discord_id` — the durable key, no email needed. - 2. Otherwise the Discord email, but **only if Discord reports it verified**. - That flag is the sole evidence the person owns the address; matching an - existing Thermograph account on an unverified one would hand that account to - whoever typed the address into Discord. Unverified is refused outright. - 3. No account for that email: create one. It has no password its owner has ever - seen, so it is flagged `discord_only` and unlink is refused (see `unlink`) — - otherwise unlinking would delete the only way back in. +``POST /discord/dm`` is not legacy and is not moving: it toggles alert delivery, +which is a notifications concern, not an authentication one. """ from __future__ import annotations -import base64 -import hashlib -import hmac -import json -import os -import time -import urllib.parse - -import httpx -from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import RedirectResponse +from fastapi import APIRouter, Depends, Request from pydantic import BaseModel -from sqlalchemy import select, update +from sqlalchemy import update from sqlalchemy.ext.asyncio import AsyncSession +from accounts import oauth from accounts.db import get_async_session from accounts.models import User from accounts.users import ( - SECRET, - attach_session, current_active_user, current_user_optional, get_access_token_db, get_user_manager, ) -from core import audit router = APIRouter(tags=["discord"]) -CLIENT_ID = os.environ.get("THERMOGRAPH_DISCORD_APP_ID", "").strip() -CLIENT_SECRET = os.environ.get("THERMOGRAPH_DISCORD_CLIENT_SECRET", "").strip() -BASE = os.environ.get("THERMOGRAPH_BASE", "/thermograph").rstrip("/") -_AUTHORIZE = "https://discord.com/api/oauth2/authorize" -_TOKEN = "https://discord.com/api/oauth2/token" -_ME = "https://discord.com/api/users/@me" - -# Linking only needs the account id. Signing in also needs the address to match or -# create a Thermograph account by, which is a separate scope Discord prompts for. -_SCOPE_LINK = "identify" -_SCOPE_LOGIN = "identify email" - -# How long a start->callback round trip may take before the signed state expires. -_STATE_TTL = 600 -_TIMEOUT = httpx.Timeout(10.0) - - -def enabled() -> bool: - return bool(CLIENT_ID and CLIENT_SECRET) - - -# --- signed state ------------------------------------------------------------ -def _b64(raw: bytes) -> str: - return base64.urlsafe_b64encode(raw).decode().rstrip("=") - - -def _unb64(s: str) -> bytes: - return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4)) - - -def _sign_state(uid: str | None, purpose: str = "link") -> str: - payload = _b64(json.dumps( - {"u": uid or "", "t": int(time.time()), "p": purpose} - ).encode()) - mac = hmac.new(SECRET.encode(), payload.encode(), hashlib.sha256).hexdigest()[:32] - return f"{payload}.{mac}" - - -def _verify_state(state: str, purpose: str = "link") -> str | None: - """The Thermograph user id this state was minted for, or None if it's missing, - tampered, expired, or was minted for a *different* purpose. - - A login state has no user id yet, so it verifies as ``""`` — falsy, but - distinct from the None that means "reject". Callers must test against None, - not truthiness, or a valid login state reads as a failure. - """ - try: - payload, mac = state.split(".", 1) - except (ValueError, AttributeError): - return None - expect = hmac.new(SECRET.encode(), payload.encode(), hashlib.sha256).hexdigest()[:32] - if not hmac.compare_digest(mac, expect): - return None - try: - data = json.loads(_unb64(payload)) - except (ValueError, json.JSONDecodeError): - return None - if time.time() - float(data.get("t", 0)) > _STATE_TTL: - return None - # States minted before the purpose field existed are links; they age out in - # _STATE_TTL, so this fallback only spans a deploy. - if data.get("p", "link") != purpose: - return None - return data.get("u") or "" - - -def _redirect_uri(request: Request) -> str: - """The callback URL, matched to how the app is actually reached (scheme/host + - base path). Must equal the redirect registered in the Discord portal.""" - proto = request.headers.get("x-forwarded-proto") or request.url.scheme - host = request.headers.get("host") or request.url.netloc - return f"{proto}://{host}{BASE}/api/v2/discord/link/callback" - - -def _authorize_redirect(request: Request, scope: str, state: str) -> RedirectResponse: - params = { - "client_id": CLIENT_ID, - "redirect_uri": _redirect_uri(request), - "response_type": "code", - "scope": scope, - "state": state, - "prompt": "consent", - } - return RedirectResponse(url=f"{_AUTHORIZE}?{urllib.parse.urlencode(params)}", - status_code=303) - - -def _account_redirect(status: str) -> RedirectResponse: - # Back to the alerts page, which surfaces the outcome as a toast; 303 so the - # browser issues a GET after the callback. The frontend serves this page at - # /alerts — the route is named for the feature, not for subscriptions.html. - return RedirectResponse(url=f"{BASE}/alerts?discord={status}", status_code=303) - - -# --- Discord API -------------------------------------------------------------- -async def _fetch_profile(request: Request, code: str) -> dict | None: - """Trade the authorization code for the Discord user object, or None if any - step fails. Callers treat None as a graceful error, never a crash.""" - try: - async with httpx.AsyncClient(timeout=_TIMEOUT) as client: - tok = await client.post(_TOKEN, data={ - "client_id": CLIENT_ID, - "client_secret": CLIENT_SECRET, - "grant_type": "authorization_code", - "code": code, - "redirect_uri": _redirect_uri(request), - }, headers={"Content-Type": "application/x-www-form-urlencoded"}) - if tok.status_code >= 300: - return None - access = tok.json().get("access_token") - me = await client.get(_ME, headers={"Authorization": f"Bearer {access}"}) - if me.status_code >= 300: - return None - profile = me.json() - except Exception: # noqa: BLE001 - any network/parse failure is a graceful error - return None - return profile if isinstance(profile, dict) else None - - -async def _user_by_discord_id(session: AsyncSession, discord_id: str) -> User | None: - return ( - await session.execute(select(User).where(User.discord_id == discord_id)) - ).scalar_one_or_none() - - -async def _create_from_discord(user_manager, email: str, discord_id: str) -> User: - """Create an account for a Discord identity that matched none of ours. - - Deliberately goes around ``UserManager.create()``: there is no password to - validate, and its ``on_after_register`` mails a confirmation link for an - address Discord has already verified. The generated password exists only - because ``hashed_password`` is NOT NULL — nobody ever sees it, which is what - ``discord_only`` records. - """ - password = user_manager.password_helper.generate() - user = await user_manager.user_db.create({ - "email": email, - "hashed_password": user_manager.password_helper.hash(password), - "is_active": True, - "is_superuser": False, - "is_verified": True, - "discord_id": discord_id, - "discord_dm": True, - "discord_only": True, - }) - audit.log_activity("auth.register", {"user_id": str(user.id), "via": "discord"}) - return user - - -# --- routes ------------------------------------------------------------------ @router.get("/discord/config") async def discord_config(): - """Whether Discord is configured on this server. The account menu and the sign-in - modal use it to offer Discord only when it will actually work — so an - unconfigured server surfaces no dead-end Discord UI, and enabling it later - (setting the OAuth env vars) makes both entries appear on their own.""" - return {"enabled": enabled()} + """Superseded by /oauth/config, which reports every provider. Kept because the + deployed frontend asks this one.""" + return {"enabled": oauth.enabled("discord")} @router.get("/discord/link/start") async def link_start(request: Request, user=Depends(current_active_user)): - if not enabled(): - return _account_redirect("unavailable") - return _authorize_redirect(request, _SCOPE_LINK, _sign_state(str(user.id), "link")) + return await oauth.link_start("discord", request, user) @router.get("/discord/login/start") async def login_start(request: Request, user=Depends(current_user_optional)): - """Sign in with Discord. Unauthenticated by design — this is how you get a - session, not something you do with one.""" - if not enabled(): - return _account_redirect("unavailable") - # Someone already signed in who lands here wants to connect, not to be - # switched into whichever account the Discord identity resolves to. - if user is not None: - return _authorize_redirect(request, _SCOPE_LINK, _sign_state(str(user.id), "link")) - return _authorize_redirect(request, _SCOPE_LOGIN, _sign_state(None, "login")) + return await oauth.login_start("discord", request, user) @router.get("/discord/link/callback") @@ -248,103 +59,9 @@ async def link_callback( user_manager=Depends(get_user_manager), access_token_db=Depends(get_access_token_db), ): - """The single redirect target both flows return to (see the module docstring).""" - if not enabled(): - return _account_redirect("unavailable") - # User declined on Discord's screen, or the state doesn't check out. - code = request.query_params.get("code") - state = request.query_params.get("state", "") - if not code: - return _account_redirect("cancelled") - - # Which flow minted this state? The purpose is inside the HMAC, so a state - # can only verify under the one it was signed for. - uid = _verify_state(state, "link") - if uid is not None: - return await _finish_link(request, code, uid, user, session) - if _verify_state(state, "login") is not None: - return await _finish_login(request, code, session, user_manager, access_token_db) - return _account_redirect("error") - - -async def _finish_link(request: Request, code: str, uid: str, user, - session: AsyncSession) -> RedirectResponse: - # Linking always acts on the person actually logged in, and only on the one - # the state was minted for. - if user is None or str(user.id) != uid: - return _account_redirect("error") - profile = await _fetch_profile(request, code) - if profile is None: - return _account_redirect("error") - discord_id = str(profile.get("id") or "") - if not discord_id: - return _account_redirect("error") - # discord_id is UNIQUE, so without this check the UPDATE would raise an - # IntegrityError and 500 instead of explaining itself. - owner = await _user_by_discord_id(session, discord_id) - if owner is not None and owner.id != user.id: - return _account_redirect("taken") - # Linking is an active opt-in, so turn DM alerts on; the user can mute them - # (POST /discord/dm) while staying linked. - await session.execute( - update(User).where(User.id == user.id).values(discord_id=discord_id, discord_dm=True) - ) - await session.commit() - return _account_redirect("linked") - - -async def _finish_login(request: Request, code: str, session: AsyncSession, - user_manager, access_token_db) -> RedirectResponse: - """Resolve the Discord identity to an account and sign in as it. - - Deliberately ignores any session already present. A login state carries no - user id — it can't, there is no user yet — so it is replayable against whoever - happens to be signed in. Treating it as a link would therefore be a forced- - linking takeover: an attacker who gets a victim to open this callback with a - code from the *attacker's* Discord would attach their identity to the victim's - account, then sign in as them. Only /discord/link/start, whose state is bound - to a specific user id, may link. The worst a replayed login state can do is - sign someone into the attacker's own account. - """ - profile = await _fetch_profile(request, code) - if profile is None: - return _account_redirect("error") - discord_id = str(profile.get("id") or "") - if not discord_id: - return _account_redirect("error") - - created = False - user = await _user_by_discord_id(session, discord_id) - if user is None: - # No account carries this Discord id, so fall back to the email — the only - # other identity Discord gives us, and only when Discord vouches for it. - email = str(profile.get("email") or "").strip().lower() - if not email: - return _account_redirect("noemail") - if profile.get("verified") is not True: - return _account_redirect("unverified") - user = await user_manager.user_db.get_by_email(email) - if user is None: - user = await _create_from_discord(user_manager, email, discord_id) - created = True - elif user.discord_id: - # That address belongs to an account already tied to a different - # Discord identity; connecting would silently move it. - return _account_redirect("mismatch") - elif not user.is_active: - # Checked before the link, not just before the session: a deactivated - # account must not quietly acquire a Discord identity on the way out. - return _account_redirect("inactive") - else: - await session.execute( - update(User).where(User.id == user.id) - .values(discord_id=discord_id, discord_dm=True) - ) - await session.commit() - if not user.is_active: - return _account_redirect("inactive") - response = _account_redirect("created" if created else "signedin") - return await attach_session(response, user, user_manager, access_token_db, request) + """The URL registered in Discord's portal. Both Discord flows land here.""" + return await oauth._callback("discord", request, user, session, user_manager, + access_token_db) @router.post("/discord/unlink", status_code=204) @@ -352,18 +69,7 @@ async def unlink( user=Depends(current_active_user), session: AsyncSession = Depends(get_async_session), ): - if user.discord_only: - # The account has no password anyone has ever seen, and no reset-password - # route is mounted to set one — unlinking here is unrecoverable. - raise HTTPException( - status_code=409, - detail="This account was created with Discord and has no password, " - "so unlinking would leave no way to sign in.", - ) - await session.execute( - update(User).where(User.id == user.id).values(discord_id=None, discord_dm=False) - ) - await session.commit() + await oauth._unlink("discord", user, session) class DmToggle(BaseModel): diff --git a/backend/tests/accounts/test_oauth.py b/backend/tests/accounts/test_oauth.py new file mode 100644 index 0000000..0b736d4 --- /dev/null +++ b/backend/tests/accounts/test_oauth.py @@ -0,0 +1,351 @@ +"""Sign in with an external provider: account resolution, session issuance, and the +guards that keep the login flow from becoming an account-takeover path. + +Parametrised over every configured provider, so Discord and Google are held to the +same rules rather than one being tested and the other assumed. Provider HTTP is +mocked; reuses the throwaway accounts DB from conftest. +""" +import pytest +from fastapi.testclient import TestClient + +from web import app as appmod +from accounts import db, oauth + +V2 = "/thermograph/api/v2" +PW = "supersecret123" + +# (provider, callback path, profile builder). Discord's callback is grandfathered +# to the URL registered in its developer portal; anything newer uses the canonical +# /oauth/{provider}/callback. +PROVIDERS = [ + ("discord", f"{V2}/discord/link/callback", + lambda sub, email, ver: {"id": sub, "email": email, "verified": ver}), + ("google", f"{V2}/oauth/google/callback", + lambda sub, email, ver: {"sub": sub, "email": email, "email_verified": ver}), +] +IDS = [p[0] for p in PROVIDERS] + + +@pytest.fixture(scope="module", autouse=True) +def _tables(): + db.Base.metadata.create_all(db.sync_engine) + + +@pytest.fixture(autouse=True) +def _configured(monkeypatch): + for name in oauth.PROVIDERS: + monkeypatch.setitem(oauth.CREDENTIALS, name, (f"{name}-id", f"{name}-secret")) + + +@pytest.fixture(params=PROVIDERS, ids=IDS) +def prov(request): + """A provider under test: .name, .callback, .profile(sub, email, verified).""" + name, callback, builder = request.param + + class P: + pass + p = P() + p.name, p.callback, p.profile = name, callback, builder + return p + + +class _Resp: + def __init__(self, code, data): self.status_code = code; self._data = data + def json(self): return self._data + + +def _mock(monkeypatch, profile): + """Stand in for httpx.AsyncClient: token exchange, then userinfo -> profile.""" + class _Client: + def __init__(self, *a, **k): pass + async def __aenter__(self): return self + async def __aexit__(self, *a): return False + async def post(self, url, **k): return _Resp(200, {"access_token": "tok"}) + async def get(self, url, **k): return _Resp(200, profile) + monkeypatch.setattr(oauth.httpx, "AsyncClient", _Client) + + +def _register(client, email): + assert client.post(f"{V2}/auth/register", + json={"email": email, "password": PW}).status_code in (201, 400) + + +def _login(client, email): + _register(client, email) + assert client.post(f"{V2}/auth/login", + data={"username": email, "password": PW}).status_code == 204 + + +def _cb(client, prov, state): + return client.get(f"{prov.callback}?code=abc&state={state}", follow_redirects=False) + + +def _me(client): + return client.get(f"{V2}/users/me").json() + + +# --- config ------------------------------------------------------------------- + +def test_config_lists_every_provider_and_its_enabled_state(monkeypatch): + c = TestClient(appmod.app) + monkeypatch.setitem(oauth.CREDENTIALS, "google", ("", "")) + by_name = {p["name"]: p for p in c.get(f"{V2}/oauth/config").json()["providers"]} + assert by_name["discord"]["enabled"] is True + assert by_name["google"]["enabled"] is False # half-configured is inert + assert by_name["google"]["label"] == "Google" + + +def test_unknown_provider_is_404(): + c = TestClient(appmod.app) + assert c.get(f"{V2}/oauth/gitlab/login/start", + follow_redirects=False).status_code == 404 + + +# --- start -------------------------------------------------------------------- + +def test_login_start_needs_no_session_and_asks_for_an_email_scope(prov): + c = TestClient(appmod.app) + r = c.get(f"{V2}/oauth/{prov.name}/login/start", follow_redirects=False) + assert r.status_code == 303 + loc = r.headers["location"] + assert loc.startswith(oauth.PROVIDERS[prov.name].authorize_url) + # Whatever the provider calls it, the login scope must be able to yield an + # address — without one there is no account to resolve or create. + assert "email" in loc and "state=" in loc + + +def test_login_start_while_signed_in_links_instead_of_switching_account(prov): + c = TestClient(appmod.app) + _login(c, f"already-in-{prov.name}@example.com") + r = c.get(f"{V2}/oauth/{prov.name}/login/start", follow_redirects=False) + state = r.headers["location"].split("state=")[1].split("&")[0] + assert oauth._verify_state(state, "link", prov.name) == _me(c)["id"] + + +def test_start_when_unconfigured_bounces_back(prov, monkeypatch): + monkeypatch.setitem(oauth.CREDENTIALS, prov.name, ("", "")) + c = TestClient(appmod.app) + r = c.get(f"{V2}/oauth/{prov.name}/login/start", follow_redirects=False) + assert r.status_code == 303 and "oauth=unavailable" in r.headers["location"] + + +# --- signed state ------------------------------------------------------------- + +def test_state_purposes_and_providers_do_not_cross_over(): + login = oauth._sign_state(None, "login", "google") + link = oauth._sign_state("user-123", "link", "google") + # A login state carries no user id but verifies as "" — distinct from reject. + assert oauth._verify_state(login, "login", "google") == "" + assert oauth._verify_state(login, "link", "google") is None + assert oauth._verify_state(link, "link", "google") == "user-123" + assert oauth._verify_state(link, "login", "google") is None + # And a state minted for one provider is worthless at another's callback. + assert oauth._verify_state(link, "link", "discord") is None + assert oauth._verify_state(login, "login", "discord") is None + + +def test_state_rejects_tampering_and_expiry(monkeypatch): + s = oauth._sign_state("u", "link", "google") + payload, mac = s.split(".", 1) + assert oauth._verify_state(f"{payload}.{'0' * len(mac)}", "link", "google") is None + assert oauth._verify_state(payload, "link", "google") is None + assert oauth._verify_state("garbage", "link", "google") is None + monkeypatch.setattr(oauth, "_STATE_TTL", -1) + assert oauth._verify_state(oauth._sign_state("u", "link", "google"), + "link", "google") is None + + +def test_state_is_secret_dependent(monkeypatch): + s = oauth._sign_state("u", "link", "google") + monkeypatch.setattr(oauth, "SECRET", oauth.SECRET + "-different") + assert oauth._verify_state(s, "link", "google") is None + + +def test_a_login_state_cannot_link_the_signed_in_account(prov, monkeypatch): + """Forced-linking guard. A login state carries no user id, so it is replayable + against whoever is signed in; if the callback treated it as a link, an attacker + could attach their own identity to a victim's account and then sign in as them. + It must complete as a plain login instead.""" + attacker = f"attacker-{prov.name}@example.com" + _mock(monkeypatch, prov.profile(f"{prov.name}-cross", attacker, True)) + c = TestClient(appmod.app) + _login(c, f"crossover-{prov.name}@example.com") + r = _cb(c, prov, oauth._sign_state(_me(c)["id"], "login", prov.name)) + assert "oauth=created" in r.headers["location"] + assert _me(c)["email"] == attacker + # The account that had been signed in is untouched. + victim = TestClient(appmod.app) + _login(victim, f"crossover-{prov.name}@example.com") + assert _me(victim)["oauth_providers"] == [] + + +# --- login resolves an account ------------------------------------------------ + +def test_login_creates_an_account_and_signs_in(prov, monkeypatch): + # Mixed case on purpose: the address must be normalised before it is matched or + # stored, or the same person gets two accounts depending on how they typed it. + _mock(monkeypatch, prov.profile(f"{prov.name}-new", + f"New.User.{prov.name}@Example.com", True)) + c = TestClient(appmod.app) + r = _cb(c, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=created" in r.headers["location"] + me = _me(c) + assert me["email"] == f"new.user.{prov.name}@example.com" # normalised + assert me["is_verified"] is True # the provider vouched + assert me["oauth_only"] is True + assert me["oauth_providers"] == [prov.name] + + +def test_login_recognises_a_previously_linked_account(prov, monkeypatch): + """The subject is the durable key: it resolves the account with no email, which + is what keeps a login working after someone changes their address upstream.""" + email = f"link-me-{prov.name}@example.com" + _mock(monkeypatch, prov.profile(f"{prov.name}-ret", email, True)) + linker = TestClient(appmod.app) + _login(linker, email) + uid = _me(linker)["id"] + assert "oauth=linked" in _cb( + linker, prov, oauth._sign_state(uid, "link", prov.name)).headers["location"] + # A fresh client, no session, and the provider now returns no email at all. + _mock(monkeypatch, prov.profile(f"{prov.name}-ret", "", False)) + c = TestClient(appmod.app) + r = _cb(c, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=signedin" in r.headers["location"] + assert _me(c)["id"] == uid + + +def test_login_connects_a_verified_email_to_an_existing_account(prov, monkeypatch): + """The 'connect the account' path: an existing password account picks up the + identity and is signed in.""" + email = f"existing-{prov.name}@example.com" + c = TestClient(appmod.app) + _register(c, email) + _mock(monkeypatch, prov.profile(f"{prov.name}-exist", email, True)) + fresh = TestClient(appmod.app) + r = _cb(fresh, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=signedin" in r.headers["location"] + me = _me(fresh) + assert me["email"] == email and me["oauth_providers"] == [prov.name] + # It kept its password, so it is not OAuth-only and may unlink. + assert me["oauth_only"] is False + + +def test_login_refuses_an_unverified_email(prov, monkeypatch): + """The provider's verified flag is the only evidence the person owns the + address. Without it, this would hand over any account whose email was typed in.""" + email = f"victim-{prov.name}@example.com" + c = TestClient(appmod.app) + _register(c, email) + _mock(monkeypatch, prov.profile(f"{prov.name}-attacker", email, False)) + fresh = TestClient(appmod.app) + r = _cb(fresh, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=unverified" in r.headers["location"] + assert fresh.get(f"{V2}/users/me").status_code == 401 # no session issued + _login(c, email) + assert _me(c)["oauth_providers"] == [] # target untouched + + +def test_login_without_an_email_cannot_create_an_account(prov, monkeypatch): + _mock(monkeypatch, prov.profile(f"{prov.name}-noemail", "", True)) + c = TestClient(appmod.app) + r = _cb(c, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=noemail" in r.headers["location"] + assert c.get(f"{V2}/users/me").status_code == 401 + + +def test_login_will_not_move_an_account_between_identities(prov, monkeypatch): + email = f"owned-{prov.name}@example.com" + owner = TestClient(appmod.app) + _login(owner, email) + uid = _me(owner)["id"] + _mock(monkeypatch, prov.profile(f"{prov.name}-owner", email, True)) + assert "oauth=linked" in _cb( + owner, prov, oauth._sign_state(uid, "link", prov.name)).headers["location"] + # A second provider account claiming the same address. + _mock(monkeypatch, prov.profile(f"{prov.name}-interloper", email, True)) + fresh = TestClient(appmod.app) + r = _cb(fresh, prov, oauth._sign_state(None, "login", prov.name)) + assert r.status_code == 303 and "oauth=mismatch" in r.headers["location"] + assert fresh.get(f"{V2}/users/me").status_code == 401 + + +def test_linking_an_identity_someone_else_owns_is_refused(prov, monkeypatch): + first = TestClient(appmod.app) + _login(first, f"first-owner-{prov.name}@example.com") + _mock(monkeypatch, prov.profile(f"{prov.name}-contested", + f"first-owner-{prov.name}@example.com", True)) + assert "oauth=linked" in _cb( + first, prov, + oauth._sign_state(_me(first)["id"], "link", prov.name)).headers["location"] + second = TestClient(appmod.app) + _login(second, f"second-owner-{prov.name}@example.com") + r = _cb(second, prov, oauth._sign_state(_me(second)["id"], "link", prov.name)) + assert r.status_code == 303 and "oauth=taken" in r.headers["location"] + assert _me(second)["oauth_providers"] == [] + + +def test_relinking_the_same_identity_is_idempotent(prov, monkeypatch): + email = f"relink-{prov.name}@example.com" + c = TestClient(appmod.app) + _login(c, email) + _mock(monkeypatch, prov.profile(f"{prov.name}-relink", email, True)) + uid = _me(c)["id"] + for _ in range(2): + r = _cb(c, prov, oauth._sign_state(uid, "link", prov.name)) + assert "oauth=linked" in r.headers["location"] + assert _me(c)["oauth_providers"] == [prov.name] # not duplicated + + +# --- unlink guards ------------------------------------------------------------ + +def test_last_provider_cannot_be_unlinked_into_a_lockout(prov, monkeypatch): + _mock(monkeypatch, prov.profile(f"{prov.name}-onlyway", + f"onlyway-{prov.name}@example.com", True)) + c = TestClient(appmod.app) + assert "oauth=created" in _cb( + c, prov, oauth._sign_state(None, "login", prov.name)).headers["location"] + r = c.post(f"{V2}/oauth/{prov.name}/unlink") + assert r.status_code == 409 and "no password" in r.json()["detail"] + assert _me(c)["oauth_providers"] == [prov.name] + + +def test_an_oauth_only_account_may_unlink_once_a_second_provider_is_linked(monkeypatch): + """The lockout rule is about the *last* credential, not about Discord. With two + providers linked, dropping one still leaves a way in.""" + _mock(monkeypatch, {"sub": "g-two", "email": "two@example.com", + "email_verified": True}) + c = TestClient(appmod.app) + assert "oauth=created" in _cb( + c, PROV_GOOGLE, oauth._sign_state(None, "login", "google")).headers["location"] + uid = _me(c)["id"] + # Now connect Discord to the same, still password-less, account. + _mock(monkeypatch, {"id": "d-two", "email": "two@example.com", "verified": True}) + assert "oauth=linked" in _cb( + c, PROV_DISCORD, oauth._sign_state(uid, "link", "discord")).headers["location"] + assert _me(c)["oauth_providers"] == ["discord", "google"] + # Either one may now go. + assert c.post(f"{V2}/oauth/google/unlink").status_code == 204 + assert _me(c)["oauth_providers"] == ["discord"] + # But the survivor is once again the only way in. + assert c.post(f"{V2}/oauth/discord/unlink").status_code == 409 + + +def test_unlink_requires_auth(prov): + c = TestClient(appmod.app) + assert c.post(f"{V2}/oauth/{prov.name}/unlink").status_code == 401 + + +def test_link_start_requires_auth(prov): + c = TestClient(appmod.app) + assert c.get(f"{V2}/oauth/{prov.name}/link/start", + follow_redirects=False).status_code == 401 + + +# Bare provider handles for the cross-provider test above, which needs both at once. +class _P: + def __init__(self, name, callback): + self.name, self.callback = name, callback + + +PROV_GOOGLE = _P("google", f"{V2}/oauth/google/callback") +PROV_DISCORD = _P("discord", f"{V2}/discord/link/callback") diff --git a/backend/tests/notifications/test_discord_dm.py b/backend/tests/notifications/test_discord_dm.py index aa9cc56..e0fc6d0 100644 --- a/backend/tests/notifications/test_discord_dm.py +++ b/backend/tests/notifications/test_discord_dm.py @@ -7,8 +7,8 @@ from fastapi.testclient import TestClient from web import app as appmod from accounts import db +from accounts import oauth from notifications import discord -from notifications import discord_link as dl from notifications import notify V2 = "/thermograph/api/v2" @@ -161,14 +161,15 @@ class _MockClient: def test_linking_opts_in_and_toggle_flips(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app") - monkeypatch.setattr(dl, "CLIENT_SECRET", "sec") - monkeypatch.setattr(dl.httpx, "AsyncClient", _MockClient) + monkeypatch.setitem(oauth.CREDENTIALS, "discord", ("app", "sec")) + monkeypatch.setattr(oauth.httpx, "AsyncClient", _MockClient) c = TestClient(appmod.app) _login(c, "dm-toggle@example.com") uid = c.get(f"{V2}/users/me").json()["id"] - # Link -> opted in by default. - r = c.get(f"{V2}/discord/link/callback?code=x&state={dl._sign_state(uid)}", follow_redirects=False) + # Link -> opted in by default. discord_id is a delivery address, so the link + # has to write it as well as the oauth_account row. + state = oauth._sign_state(uid, "link", "discord") + r = c.get(f"{V2}/discord/link/callback?code=x&state={state}", follow_redirects=False) assert r.status_code == 303 me = c.get(f"{V2}/users/me").json() assert me["discord_id"] == "discord-777" and me["discord_dm"] is True diff --git a/backend/tests/notifications/test_discord_link.py b/backend/tests/notifications/test_discord_link.py index ffaade0..fdf56c8 100644 --- a/backend/tests/notifications/test_discord_link.py +++ b/backend/tests/notifications/test_discord_link.py @@ -1,11 +1,20 @@ -"""Discord account linking: signed-state integrity, OAuth callback (Discord HTTP -mocked), unlink, and auth gating. Reuses the throwaway accounts DB from conftest.""" +"""The legacy /discord/* routes. + +These are not duplicate coverage of tests/accounts/test_oauth.py — they pin the two +compatibility promises that outlive the refactor: + +* **/discord/link/callback is registered in Discord's developer portal.** If it + stops answering, every Discord login breaks until an operator edits the portal. +* The other /discord/* paths are what the deployed frontend calls, and frontend and + backend deploy independently, so a newer backend must keep serving an older one. + +The flow itself lives in accounts/oauth.py and is tested there. +""" import pytest from fastapi.testclient import TestClient from web import app as appmod -from accounts import db -from notifications import discord_link as dl +from accounts import db, oauth V2 = "/thermograph/api/v2" PW = "supersecret123" @@ -16,72 +25,10 @@ def _tables(): db.Base.metadata.create_all(db.sync_engine) -def _login(client, email): - r = client.post(f"{V2}/auth/register", json={"email": email, "password": PW}) - assert r.status_code in (201, 400) - assert client.post(f"{V2}/auth/login", data={"username": email, "password": PW}).status_code == 204 - - -# --- signed state ------------------------------------------------------------ - -def test_state_round_trips_and_rejects_tampering(): - s = dl._sign_state("user-123") - assert dl._verify_state(s) == "user-123" - payload, mac = s.split(".", 1) - assert dl._verify_state(f"{payload}.{'0' * len(mac)}") is None # bad mac - assert dl._verify_state(payload) is None # no mac - assert dl._verify_state("garbage") is None - - -def test_state_expires(monkeypatch): - monkeypatch.setattr(dl, "_STATE_TTL", -1) # already expired - assert dl._verify_state(dl._sign_state("u")) is None - - -def test_state_is_secret_dependent(monkeypatch): - s = dl._sign_state("u") - monkeypatch.setattr(dl, "SECRET", dl.SECRET + "-different") - assert dl._verify_state(s) is None # signed under the old secret - - -# --- routes ------------------------------------------------------------------ - -def test_config_reports_enabled_state(monkeypatch): - c = TestClient(appmod.app) - monkeypatch.setattr(dl, "CLIENT_ID", "") - monkeypatch.setattr(dl, "CLIENT_SECRET", "") - r = c.get(f"{V2}/discord/config") # no auth required - assert r.status_code == 200 and r.json() == {"enabled": False} - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") - assert c.get(f"{V2}/discord/config").json() == {"enabled": True} - - -def test_link_requires_auth(): - c = TestClient(appmod.app) - assert c.get(f"{V2}/discord/link/start", follow_redirects=False).status_code == 401 - assert c.post(f"{V2}/discord/unlink").status_code == 401 - - -def test_start_redirects_to_discord(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") - c = TestClient(appmod.app) - _login(c, "start@example.com") - r = c.get(f"{V2}/discord/link/start", follow_redirects=False) - assert r.status_code == 303 - loc = r.headers["location"] - assert loc.startswith("https://discord.com/api/oauth2/authorize") - assert "client_id=app123" in loc and "scope=identify" in loc and "state=" in loc - - -def test_start_when_unconfigured_bounces_back(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "") - monkeypatch.setattr(dl, "CLIENT_SECRET", "") - c = TestClient(appmod.app) - _login(c, "noconf@example.com") - r = c.get(f"{V2}/discord/link/start", follow_redirects=False) - assert r.status_code == 303 and "discord=unavailable" in r.headers["location"] +@pytest.fixture(autouse=True) +def _configured(monkeypatch): + monkeypatch.setitem(oauth.CREDENTIALS, "discord", ("app123", "secret456")) + monkeypatch.setattr(oauth.httpx, "AsyncClient", _MockClient) class _Resp: @@ -90,47 +37,88 @@ class _Resp: class _MockClient: - """Stands in for httpx.AsyncClient: token exchange then /users/@me.""" def __init__(self, *a, **k): pass async def __aenter__(self): return self async def __aexit__(self, *a): return False async def post(self, url, **k): return _Resp(200, {"access_token": "tok"}) - async def get(self, url, **k): return _Resp(200, {"id": "discord-999", "username": "someone"}) + async def get(self, url, **k): + return _Resp(200, {"id": "discord-999", "email": "legacy@example.com", + "verified": True}) -def test_callback_links_the_account(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") - monkeypatch.setattr(dl.httpx, "AsyncClient", _MockClient) +def _login(client, email): + assert client.post(f"{V2}/auth/register", + json={"email": email, "password": PW}).status_code in (201, 400) + assert client.post(f"{V2}/auth/login", + data={"username": email, "password": PW}).status_code == 204 + + +def test_legacy_config_still_reports_enabled_state(monkeypatch): c = TestClient(appmod.app) - _login(c, "callback@example.com") + monkeypatch.setitem(oauth.CREDENTIALS, "discord", ("", "")) + assert c.get(f"{V2}/discord/config").json() == {"enabled": False} + monkeypatch.setitem(oauth.CREDENTIALS, "discord", ("app123", "secret456")) + assert c.get(f"{V2}/discord/config").json() == {"enabled": True} + + +def test_legacy_link_start_still_requires_auth(): + c = TestClient(appmod.app) + assert c.get(f"{V2}/discord/link/start", follow_redirects=False).status_code == 401 + assert c.post(f"{V2}/discord/unlink").status_code == 401 + + +def test_legacy_start_redirects_to_discord(): + c = TestClient(appmod.app) + _login(c, "legacy-start@example.com") + r = c.get(f"{V2}/discord/link/start", follow_redirects=False) + assert r.status_code == 303 + loc = r.headers["location"] + assert loc.startswith("https://discord.com/api/oauth2/authorize") + assert "client_id=app123" in loc and "scope=identify" in loc and "state=" in loc + # The redirect_uri it asks Discord to call back on must be the registered one. + assert "discord%2Flink%2Fcallback" in loc + + +def test_the_registered_callback_url_still_completes_a_link(): + c = TestClient(appmod.app) + _login(c, "legacy-cb@example.com") uid = c.get(f"{V2}/users/me").json()["id"] - state = dl._sign_state(uid) - r = c.get(f"{V2}/discord/link/callback?code=abc&state={state}", follow_redirects=False) - assert r.status_code == 303 and "discord=linked" in r.headers["location"] - # /users/me now reports the linked id, and unlink clears it. - assert c.get(f"{V2}/users/me").json()["discord_id"] == "discord-999" - assert c.post(f"{V2}/discord/unlink").status_code == 204 - assert c.get(f"{V2}/users/me").json()["discord_id"] is None - - -def test_callback_rejects_a_forged_state(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") - monkeypatch.setattr(dl.httpx, "AsyncClient", _MockClient) - c = TestClient(appmod.app) - _login(c, "forged@example.com") - # State signed for a different user id must not link this session. - r = c.get(f"{V2}/discord/link/callback?code=abc&state={dl._sign_state('someone-else')}", + state = oauth._sign_state(uid, "link", "discord") + r = c.get(f"{V2}/discord/link/callback?code=abc&state={state}", follow_redirects=False) - assert r.status_code == 303 and "discord=error" in r.headers["location"] - assert c.get(f"{V2}/users/me").json()["discord_id"] is None + assert r.status_code == 303 and "discord=linked" in r.headers["location"] + me = c.get(f"{V2}/users/me").json() + assert me["discord_id"] == "discord-999" # the DM delivery address + assert me["oauth_providers"] == ["discord"] # and the identity row + # Legacy unlink clears both. + assert c.post(f"{V2}/discord/unlink").status_code == 204 + me = c.get(f"{V2}/users/me").json() + assert me["discord_id"] is None and me["oauth_providers"] == [] -def test_callback_without_code_is_cancelled(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") +def test_the_callback_still_emits_the_discord_query_param(): + """The deployed frontend reads ?discord=, not ?oauth=. Dropping it would + silently stop it showing any outcome at all.""" c = TestClient(appmod.app) - _login(c, "cancel@example.com") r = c.get(f"{V2}/discord/link/callback?error=access_denied", follow_redirects=False) - assert r.status_code == 303 and "discord=cancelled" in r.headers["location"] + loc = r.headers["location"] + assert "discord=cancelled" in loc and "oauth=cancelled" in loc + + +def test_users_me_still_carries_the_deprecated_discord_only_field(monkeypatch): + """Renamed to oauth_only, but the deployed frontend still reads the old name.""" + monkeypatch.setattr(oauth.httpx, "AsyncClient", _MockClient) + c = TestClient(appmod.app) + r = c.get(f"{V2}/discord/link/callback?code=abc" + f"&state={oauth._sign_state(None, 'login', 'discord')}", + follow_redirects=False) + assert "discord=created" in r.headers["location"] + me = c.get(f"{V2}/users/me").json() + assert me["oauth_only"] is True and me["discord_only"] is True + + +def test_legacy_login_start_is_still_mounted(): + c = TestClient(appmod.app) + r = c.get(f"{V2}/discord/login/start", follow_redirects=False) + assert r.status_code == 303 + assert r.headers["location"].startswith("https://discord.com/api/oauth2/authorize") diff --git a/backend/tests/notifications/test_discord_login.py b/backend/tests/notifications/test_discord_login.py deleted file mode 100644 index cc3d4f9..0000000 --- a/backend/tests/notifications/test_discord_login.py +++ /dev/null @@ -1,251 +0,0 @@ -"""Sign in with Discord: account resolution, session issuance, and the guards that -keep the login flow from becoming an account-takeover path. Discord HTTP is mocked; -reuses the throwaway accounts DB from conftest.""" -import pytest -from fastapi.testclient import TestClient - -from web import app as appmod -from accounts import db -from notifications import discord_link as dl - -V2 = "/thermograph/api/v2" -PW = "supersecret123" - - -@pytest.fixture(scope="module", autouse=True) -def _tables(): - db.Base.metadata.create_all(db.sync_engine) - - -@pytest.fixture(autouse=True) -def _configured(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "app123") - monkeypatch.setattr(dl, "CLIENT_SECRET", "secret456") - - -class _Resp: - def __init__(self, code, data): self.status_code = code; self._data = data - def json(self): return self._data - - -def _mock_discord(monkeypatch, profile): - """Stand in for httpx.AsyncClient: token exchange, then /users/@me -> profile.""" - class _Client: - def __init__(self, *a, **k): pass - async def __aenter__(self): return self - async def __aexit__(self, *a): return False - async def post(self, url, **k): return _Resp(200, {"access_token": "tok"}) - async def get(self, url, **k): return _Resp(200, profile) - monkeypatch.setattr(dl.httpx, "AsyncClient", _Client) - - -def _register(client, email): - r = client.post(f"{V2}/auth/register", json={"email": email, "password": PW}) - assert r.status_code in (201, 400) - - -def _login(client, email): - _register(client, email) - assert client.post(f"{V2}/auth/login", - data={"username": email, "password": PW}).status_code == 204 - - -def _callback(client, state): - return client.get(f"{V2}/discord/link/callback?code=abc&state={state}", - follow_redirects=False) - - -# --- start ------------------------------------------------------------------- - -def test_login_start_needs_no_session_and_asks_for_email(): - c = TestClient(appmod.app) - r = c.get(f"{V2}/discord/login/start", follow_redirects=False) - assert r.status_code == 303 - loc = r.headers["location"] - assert loc.startswith("https://discord.com/api/oauth2/authorize") - # The email scope is what makes account resolution possible at all. - assert "scope=identify+email" in loc and "state=" in loc - - -def test_login_start_while_signed_in_links_instead_of_switching_account(): - """Someone already signed in wants to connect Discord, not be switched into - whichever account the Discord identity happens to resolve to.""" - c = TestClient(appmod.app) - _login(c, "already-in@example.com") - r = c.get(f"{V2}/discord/login/start", follow_redirects=False) - assert r.status_code == 303 - loc = r.headers["location"] - assert "scope=identify&" in loc or loc.endswith("scope=identify") - state = loc.split("state=")[1].split("&")[0] - uid = c.get(f"{V2}/users/me").json()["id"] - assert dl._verify_state(state, "link") == uid - - -def test_login_start_when_unconfigured_bounces_back(monkeypatch): - monkeypatch.setattr(dl, "CLIENT_ID", "") - monkeypatch.setattr(dl, "CLIENT_SECRET", "") - c = TestClient(appmod.app) - r = c.get(f"{V2}/discord/login/start", follow_redirects=False) - assert r.status_code == 303 and "discord=unavailable" in r.headers["location"] - - -# --- state purposes are not interchangeable ---------------------------------- - -def test_state_purposes_do_not_cross_over(): - login_state = dl._sign_state(None, "login") - link_state = dl._sign_state("user-123", "link") - # A login state carries no user id, but verifies as "" — distinct from reject. - assert dl._verify_state(login_state, "login") == "" - assert dl._verify_state(login_state, "link") is None - assert dl._verify_state(link_state, "link") == "user-123" - assert dl._verify_state(link_state, "login") is None - - -def test_a_login_state_cannot_link_the_signed_in_account(monkeypatch): - """Forced-linking guard. A login state carries no user id, so it is replayable - against whoever is signed in; if the callback treated it as a link, an attacker - could attach their own Discord identity to a victim's account and then sign in - as them. It must complete as a plain login instead.""" - _mock_discord(monkeypatch, {"id": "d-crossover", "email": "attacker@example.com", - "verified": True}) - c = TestClient(appmod.app) - _login(c, "crossover@example.com") - # A state whose purpose says login, but naming the signed-in user — the shape - # a forced-link attempt would take. - r = _callback(c, dl._sign_state(c.get(f"{V2}/users/me").json()["id"], "login")) - # Resolved as a login into the Discord identity's own account, not as a link. - assert "discord=created" in r.headers["location"] - assert c.get(f"{V2}/users/me").json()["email"] == "attacker@example.com" - # The account that had been signed in is untouched. - victim = TestClient(appmod.app) - _login(victim, "crossover@example.com") - assert victim.get(f"{V2}/users/me").json()["discord_id"] is None - - -# --- login resolves an account ----------------------------------------------- - -def test_login_creates_an_account_and_signs_in(monkeypatch): - _mock_discord(monkeypatch, {"id": "d-new", "email": "New.User@Example.com", - "verified": True}) - c = TestClient(appmod.app) - r = _callback(c, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=created" in r.headers["location"] - me = c.get(f"{V2}/users/me") - assert me.status_code == 200 - body = me.json() - assert body["email"] == "new.user@example.com" # normalised - assert body["discord_id"] == "d-new" - assert body["is_verified"] is True # Discord vouched for it - assert body["discord_only"] is True - assert body["discord_dm"] is True - - -def test_login_recognises_a_previously_linked_account(monkeypatch): - """The discord_id is the durable key: it resolves the account with no email.""" - _mock_discord(monkeypatch, {"id": "d-returning", "email": "link-me@example.com", - "verified": True}) - linker = TestClient(appmod.app) - _login(linker, "link-me@example.com") - uid = linker.get(f"{V2}/users/me").json()["id"] - assert "discord=linked" in _callback(linker, dl._sign_state(uid, "link") - ).headers["location"] - # A fresh client, no session, and Discord now returns no email at all. - _mock_discord(monkeypatch, {"id": "d-returning"}) - c = TestClient(appmod.app) - r = _callback(c, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=signedin" in r.headers["location"] - assert c.get(f"{V2}/users/me").json()["id"] == uid - - -def test_login_connects_a_verified_email_to_an_existing_account(monkeypatch): - """This is the 'connect the account with that' path: an existing password - account picks up the Discord id and is signed in.""" - c = TestClient(appmod.app) - _register(c, "existing@example.com") - _mock_discord(monkeypatch, {"id": "d-existing", "email": "existing@example.com", - "verified": True}) - fresh = TestClient(appmod.app) - r = _callback(fresh, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=signedin" in r.headers["location"] - body = fresh.get(f"{V2}/users/me").json() - assert body["email"] == "existing@example.com" - assert body["discord_id"] == "d-existing" - # It kept its password, so it is not Discord-only and may unlink. - assert body["discord_only"] is False - - -def test_login_refuses_an_unverified_discord_email(monkeypatch): - """Discord's verified flag is the only evidence the person owns the address. - Without it, this would hand over any account whose email someone typed in.""" - c = TestClient(appmod.app) - _register(c, "victim@example.com") - _mock_discord(monkeypatch, {"id": "d-attacker", "email": "victim@example.com", - "verified": False}) - fresh = TestClient(appmod.app) - r = _callback(fresh, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=unverified" in r.headers["location"] - assert fresh.get(f"{V2}/users/me").status_code == 401 # no session issued - # And the targeted account is untouched. - _login(c, "victim@example.com") - assert c.get(f"{V2}/users/me").json()["discord_id"] is None - - -def test_login_without_an_email_cannot_create_an_account(monkeypatch): - _mock_discord(monkeypatch, {"id": "d-noemail"}) - c = TestClient(appmod.app) - r = _callback(c, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=noemail" in r.headers["location"] - assert c.get(f"{V2}/users/me").status_code == 401 - - -def test_login_will_not_move_an_account_between_discord_identities(monkeypatch): - """The email matches an account that is already linked to a different Discord - id — signing in would silently re-point it.""" - owner = TestClient(appmod.app) - _login(owner, "owned@example.com") - uid = owner.get(f"{V2}/users/me").json()["id"] - _mock_discord(monkeypatch, {"id": "d-owner", "email": "owned@example.com", - "verified": True}) - assert "discord=linked" in _callback(owner, dl._sign_state(uid, "link") - ).headers["location"] - # A second Discord account claiming the same address. - _mock_discord(monkeypatch, {"id": "d-interloper", "email": "owned@example.com", - "verified": True}) - fresh = TestClient(appmod.app) - r = _callback(fresh, dl._sign_state(None, "login")) - assert r.status_code == 303 and "discord=mismatch" in r.headers["location"] - assert fresh.get(f"{V2}/users/me").status_code == 401 - assert owner.get(f"{V2}/users/me").json()["discord_id"] == "d-owner" - - -# --- guards on the connected account ----------------------------------------- - -def test_linking_a_discord_account_someone_else_owns_is_refused(monkeypatch): - first = TestClient(appmod.app) - _login(first, "first-owner@example.com") - _mock_discord(monkeypatch, {"id": "d-contested", "email": "first-owner@example.com", - "verified": True}) - uid = first.get(f"{V2}/users/me").json()["id"] - assert "discord=linked" in _callback(first, dl._sign_state(uid, "link") - ).headers["location"] - # A different Thermograph account tries to claim the same Discord identity. - second = TestClient(appmod.app) - _login(second, "second-owner@example.com") - uid2 = second.get(f"{V2}/users/me").json()["id"] - r = _callback(second, dl._sign_state(uid2, "link")) - assert r.status_code == 303 and "discord=taken" in r.headers["location"] - assert second.get(f"{V2}/users/me").json()["discord_id"] is None - - -def test_a_discord_only_account_cannot_unlink_itself_into_a_lockout(monkeypatch): - _mock_discord(monkeypatch, {"id": "d-onlyway", "email": "onlyway@example.com", - "verified": True}) - c = TestClient(appmod.app) - assert "discord=created" in _callback(c, dl._sign_state(None, "login") - ).headers["location"] - r = c.post(f"{V2}/discord/unlink") - assert r.status_code == 409 - assert "no password" in r.json()["detail"] - assert c.get(f"{V2}/users/me").json()["discord_id"] == "d-onlyway" - # Muting DMs is still fine — that is not a lockout. - assert c.post(f"{V2}/discord/dm", json={"enabled": False}).status_code == 204 diff --git a/backend/web/app.py b/backend/web/app.py index cbec4ae..dd29000 100644 --- a/backend/web/app.py +++ b/backend/web/app.py @@ -25,7 +25,7 @@ from data import climate from notifications import digest from notifications import discord_interactions as discord_interactions_mod from notifications import discord_link -from accounts import db +from accounts import db, oauth from data import grid from core import metrics from notifications import notify @@ -988,7 +988,12 @@ app.include_router( ) # Subscriptions + notifications (authed, user-scoped). app.include_router(api_accounts.router, prefix=f"{BASE}/api/v2") -# Discord account linking (OAuth2, authed). +# Sign in with / connect an external provider (Discord, Google). +app.include_router(oauth.router, prefix=f"{BASE}/api/v2") +# Discord's legacy /discord/* paths: the callback URL registered in Discord's +# portal, plus what the currently-deployed frontend calls. Mounted AFTER the +# generic router so neither shadows the other — the paths are disjoint, and +# /oauth/{provider}/... would otherwise be a candidate match for /discord/.... app.include_router(discord_link.router, prefix=f"{BASE}/api/v2") diff --git a/frontend/static/account.js b/frontend/static/account.js index f3eb0c6..8b23790 100644 --- a/frontend/static/account.js +++ b/frontend/static/account.js @@ -10,8 +10,8 @@ // units.js is (imported by each page's entry module). let currentUser = null; // {id, email, display_name} or null -let discordEnabled = false; // is Discord configured on the server? -let discordChecked = false; // have we asked yet? (once per page load) +let providers = []; // [{name, label, enabled}] from /oauth/config +let providersChecked = false; // have we asked yet? (once per page load) const authCbs = []; // notified on login/logout so pages can re-gate // backend's own origin+base (e.g. "https://thermograph.org/thermograph", or @@ -92,16 +92,24 @@ async function refreshUser() { // Learn once whether Discord is configured, so we only offer "Link Discord" and // "Continue with Discord" when they'll actually work. Asked regardless of sign-in // state: the sign-in modal needs the answer precisely when nobody is signed in. - if (!discordChecked) { - discordChecked = true; + if (!providersChecked) { + providersChecked = true; try { - const r = await apiFetch(uv("discord/config")); - if (r.ok) discordEnabled = (await r.json()).enabled === true; - } catch (e) { /* leave it hidden */ } + const r = await apiFetch(uv("oauth/config")); + if (r.ok) providers = ((await r.json()).providers || []).filter((p) => p.enabled); + } catch (e) { /* leave them hidden */ } } return currentUser; } +// Providers the server can complete a flow with. Empty until refreshUser() has +// run, which is why the sign-in modal re-renders its provider row on every open +// rather than only when it is first built. +function enabledProviders() { return providers; } +function isLinked(name) { + return !!(currentUser && (currentUser.oauth_providers || []).includes(name)); +} + // --- auth calls -------------------------------------------------------------- async function login(email, password) { // fastapi-users login is an OAuth2 form: username=email, password. @@ -130,6 +138,23 @@ let modal = null, mode = "login"; // extra request and inherits currentColor. const DISCORD_IC = ``; +// Google's mark is four fixed brand colours, so unlike the others it does not take +// currentColor — hence the white button beneath it, which is also what Google's +// branding terms require. +const GOOGLE_IC = ``; + +const PROVIDER_IC = { discord: DISCORD_IC, google: GOOGLE_IC }; + +// The provider buttons under the sign-in form. Rebuilt on every open because +// `providers` is populated asynchronously and may still have been empty when the +// modal was first constructed. +function providerButtonsHtml() { + return enabledProviders().map((p) => ` + + ${PROVIDER_IC[p.name] || ""}Continue with ${escapeHtml(p.label)} + `).join(""); +} + function buildModal() { modal = document.createElement("div"); modal.className = "mp-overlay acct-overlay"; @@ -151,9 +176,7 @@ function buildModal() {

@@ -183,9 +206,11 @@ function setMode(m) { modal.querySelector(".acct-switch").innerHTML = isLogin ? 'Need an account? ' : 'Already have an account? '; - // One label for both modes: Discord signs you in or creates the account, + // One label for both modes: the provider signs you in or creates the account, // whichever applies, so making the user pick first would be a false choice. - modal.querySelector(".acct-alt").hidden = !discordEnabled; + const enabled = enabledProviders(); + modal.querySelector(".acct-oauth-row").innerHTML = providerButtonsHtml(); + modal.querySelector(".acct-alt").hidden = enabled.length === 0; showError(""); } @@ -341,6 +366,30 @@ const USER_IC = ` { + if (!isLinked(p.name)) { + return `` + + `Link ${escapeHtml(p.label)}`; + } + const dm = p.name === "discord" + ? `` + : ""; + // With no password and nothing else linked, this is the only way back in and + // the server refuses to unlink it — so don't offer a button that can't work. + const stuck = currentUser.oauth_only && linkedCount <= 1; + return dm + (stuck + ? `Signed in with ${escapeHtml(p.label)}` + : ``); + }).join(""); +} + function ensureAcctEl() { const brand = document.querySelector(".brand"); if (!brand) return null; @@ -382,14 +431,7 @@ function renderHeader() { `; @@ -417,22 +459,25 @@ function renderHeader() { } }); el.querySelector(".acct-signout").addEventListener("click", logout); - const unlinkBtn = el.querySelector(".acct-discord-unlink"); - if (unlinkBtn) unlinkBtn.addEventListener("click", async () => { - unlinkBtn.disabled = true; - try { - const res = await apiFetch(uv("discord/unlink"), { method: "POST" }); - // 409: the account was created with Discord and has no password, so - // unlinking would lock it out. Say why instead of appearing to do nothing. - if (res.status === 409) { - const body = await res.json().catch(() => null); - showToast((body && body.detail) || "Discord can't be unlinked from this account.", true); - unlinkBtn.disabled = false; - return; - } - } catch (e) {} - await refreshUser(); - emitAuth(); // repaint the popover in its unlinked state + el.querySelectorAll(".acct-oauth-unlink").forEach((unlinkBtn) => { + unlinkBtn.addEventListener("click", async () => { + const provider = unlinkBtn.dataset.provider; + unlinkBtn.disabled = true; + try { + const res = await apiFetch(uv(`oauth/${provider}/unlink`), { method: "POST" }); + // 409: the account has no password and this was its last provider, so + // unlinking would lock it out. Say why instead of appearing to do nothing. + if (res.status === 409) { + const body = await res.json().catch(() => null); + showToast((body && body.detail) + || "That account can't be unlinked — it's the only way to sign in.", true); + unlinkBtn.disabled = false; + return; + } + } catch (e) {} + await refreshUser(); + emitAuth(); // repaint the popover in its unlinked state + }); }); const dmBtn = el.querySelector(".acct-discord-dm"); if (dmBtn) dmBtn.addEventListener("click", async () => { @@ -479,28 +524,41 @@ function showToast(msg, isError = false) { toastTimer = setTimeout(() => { el.hidden = true; }, 6000); } -// --- Discord OAuth outcomes -------------------------------------------------- +// --- OAuth outcomes ---------------------------------------------------------- // The link/login callback can only talk back to us through a redirect, so it -// lands on ?discord=. Each status maps to [message, isError]; anything -// unrecognised is treated as a generic failure rather than shown raw. -const DISCORD_STATUS = { - linked: ["Discord connected.", false], - signedin: ["Signed in with Discord.", false], - created: ["Account created with Discord — welcome to Thermograph.", false], - cancelled: ["Discord sign-in cancelled.", false], - taken: ["That Discord account is already connected to another Thermograph account.", true], - mismatch: ["That email belongs to an account connected to a different Discord account. " - + "Sign in with your password to change it.", true], - noemail: ["Discord didn't share an email address, so there's no account to sign in to.", true], - unverified: ["Verify your email address with Discord first, then try again.", true], - inactive: ["That account is deactivated.", true], - unavailable: ["Discord sign-in isn't available on this server.", true], +// lands on ?oauth=&provider=. Each status maps to a message builder +// taking the provider's display name; anything unrecognised is treated as a +// generic failure rather than shown raw. +const OAUTH_STATUS = { + linked: [(p) => `${p} connected.`, false], + signedin: [(p) => `Signed in with ${p}.`, false], + created: [(p) => `Account created with ${p} — welcome to Thermograph.`, false], + cancelled: [(p) => `${p} sign-in cancelled.`, false], + already: [(p) => `A different ${p} account is already connected here.`, true], + taken: [(p) => `That ${p} account is already connected to another Thermograph account.`, true], + mismatch: [(p) => `That email belongs to an account connected to a different ${p} ` + + "account. Sign in with your password to change it.", true], + noemail: [(p) => `${p} didn't share an email address, so there's no account to sign in to.`, true], + unverified: [(p) => `Verify your email address with ${p} first, then try again.`, true], + inactive: [() => "That account is deactivated.", true], + unavailable: [(p) => `${p} sign-in isn't available on this server.`, true], }; -function showDiscordStatus(status) { - const [msg, isError] = DISCORD_STATUS[status] - || ["Something went wrong with Discord. Please try again.", true]; - showToast(msg, isError); +function providerLabel(name) { + const known = providers.find((p) => p.name === name); + if (known) return known.label; + // The config probe may have failed, or the provider may have been switched off + // between starting the flow and coming back. Title-case the raw name rather + // than showing "undefined" in the toast. + return name ? name.charAt(0).toUpperCase() + name.slice(1) : "That provider"; +} + +function showOAuthStatus(status, provider) { + const label = providerLabel(provider); + const entry = OAUTH_STATUS[status]; + if (!entry) return showToast(`Something went wrong with ${label}. Please try again.`, true); + const [build, isError] = entry; + showToast(build(label), isError); } // --- boot -------------------------------------------------------------------- @@ -513,18 +571,20 @@ function showDiscordStatus(status) { const params = new URLSearchParams(location.search); const token = params.get("verify_token"); - // Both of these are one-shot handoffs from a redirect; strip them together so a - // reload can't replay either, and so the two never fight over the URL. - const discordStatus = params.get("discord"); - if (token || discordStatus) { - params.delete("verify_token"); - params.delete("discord"); + // `discord` is the pre-Google spelling: a backend older than this frontend sends + // only that one, so fall back to it and assume the provider it implies. + const oauthStatus = params.get("oauth") || params.get("discord"); + const oauthProvider = params.get("provider") || (params.get("discord") ? "discord" : ""); + // These are one-shot handoffs from a redirect; strip them together so a reload + // can't replay any of them, and so they never fight over the URL. + if (token || oauthStatus) { + ["verify_token", "oauth", "provider", "discord"].forEach((k) => params.delete(k)); const clean = location.pathname + (params.toString() ? `?${params}` : "") + location.hash; history.replaceState(null, "", clean); } // refreshUser() above already ran, and the callback's redirect carried the new // session cookie, so the header is painted signed-in before this toast lands. - if (discordStatus) showDiscordStatus(discordStatus); + if (oauthStatus) showOAuthStatus(oauthStatus, oauthProvider); if (token) { try { await verifyEmail(token); diff --git a/frontend/static/style.css b/frontend/static/style.css index f50112b..116c908 100644 --- a/frontend/static/style.css +++ b/frontend/static/style.css @@ -436,14 +436,26 @@ main { max-width: 1200px; margin: 0 auto; padding: 20px 24px 60px; } .acct-or::before, .acct-or::after { content: ""; flex: 1; height: 1px; background: var(--border); } -.acct-discord-login { +.acct-oauth-row { display: flex; flex-direction: column; gap: 8px; } +.acct-oauth-login { display: flex; align-items: center; justify-content: center; gap: 9px; padding: 12px 16px; min-height: 44px; border-radius: 10px; - background: var(--discord); color: #fff; font-weight: 700; font-size: 15px; - text-decoration: none; border: 1px solid transparent; + font-weight: 700; font-size: 15px; text-decoration: none; + border: 1px solid transparent; } -.acct-discord-login:hover { filter: brightness(1.08); } -.acct-discord-login svg { width: 20px; height: 15px; flex: none; } +.acct-oauth-login:hover { filter: brightness(1.08); } +.acct-oauth-login svg { width: 20px; height: 20px; flex: none; } +.acct-oauth-login.is-discord { + background: var(--discord); color: #fff; +} +.acct-oauth-login.is-discord svg { height: 15px; } +/* Google's branding terms require their mark on white (or their own grey), with a + visible border — so this one button is deliberately light in both schemes and + does not follow the surface tokens. */ +.acct-oauth-login.is-google { + background: #fff; color: #1f1f1f; border-color: #747775; +} +.acct-oauth-login.is-google:hover { filter: none; background: #f2f2f2; } .acct-pop-note { display: block; padding: 9px 12px; font-size: 13px; } .acct-switch { margin: 0; font-size: 13px; color: var(--muted); text-align: center; } diff --git a/infra/deploy/thermograph.env.example b/infra/deploy/thermograph.env.example index ef158f8..cdf6403 100644 --- a/infra/deploy/thermograph.env.example +++ b/infra/deploy/thermograph.env.example @@ -224,6 +224,18 @@ THERMOGRAPH_BASE_URL=https://thermograph.org # for per request, not registered, so the email scope needs no portal change # either. An environment without these two vars simply shows no Discord UI. #THERMOGRAPH_DISCORD_CLIENT_SECRET= +# Sign in with Google / connect a Google account — the same engine as Discord +# above (accounts/oauth.py), configured independently, so an environment may offer +# either, both, or neither. From Google Cloud Console -> APIs & Services -> +# Credentials -> OAuth 2.0 Client ID, type "Web application". The CLIENT_SECRET is +# a credential. Register this exact redirect URI on the client: +# https://thermograph.org/api/v2/oauth/google/callback +# (Google matches redirect URIs exactly — scheme, host and path — so beta and dev +# each need their own entry on the same client, or their own client.) The consent +# screen needs only the `openid` and `email` scopes, both non-sensitive, so it +# requires no Google verification review. +#THERMOGRAPH_GOOGLE_CLIENT_ID= +#THERMOGRAPH_GOOGLE_CLIENT_SECRET= # Gateway bot (opt-in): holds a live websocket so the bot replies to messages that # @mention it (or DM it) with a city grade — e.g. "@Thermograph Phoenix". Reuses # THERMOGRAPH_DISCORD_BOT_TOKEN above. Runs on the single notifier leader only, so