Add an admin-issued magic link that bypasses Discord login

johnsturgeon commented

2026-09-09 20:43:44 +02:00

Owner

Discord OAuth is the only way into the pool, so a player who loses access
to their Discord account -- 2FA on a phone that stops receiving SMS the
moment they leave the country -- has no route back in and nothing they can
do about it themselves.

A key button next to the welcome button on Admin -> Players emails that
player a link good for 24 hours. The token is their Discord id, signed with
SESSION_SECRET_KEY, so there is no table, no row to expire and nothing to
clean up; pressing the button twice is harmless. Redeeming it sets exactly
the cookie the OAuth callback sets, which is now minted in one shared
helper so the two ways in cannot drift apart.

The id inside the token is readable, which costs nothing: tgfp-discord-id
is itself an unsigned cookie holding that same id, so the link discloses
nothing its own destination does not hand over. The signature is the part
that matters, and it is what stops a link for one player being edited into
a link for another.

The button is disabled for a Pending player: the link resolves through
by_discord_id, so a NULL there would make it a dead end rather than a login.

resolves #502

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

Discord OAuth is the only way into the pool, so a player who loses access to their Discord account -- 2FA on a phone that stops receiving SMS the moment they leave the country -- has no route back in and nothing they can do about it themselves. A key button next to the welcome button on Admin -> Players emails that player a link good for 24 hours. The token is their Discord id, signed with SESSION_SECRET_KEY, so there is no table, no row to expire and nothing to clean up; pressing the button twice is harmless. Redeeming it sets exactly the cookie the OAuth callback sets, which is now minted in one shared helper so the two ways in cannot drift apart. The id inside the token is readable, which costs nothing: tgfp-discord-id is itself an unsigned cookie holding that same id, so the link discloses nothing its own destination does not hand over. The signature is the part that matters, and it is what stops a link for one player being edited into a link for another. The button is disabled for a Pending player: the link resolves through by_discord_id, so a NULL there would make it a dead end rather than a login. resolves #502 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

johnsturgeon added 1 commit

2026-09-09 20:43:44 +02:00

Add an admin-issued magic link that bypasses Discord login

Claude Code Review / claude-review (pull_request) Successful in 1m55s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 28s

Details

Tests / pytest (pull_request) Successful in 16s

Details

b587b2c089

Discord OAuth is the only way into the pool, so a player who loses access
to their Discord account -- 2FA on a phone that stops receiving SMS the
moment they leave the country -- has no route back in and nothing they can
do about it themselves.

A key button next to the welcome button on Admin -> Players emails that
player a link good for 24 hours. The token is their Discord id, signed with
SESSION_SECRET_KEY, so there is no table, no row to expire and nothing to
clean up; pressing the button twice is harmless. Redeeming it sets exactly
the cookie the OAuth callback sets, which is now minted in one shared
helper so the two ways in cannot drift apart.

The id inside the token is readable, which costs nothing: tgfp-discord-id
is itself an unsigned cookie holding that same id, so the link discloses
nothing its own destination does not hand over. The signature is the part
that matters, and it is what stops a link for one player being edited into
a link for another.

The button is disabled for a Pending player: the link resolves through
by_discord_id, so a NULL there would make it a dead end rather than a login.

resolves #502

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

johnsturgeon commented

2026-09-09 20:45:39 +02:00

Author

Owner

This is a clean, well-tested change. Here is my review.

This is a well-scoped, thoroughly tested feature: the stateless signed-token design is sound, the cookie-minting consolidation into _login_cookie is a genuine improvement, and the dev-recipient rewrite is both refactored and fixed for display-name addresses. I found no blocking issues; a few minor points below.

Worth fixing

The consolidated auth cookie sets no httponly / secure / samesite flags — app/routers/auth.py:77-81. This is pre-existing behavior extracted verbatim, but this PR is the moment the app's only session-identifying cookie (tgfp-discord-id) gets minted in exactly one place, so it's the natural place to harden it. Without httponly, any XSS can exfiltrate the cookie; without secure, it can traverse plain HTTP. Adding httponly=True, secure=True, samesite="lax" here fixes both login paths at once. Out of strict scope, but cheap and central now.

Nits

