From 8b70a875555b9144e07c5bfcf2f96d130b7ec4c3 Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Sun, 26 Jul 2026 10:43:17 -0700 Subject: [PATCH] accounts: sign in with Discord, sharing the existing link callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discord was already linkable from a signed-in session; it could not authenticate one. Adds /discord/login/start, which resolves a Discord identity to an account and issues a session. Both flows return to the existing /discord/link/callback. Discord only honours redirect URIs registered in the developer portal, so a second callback path would have blocked this behind an operator change; the signed state now carries a purpose, and since that is inside the HMAC a state can only verify under the flow it was minted for. Account resolution, in order: an existing discord_id (the durable key, no email needed); otherwise the Discord email, but only when Discord reports it verified — that flag is the sole evidence the person owns the address, and matching on an unverified one would hand over the account. Failing both, a new account is created. A login state carries no user id, so it is replayable against whoever is signed in. It therefore never links: doing so would be a forced-linking takeover. Only link/start, whose state is bound to a user id, may link. Accounts created this way have no password their owner has ever seen, so user.discord_only records that and unlink is refused for them — no reset-password router is mounted, so unlinking would be unrecoverable. Migration 0003 adds the column conditionally: 0001 builds the schema from live model metadata, so a fresh database already has it. Also fixes two latent bugs in the link flow: the callback redirected to /subscriptions, which no route serves (it is /alerts), and nothing ever read the ?discord= status it has always sent, so a completed link gave no feedback. Both now surface as a toast. Linking a Discord account that another account already owns returned a 500 from the unique constraint; it now explains itself. --- backend/accounts/models.py | 10 +- backend/accounts/schemas.py | 3 + backend/accounts/users.py | 20 ++ .../versions/0003_user_discord_only.py | 50 +++ backend/notifications/discord_link.py | 304 ++++++++++++++---- .../tests/notifications/test_discord_login.py | 251 +++++++++++++++ frontend/static/account.js | 76 ++++- frontend/static/style.css | 24 ++ infra/deploy/thermograph.env.example | 17 +- 9 files changed, 682 insertions(+), 73 deletions(-) create mode 100644 backend/alembic/versions/0003_user_discord_only.py create mode 100644 backend/tests/notifications/test_discord_login.py diff --git a/backend/accounts/models.py b/backend/accounts/models.py index 40b6df4..bc3dc54 100644 --- a/backend/accounts/models.py +++ b/backend/accounts/models.py @@ -34,13 +34,19 @@ class User(SQLAlchemyBaseUserTableUUID, Base): # is_superuser, is_verified. Optional extras: display_name: Mapped[str | None] = mapped_column(String(120), nullable=True) # Linked Discord account id (OAuth2 identify) — the key DM alerts reach the user - # by. NB: create_all only makes this on a fresh DB; an existing prod accounts.db - # needs the manual migration in deploy/migrations/ (no Alembic in this project). + # by, and the identity "Sign in with Discord" resolves an account from. Unique, + # so one Discord account can never own two Thermograph accounts. discord_id: Mapped[str | None] = mapped_column(String(32), unique=True, nullable=True) # Whether to also deliver alerts as a Discord DM. Set True on linking (an active # 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()) class AccessToken(SQLAlchemyBaseAccessTokenTableUUID, Base): diff --git a/backend/accounts/schemas.py b/backend/accounts/schemas.py index 0898910..0715e12 100644 --- a/backend/accounts/schemas.py +++ b/backend/accounts/schemas.py @@ -24,6 +24,9 @@ class UserRead(schemas.BaseUser[uuid.UUID]): # flow (discord_link.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 class UserCreate(schemas.BaseUserCreate): diff --git a/backend/accounts/users.py b/backend/accounts/users.py index 8967126..a86a4d7 100644 --- a/backend/accounts/users.py +++ b/backend/accounts/users.py @@ -140,3 +140,23 @@ fastapi_users = FastAPIUsers[User, uuid.UUID](get_user_manager, [auth_backend]) # Dependencies handlers use to require / peek at the logged-in user. current_active_user = fastapi_users.current_user(active=True) current_user_optional = fastapi_users.current_user(active=True, optional=True) + + +async def attach_session(response, user, user_manager, access_token_db, request=None): + """Log `user` in by copying a fresh session cookie onto `response`. + + The library's routers own this for password login, but an OAuth callback has to + end in a *redirect* the browser follows, not the 204 the login router returns. + So mint the session the normal way and lift the Set-Cookie header off the + library's response rather than re-deriving the cookie's name/TTL/flags here — + duplicating those would let the two logins drift into issuing different cookies. + """ + strategy = get_database_strategy(access_token_db) + issued = await auth_backend.login(strategy, user) + for key, value in issued.raw_headers: + if key.lower() == b"set-cookie": + response.raw_headers.append((key, value)) + # The routers call this hook; a hand-rolled login has to as well, or Discord + # sign-ins go missing from the audit log that every password login appears in. + await user_manager.on_after_login(user, request, response) + return response diff --git a/backend/alembic/versions/0003_user_discord_only.py b/backend/alembic/versions/0003_user_discord_only.py new file mode 100644 index 0000000..7a84e6a --- /dev/null +++ b/backend/alembic/versions/0003_user_discord_only.py @@ -0,0 +1,50 @@ +"""user.discord_only — mark accounts whose only credential is Discord + +"Sign in with Discord" (notifications/discord_link.py) creates accounts for people +who never chose a password. ``hashed_password`` is NOT NULL, so those rows carry a +generated one nobody has ever seen; without a flag there is no way to tell such an +account apart from a password account that merely linked Discord later. That +distinction is load-bearing: unlinking Discord from a Discord-only account would +delete its only way in, and no reset-password router is mounted to recover it. + +Conditional on purpose. Revision 0001 builds the schema with +``Base.metadata.create_all``, which reads the *current* models.py — so a database +created after this column was added to the ORM already has it, and an +unconditional ``add_column`` would fail there with a duplicate-column error. Only +a database stamped before this revision is missing it. + +Revision ID: 0003_user_discord_only +Revises: 0002_climate_hypertables +Create Date: 2026-07-26 +""" +import sqlalchemy as sa +from alembic import op + +revision = "0003_user_discord_only" +down_revision = "0002_climate_hypertables" +branch_labels = None +depends_on = None + + +def _has_column() -> bool: + cols = sa.inspect(op.get_bind()).get_columns("user") + return any(c["name"] == "discord_only" for c in cols) + + +def upgrade() -> None: + if _has_column(): + return + # server_default (not just a Python-side default) so the backfill of existing + # rows happens in the same statement: every account that predates Discord + # login has a password, so False is correct for all of them. + op.add_column( + "user", + sa.Column("discord_only", sa.Boolean(), nullable=False, + server_default=sa.false()), + ) + + +def downgrade() -> None: + if not _has_column(): + return + op.drop_column("user", "discord_only") diff --git a/backend/notifications/discord_link.py b/backend/notifications/discord_link.py index c4feffd..272d81c 100644 --- a/backend/notifications/discord_link.py +++ b/backend/notifications/discord_link.py @@ -1,16 +1,33 @@ -"""Link a Thermograph account to a Discord account via OAuth2 (identify scope). +"""Discord as an identity: sign in with it, and connect it to an account. -The Discord user id we store here is the durable key a later feature (DM alerts) -uses to reach the person. The flow is the standard authorization-code grant: +Two flows over one authorization-code grant, told apart by the signed `state`: - /link/start -> redirect the logged-in user to Discord's consent screen - /link/callback -> exchange the code, read the Discord user id, store it - /unlink -> forget it + 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 + +**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 Thermograph user id, so the callback can't be replayed or bound to a -different account. Everything requires an active session, so linking always acts on -the person who is actually logged in. +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. """ from __future__ import annotations @@ -23,15 +40,23 @@ import time import urllib.parse import httpx -from fastapi import APIRouter, Depends, Request +from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import RedirectResponse from pydantic import BaseModel -from sqlalchemy import update +from sqlalchemy import select, update from sqlalchemy.ext.asyncio import AsyncSession from accounts.db import get_async_session from accounts.models import User -from accounts.users import SECRET, current_active_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"]) @@ -43,6 +68,11 @@ _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) @@ -61,15 +91,22 @@ def _unb64(s: str) -> bytes: return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4)) -def _sign_state(uid: str) -> str: - payload = _b64(json.dumps({"u": uid, "t": int(time.time())}).encode()) +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) -> str | None: - """The Thermograph user id the state was minted for, or None if it's missing, - tampered, or older than the TTL.""" +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): @@ -83,7 +120,11 @@ def _verify_state(state: str) -> str | None: return None if time.time() - float(data.get("t", 0)) > _STATE_TTL: return None - return data.get("u") + # 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: @@ -94,52 +135,30 @@ def _redirect_uri(request: Request) -> str: return f"{proto}://{host}{BASE}/api/v2/discord/link/callback" -def _account_redirect(status: str) -> RedirectResponse: - # Back to the alerts page, which surfaces the link state; 303 so the browser - # issues a GET after the callback. - return RedirectResponse(url=f"{BASE}/subscriptions?discord={status}", status_code=303) - - -# --- routes ------------------------------------------------------------------ -@router.get("/discord/config") -async def discord_config(): - """Whether Discord account-linking is configured on this server. The account - menu uses it to show the "Link Discord" entry only when linking will actually - work — so an unconfigured server surfaces no dead-end Discord UI, and enabling - it later (setting the OAuth env vars) makes the entry appear on its own.""" - return {"enabled": enabled()} - - -@router.get("/discord/link/start") -async def link_start(request: Request, user=Depends(current_active_user)): - if not enabled(): - return _account_redirect("unavailable") +def _authorize_redirect(request: Request, scope: str, state: str) -> RedirectResponse: params = { "client_id": CLIENT_ID, "redirect_uri": _redirect_uri(request), "response_type": "code", - "scope": "identify", - "state": _sign_state(str(user.id)), + "scope": scope, + "state": state, "prompt": "consent", } - return RedirectResponse(url=f"{_AUTHORIZE}?{urllib.parse.urlencode(params)}", status_code=303) + return RedirectResponse(url=f"{_AUTHORIZE}?{urllib.parse.urlencode(params)}", + status_code=303) -@router.get("/discord/link/callback") -async def link_callback( - request: Request, - user=Depends(current_active_user), - session: AsyncSession = Depends(get_async_session), -): - 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") - if _verify_state(state) != str(user.id): - return _account_redirect("error") +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={ @@ -150,16 +169,121 @@ async def link_callback( "redirect_uri": _redirect_uri(request), }, headers={"Content-Type": "application/x-www-form-urlencoded"}) if tok.status_code >= 300: - return _account_redirect("error") + return None access = tok.json().get("access_token") me = await client.get(_ME, headers={"Authorization": f"Bearer {access}"}) if me.status_code >= 300: - return _account_redirect("error") - discord_id = str(me.json().get("id") or "") + 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()} + + +@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")) + + +@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")) + + +@router.get("/discord/link/callback") +async def link_callback( + 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), +): + """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( @@ -169,11 +293,73 @@ async def link_callback( 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) + + @router.post("/discord/unlink", status_code=204) 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) ) diff --git a/backend/tests/notifications/test_discord_login.py b/backend/tests/notifications/test_discord_login.py new file mode 100644 index 0000000..cc3d4f9 --- /dev/null +++ b/backend/tests/notifications/test_discord_login.py @@ -0,0 +1,251 @@ +"""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/frontend/static/account.js b/frontend/static/account.js index 1d17376..f3eb0c6 100644 --- a/frontend/static/account.js +++ b/frontend/static/account.js @@ -10,7 +10,7 @@ // units.js is (imported by each page's entry module). let currentUser = null; // {id, email, display_name} or null -let discordEnabled = false; // is Discord linking configured on the server? +let discordEnabled = false; // is Discord configured on the server? let discordChecked = false; // have we asked yet? (once per page load) const authCbs = []; // notified on login/logout so pages can re-gate @@ -89,10 +89,10 @@ async function refreshUser() { const res = await apiFetch(uv("users/me")); currentUser = res.ok ? await res.json() : null; } catch (e) { currentUser = null; } - // Learn once whether Discord linking is configured, so the account menu only - // offers "Link Discord" when it will actually work (checked lazily, and only - // for a signed-in user — the menu never shows it to anyone else). - if (currentUser && !discordChecked) { + // 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; try { const r = await apiFetch(uv("discord/config")); @@ -126,6 +126,10 @@ export async function logout() { // --- auth modal -------------------------------------------------------------- let modal = null, mode = "login"; +// Discord's brand mark ("Clyde"), inline like the other icons here so it needs no +// extra request and inherits currentColor. +const DISCORD_IC = ``; + function buildModal() { modal = document.createElement("div"); modal.className = "mp-overlay acct-overlay"; @@ -145,6 +149,12 @@ function buildModal() { +

