From 890cb4a764461b5127b79286dc21f2a64607b8da Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Sun, 26 Jul 2026 12:53:33 -0700 Subject: [PATCH 1/3] docs: the orchestrator runbook and the module map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .claude/BRANCHING.md — how the trunk-based flow is actually run: the serialised merge queue (Forgejo has no native one, so whoever orchestrates is it), the two promotions and who decides each, and the hotfix down-merge that has to happen in the same session or it becomes a regression scheduled for the next promotion. Two things it states because getting them wrong is expensive here. CI does not gate PRs into `release` — pr-build.yml fires only for dev and main — so a required check on that branch would make every production promotion unmergeable with a 405 naming no cause; closing that gap means changing the workflow first and turning the requirement on second. And the chain cannot fast-forward: every promotion leaves a merge commit on the target the source never receives, so promotions are merge commits until someone deliberately reconciles the branches. .claude/ownership.md — which tasks may run concurrently, seamed along the four domains, plus the shared spine that has to be serialised: the workflows (CI lives only in the root .forgejo/), env-topology.sh, the deploy entry points and the CLAUDE.md files. Names the recurring collisions too — a schema change spans alembic and whatever reads the column, and if the payload shape moves it is PAYLOAD_VER and the frontend as well, which is one task and not two. --- .claude/BRANCHING.md | 190 +++++++++++++++++++++++++++++++++++++++++++ .claude/ownership.md | 60 ++++++++++++++ CLAUDE.md | 5 ++ 3 files changed, 255 insertions(+) create mode 100644 .claude/BRANCHING.md create mode 100644 .claude/ownership.md diff --git a/.claude/BRANCHING.md b/.claude/BRANCHING.md new file mode 100644 index 0000000..f4b7366 --- /dev/null +++ b/.claude/BRANCHING.md @@ -0,0 +1,190 @@ +# Orchestrator instructions — trunk-based flow + +`feat/*` → `dev` → `main` → `release`. One PR per hop, merges serialised by the +orchestrator. Forgejo has no native merge queue, so you are the merge queue. + +Use the Centralis `forge_*` tools for every remote operation — PRs, protection, +merges, tags. Use local git only for rebases you have to resolve by hand. +Never call the Forgejo API with curl and a pasted token; the server holds it. + +The same file exists in `centralis` with the same rules. Where the two repos +differ it is noted inline. + +--- + +## The branches + +Each branch has its own environment here, and `infra/deploy/env-topology.sh` is +the single source of truth for where each one lives: + +| Branch | Deploys to | Receives | +|---|---|---| +| `feat/*`, `fix/*` | nothing | your work | +| `dev` | dev — vps1, mesh-only, own Postgres | feature PRs | +| `main` | beta — beta.thermograph.org, vps2 | promotions from `dev` | +| `release` | prod — thermograph.org, vps2 | promotions from `main`, and hotfixes | + +`deploy.yml` handles all three: the branch selects the environment, a matrix +covers backend and frontend, and each leg checks whether the push actually +touched its domain before rolling. So **FE and BE ship out of lockstep** — a +backend change lands without a frontend deploy. That is the point of the split, +and it is why `/api/version` and `PAYLOAD_VER` discipline are load-bearing: +whatever you promote, the other half may already be running something older. + +`dev` is the default branch, so PRs target it without anyone remembering to +retarget. All three are protected: **everything is a PR**, for humans and agents +alike. + +## Who may merge what + +This is not the same question as how a change travels, and it is deliberately +asymmetric. `MERGE_AUTHORITY` in centralis' +`src/lib/promotion.ts` is the whole policy; `forge_pr_merge` enforces it. + +- **Into `dev` — free.** Merge feature PRs yourself, and do not leave them open. + A PR opened against `dev` and abandoned is an unfinished task wearing the + costume of a delivered one. +- **`dev` → `main` — yours, batched.** Beta is the test environment, so + withholding this merge withholds the testing. The judgement being asked of you + is *when a batch is coherent enough to be worth testing*, not whether you are + allowed to test it. +- **`main` → `release` — the owner's.** That merge deploys to prod. Call + `promote(from="main", to="release")`, which prepares the PR and returns what + would ship, the CI evidence and a recommendation. Then put it to the owner — + including "I would wait", if that is what you think. No flag overrides this, + and routing around it is not an option. + +`confirm_protected_base` is unrelated to the above: it is the hotfix path for a +*non-promotion* PR aimed straight at `main` or `release`, and it is audited +loudly. + +--- + +## Phase 1 — repo setup + +Idempotent. Re-run it whenever you are unsure; every step reports "already +correct" rather than erroring. + +1. **The three branches exist.** + `forge_branches(create={branch:"main", from:"dev"})` and the same for + `release`. If a branch exists but points elsewhere, the tool says so — that + is a divergence to reconcile deliberately, not to overwrite. +2. **`dev` is the default branch.** + `forge_repo_settings(apply_flow_defaults=true)` — sets the default branch and + permits every merge style the chain uses, including `fast-forward-only`, plus + rebase-updates (without which the merge queue's core primitive 403s). +3. **Protection.** `forge_branch_protection(branch="dev", apply=true)`, then + `main`, then `release`. + + `dev` and `main` get the required check; **`release` does not**, and that is + deliberate. `pr-build.yml` fires only for PRs into `dev` and `main`, so a PR + into `release` reports no status at all — requiring a context nothing + produces would make every production promotion permanently unmergeable, with + a 405 that names no cause. This estate has already lost an afternoon to + exactly that, from a single mistyped check name. If you want it closed + properly, do it in this order: add `release` to `pr-build.yml`'s + `on.pull_request.branches`, confirm on a real PR that the context appears, + *then* pass `require_checks_on_release: true`. The other order blocks prod. +4. **Stale branches.** `forge_branches()` lists every branch with its age, how + much unmerged work it carries, and a verdict: + - `merged` — nothing on it the trunk lacks. Delete freely. + - `stale` — unmerged work, untouched 14+ days. **File a `stale-branch: ` + issue summarising its diff first**, then delete. Never discard work + silently. + - `ageing` — 2+ days with work still on it. Report it; do not delete. +5. **Module map.** `.claude/ownership.md` — read it before partitioning work. + +## Phase 2 — the ongoing loop + +### Dispatching subagents + +Partition by module using `.claude/ownership.md`. Never give two concurrent +agents tasks touching the same files; if that is unavoidable, sequence them. + +Inject this into every subagent task: + +``` +Branch off latest `dev` as feat/. Touch only these files/modules: . +One logical change, small diff. Rebase onto dev before opening your PR. +PR targets `dev`. The description must list: what changed, how it was tested, +files touched. Do not merge your own PR — the orchestrator handles all merges. +``` + +Keep a branch alive **less than a day**. A task bigger than that is several +stacked tasks. + +### The merge queue + +You merge **one PR into `dev` at a time**. For each: + +1. `forge_prs(base="dev")` — the queue, with each PR's head sha and triage flag. +2. `forge_pr_update(pr=N)` — replay it onto the current tip. If the head sha + does not move, it was already current and no CI will re-fire; do not sit + waiting for a run that was never scheduled. +3. `forge_pr_await(pr=N, until="checks_complete")` — checks on the **rebased** + head. The run you saw before the update was for a merge base that no longer + exists. +4. `forge_pr_merge(pr=N, method="rebase", delete_branch=true)`. +5. Only then move to the next PR. + +`block_on_outdated_branch` is set on `dev`, so Forgejo enforces step 2 rather +than trusting you to remember it: after one merge, the next PR is refused until +it has been replayed. Treat that 405 as the queue working, not as a fault. + +If a rebase conflicts: resolve it yourself if it is trivial and mechanical +(under ~20 lines), otherwise hand it back to the owning agent with the conflict +context. Do not resolve a large conflict on someone else's behalf. + +### Promotion: `dev` → `main` + +Promote when `dev` is green and a coherent batch is complete — not on a timer, +not mid-feature. `promote(from="dev", to="main")` opens the PR with the +divergence, the conflict prediction and the changelog written into its body. +Merge it with `method="merge"`. + +Unready work belongs behind a feature flag, or not on `dev` yet. **Never promote +around it with cherry-picks.** + +`promote` refuses a hop whose two tips have identical trees — the commit counts +look like real work while the content is already on the target under different +SHAs, and merging it would fire the target's deploys for nothing. + +### Promotion: `main` → `release` + +Prepare with `promote`, then ask. On the owner's yes, merge with `method="merge"` +and tag it: `forge_releases(create={tag:"v", target:"release", ...})`. + +**On fast-forward.** The written policy is that `release` moves only by +fast-forward. It cannot today: every promotion so far has been a merge commit +*into* the target, which the source never receives, so the branches are mutually +divergent by construction and Forgejo will refuse a `fast-forward-only` merge — +correctly. The style is implemented and accepted by `forge_pr_merge`; adopting +it needs a one-time reconciliation of the protected branches, which is the +owner's decision. Until that happens, promotions are merge commits. Do not +attempt to "fix" this with a rebase or a force-push. + +### Hotfix + +1. `forge_branches(create={branch:"hotfix/", from:"release"})`. +2. Fix, PR into `release`, checks green, merge with + `confirm_protected_base: true`, tag `v`. +3. **Immediately down-merge** `release` → `main` and `main` → `dev`, as merges, + not cherry-picks, in the same session. A hotfix that exists only on `release` + is a regression scheduled for the next promotion. + +Note that CI does not gate PRs into `release` — `pr-build.yml` fires only for +`dev` and `main`. A hotfix's green evidence therefore has to come from somewhere +else; say plainly where it came from rather than implying a check passed. + +A hotfix that touches only one domain deploys only that domain. Check the other +half's `/api/version` before assuming the fix is live end to end. + +## Guardrails + +- Never force-push `dev`, `main` or `release`. +- Never merge a red PR, hotfix included. Fix CI, or fix the tests in the same PR. +- Never merge two green PRs back to back without re-checking the second against + the new tip. +- If two down-merges or promotions conflict beyond the trivial, stop and report + rather than resolving autonomously. +- Weekly: `forge_branches()` and report anything `ageing` or `stale`. diff --git a/.claude/ownership.md b/.claude/ownership.md new file mode 100644 index 0000000..dcf8239 --- /dev/null +++ b/.claude/ownership.md @@ -0,0 +1,60 @@ +# Module map — what can be worked on concurrently + +Used for partitioning work across concurrent subagents. The rule is one owner +per file at a time: **never** give two concurrent agents tasks that touch the +same module. If two tasks genuinely need the same file, sequence them instead of +racing them — a rebase conflict costs more than the parallelism saved. + +Read this before dispatching. It answers one question: *can these two tasks run +at the same time?* + +The monorepo's four domains are the natural seam, and they are a real one: FE +and BE build and deploy independently, and `deploy.yml` checks per-leg whether a +push actually touched that domain. Two agents in two different domains will not +collide in CI either. + +## Safe to assign concurrently + +| Module | Files | Notes | +|---|---|---| +| Backend — API | `backend/api/**`, `backend/app.py` | Route handlers and serialisation. | +| Backend — core | `backend/core/**` | Grading, percentiles, climatology. The domain logic. | +| Backend — accounts | `backend/accounts/**` | Auth, sessions, OAuth links. | +| Backend — daemon | `backend/daemon/**` | Scheduled work. Anything named "prefetch" must not spend the Open-Meteo quota. | +| Backend — data & lake | `backend/data/**`, `backend/gen_era5_lake.py`, `backend/gen_cities.py` | | +| Backend — migrations | `backend/alembic/**`, `backend/alembic.ini` | One owner, always. Two agents generating revisions produce two heads. | +| Frontend — server | `frontend/server/**`, `frontend/app.py`, `frontend/api_client.py` | Go SSR and the Python app. | +| Frontend — static | `frontend/static/**` | | +| Frontend — templates | `frontend/templates/**`, `frontend/content/**`, `frontend/content.py` | | +| Infra — deploy | `infra/deploy/**` (excluding `secrets/`), `infra/docker-compose*.yml` | | +| Infra — secrets | `infra/deploy/secrets/**` | SOPS vault. One owner, and never alongside `infra/deploy`. | +| Infra — ops | `infra/ops/**`, `infra/lake-iceberg/**` | | +| Infra — terraform | `infra/terraform/**` | | +| Observability | `observability/**` | Grafana dashboards are provisioned from this JSON; a UI edit is overwritten. | +| Docs | `docs/**` | | +| Agent tooling | `.claude/hooks/**` | | + +## Serialise — shared spine + +Touched by most changes. A change here is its own task with nothing else running +against it. + +| File | Why | +|---|---| +| `.forgejo/workflows/**` | CI lives **only** here, path-filtered per domain. Every domain's changes route through the same files, so two agents editing workflows conflict even when their domains do not. | +| `CLAUDE.md` and every domain `CLAUDE.md` | Read before every change; a stale one is a correctness bug. Small edits, landed fast. | +| `CUTOVER-NOTES.md` | The source of truth for what is and is not live. | +| `infra/deploy/env-topology.sh` | The single source of truth for where each environment's checkout, branch, stack and ports live. | +| `infra/deploy/deploy.sh`, `infra/deploy/stack/deploy-stack.sh` | The deploy contract's entry points. | +| `backend/CLAUDE.md` + the `/api/version` contract | FE and BE ship out of lockstep, so `PAYLOAD_VER` discipline is load-bearing. A change to the payload shape is one task spanning both domains — not two concurrent ones. | + +## The recurring collisions + +- **A schema change is not a backend-only task.** It is `backend/alembic` plus + whatever reads the column, and if the payload shape moves it is `PAYLOAD_VER` + and the frontend too. Scope it as one task across both domains rather than + two agents discovering each other mid-flight. +- **Adding CI for a domain edits the shared workflows.** Sequence it against any + other workflow work. +- **`infra/deploy/secrets/` and `infra/deploy/` are one owner between them.** + The rendered artifact and the thing that renders it move together. diff --git a/CLAUDE.md b/CLAUDE.md index 3b640aa..ddf58a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,6 +22,11 @@ every change, so a stale one is a correctness bug, not a documentation bug. `dev`, `main` and `release` are protected: **everything is a PR**, for humans and agents alike. Promotion is one PR per hop, `dev` → `main` → `release`. +`.claude/BRANCHING.md` is the orchestrator's runbook for that flow — the +serialised merge queue, promotions, hotfix down-merges — and +`.claude/ownership.md` is the module map saying which tasks may run +concurrently. Read both before dispatching parallel work. + **`dev` is a first-class hosted environment, not just an integration branch.** It runs on vps1 — the same box as Forgejo and the monitoring stack — with its own Postgres container, reachable only on the WireGuard mesh From 1d4defefb9701535ed8c9249084ee67bf2b89ed5 Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Sun, 26 Jul 2026 15:05:26 -0700 Subject: [PATCH 2/3] BRANCHING: the escape hatch, now that apply_to_admins binds the owner too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Protection an admin walks through is documentation rather than a control, so apply_to_admins goes on — but that removes the direct-push route for everyone, including in an emergency, and the way back in should be written down before it is needed rather than improvised. There is no force flag; the supported route is lift the rule, act, restore it, with both halves audited. Names the failure mode that actually justifies it: a renamed status context makes the required check unsatisfiable, and with apply_to_admins on that blocks every merge into dev and main with no exemption for anyone. --- .claude/BRANCHING.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/.claude/BRANCHING.md b/.claude/BRANCHING.md index f4b7366..e602fbc 100644 --- a/.claude/BRANCHING.md +++ b/.claude/BRANCHING.md @@ -179,6 +179,35 @@ else; say plainly where it came from rather than implying a check passed. A hotfix that touches only one domain deploys only that domain. Check the other half's `/api/version` before assuming the fix is live end to end. +### When you genuinely need to push directly + +`apply_to_admins` is on, which means these rules bind the owner too. That is +deliberate — protection an admin walks straight through is documentation, not a +control, and every account here is an admin — but it does remove the escape +hatch, so the way back in is worth writing down before it is needed at 3am. + +There is no force flag. The supported route is to lift the rule, do the thing, +and put it back: + +``` +forge_branch_protection(branch="dev", delete=true) # rule gone; direct push allowed +git push origin dev # the emergency change +forge_branch_protection(branch="dev", apply=true) # restored to policy +``` + +Both the removal and the restore are audited, which is the point: the hatch is +open on the record rather than by a quiet exception. Restore it in the same +session — a protection rule "temporarily" removed is how a repo ends up +unprotected for a month. + +The failure mode that actually justifies this: if the required status context +is ever renamed and no longer matches, **nothing can merge into `dev` or `main` +at all**, including by you, because `apply_to_admins` no longer exempts anyone. +`forge_pr_await` and `forge_pr_merge` both name that case explicitly when they +see it. The fix is to correct `pr-build.yml`'s workflow/job names and +`CENTRALIS_REQUIRED_CHECK` together — but if you need to land the fix itself, +lift the rule as above. + ## Guardrails - Never force-push `dev`, `main` or `release`. From deb039ee249f9c9595a1836c42492082aaec3327 Mon Sep 17 00:00:00 2001 From: emi Date: Mon, 27 Jul 2026 00:56:43 +0000 Subject: [PATCH 3/3] accounts: sign in with Google, on a shared provider-agnostic OAuth engine (#122) --- backend/accounts/models.py | 62 +- backend/accounts/oauth.py | 528 ++++++++++++++++++ backend/accounts/schemas.py | 22 +- .../alembic/versions/0005_oauth_accounts.py | 104 ++++ backend/notifications/discord_link.py | 342 +----------- backend/tests/accounts/test_oauth.py | 351 ++++++++++++ .../tests/notifications/test_discord_dm.py | 13 +- .../tests/notifications/test_discord_link.py | 190 +++---- .../tests/notifications/test_discord_login.py | 251 --------- backend/web/app.py | 9 +- frontend/static/account.js | 184 ++++-- frontend/static/style.css | 22 +- infra/deploy/thermograph.env.example | 12 + 13 files changed, 1333 insertions(+), 757 deletions(-) create mode 100644 backend/accounts/oauth.py create mode 100644 backend/alembic/versions/0005_oauth_accounts.py create mode 100644 backend/tests/accounts/test_oauth.py delete mode 100644 backend/tests/notifications/test_discord_login.py 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) => ` + `).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