read_magic_token catches (BadSignature, ValueError) but not the broader itsdangerous.BadData — app/magic_link.py:27. In practice BadPayload/BadData is unreachable here (an attacker can't produce a valid signature to reach payload deserialization, and expired/tampered tokens raise SignatureExpired/BadSignature, both subclasses of BadSignature), so this is not a correctness bug. Catching BadData would just be marginally more robust and self-documenting.
No test for the magic_failed branch — app/routers/v2.py:572-573. The mailer-raises path (ConnectionError/OSError → magic_failed) is untested. resend_welcome has the same gap, so this only matches existing coverage rather than falling short of it, but a monkeypatched send_magic_link_email that raises would close it in a couple of lines.
Neither login path checks player.active — app/routers/auth.py:114 (and the OAuth callback at :97). A deactivated player who still has a Discord id and a row can be issued and redeem a magic link, and the admin button is enabled for them. This is consistent with the existing OAuth behavior, so it's not a regression — just worth a conscious decision about whether the key button should be gated on active too.
Unclosed <li> tags in app/templates/email_magic_link.j2:20-27 — valid HTML5 (closing tags are optional), purely cosmetic.

Nice touches worth calling out: the stateless token means email link-scanners/prefetchers can't burn the link, the Pending-player dead-end is handled explicitly and tested, and the tamper/forged-key/expiry cases all have direct unit tests.

This is a clean, well-tested change. Here is my review. --- ## Review: Add an admin-issued magic link that bypasses Discord login This is a well-scoped, thoroughly tested feature: the stateless signed-token design is sound, the cookie-minting consolidation into `_login_cookie` is a genuine improvement, and the dev-recipient rewrite is both refactored and fixed for display-name addresses. I found no blocking issues; a few minor points below. ### Worth fixing - **The consolidated auth cookie sets no `httponly` / `secure` / `samesite` flags** — `app/routers/auth.py:77-81`. This is pre-existing behavior extracted verbatim, but this PR is the moment the app's only session-identifying cookie (`tgfp-discord-id`) gets minted in exactly one place, so it's the natural place to harden it. Without `httponly`, any XSS can exfiltrate the cookie; without `secure`, it can traverse plain HTTP. Adding `httponly=True, secure=True, samesite="lax"` here fixes both login paths at once. Out of strict scope, but cheap and central now. ### Nits - **`read_magic_token` catches `(BadSignature, ValueError)` but not the broader `itsdangerous.BadData`** — `app/magic_link.py:27`. In practice `BadPayload`/`BadData` is unreachable here (an attacker can't produce a valid signature to reach payload deserialization, and expired/tampered tokens raise `SignatureExpired`/`BadSignature`, both subclasses of `BadSignature`), so this is not a correctness bug. Catching `BadData` would just be marginally more robust and self-documenting. - **No test for the `magic_failed` branch** — `app/routers/v2.py:572-573`. The mailer-raises path (`ConnectionError`/`OSError` → `magic_failed`) is untested. `resend_welcome` has the same gap, so this only matches existing coverage rather than falling short of it, but a monkeypatched `send_magic_link_email` that raises would close it in a couple of lines. - **Neither login path checks `player.active`** — `app/routers/auth.py:114` (and the OAuth callback at `:97`). A deactivated player who still has a Discord id and a row can be issued and redeem a magic link, and the admin button is enabled for them. This is consistent with the existing OAuth behavior, so it's not a regression — just worth a conscious decision about whether the key button should be gated on `active` too. - **Unclosed `<li>` tags in `app/templates/email_magic_link.j2:20-27`** — valid HTML5 (closing tags are optional), purely cosmetic. Nice touches worth calling out: the stateless token means email link-scanners/prefetchers can't burn the link, the Pending-player dead-end is handled explicitly and tested, and the tamper/forged-key/expiry cases all have direct unit tests.

johnsturgeon added 1 commit

2026-09-09 21:00:20 +02:00

Fix the development recipient rewrite for display-name addresses

Claude Code Review / claude-review (pull_request) Successful in 1m43s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 30s

Details

Tests / pytest (pull_request) Successful in 15s

Details

a00efedfbc

The rewrite landed in #501 split the whole recipient string on its "@",
which is right for a bare address and wrong for the RFC 5322 form every
caller going through Player.email_recipient produces. "John Sturgeon
<john@sturgeon.me>" came out as "John Sturgeon <john@tgfp.us", with the
closing bracket dropped, and the admin comparison never matched the
display form either, so the admin's own mail was redirected too.

parseaddr the recipient, rewrite only the address, and formataddr it back.

Also drops the explanatory comments from the magic-link work earlier on
this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

johnsturgeon added 1 commit

2026-09-09 21:07:22 +02:00

Ignore the Tailwind toolchain

Claude Code Review / claude-review (pull_request) Successful in 1m54s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 29s

Details

Tests / pytest (pull_request) Successful in 16s

Details

Release on merge / release (pull_request) Successful in 1s

Details

701fabe4a6

node_modules/ and bin/ hold the npm install and the 79MB tailwindcss
binary that builds v3.css. Neither belongs in the repo, and both were
swept into a commit by a `git add -A`.

app/static/v3.css is deliberately not ignored: the Dockerfile has no node
step, so the built stylesheet ships from the repo the same way
mainstyle.css and v2.css do.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

johnsturgeon merged commit b204d7da6a into main

2026-09-09 21:08:58 +02:00

johnsturgeon deleted branch add-the-ability-for-the-admin-to-send-a-magic-link-live-for-24-hours-to-bypass-login-502

2026-09-09 21:08:59 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-09 21:09:00 +02:00

Merge pull request 'Add an admin-issued magic link that bypasses Discord login' (#503) from add-the-ability-for-the-admin-to-send-a-magic-link-live-for-24-hours-to-bypass-login-502 into main

Rows
Columns

Add an admin-issued magic link that bypasses Discord login #503

Review: Add an admin-issued magic link that bypasses Discord login

Worth fixing

Nits