`; @@ -173,6 +183,9 @@ 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, + // whichever applies, so making the user pick first would be a false choice. + modal.querySelector(".acct-alt").hidden = !discordEnabled; showError(""); } @@ -371,7 +384,11 @@ function renderHeader() { My alerts ${currentUser.discord_id ? `` - + '' + // A Discord-created account has no password, so Discord is its only + // way in and the server refuses to unlink it — don't offer the button. + + (currentUser.discord_only + ? 'Signed in with Discord' + : '') : (discordEnabled ? `Link Discord` : "")} @@ -403,7 +420,17 @@ function renderHeader() { const unlinkBtn = el.querySelector(".acct-discord-unlink"); if (unlinkBtn) unlinkBtn.addEventListener("click", async () => { unlinkBtn.disabled = true; - try { await apiFetch(uv("discord/unlink"), { method: "POST" }); } catch (e) {} + 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 }); @@ -452,6 +479,30 @@ function showToast(msg, isError = false) { toastTimer = setTimeout(() => { el.hidden = true; }, 6000); } +// --- Discord 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], +}; + +function showDiscordStatus(status) { + const [msg, isError] = DISCORD_STATUS[status] + || ["Something went wrong with Discord. Please try again.", true]; + showToast(msg, isError); +} + // --- boot -------------------------------------------------------------------- (async function initAccount() { ensureAcctEl(); @@ -462,10 +513,19 @@ function showToast(msg, isError = false) { const params = new URLSearchParams(location.search); const token = params.get("verify_token"); - if (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"); 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 (token) { try { await verifyEmail(token); await refreshUser(); diff --git a/frontend/static/style.css b/frontend/static/style.css index c794f15..f50112b 100644 --- a/frontend/static/style.css +++ b/frontend/static/style.css @@ -6,6 +6,9 @@ --text: #e7ecf2; --muted: #9aa6b2; --accent: #f0803c; + /* Discord's brand blurple. Fixed in both schemes on purpose: it identifies the + provider, so it is their colour rather than one of ours to re-theme. */ + --discord: #5865f2; /* temperature grades: 9-step diverging cold -> green -> hot (colorblind-safe). rec-* are the "Near Record" danger tiers — deliberately dark/saturated. */ @@ -422,6 +425,27 @@ main { max-width: 1200px; margin: 0 auto; padding: 20px 24px 60px; } color: var(--text); border: 1px solid var(--rec-hot, #d64545); } .acct-error[hidden] { display: none; } +/* "or / Continue with Discord" under the submit button. Hidden unless the server + reports Discord configured (account.js sets [hidden]). */ +.acct-alt { display: flex; flex-direction: column; gap: 10px; } +.acct-alt[hidden] { display: none; } +.acct-or { + display: flex; align-items: center; gap: 10px; + font-size: 12px; color: var(--muted); text-transform: uppercase; letter-spacing: .06em; +} +.acct-or::before, .acct-or::after { + content: ""; flex: 1; height: 1px; background: var(--border); +} +.acct-discord-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; +} +.acct-discord-login:hover { filter: brightness(1.08); } +.acct-discord-login svg { width: 20px; height: 15px; flex: none; } + +.acct-pop-note { display: block; padding: 9px 12px; font-size: 13px; } .acct-switch { margin: 0; font-size: 13px; color: var(--muted); text-align: center; } .acct-switch button { background: none; border: 0; color: var(--accent); cursor: pointer; diff --git a/infra/deploy/thermograph.env.example b/infra/deploy/thermograph.env.example index f35537f..ef158f8 100644 --- a/infra/deploy/thermograph.env.example +++ b/infra/deploy/thermograph.env.example @@ -210,10 +210,19 @@ THERMOGRAPH_BASE_URL=https://thermograph.org # Discord allows ONE gateway connection per bot token, so enable this in exactly # one environment — prod, the only vault with the token. #THERMOGRAPH_DISCORD_BOT= -# Account linking (OAuth2 "identify"): lets a signed-in user connect their Discord -# account, storing their Discord user id for DM alerts. CLIENT_ID is the same App -# ID above. The CLIENT_SECRET is from OAuth2 -> Client Secret (a credential). In the -# portal, add the redirect: https://thermograph.org/api/v2/discord/link/callback +# Discord as an identity (notifications/discord_link.py). One pair of credentials +# gates BOTH flows, and setting them makes both appear in the UI at once: +# - account linking (scope "identify") — a signed-in user connects their Discord +# account, storing their Discord user id for DM alerts; +# - sign in with Discord (scope "identify email") — an anonymous visitor +# authenticates, resolving or creating an account from the Discord identity. +# CLIENT_ID is the same App ID above. The CLIENT_SECRET is from OAuth2 -> Client +# Secret (a credential). In the portal, add the redirect: +# https://thermograph.org/api/v2/discord/link/callback +# Both flows come back to that one path (the purpose is carried in the signed +# state), so enabling sign-in needs no new redirect registered. Scopes are asked +# 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= # 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