Slides signs in through Keymail, without revealing who is admitted
Every Keymail-classified address, permitted or not, gets the same continuation page to Keymail (never a 303 from the form, which Safari's form-action blocks); other addresses get ?sent=1. Admission is checked after verification in a bounded, non-blocking mail queue, so no response waits on SMTP or branches on the allowlist. A refused address sees the door with an error, rendered in the refusal itself, not bare text and not a spoofable ?err=refused. err=keymail still falls back to an email link. Reviewed by GPT-6-Astra via Codex over four rounds (#20). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Squashed from fix/keymail-signin into main.
21 files changed,
+1576
−138
AGENTS.md+12 −6internal/slides/app.go+17 −7internal/slides/auth.go+222 −96internal/slides/auth_test.go+9 −3internal/slides/config.go+11 −10internal/slides/config_test.go+1 −0internal/slides/handlers.go+15 −0internal/slides/home_test.go+1 −0internal/slides/keymail_home_test.go+122 −0internal/slides/keymailtest/keymailtest.go+190 −0internal/slides/locales/en.toml+6 −1internal/slides/mailqueue.go+283 −0internal/slides/mailqueue_test.go+283 −0internal/slides/render.go+3 −0internal/slides/templates/signin.html+11 −8internal/slidestest/browser_test.go+1 −0internal/slidestest/capabilities_test.go+1 −0internal/slidestest/harness_test.go+10 −0internal/slidestest/keymail_signin_test.go+246 −0internal/slidestest/signin_test.go+7 −7internal/slidestest/signin_timing_test.go+125 −0
CI passed — run details
CI log
=== gate ===
=== rust: fmt
=== rust: clippy
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
=== wasm: the artifact the host embeds
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
Finished `release` profile [optimized] target(s) in 0.07s
core/target/wasm32-unknown-unknown/release/slides_core.wasm (1228229 bytes)
=== the embedded wasm must match a fresh build
=== rust: test
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.04s
Running unittests src/lib.rs (core/target/debug/deps/slides_core-929bfceb2edd4a94)
running 30 tests
test layout::tests::body_region_rect_narrows_for_an_aside ... ok
test layout::tests::default_region_by_kind ... ok
test layout::tests::heading_region_rect ... ok
test layout::tests::aside_region_rect ... ok
test layout::tests::media_region_rect ... ok
test markdown::tests::empty_input_is_no_paragraphs_and_lines_keep_blanks ... ok
test markdown::tests::inline_marks ... ok
test markdown::tests::marks_nest_and_links_carry_marks ... ok
test markdown::tests::paragraphs_split_on_blank_lines_and_list_markers ... ok
test check::tests::the_empty_document_passes ... ok
test schema::tests::an_empty_map_reads_as_a_set_mark ... ok
test schema::tests::an_href_map_without_a_string_url_reads_as_no_link ... ok
test schema::tests::href_is_written_as_a_one_key_map_and_reads_back ... ok
test layout::tests::to_canvas_narrows_body_when_an_aside_is_present ... ok
test layout::tests::to_flow_is_a_no_op_when_already_in_order ... ok
test layout::tests::to_flow_classifies_by_position_and_kind ... ok
test layout::tests::to_canvas_splits_a_shared_region_equally_and_sets_z_by_index ... ok
test layout::tests::to_flow_orders_y_then_x ... ok
test schema::tests::href_written_as_a_plain_string_still_reads_back ... ok
test check::tests::what_the_ops_write_passes ... ok
test check::tests::bare_text_at_fragment_level_is_reported ... ok
test check::tests::a_non_paragraph_child_is_reported ... ok
test view::tests::assets_are_typed_or_skipped ... ok
test view::tests::malformed_background_is_the_default ... ok
test check::tests::y_prosemirror_shapes_pass ... ok
test view::tests::malformed_fonts_are_null ... ok
test view::tests::malformed_box_is_null_and_phone_keeps_only_typed_fields ... ok
test check::tests::an_unknown_formatting_attribute_is_reported ... ok
test check::tests::an_element_inside_a_paragraph_is_reported ... ok
test layout::tests::to_flow_keeps_a_concurrent_text_edit ... ok
test result: ok. 30 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running tests/no_host_imports.rs (core/target/debug/deps/no_host_imports-184c0d8bf00e14f9)
running 1 test
test no_host_imports ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running tests/ops.rs (core/target/debug/deps/ops-256addce27f366d1)
running 12 tests
test add_slide_then_view_lists_it ... ok
test ops_are_atomic ... ok
test add_block_with_markdown_produces_runs ... ok
test remove_block_drops_it_from_columns_children ... ok
test layout_switch_assigns_boxes ... ok
test update_roundtrip ... ok
test columns_children_must_name_blocks_on_the_slide ... ok
test columns_children_have_one_parent_and_no_cycles ... ok
test duplicate_slide_remaps_columns_children ... ok
test move_block_keeps_a_concurrent_text_edit ... ok
test move_slide_keeps_a_concurrent_text_edit ... ok
test remaining_ops_roundtrip ... ok
test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
Doc-tests slides_core
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
=== rust: fmt (pdf)
=== rust: clippy (pdf)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
=== wasm: the pdf artifact the host embeds
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
Finished `release` profile [optimized] target(s) in 0.10s
core/pdf/target/wasm32-unknown-unknown/release/slides_pdf.wasm (5301002 bytes)
=== the embedded pdf wasm must match a fresh build
=== rust: test (pdf)
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.06s
Running unittests src/lib.rs (core/pdf/target/debug/deps/slides_pdf-891fafaa44f9fd64)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running tests/no_host_imports.rs (core/pdf/target/debug/deps/no_host_imports-17ccbb1d62120093)
running 1 test
test no_host_imports ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
Running tests/render.rs (core/pdf/target/debug/deps/render-2e1d7e8b19d78595)
running 6 tests
test a_null_zero_length_pair_is_no_frames_not_a_dereference ... ok
test text_with_no_fonts_at_all_still_renders_the_shapes ... ok
test raster_image_is_embedded ... ok
test abi_frames_round_trip ... ok
test two_svgs_make_two_pages ... ok
test renders_text_as_embedded_glyphs ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s
Doc-tests slides_pdf
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
=== fixtures: node dependencies
v24.19.0
11.17.0
=== fixtures: npm ci (needs the registry — see .amadan/README.md)
added 19 packages, and audited 20 packages in 544ms
5 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
npm warn allow-scripts 1 package has install scripts not yet covered by allowScripts:
npm warn allow-scripts esbuild@0.28.2 (postinstall: node install.js)
npm warn allow-scripts
npm warn allow-scripts Run `npm approve-scripts --allow-scripts-pending` to review, or `npm approve-scripts <pkg>` to allow.
=== fixtures: the JS side's own tests
✔ one finished upload is register_asset plus the block that uses it, in one batch (12.577127ms)
✔ a record missing its numbers still makes an op the core accepts (0.969006ms)
✔ unusedAssets is what no slide refers to, and remove_asset drops one (2.565316ms)
✔ the Unused images list reconciles by id and names each Remove button (0.877336ms)
✔ an upload refusal is said in words the person can act on (0.243782ms)
✔ clampBox keeps the box inside the frame and no smaller than the minimum (1.404063ms)
✔ fixBox writes four decimals (0.365568ms)
✔ guides are the thirds plus every other block edge (0.250344ms)
✔ snapMove pulls the nearest edge onto a guide within one per cent, and leaves a far one (0.379173ms)
✔ snapResize moves only the edges the handle drags (0.216211ms)
✔ moveBox and resizeBox by handle (0.233874ms)
✔ nudge: arrows move by half a per cent, Shift by two, Alt resizes (0.211552ms)
✔ rectBox reads a block rectangle against its frame (0.198017ms)
✔ typing into a block is a text change on its slide only (9.683563ms)
✔ a block's props changing is structure on its slide (3.089268ms)
✔ adding, moving or removing a block is structure on its slide (1.841825ms)
✔ the notes are text on their slide (1.10048ms)
✔ a slide added, removed or moved is an order change (1.535877ms)
✔ one transaction touching several things reports each once (0.892996ms)
✔ what the ops write passes, and so does an empty document (9.726502ms)
✔ the shapes y-prosemirror and older documents write pass (1.928356ms)
✔ an unknown formatting attribute is reported with the core's message (1.804165ms)
✔ an element inside a paragraph, a non-paragraph child and bare text are reported (2.727436ms)
✔ the unknownmark poison shape is what the check refuses (1.628759ms)
✔ y-prosemirror 1.3.7 deletes the words under an unknown mark; sanitising first keeps them (5.117267ms)
✔ the vocabulary is the five marks and nothing else (0.391316ms)
✔ a reload reuses the id this tab stored for the deck (1.060305ms)
✔ two tabs, and two decks in one tab, get different ids (0.227331ms)
✔ a stored value that is not an id, or storage that throws, gives a fresh id (0.416432ms)
✔ a duplicated tab gives up the copied id; the original keeps it (8.389154ms)
✔ a reload finds no rival and keeps its id (2.583459ms)
✔ presence switches id: beats carry the new one and the old one is somebody else (2.761069ms)
✔ an empty body is no frames (1.675416ms)
✔ frames come back in order with their exact bytes (0.384463ms)
✔ a length prefix is big-endian (0.205812ms)
✔ a truncated frame throws rather than returning a short update (1.464786ms)
✔ deck controls are set_meta (16.347748ms)
✔ slide controls are set_slide, and background keeps its colour (2.462725ms)
✔ move up and down are move_slide with the right after (2.212211ms)
✔ block props: value kinds, region, a shape label (2.096738ms)
✔ box fields are set_block on the desktop and set_phone in the phone view (14.006688ms)
✔ move block and the layer buttons (1.934567ms)
✔ add block: region after the selection on flow, a default box on canvas, a heading level that fits (6.086854ms)
✔ a block is named by its type and its first words (1.00856ms)
✔ blockChange: a control element to its op (2.489255ms)
✔ the Blocks list reconciles by id and keeps its buttons (1.208741ms)
✔ open resolves to the button that closed it and the trimmed field (1.651773ms)
✔ a second open while the dialog is up joins the first (0.46938ms)
✔ a missing dialog mounts as nothing (0.126244ms)
✔ a tab advances its own cursor only when its POST is the very next row (7.943347ms)
✔ the cursor never moves backwards when a response is overtaken by this tab's own write (0.565508ms)
✔ a later response still carries the cursor forward (0.303233ms)
✔ a burst of notifications collapses into one fetch at a time (0.281462ms)
✔ an event naming a seq the tab already has fetches nothing (0.698525ms)
✔ a stopped subscriber drops a response that arrives late (0.309313ms)
✔ stop closes the stream, and start opens exactly one (0.234354ms)
✔ a hidden tab closes its stream on a shortened deadline, and activity reopens it (0.957435ms)
✔ switching away and back inside the grace never drops the stream, so nothing has to be caught up (0.276302ms)
✔ a visible tab that is only being READ closes when idle and comes back on any activity (1.548621ms)
✔ activity while streaming defers the idle close rather than reopening anything (0.293484ms)
✔ a stream failure is never reported as a save failure, and heals on activity (0.2533ms)
✔ an empty response advances the cursor without applying anything (0.357753ms)
✔ a failed fetch leaves the cursor alone so the next notification retries it (2.120732ms)
✔ a decode failure retries once through the raw log, then gives up (0.270131ms)
✔ updatesURL spells the query string exactly, in particular raw as the literal raw=1 (0.167661ms)
✔ empty_doc_views_as_defaults (10.485299ms)
✔ set_meta (3.394133ms)
✔ slides: add, move, remove, set_slide, set_notes (6.498037ms)
✔ duplicate_slide (3.844117ms)
✔ blocks: add, move, remove, set_block (4.031064ms)
✔ set_phone (1.43991ms)
✔ table cells follow rows and cols (1.620484ms)
✔ canvas blocks: box defaults from the region, z from the index (0.963356ms)
✔ assets: register and remove (0.745302ms)
✔ markdown: every mark, code blocks are plain lines (1.666269ms)
✔ batch_is_atomic (5.558887ms)
✔ a batch applies in one transaction with the given origin (1.746648ms)
✔ columns_children_validated (1.188403ms)
✔ layout_switch_round_trip (1.752108ms)
✔ view_matches_snapshot_fixtures (16.00855ms)
✔ columns children have one parent and no cycles (codex 7) (1.239378ms)
✔ canvas-to-flow keeps a concurrent text edit (codex 4) (4.070216ms)
✔ move_block keeps a concurrent text edit, and later inserts land where after says (codex 4) (3.205592ms)
✔ move_slide keeps a concurrent text edit, and later inserts land where after says (codex 4 at slide level) (2.564254ms)
✔ format: {name} and {n} interpolate; an unknown key is left as text (0.79837ms)
✔ a beat on connect, then one every twenty seconds (1.923355ms)
✔ a move posts once, 200ms after the last of a burst, and re-arms the beat (0.268056ms)
✔ a hidden tab stops beating; coming back posts at once (0.354177ms)
✔ applySet leaves this tab out and answers what changed (0.580185ms)
✔ a set with rubbish in it is read for what it does carry (0.186856ms)
✔ initials, and a client id per tab (3.150481ms)
✔ a resync that fails is retried with backoff until it succeeds, without ever running twice at once (2.555517ms)
✔ the browser's online event retries immediately, not just on the next backoff tick (0.336584ms)
✔ reportDivergence before any failure runs the first attempt without a caller ever calling online (1.60735ms)
✔ a permanent 4xx drops its batch instead of retrying it, and does not poison the next edit (84.891655ms)
✔ a network failure or a 5xx is retried, not dropped (60.362779ms)
✔ a 500 is retried the same way a network failure is, never dropped (60.326492ms)
✔ the core-written structure reads as ProseMirror marks (5.091288ms)
✔ a y-prosemirror round trip preserves bold and the link (2.073925ms)
✔ y-prosemirror writes href as the one-key map and marks as {} (0.790606ms)
✔ a plain-string href, written before the map shape, still reads (0.45294ms)
✔ textFragment writes the same shape as writeText (0.372711ms)
✔ safeHref is the server allowlist, and a disallowed href never draws an anchor (0.277003ms)
✔ parseHash splits the slide and the block, and survives a stray % (1.628129ms)
✔ destroy takes the listeners off, and a second mount leaves one of each (0.698685ms)
✔ formatHash is the inverse (1.489552ms)
✔ textBlocks finds the text-bearing blocks and skips the rest (1.764592ms)
✔ mountTarget is the element whose children are the paragraphs (0.228113ms)
✔ paragraphTag keeps the renderer's own markup per type (0.145861ms)
✔ blockFragment finds a block's text by slide and block id (6.603332ms)
✔ notesFragment finds the slide's notes (1.069984ms)
✔ blockIDs and slideIDs are the orders on the stage and the strip (0.996758ms)
✔ cellFragments are a table's cells, row-major (1.486436ms)
✔ a load that fails leaves the stage on the slide it was showing and says so (1.530317ms)
✔ slideAfter is the slide to show once one is gone (0.219386ms)
✔ the queue words map to savestatus states, failed staying failed (0.965039ms)
✔ a dropped batch outranks the queue, for the rest of the session (0.223675ms)
✔ the editor status: the save line and the load error are two lines (0.600283ms)
✔ mountSaveStatus loads estilo's module and mounts it on the line (0.975028ms)
✔ a save-status module that will not load says so and refuses to mount (0.461015ms)
✔ dropTarget: before puts it ahead of the item, after behind it, null means first (1.281506ms)
✔ a drop that leaves the order as it is is no move (0.235367ms)
✔ dropOp is the move_slide (0.859714ms)
✔ labelFor is the slide's first heading, else the numbered fallback (4.448077ms)
✔ flush re-points every dirty thumbnail and relabels it (1.680737ms)
✔ destroy takes the item-menu listener off; a second mount leaves one (0.287443ms)
✔ plan says what to add, drop and where each item goes (0.672877ms)
✔ sync moves only the items out of place and keeps focus (1.034317ms)
✔ desktop view: box and size are the block; hidden and order are always the phone map (2.045131ms)
✔ phone view: everything goes to set_phone, carrying the rest of the map (0.51749ms)
✔ the ops it makes are ones the document takes (10.828927ms)
✔ phoneMap reads only the four keys, and only well-typed (0.20524ms)
✔ the stored view is read defensively (0.352684ms)
✔ linkAt reads the link under the cursor and nothing elsewhere (2.124489ms)
✔ setLink marks the selection (1.885406ms)
✔ setLink on a collapsed cursor inside a link replaces that link (0.530964ms)
✔ setLink with nothing selected and no link does nothing (0.16719ms)
✔ removeLink clears the link under the cursor (0.347163ms)
✔ the keymap binds every documented shortcut (0.186696ms)
✔ Mod-b toggles bold through the keymap (0.458951ms)
✔ setLink refuses an address outside the allowlist and writes nothing (0.43138ms)
✔ the state machine runs choose → sign → put → register → op (2.958985ms)
✔ a file the deck will not take never reaches the network (0.742597ms)
✔ alt text is required before the bytes move, unless the image is decorative (0.354767ms)
✔ a step that fails stops the ones after it and reports its own code (0.627463ms)
✔ a failure that is not ours still becomes an UploadError (0.389933ms)
✔ a second pick while one is in flight is refused, not raced (0.541584ms)
✔ an unreadable size is zero rather than a failure (0.319453ms)
✔ a per-call applyOp wins, so an upload lands on the block it was chosen for (0.346012ms)
✔ checkFile takes exactly the four types (0.505777ms)
✔ sha256Hex is the hex digest of the bytes (12.591103ms)
✔ refusal reads this app's JSON error shape, and survives one that is not (0.35086ms)
✔ malformed fonts are null (3.084659ms)
✔ a malformed background is the theme default (1.531419ms)
✔ a malformed box is null (0.764668ms)
✔ the phone layer keeps only its typed fields, order as a whole number (0.546233ms)
✔ assets are typed or skipped (0.341333ms)
✔ every poison update is deterministic bytes that apply to any deck (7.425858ms)
ℹ tests 158
ℹ suites 0
ℹ pass 158
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 306.340889
=== the vectors must match a fresh generation
canvas: 22 ops, 3715 bytes
columns: 23 ops, 3134 bytes
every-block: 36 ops, 3180 bytes
table: 14 ops, 1509 bytes
title-only: 6 ops, 1136 bytes
=== the editor bundle must match a fresh build
wrote internal/slides/static/editor.0463180baf5aab9f.js (376416 bytes)
=== go: vet, gofmt, test, migration check
go vet ./...
go test ./...
? amadan.net/oficina/slides/cmd/slides [no test files]
ok amadan.net/oficina/slides/internal/slides 12.561s
ok amadan.net/oficina/slides/internal/slides/core 2.861s
ok amadan.net/oficina/slides/internal/slides/deck 0.003s
ok amadan.net/oficina/slides/internal/slides/fonts 0.016s
? amadan.net/oficina/slides/internal/slides/icons [no test files]
? amadan.net/oficina/slides/internal/slides/keymailtest [no test files]
ok amadan.net/oficina/slides/internal/slides/layout 0.019s
ok amadan.net/oficina/slides/internal/slides/live 0.047s
ok amadan.net/oficina/slides/internal/slides/pdfcore 4.185s
ok amadan.net/oficina/slides/internal/slides/present 0.002s
ok amadan.net/oficina/slides/internal/slides/render 0.018s
ok amadan.net/oficina/slides/internal/slides/svg 0.012s
ok amadan.net/oficina/slides/internal/slidestest 38.080s
? amadan.net/oficina/slides/scripts/writeestilo [no test files]
go run amadan.net/rastrillo/rastrillo/cmd/rastrillo migration check
rastrillo migration check: models and migrations agree
python3 scripts/credits.py --check
credits: checked
gate: ok
diff --git a/AGENTS.md b/AGENTS.md| index 24aa65b..58547a9 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -44,12 +44,18 @@ Slices 1, 2 and 3. A deck is edited in the browser at `/d/{sqid}/edit`, |
| driven through the API (`docs/api.md`), or arrives as the sample deck |
| seeded on first sign-in — and comes back out as a PDF. |
| -- **Sign-in** is a magic link (`internal/slides/auth.go`), gated by |
| - `SLIDES_ALLOWED_ADDRESSES` in front of `POST /signin` as well as in |
| - rastrillo/auth's `Authorize`. The gate in front matters more, because |
| - that is the one that stops the mail; a non-permitted address gets |
| - byte-for-byte the same `303 ?sent=1` a permitted one gets, or the |
| - endpoint becomes a membership oracle. The list is also **rechecked on |
| +- **Sign-in** (`internal/slides/auth.go`) is Keymail for an address |
| + whose domain runs Keymail and an emailed magic link for every other |
| + address — decided by the domain alone, never by |
| + `SLIDES_ALLOWED_ADDRESSES`, or the endpoint becomes a membership |
| + oracle. A Keymail address, permitted or not, gets the continuation |
| + page to Keymail (never a 303 out of the form POST: Safari applies |
| + `form-action` to it); any other gets `303 ?sent=1`, and only a |
| + permitted one is mailed, by a worker after the response (`mailQueue`), so not even the response time differs. Admission is |
| + rastrillo/auth's `Authorize`, after verification, and its refusal is |
| + the door, rendered in the refusal's own 403 (`doorOnRefusal`; never a query anyone could type), not rastrillo's bare |
| + text. `?force=1&err=keymail` (a failed Keymail exchange) makes the |
| + form ask for a link instead. The list is also **rechecked on |
| every request** that resolves a principal, so taking an address off it |
| signs that person out of every device and refuses their agents' tokens |
| rather than leaving them everything they already held (spec §6.1, |
diff --git a/internal/slides/app.go b/internal/slides/app.go| index 2af5061..f81d272 100644 |
| --- a/internal/slides/app.go |
| +++ b/internal/slides/app.go |
| @@ -79,9 +79,9 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| // get in. |
| if a.homeRoutes == nil { |
| r.Get(signinPath, a.signin) |
| - r.Post(signinPath, gateBegin(authPlugin.Begin, a.allowed, a.logger)) |
| - r.Get("/auth/callback", authPlugin.Callback) |
| - r.Get("/auth/verify", authPlugin.Verify) |
| + r.Post(signinPath, a.newBegin(authPlugin.Begin)) |
| + r.Get("/auth/callback", doorOnRefusal(authPlugin.Callback, a.signin)) |
| + r.Get("/auth/verify", doorOnRefusal(authPlugin.Verify, a.signin)) |
| r.Get(landedPath, a.landed) |
| r.Post("/signout", signedOutAfter(authPlugin.Signout)) |
| r.Get("/signed-out", a.signedOut) |
| @@ -89,9 +89,9 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| h := a.homeRoutes |
| r.Get(signinPath, a.homeSignin) |
| r.Get("/signin/local", a.signin) |
| - r.Post(signinPath, h.LocalGuard(gateBegin(authPlugin.Begin, a.allowed, a.logger))) |
| - r.Get("/auth/callback", h.LocalGuard(authPlugin.Callback)) |
| - r.Get("/auth/verify", h.LocalGuard(authPlugin.Verify)) |
| + r.Post(signinPath, h.LocalGuard(a.newBegin(authPlugin.Begin))) |
| + r.Get("/auth/callback", h.LocalGuard(doorOnRefusal(authPlugin.Callback, a.signin))) |
| + r.Get("/auth/verify", h.LocalGuard(doorOnRefusal(authPlugin.Verify, a.signin))) |
| r.Post("/signout", h.Signout) |
| r.Get("/signed-out", h.SignedOut) |
| r.Get(connect.LocalCompletePath, h.LocalComplete) |
| @@ -302,12 +302,17 @@ func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| logger.Info("migrate", "applied", migrated.Applied, |
| "skipped", migrated.Skipped, "adopted", migrated.Adopted) |
| - authPlugin, err := newAuthPlugin(cfg, d.Writer(), list) |
| + // The sign-in mailer: links are queued and sent after the response, |
| + // to permitted addresses only (mailqueue.go). Closed by app.Close. |
| + mailer := newMailQueue(cfg.Mailer, list.allows, nil, logger) |
| + authPlugin, err := newAuthPlugin(cfg, d.Writer(), list, mailer) |
| if err != nil { |
| + mailer.close() |
| return nil, nil, err |
| } |
| returnSeal, err := newReturnSealer(cfg.InstanceKey) |
| if err != nil { |
| + mailer.close() |
| return nil, nil, err |
| } |
| @@ -320,6 +325,7 @@ func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| // request happens to open the first deck. |
| rt, err := core.New(context.Background()) |
| if err != nil { |
| + mailer.close() |
| return nil, nil, err |
| } |
| @@ -332,6 +338,7 @@ func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| returnNow: time.Now, |
| core: rt, |
| auth: authPlugin, |
| + mail: mailer, |
| hub: present.New(), |
| live: live.New(), |
| jobs: jobs.New(logger), |
| @@ -345,12 +352,14 @@ func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| sess, err := sessions.New(sessions.Config{DB: d.Writer(), Origin: cfg.Origin, Logger: logger}) |
| if err != nil { |
| rt.Close(context.Background()) |
| + mailer.close() |
| return nil, nil, err |
| } |
| a.sessions = sess |
| if err := a.initHome(homeConfig); err != nil { |
| rt.Close(context.Background()) |
| + mailer.close() |
| return nil, nil, err |
| } |
| @@ -365,6 +374,7 @@ func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| }) |
| if err != nil { |
| rt.Close(context.Background()) |
| + mailer.close() |
| return nil, nil, err |
| } |
| a.jobPages = pages |
diff --git a/internal/slides/auth.go b/internal/slides/auth.go| index 884fea1..45772e7 100644 |
| --- a/internal/slides/auth.go |
| +++ b/internal/slides/auth.go |
| @@ -1,14 +1,24 @@ |
| -// auth.go is sign-in: rastrillo/auth's magic-link plugin, the |
| -// allowlist that gates it at both ends, and the mapping from a verified |
| +// auth.go is sign-in: rastrillo/auth's plugin, the allowlist that gates |
| +// who is mailed and who is admitted, and the mapping from a verified |
| // address to this app's own users row. |
| // |
| -// Per spec §6.1 a single instance signs people in itself — an emailed, |
| -// single-use link, no password, no credential enrolled. Deliberately |
| -// magic link only, never a passkey second factor: a passkey's relying |
| -// party id derives from this instance's own host, so a person on three |
| -// team instances would enrol three separate credentials under one app. |
| -// A magic link enrols nothing, so it creates no relying party and no |
| -// sprawl. That is a design ruling, not an omission to improve on later. |
| +// Per spec §6.1 a single instance signs people in itself, with no |
| +// password and no credential enrolled here. How an address proves |
| +// itself depends on its domain alone, through rastrillo/auth's |
| +// classification: an address whose domain runs Keymail (keymail.dev |
| +// itself, or a domain delegating through a `_keymail` TXT record) |
| +// signs in at Keymail, where its passkey lives; every other address is |
| +// emailed a single-use link. Paul ruled on 2026-10-06 that Slides uses |
| +// Keymail. Never a passkey enrolled at this instance: its relying party |
| +// id would derive from this host, so a person on three team instances |
| +// would hold three credentials for one app. Keymail keeps the |
| +// credential at Keymail's own origin, and a magic link enrols nothing. |
| +// |
| +// Admission is a separate question from the ceremony, asked at two |
| +// places that cannot be observed from the form: the mail worker |
| +// (mailQueue — a non-permitted address is never mailed) and |
| +// rauth's Authorize, after Keymail or the link has verified the |
| +// address. Neither changes what POST /signin answers (gateBegin). |
| // |
| // This file is a port of oficina/docs' internal/docs/auth.go, adapted |
| // to rastrillo v0.26.0 and to Config.AllowedAddresses being a slice |
| @@ -138,89 +148,189 @@ func resolveAllowlist(cfg Config) (allowlist, error) { |
| return allowlist{}, fmt.Errorf("slides: Config.AllowedAddresses is empty and Env is %q, not %q — refusing to start with no allowlist: an instance admitting nobody is indistinguishable from one working correctly, since every POST /signin answers the same 303 either way; set $SLIDES_ALLOWED_ADDRESSES", cfg.Env, EnvDevelopment) |
| } |
| -// gateBegin wraps rauth.Auth.Begin so a non-permitted address never |
| -// reaches it at all. Begin is what sends the mail; |
| -// rauth.Config.Authorize runs only afterwards, inside rauth's own |
| -// admit(), at the callback — by which point a link has already been |
| -// mailed and clicked. Gating only Authorize would leave this instance's |
| -// sending identity an open relay for any address a stranger types into |
| -// the form. |
| -// |
| -// THE GUARANTEE: every answer this endpoint gives is byte-identical — |
| -// `303 Location: /signin?sent=1`, no body — whatever address was |
| -// submitted and whatever happened next. Any observable difference at |
| -// all would turn this into a membership oracle, letting anyone learn |
| -// which addresses this instance admits by watching how its answers |
| -// differ. |
| +// gateBegin is POST /signin: rauth.Auth.Begin for EVERY address, |
| +// permitted or not, so what the form answers depends on the address's |
| +// domain and never on the allowlist. That is the whole guarantee, and |
| +// any difference at all would turn this endpoint into a membership |
| +// oracle, letting anyone learn who this instance admits by watching how |
| +// its answers differ. |
| // |
| -// Making the denied path match the happy path is only half of that, and |
| -// the half that is easy to get wrong is the other one: the PERMITTED |
| -// path has outcomes the denied path can never reach. `signin.Flow.Begin` |
| -// enforces a per-address budget (5 in a rolling 15 minutes) and a |
| -// per-IP one (20), and over either budget rauth redirects to |
| -// `?err=rate` instead — so six POSTs of one candidate address would |
| -// separate "on the list" (`?err=rate` on the sixth) from "not on it" |
| -// (`?sent=1` forever), and the whole guarantee with it. `?err=address` |
| -// and `?err=1` are the same shape of leak: only a permitted address can |
| -// reach either. |
| +// Every address therefore takes the same road through Begin: the same |
| +// per-IP and per-address budgets, the same classification, the same |
| +// ceremony. |
| // |
| -// So the permitted branch runs under sameAnswer, which rewrites every |
| -// one of those outcomes back to `?sent=1` on the way out. What that |
| -// costs is inline feedback: a person over the budget, or one whose |
| -// address rauth could not parse, is told a link is on its way when it |
| -// is not. On a closed allowlist that was already true — such an address |
| -// is almost always denied and short-circuited above — so the loss is |
| -// confined to development's admit-everyone list, where the log carries |
| -// the real outcome instead. That is the trade, made deliberately: a |
| -// recoverable "wait five minutes and try again" that nobody sees, in |
| -// exchange for an allowlist nobody can enumerate. |
| +// - A Keymail address — permitted or not — goes to Keymail. Begin |
| +// answers with a 303 to Keymail's authorize URL, which newBegin's |
| +// continuation turns into a page (Safari applies form-action to the |
| +// whole redirect chain of a form POST, and ours is 'self'). Whether |
| +// the address may sign in here is asked only after Keymail has |
| +// verified it, by rauth's Authorize, and a refusal is the sign-in |
| +// door saying so (doorOnRefusal) — told only to somebody who has |
| +// just proved they hold the address. |
| +// - Any other address is answered `303 /signin?sent=1`, and a link is |
| +// mailed only if it is permitted: mailQueue drops the rest, after |
| +// the response, so not even its timing differs (mailqueue.go). |
| +// So this instance's sending identity is no open relay for whatever |
| +// a stranger types, which is why a gate exists at all |
| +// (rauth.Config.Authorize alone runs only after a link was mailed |
| +// and clicked). |
| // |
| -// The denied branch deliberately spends no budget of its own. That is |
| -// not an oversight — a budget consumed only by denied addresses would |
| -// itself be observable, since a permitted address's own budget would |
| -// then behave differently depending on what else had been tried. |
| -// Rate limiting the front door as a whole belongs at the edge, not |
| -// here, and the per-IP budget inside Begin already bounds what a |
| -// permitted address can spend. |
| +// force (any non-empty value) is passed through as the form sent it. |
| +// Only the recovery form carries it — the page rastrillo lands a failed |
| +// Keymail exchange on, `?force=1&err=keymail` — and it selects an |
| +// emailed link over Keymail, so a person whose Keymail failed is never |
| +// stuck. A stranger posting force for any address gets exactly the |
| +// plain-email answer above. |
| // |
| -// force=1 is set unconditionally on the permitted branch, not left to |
| -// what the caller's form said. It selects rauth's plain-magic-link path |
| -// over keymail classification, which this app never wants (see this |
| -// file's package comment). Without it a permitted keymail-hosted |
| -// address would redirect out to keymail while a non-permitted one got |
| -// ?sent=1 — the same oracle again. It also saves the classification |
| -// network call outright. |
| +// No outcome of Begin depends on admission any more: both kinds of |
| +// address spend the same budgets and parse the same way, and the send |
| +// that only a permitted address reaches happens in mailQueue's worker, |
| +// after the response, where its failures are logged rather than |
| +// answered. sameAnswer still rewrites `?err=rate|address|1` to sent, as |
| +// it always has; giving rate and address back their own sentences is a |
| +// separate, safe change, not made here. |
| +func gateBegin(next http.HandlerFunc, logger *slog.Logger) http.HandlerFunc { |
| + return func(w http.ResponseWriter, r *http.Request) { |
| + next(&sameAnswer{ResponseWriter: w, logger: logger}, r) |
| + } |
| +} |
| + |
| +// newBegin is the POST /signin handler both modes mount: gateBegin |
| +// under connect.OAuthContinuation, which ends the POST with a page |
| +// (renderKeymailContinuation) when Begin's answer is Keymail's |
| +// authorize URL, and passes every other answer through untouched. |
| +func (a *app) newBegin(begin http.HandlerFunc) http.HandlerFunc { |
| + return connect.OAuthContinuation(a.cfg.Origin, gateBegin(begin, a.logger), a.renderKeymailContinuation) |
| +} |
| + |
| +// renderKeymailContinuation is the page between the form and Keymail: |
| +// the door's own card, saying where the person is going, with a meta |
| +// refresh that goes there by itself and a link that does when the |
| +// refresh is blocked. A navigation from a page, unlike a redirect from |
| +// a form POST, is outside form-action. |
| +func (a *app) renderKeymailContinuation(w http.ResponseWriter, r *http.Request, destination string) { |
| + p := connect.OAuthPage(destination) |
| + p.Title = rastrillo.T(r, "slides.signin.keymail.title") |
| + p.ActionLabel = rastrillo.T(r, "slides.signin.keymail.action") |
| + notice := chrome.SignInNotice{ |
| + Icon: brand.Path("slides"), App: "Slides", Sky: sky.For(r.Host), |
| + Heading: p.Title, Body: rastrillo.T(r, "slides.signin.keymail.body"), |
| + Link: &chrome.Link{Href: p.URL, Label: p.ActionLabel}, |
| + } |
| + render(w, "signin", signinView{ |
| + shell: a.publicShell(r, frame{parts: []string{p.Title}, signinPage: true}), |
| + Sent: ¬ice, |
| + Continue: &p, |
| + }) |
| +} |
| + |
| +// rauthRefusal is the bare-text body rastrillo/auth's admit() writes, |
| +// with a 403, when a verified address is not admitted (Authorize said |
| +// no). rastrillo at the version pinned here has no hook to render |
| +// anything else; doorOnRefusal recognises this exact answer instead. |
| +// The Keymail refusal test pins it, so a rastrillo bump that changes |
| +// the sentence fails loudly rather than letting the bare page back. |
| +const rauthRefusal = "This address is verified but not admitted here." |
| + |
| +// doorOnRefusal wraps rauth's Callback and Verify, the two handlers |
| +// that end in admit(), and turns its bare-text refusal into the sign-in |
| +// door, rendered right there in the refusal's own response: the form, |
| +// with "That address can't sign in to Slides" above it, and a 403. Every |
| +// other answer, cookies included, passes through untouched. |
| // |
| -// r.ParseForm is documented idempotent, so reading the address here and |
| -// again inside Begin costs nothing and cannot disagree with itself. A |
| -// malformed body reads back as address == "", which is never on a |
| -// non-empty allowlist — the same answer Begin would eventually reach, |
| -// just reached before any mail could go out. |
| -func gateBegin(next http.HandlerFunc, list allowlist, logger *slog.Logger) http.HandlerFunc { |
| +// Rendered, not redirected to ?err=refused: a query parameter is |
| +// something anybody can type, and then the sentence that means "you |
| +// proved this address and it is not admitted" could be shown to |
| +// anybody about nothing (Astra on #20, round 3). The refusal reaches |
| +// door through the request context (refusedHere), which only this |
| +// wrapper sets. It is told only to somebody who has just verified the |
| +// address, so it is no oracle. Remove it when rastrillo grows a refusal |
| +// hook (feat/auth-refused-hook) and the pin is bumped. |
| +func doorOnRefusal(next, door http.HandlerFunc) http.HandlerFunc { |
| return func(w http.ResponseWriter, r *http.Request) { |
| - _ = r.ParseForm() |
| - if !list.allows(r.FormValue("address")) { |
| - http.Redirect(w, r, sentLocation, http.StatusSeeOther) |
| + rec := &bufferedResponse{header: make(http.Header)} |
| + next(rec, r) |
| + if rec.status == http.StatusForbidden && strings.TrimSpace(rec.body.String()) == rauthRefusal { |
| + for _, c := range rec.header.Values("Set-Cookie") { |
| + w.Header().Add("Set-Cookie", c) |
| + } |
| + door(&forcedStatus{ResponseWriter: w, status: http.StatusForbidden}, |
| + r.WithContext(context.WithValue(r.Context(), refusedKey{}, true))) |
| return |
| } |
| - if r.Form == nil { |
| - r.Form = url.Values{} |
| + for name, values := range rec.header { |
| + for _, v := range values { |
| + w.Header().Add(name, v) |
| + } |
| + } |
| + if rec.status == 0 { |
| + rec.status = http.StatusOK |
| } |
| - r.Form.Set("force", "1") |
| - next(&sameAnswer{ResponseWriter: w, logger: logger}, r) |
| + w.WriteHeader(rec.status) |
| + _, _ = w.Write(rec.body.Bytes()) |
| } |
| } |
| -// sentLocation is the one answer POST /signin ever gives — see |
| -// gateBegin's guarantee. Spelled once so the two paths that write it |
| -// cannot drift apart by a character. |
| +// refusedKey marks a request doorOnRefusal is answering with the door. |
| +type refusedKey struct{} |
| + |
| +// refusedHere reports whether r is a refusal doorOnRefusal is rendering. |
| +func refusedHere(r *http.Request) bool { |
| + v, _ := r.Context().Value(refusedKey{}).(bool) |
| + return v |
| +} |
| + |
| +// forcedStatus answers with status whatever status the page writes. |
| +type forcedStatus struct { |
| + http.ResponseWriter |
| + status int |
| + wrote bool |
| +} |
| + |
| +func (f *forcedStatus) WriteHeader(int) { |
| + if !f.wrote { |
| + f.wrote = true |
| + f.ResponseWriter.WriteHeader(f.status) |
| + } |
| +} |
| + |
| +func (f *forcedStatus) Write(p []byte) (int, error) { |
| + f.WriteHeader(f.status) |
| + return f.ResponseWriter.Write(p) |
| +} |
| + |
| +// bufferedResponse holds a whole answer so doorOnRefusal can decide |
| +// what to send after seeing it. Callback and Verify answer with a |
| +// redirect or a line of text, never anything large. |
| +type bufferedResponse struct { |
| + header http.Header |
| + status int |
| + body bytes.Buffer |
| +} |
| + |
| +func (b *bufferedResponse) Header() http.Header { return b.header } |
| +func (b *bufferedResponse) WriteHeader(status int) { |
| + if b.status == 0 { |
| + b.status = status |
| + } |
| +} |
| +func (b *bufferedResponse) Write(p []byte) (int, error) { |
| + if b.status == 0 { |
| + b.status = http.StatusOK |
| + } |
| + return b.body.Write(p) |
| +} |
| + |
| +// sentLocation is the answer POST /signin gives every address that is |
| +// not going to Keymail — see gateBegin. Spelled once so the paths that |
| +// write it cannot drift apart by a character. |
| const sentLocation = signinPath + "?sent=1" |
| // sameAnswer is the ResponseWriter gateBegin hands to Begin: it |
| // rewrites any `?err=…` outcome back to sentLocation before the status |
| // line goes out, so a permitted address cannot be told apart from a |
| -// denied one by its failure mode. See gateBegin's own doc comment for |
| -// what that buys and what it costs. |
| +// denied one by a failed send. See gateBegin's own doc comment for |
| +// which outcomes that still matters for. |
| // |
| // It hooks WriteHeader rather than the Location header itself because |
| // that is the last moment a header is still settable: http.Redirect |
| @@ -251,12 +361,12 @@ func (s *sameAnswer) WriteHeader(status int) { |
| // both the emailed link's base and the cookie decision, InstanceKey and |
| // Mailer already resolved and fail-closed above. |
| // |
| -// Authorize is list.allows as well as gateBegin's gate — belt and |
| -// braces, defending two different things: gateBegin stops the mail, |
| -// Authorize stops a session being minted from a link that was somehow |
| -// issued for a non-permitted address (mailed before the allowlist |
| -// changed, say). SecondFactor stays nil, per this file's package |
| -// comment. |
| +// Authorize is list.allows as well as mailQueue — two checks |
| +// defending two different things: mailQueue stops the mail, |
| +// Authorize stops a session being minted for a non-permitted address, |
| +// whether Keymail verified it or a link was somehow issued for it |
| +// (mailed before the allowlist changed, say). SecondFactor stays nil, |
| +// per this file's package comment. |
| // |
| // cfg must be the resolved Config (config.go's resolve), not a |
| // caller's raw one: Origin, InstanceKey and Mailer are all required |
| @@ -267,7 +377,7 @@ func (s *sameAnswer) WriteHeader(status int) { |
| // copy of that literal and app.go mounts the page at ours; saying it |
| // once, out loud, is what keeps the two from drifting silently if |
| // either default ever changes. |
| -func newAuthPlugin(cfg Config, writer *sql.DB, list allowlist) (*rauth.Auth, error) { |
| +func newAuthPlugin(cfg Config, writer *sql.DB, list allowlist, mailer *mailQueue) (*rauth.Auth, error) { |
| signedInPath := landedPath |
| if cfg.Home != nil { |
| signedInPath = connect.LocalCompletePath |
| @@ -277,7 +387,7 @@ func newAuthPlugin(cfg Config, writer *sql.DB, list allowlist) (*rauth.Auth, err |
| DB: writer, |
| Origin: cfg.Origin, |
| InstanceKey: cfg.InstanceKey, |
| - Mailer: cfg.Mailer, |
| + Mailer: mailer, |
| Subject: chrome.EmailSubject("Slides"), |
| Authorize: list.allows, |
| SigninPath: signinPath, |
| @@ -546,15 +656,21 @@ type signinView struct { |
| shell |
| SignIn chrome.SignIn |
| Sent *chrome.SignInNotice |
| + // Continue is set on the page between the form and Keymail |
| + // (renderKeymailContinuation): its meta refresh goes on to |
| + // Keymail, and Sent holds the door's card saying so. |
| + Continue *connect.OAuthData |
| } |
| // signin serves GET /signin: the form, and the landing spot for every |
| // outcome rauth redirects back to (?sent=1, ?err=rate|address|expired| |
| -// keymail|removed|1). rate, address, expired and removed each get their |
| -// own sentence; keymail and a bare failure share the default, because |
| -// every outcome on this page means the same operational thing — sign-in |
| -// did not complete, ask for a new link — and rauth's Verify deliberately |
| -// refuses to be more specific than that. |
| +// keymail|removed|1, ?force=1), and the door doorOnRefusal renders in a |
| +// refusal's own response (refusedHere). rate, address, expired, removed |
| +// and a refusal each get their own sentence; keymail and a bare failure |
| +// share the default, because both mean the same operational thing — |
| +// sign-in did not complete, ask for a new link — and rauth's Verify |
| +// deliberately refuses to be more specific than that. ?force=1 (with |
| +// err=keymail) makes the form ask for an emailed link. |
| // |
| // Already signed in, it skips the form (spec §9.2): 303 to return_to or |
| // the deck list. The admission recheck runs FIRST (currentUser), so an |
| @@ -571,7 +687,7 @@ type signinView struct { |
| func (a *app) signin(w http.ResponseWriter, r *http.Request) { |
| q := r.URL.Query() |
| ret, hasReturn := safeReturn(q) |
| - if a.home == nil || r.URL.Path == "/signin/local" { |
| + if (a.home == nil || r.URL.Path == "/signin/local") && !refusedHere(r) { |
| if s, ok := a.sessions.From(r); ok { |
| _, outcome := a.currentUser(w, sessions.WithSession(r, s)) |
| switch outcome { |
| @@ -621,23 +737,33 @@ func (a *app) signin(w http.ResponseWriter, r *http.Request) { |
| App: "Slides", |
| Sky: sky.For(r.Host), |
| Action: signinPath, |
| - Hidden: []chrome.Hidden{{Name: "force", Value: "1"}}, |
| OpenSource: &chrome.Link{Href: "/help/open-source", Label: rastrillo.T(r, "slides.credits.title")}, |
| } |
| + if q.Get("force") != "" { |
| + // A failed Keymail exchange lands here as ?force=1&err=keymail. |
| + // Posting force back asks for an emailed link instead, so the |
| + // next attempt does not return to the Keymail that just failed. |
| + form.Hidden = []chrome.Hidden{{Name: "force", Value: "1"}} |
| + } |
| if hasReturn { |
| form.Return = ret |
| } |
| - switch q.Get("err") { |
| - case "": |
| + switch err := q.Get("err"); { |
| + case refusedHere(r): |
| + // Keymail or a link verified the address and the allowlist does |
| + // not hold it (doorOnRefusal). The form is the way on. Never |
| + // from the query: ?err=refused is the unknown-error default. |
| + form.Error = rastrillo.T(r, "slides.signin.error.refused") |
| + case err == "": |
| // nothing to report |
| - case "rate": |
| + case err == "rate": |
| form.Error = rastrillo.T(r, "slides.signin.error.rate") |
| - case "address": |
| + case err == "address": |
| // An address error belongs to the field, not a banner. |
| form.AddressError = rastrillo.T(r, "slides.signin.error.address") |
| - case "expired": |
| + case err == "expired": |
| form.Error = rastrillo.T(r, "slides.signin.error.expired") |
| - case "removed": |
| + case err == "removed": |
| // Not "sign-in did not complete": this address had access and |
| // no longer does (offboard). Asking for another link will not |
| // help, and the sentence says so. |
diff --git a/internal/slides/auth_test.go b/internal/slides/auth_test.go| index 3c6cb55..8865cf0 100644 |
| --- a/internal/slides/auth_test.go |
| +++ b/internal/slides/auth_test.go |
| @@ -86,8 +86,10 @@ func countCreates(t *testing.T, g *gorm.DB) func() int { |
| // insert. |
| func TestEnsureUserWritesOnceOnly(t *testing.T) { |
| d := testDB(t) |
| - if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| + if a, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| t.Fatalf("newApp: %v", err) |
| + } else { |
| + t.Cleanup(func() { a.Close() }) |
| } |
| creates := countCreates(t, d.G) |
| @@ -135,8 +137,10 @@ func TestEnsureUserWritesOnceOnly(t *testing.T) { |
| // disjoint sets of decks depending on which one a request found. |
| func TestEnsureUserNormalizesTheAddress(t *testing.T) { |
| d := testDB(t) |
| - if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| + if a, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| t.Fatalf("newApp: %v", err) |
| + } else { |
| + t.Cleanup(func() { a.Close() }) |
| } |
| ctx := context.Background() |
| @@ -168,8 +172,10 @@ func TestEnsureUserNormalizesTheAddress(t *testing.T) { |
| // create a row owning decks that nobody could ever sign in as. |
| func TestEnsureUserRefusesAnEmptyAddress(t *testing.T) { |
| d := testDB(t) |
| - if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| + if a, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil { |
| t.Fatalf("newApp: %v", err) |
| + } else { |
| + t.Cleanup(func() { a.Close() }) |
| } |
| if _, _, err := ensureUser(context.Background(), d.G, " "); err == nil { |
| t.Fatal("ensureUser accepted a blank address; want an error") |
diff --git a/internal/slides/config.go b/internal/slides/config.go| index 85c0af4..89bcff4 100644 |
| --- a/internal/slides/config.go |
| +++ b/internal/slides/config.go |
| @@ -91,21 +91,22 @@ type Config struct { |
| // normalizeAddress). |
| // |
| // It is wired to two places on purpose: rauth.Config.Authorize |
| - // gates admission, and gateBegin gates POST /signin ahead of |
| - // rauth's own Begin. The second is the one that matters — |
| - // Authorize fires only inside rauth's admit(), after the link has |
| - // already been mailed, so Authorize alone would leave this |
| - // instance's sending identity an open relay for any address a |
| - // stranger types into the form. |
| + // gates admission, after Keymail or a link has verified the |
| + // address, and mailQueue (mailqueue.go) drops the mail for an |
| + // address not on it. The second is what keeps this instance's |
| + // sending identity from being an open relay for any address a |
| + // stranger types into the form, since Authorize fires only after |
| + // a link has been mailed and clicked. Neither changes what POST |
| + // /signin answers (gateBegin). |
| // |
| // Empty is allowed only in development, where it means "admit |
| // everyone" so the dev script and the suite need no setup. |
| // Anywhere else it is a startup refusal rather than "admit |
| // nobody": an instance admitting nobody boots looking perfectly |
| - // healthy — /signin renders, POST /signin answers its ordinary 303 |
| - // for every address, because that identical answer is the whole |
| - // point of gateBegin — and the only symptom is that no link ever |
| - // arrives. |
| + // healthy — /signin renders, POST /signin answers as it would for |
| + // anyone, because an answer that does not depend on the list is |
| + // the whole point of gateBegin — and the only symptom is that no |
| + // link ever arrives. |
| AllowedAddresses []string |
| // Blobs is where uploaded bytes live — the object store, never the |
diff --git a/internal/slides/config_test.go b/internal/slides/config_test.go| index 17060bc..c8b1568 100644 |
| --- a/internal/slides/config_test.go |
| +++ b/internal/slides/config_test.go |
| @@ -42,6 +42,7 @@ func TestDevelopmentStoresTheResolvedConfig(t *testing.T) { |
| if err != nil { |
| t.Fatalf("newApp: %v", err) |
| } |
| + t.Cleanup(func() { a.Close() }) |
| if a.cfg.Origin != DefaultOrigin { |
| t.Errorf("a.cfg.Origin = %q, want the resolved %q", a.cfg.Origin, DefaultOrigin) |
| } |
diff --git a/internal/slides/handlers.go b/internal/slides/handlers.go| index 83fa42d..2de7561 100644 |
| --- a/internal/slides/handlers.go |
| +++ b/internal/slides/handlers.go |
| @@ -42,6 +42,8 @@ type app struct { |
| logger *slog.Logger |
| cfg Config |
| allowed allowlist |
| + // mail is the sign-in mail queue rauth sends through (mailqueue.go). |
| + mail *mailQueue |
| // returnSeal authenticates the short-lived return_to cookie. It is |
| // derived from this instance's sign-in key, never from a browser |
| // value, and the clock is a seam for expiry tests. |
| @@ -113,6 +115,10 @@ func (a *app) Close() error { |
| if a.live != nil { |
| a.live.Close() |
| } |
| + // Queued sign-in links are sent before the process lets go. |
| + if a.mail != nil { |
| + a.mail.close() |
| + } |
| // The PDF module, if any export ever built one. Same argument as |
| // the deck core's below: a test binary builds an app per test, and |
| // a compiled wasm module nobody releases is a leak per app. |
| @@ -124,3 +130,12 @@ func (a *app) Close() error { |
| } |
| return a.core.Close(context.Background()) |
| } |
| + |
| +// FlushMail waits until every sign-in link queued so far has been sent |
| +// or dropped. The links go out after the response (mailqueue.go), so a |
| +// test reads the mailer only after this. |
| +func (a *app) FlushMail() { |
| + if a.mail != nil { |
| + a.mail.wait() |
| + } |
| +} |
diff --git a/internal/slides/home_test.go b/internal/slides/home_test.go| index 6400586..700de7b 100644 |
| --- a/internal/slides/home_test.go |
| +++ b/internal/slides/home_test.go |
| @@ -469,6 +469,7 @@ func TestHomeLocalPluginRecoveryRotatesThenReturnsToLink(t *testing.T) { |
| if sent.Code != 303 { |
| t.Fatal(sent.Code) |
| } |
| + a.FlushMail() |
| body := a.cfg.Mailer.(*homeTestMail).body |
| link := regexp.MustCompile(`https://[^\s<>]+/auth/verify\?token=[A-Za-z0-9_-]+`).FindString(body) |
| if link == "" { |
diff --git a/internal/slides/keymail_home_test.go b/internal/slides/keymail_home_test.go| new file mode 100644 |
| index 0000000..667c608 |
| --- /dev/null |
| +++ b/internal/slides/keymail_home_test.go |
| @@ -0,0 +1,122 @@ |
| +package slides |
| + |
| +import ( |
| + "bytes" |
| + "encoding/base64" |
| + "encoding/json" |
| + "fmt" |
| + "html" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "os" |
| + "regexp" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/oficina/home/connect" |
| + "amadan.net/oficina/slides/internal/slides/keymailtest" |
| + assertioncrypto "amadan.net/rastrillo/rastrillo/crypto" |
| +) |
| + |
| +// TestMain keeps Keymail classification inside the process for this |
| +// package's tests: every DNS query is answered NXDOMAIN and only the |
| +// Keymail fixtures a test registers answer HTTPS (keymailtest). |
| +func TestMain(m *testing.M) { |
| + restore, err := keymailtest.Install() |
| + if err != nil { |
| + fmt.Fprintln(os.Stderr, "keymailtest.Install:", err) |
| + os.Exit(1) |
| + } |
| + code := m.Run() |
| + // No test may leave a sign-in mail worker running: every app and |
| + // every queue a test builds is closed in its cleanup. |
| + if code == 0 { |
| + deadline := time.Now().Add(5 * time.Second) |
| + for mailWorkers() > 0 && time.Now().Before(deadline) { |
| + time.Sleep(10 * time.Millisecond) |
| + } |
| + if n := mailWorkers(); n > 0 { |
| + fmt.Fprintf(os.Stderr, "FAIL: %d sign-in mail queue goroutines still running after the tests\n", n) |
| + code = 1 |
| + } |
| + } |
| + restore() |
| + os.Exit(code) |
| +} |
| + |
| +// TestHomeLocalKeymailIsAContinuationAndRefusesAtTheDoor is the |
| +// Home-mode half of the Keymail sign-in: with Home configured, the |
| +// local form (/signin/local, posting to /signin under LocalGuard) sends |
| +// a permitted and a non-permitted Keymail address to the same |
| +// continuation page — never a 303 out of the form POST, whose |
| +// form-action is 'self' plus the Home issuer — and a non-permitted one |
| +// that Keymail verified is refused on the local door, not in bare text. |
| +func TestHomeLocalKeymailIsAContinuationAndRefusesAtTheDoor(t *testing.T) { |
| + const host = "keymail.home.test" |
| + member, stranger := "owner@"+host, "stranger@"+host |
| + km := keymailtest.Serve(t, host, member, stranger) |
| + |
| + key, err := assertioncrypto.Generate() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + raw, _ := json.Marshal(map[string]string{"test": base64.StdEncoding.EncodeToString(key.SignPub())}) |
| + mail := &homeTestMail{} |
| + mux, a, err := New(testDB(t), Config{Mailer: mail, Env: EnvDevelopment, Origin: homeTestOrigin, Logger: quietLogger(), |
| + AllowedAddresses: []string{member}, |
| + Home: &connect.Credentials{Issuer: "https://home.example.com", ConsumerID: "slides-test", |
| + KeysBase64: base64.StdEncoding.EncodeToString(raw), DEKBase64: base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{8}, 32))}}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + |
| + do := func(method, path string, form url.Values, cookies []*http.Cookie) *httptest.ResponseRecorder { |
| + r := httptest.NewRequest(method, homeTestOrigin+path, strings.NewReader(form.Encode())) |
| + if method == http.MethodPost { |
| + r.Header.Set("Origin", homeTestOrigin) |
| + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + } |
| + for _, c := range cookies { |
| + r.AddCookie(c) |
| + } |
| + w := httptest.NewRecorder() |
| + mux.ServeHTTP(w, r) |
| + return w |
| + } |
| + href := regexp.MustCompile(`href="(https://` + regexp.QuoteMeta(host) + `/oauth/authorize\?[^"]+)"`) |
| + begin := func(address string) (*httptest.ResponseRecorder, *url.URL) { |
| + rec := do(http.MethodPost, "/signin", url.Values{"address": {address}}, nil) |
| + if rec.Code != http.StatusOK || rec.Header().Get("Location") != "" { |
| + t.Fatalf("POST /signin %q in Home mode: %d %q, want 200 and the continuation page", address, rec.Code, rec.Header().Get("Location")) |
| + } |
| + m := href.FindStringSubmatch(rec.Body.String()) |
| + if m == nil || !strings.Contains(rec.Body.String(), `http-equiv="refresh"`) { |
| + t.Fatalf("POST /signin %q in Home mode: no Keymail continuation:\n%s", address, rec.Body.String()) |
| + } |
| + u, err := url.Parse(html.UnescapeString(m[1])) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return rec, u |
| + } |
| + |
| + _, _ = begin(member) |
| + rec, u := begin(stranger) |
| + a.FlushMail() |
| + if mail.body != "" { |
| + t.Fatal("a Keymail address was mailed a link") |
| + } |
| + |
| + cb := do(http.MethodGet, "/auth/callback?"+url.Values{"state": {u.Query().Get("state")}, "code": {km.Approve(stranger)}}.Encode(), nil, rec.Result().Cookies()) |
| + // This mux has no locale middleware, so the catalog key itself is |
| + // what proves the refusal sentence was chosen. |
| + if body := cb.Body.String(); cb.Code != http.StatusForbidden || !strings.Contains(body, "slides.signin.error.refused") || !strings.Contains(body, `name="address"`) { |
| + t.Fatalf("Home-mode callback for a non-permitted address: %d %q, want 403 and the local door with the refusal\n%s", cb.Code, cb.Header().Get("Location"), body) |
| + } |
| + if spoof := do(http.MethodGet, "/signin/local?err=refused", nil, nil).Body.String(); strings.Contains(spoof, "slides.signin.error.refused") { |
| + t.Fatal("anybody can show the refusal by visiting /signin/local?err=refused") |
| + } |
| +} |
diff --git a/internal/slides/keymailtest/keymailtest.go b/internal/slides/keymailtest/keymailtest.go| new file mode 100644 |
| index 0000000..92ad056 |
| --- /dev/null |
| +++ b/internal/slides/keymailtest/keymailtest.go |
| @@ -0,0 +1,190 @@ |
| +// Package keymailtest keeps sign-in's Keymail classification inside the |
| +// test process. It is imported by tests only. |
| +// |
| +// rastrillo/auth (at the version this app pins) builds its classifier |
| +// with no seams: TXT lookups go through net.DefaultResolver and the |
| +// well-known probe, the federation lookup and the token exchange go |
| +// through http.DefaultTransport. So those two globals are the hooks, |
| +// exactly as oficina/docs' auth_keymail_test.go uses them. Install |
| +// replaces both for the life of the test binary: |
| +// |
| +// - every DNS query is answered NXDOMAIN by a resolver on loopback, |
| +// so no DNS packet leaves the process, and a domain with no |
| +// `_keymail` record is probed directly — keymail.dev's own shape; |
| +// - an https request to a host registered with Serve is answered |
| +// in-process by that fixture; anything else falls through to the |
| +// real transport, where the resolver above makes a name |
| +// unreachable (IP literals, which httptest servers use, still work). |
| +// |
| +// Every address at a domain nobody registered therefore classifies as |
| +// "not Keymail", which is what every pre-existing sign-in test assumes. |
| +package keymailtest |
| + |
| +import ( |
| + "context" |
| + "crypto/rand" |
| + "encoding/hex" |
| + "encoding/json" |
| + "net" |
| + "net/http" |
| + "net/http/httptest" |
| + "strings" |
| + "sync" |
| + "time" |
| +) |
| + |
| +var ( |
| + mu sync.Mutex |
| + servers = map[string]*Server{} |
| +) |
| + |
| +// Install swaps net.DefaultResolver and http.DefaultTransport for the |
| +// stubs described in the package comment and returns a function that |
| +// puts them back. Call it once, from TestMain. |
| +func Install() (restore func(), err error) { |
| + dns, err := net.ListenPacket("udp", "127.0.0.1:0") |
| + if err != nil { |
| + return nil, err |
| + } |
| + go answerNXDOMAIN(dns) |
| + oldResolver, oldTransport := net.DefaultResolver, http.DefaultTransport |
| + net.DefaultResolver = &net.Resolver{PreferGo: true, Dial: func(ctx context.Context, _, _ string) (net.Conn, error) { |
| + return (&net.Dialer{}).DialContext(ctx, "udp", dns.LocalAddr().String()) |
| + }} |
| + http.DefaultTransport = transport{next: oldTransport} |
| + return func() { |
| + net.DefaultResolver, http.DefaultTransport = oldResolver, oldTransport |
| + _ = dns.Close() |
| + }, nil |
| +} |
| + |
| +// answerNXDOMAIN echoes each query back as a "no such name" answer. |
| +func answerNXDOMAIN(conn net.PacketConn) { |
| + b := make([]byte, 4096) |
| + for { |
| + n, addr, err := conn.ReadFrom(b) |
| + if err != nil { |
| + return |
| + } |
| + if n < 12 { |
| + continue |
| + } |
| + reply := append([]byte(nil), b[:n]...) |
| + reply[2], reply[3] = 0x81, 0x83 // response, recursion, NXDOMAIN |
| + for i := 6; i < 12; i++ { |
| + reply[i] = 0 // no answer, authority or additional records |
| + } |
| + _, _ = conn.WriteTo(reply, addr) |
| + } |
| +} |
| + |
| +type transport struct{ next http.RoundTripper } |
| + |
| +func (t transport) RoundTrip(r *http.Request) (*http.Response, error) { |
| + mu.Lock() |
| + s := servers[r.URL.Host] |
| + mu.Unlock() |
| + if s == nil || r.URL.Scheme != "https" { |
| + return t.next.RoundTrip(r) |
| + } |
| + rec := httptest.NewRecorder() |
| + s.ServeHTTP(rec, r) |
| + res := rec.Result() |
| + res.Request = r |
| + return res, nil |
| +} |
| + |
| +// Server is one Keymail deployment at one host: the well-known |
| +// document, the federation lookup for the accounts it holds, and the |
| +// token exchange for the codes Approve hands out. |
| +type Server struct { |
| + host string |
| + |
| + mu sync.Mutex |
| + accounts map[string]bool |
| + codes map[string]string |
| + lookups int |
| + tokens int |
| +} |
| + |
| +// Serve registers a Keymail deployment at host (which is also the |
| +// address domain it answers for, since nothing publishes a `_keymail` |
| +// record) holding the given accounts. It is unregistered when the test |
| +// ends. |
| +func Serve(t interface{ Cleanup(func()) }, host string, accounts ...string) *Server { |
| + s := &Server{host: host, accounts: map[string]bool{}, codes: map[string]string{}} |
| + for _, a := range accounts { |
| + s.accounts[strings.ToLower(a)] = true |
| + } |
| + mu.Lock() |
| + servers[host] = s |
| + mu.Unlock() |
| + t.Cleanup(func() { |
| + mu.Lock() |
| + delete(servers, host) |
| + mu.Unlock() |
| + }) |
| + return s |
| +} |
| + |
| +// Approve is Keymail's consent screen saying yes for address: it |
| +// returns the code the browser would carry back to /auth/callback. |
| +func (s *Server) Approve(address string) string { |
| + var b [12]byte |
| + _, _ = rand.Read(b[:]) |
| + code := hex.EncodeToString(b[:]) |
| + s.mu.Lock() |
| + s.codes[code] = address |
| + s.mu.Unlock() |
| + return code |
| +} |
| + |
| +// Lookups is how many federation lookups reached this deployment. |
| +func (s *Server) Lookups() int { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + return s.lookups |
| +} |
| + |
| +// Tokens is how many token exchanges reached this deployment. |
| +func (s *Server) Tokens() int { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + return s.tokens |
| +} |
| + |
| +func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| + w.Header().Set("Content-Type", "application/json") |
| + switch r.URL.Path { |
| + case "/.well-known/keymail": |
| + _ = json.NewEncoder(w).Encode(map[string]any{"version": "1", "reauth": true}) |
| + case "/api/federation/lookup": |
| + s.mu.Lock() |
| + s.lookups++ |
| + ok := s.accounts[strings.ToLower(r.URL.Query().Get("addr"))] |
| + s.mu.Unlock() |
| + if !ok { |
| + w.WriteHeader(http.StatusNotFound) |
| + } |
| + _ = json.NewEncoder(w).Encode(map[string]bool{"ok": ok}) |
| + case "/api/oauth/token": |
| + var in map[string]string |
| + _ = json.NewDecoder(r.Body).Decode(&in) |
| + s.mu.Lock() |
| + s.tokens++ |
| + address, ok := s.codes[in["code"]] |
| + delete(s.codes, in["code"]) |
| + s.mu.Unlock() |
| + if r.Method != http.MethodPost || !ok { |
| + w.WriteHeader(http.StatusBadRequest) |
| + _ = json.NewEncoder(w).Encode(map[string]string{"error": "invalid_grant"}) |
| + return |
| + } |
| + _ = json.NewEncoder(w).Encode(map[string]any{ |
| + "access_token": "synthetic", "token_type": "bearer", |
| + "address": address, "auth_time": time.Now().Unix(), |
| + }) |
| + default: |
| + w.WriteHeader(http.StatusNotFound) |
| + } |
| +} |
diff --git a/internal/slides/locales/en.toml b/internal/slides/locales/en.toml| index 64292ef..1afb195 100644 |
| --- a/internal/slides/locales/en.toml |
| +++ b/internal/slides/locales/en.toml |
| @@ -10,7 +10,8 @@ |
| # slides.signin.* — GET /signin (auth.go's signin handler): the errors |
| # estilo's sign-in door shows, on the way back from rastrillo/auth, for |
| # whichever outcome it was redirected here with |
| -# (?err=rate|address|expired|keymail|removed|1). The door's own words |
| +# (?err=rate|address|expired|keymail|removed|refused|1), and the card |
| +# between the form and Keymail (slides.signin.keymail.*). The door's own words |
| # (form, check-your-email, signed out) are estilo's chrome.English, not |
| # keys here, so no app can drift from the others (shared chrome spec §9.2). |
| # |
| @@ -24,6 +25,10 @@ slides.signin.error.address = "That doesn’t look like an email address." |
| slides.signin.error.expired = "That link has expired or was already used. Enter your email to get a new one." |
| slides.signin.error.unknown = "We couldn’t sign you in. Enter your email to get a new link." |
| slides.signin.error.removed = "Your access has been removed, so you’ve been signed out. Ask whoever runs this team if that’s wrong." |
| +slides.signin.error.refused = "That address can’t sign in to Slides. Use a different address, or ask whoever runs this team to add it." |
| +slides.signin.keymail.title = "Continue signing in" |
| +slides.signin.keymail.body = "Taking you to Keymail to finish signing in." |
| +slides.signin.keymail.action = "Continue to Keymail" |
| # slides.chrome.* — estilo's shared bar and account menu (chrome.go). |
| slides.chrome.skip = "Skip to content" |
diff --git a/internal/slides/mailqueue.go b/internal/slides/mailqueue.go| new file mode 100644 |
| index 0000000..d8c23bc |
| --- /dev/null |
| +++ b/internal/slides/mailqueue.go |
| @@ -0,0 +1,283 @@ |
| +package slides |
| + |
| +import ( |
| + "context" |
| + "fmt" |
| + "log/slog" |
| + "sync" |
| + "sync/atomic" |
| + "time" |
| + |
| + "amadan.net/rastrillo/rastrillo/mail" |
| +) |
| + |
| +// mailQueue is the sign-in mailer the auth plugin is given: Send queues |
| +// the link and returns at once, and one background worker decides |
| +// whether the address may have it and only then talks to SMTP. |
| +// |
| +// Both halves are the no-oracle guarantee, measured rather than |
| +// reasoned about. When the send ran inside the request, a permitted |
| +// address waited for the SMTP round trip before its `303 ?sent=1` and a |
| +// denied one did not: 252 ms against 0.35 ms (Astra on oficina/slides |
| +// #20). A response time is an answer like any other. So POST /signin |
| +// does the same synchronous work for every address — the auth plugin's |
| +// budgets, its link row, this enqueue — and the one admission-dependent |
| +// step, the send, happens after the response, where nobody can time it. |
| +// |
| +// The request path is Send, and Send does no I/O and never blocks: not |
| +// a send, not an admission check, not even a log line. Every address's |
| +// link is queued, permitted or not, so how full the queue is never |
| +// depends on admission either; the worker decides afterwards. A full |
| +// queue drops the link and counts it in an atomic; the worker logs the |
| +// count when it next runs. The person is answered exactly as when the |
| +// queue is empty, because a different answer for a full queue would be |
| +// one more thing to read. (Astra on #20, round 2.) |
| +// |
| +// The worker survives a panic in the mailer: it recovers per message, |
| +// logs, and keeps draining. Delivery failures are logged, never |
| +// surfaced: a failure only a permitted address can reach is the oracle |
| +// again. |
| +// |
| +// Nothing but the log pump ever writes to the logger. The worker and |
| +// close hand their lines to logf, which never blocks: a buffered |
| +// channel, and a counted drop when it is full. A stalled log sink can |
| +// therefore hold only the pump's one write — never a request, never the |
| +// worker, never close (Astra on #20, round 3). The pump ends when the |
| +// worker does. |
| +// |
| +// close drains what is queued within drainTimeout. Past it, the send |
| +// in flight is cancelled through its context, everything still queued |
| +// is abandoned and counted, and close waits a short grace for the |
| +// worker to stop. A mailer that ignores its context is the only way a |
| +// worker outlives close; that is counted in stuck and logged, best |
| +// effort, too. |
| +type mailQueue struct { |
| + next mail.Sender |
| + admits func(to string) bool |
| + // gate, when set, is read off the request's context at enqueue time |
| + // and travels with the link; the worker sends only if it said yes. |
| + gate func(context.Context) bool |
| + logger *slog.Logger |
| + |
| + drainTimeout time.Duration |
| + sendTimeout time.Duration |
| + |
| + mu sync.RWMutex // held for reading by Send, for writing by close |
| + closed bool |
| + jobs chan mailJob |
| + dropped atomic.Int64 |
| + pending sync.WaitGroup |
| + |
| + ctx context.Context // cancelled when close gives up waiting |
| + cancel context.CancelFunc |
| + done chan struct{} |
| + |
| + logMu sync.RWMutex // logf reads logsClosed; the worker's exit sets it |
| + logsClosed bool |
| + logs chan logLine // to the pump; logf never blocks on it |
| + unlogged atomic.Int64 // lines handed over and not yet written |
| + lostLogs atomic.Int64 // lines logf could not hand over |
| + stuck atomic.Bool // close gave up on a worker that ignored cancellation |
| + |
| + // refusedLog, when set, is the line the worker logs (through logf, |
| + // with a running count and never the address) for each link it |
| + // refuses. |
| + refusedLog string |
| + refusals atomic.Int64 |
| +} |
| + |
| +type logLine struct { |
| + msg string |
| + args []any |
| +} |
| + |
| +type mailJob struct { |
| + to, subject, body string |
| + gated bool |
| +} |
| + |
| +const ( |
| + mailQueueDepth = 64 |
| + mailSendTimeout = time.Minute |
| + mailDrainTimeout = 30 * time.Second |
| + mailAbandonGrace = 5 * time.Second |
| + mailLogDepth = 32 |
| +) |
| + |
| +func newMailQueue(next mail.Sender, admits func(string) bool, gate func(context.Context) bool, logger *slog.Logger) *mailQueue { |
| + return newMailQueueLogging(next, admits, gate, logger, "") |
| +} |
| + |
| +// newMailQueueLogging is newMailQueue with refusedLog set (see the |
| +// field). |
| +func newMailQueueLogging(next mail.Sender, admits func(string) bool, gate func(context.Context) bool, logger *slog.Logger, refusedLog string) *mailQueue { |
| + if logger == nil { |
| + logger = slog.Default() |
| + } |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + q := &mailQueue{next: next, admits: admits, gate: gate, logger: logger, |
| + drainTimeout: mailDrainTimeout, sendTimeout: mailSendTimeout, |
| + jobs: make(chan mailJob, mailQueueDepth), ctx: ctx, cancel: cancel, |
| + done: make(chan struct{}), logs: make(chan logLine, mailLogDepth), refusedLog: refusedLog} |
| + go q.pump() |
| + go q.run() |
| + return q |
| +} |
| + |
| +// Send queues one link. It never blocks, never does I/O and never |
| +// fails: the request that called it answers the same way whatever |
| +// happens next. |
| +func (q *mailQueue) Send(ctx context.Context, to, subject, body string) error { |
| + job := mailJob{to: to, subject: subject, body: body, gated: true} |
| + if q.gate != nil { |
| + job.gated = q.gate(ctx) |
| + } |
| + q.mu.RLock() |
| + defer q.mu.RUnlock() |
| + if q.closed { |
| + q.dropped.Add(1) |
| + return nil |
| + } |
| + q.pending.Add(1) |
| + select { |
| + case q.jobs <- job: |
| + default: |
| + q.pending.Done() |
| + q.dropped.Add(1) |
| + } |
| + return nil |
| +} |
| + |
| +// logf hands one error line to the pump. It never blocks: when the |
| +// pump is behind, the line is counted in lostLogs instead. |
| +func (q *mailQueue) logf(msg string, args ...any) { |
| + q.logMu.RLock() |
| + defer q.logMu.RUnlock() |
| + if q.logsClosed { |
| + q.lostLogs.Add(1) |
| + return |
| + } |
| + q.unlogged.Add(1) |
| + select { |
| + case q.logs <- logLine{msg: msg, args: args}: |
| + default: |
| + q.unlogged.Add(-1) |
| + q.lostLogs.Add(1) |
| + } |
| +} |
| + |
| +// pump is the only goroutine that writes to the logger. It ends when |
| +// the worker closes logs on its way out. |
| +// |
| +// Its limit, stated rather than engineered around: a write blocked |
| +// inside the logger (a stalled sink) cannot be interrupted from Go. It |
| +// can only finish, or die with the process. Closing logs does not reach |
| +// it mid-write, so a stalled sink leaves this one goroutine behind |
| +// until the sink moves. Nothing else waits on it: not Send, not the |
| +// worker, not close. |
| +func (q *mailQueue) pump() { |
| + for l := range q.logs { |
| + if n := q.lostLogs.Swap(0); n > 0 { |
| + q.logger.Error("slides: sign-in mail log lines lost to a slow log sink", "count", n) |
| + } |
| + q.logger.Error(l.msg, l.args...) |
| + q.unlogged.Add(-1) |
| + } |
| +} |
| + |
| +func (q *mailQueue) run() { |
| + defer close(q.done) |
| + defer func() { // after the last logf below; ends the pump |
| + q.logMu.Lock() |
| + q.logsClosed = true |
| + close(q.logs) |
| + q.logMu.Unlock() |
| + }() |
| + abandoned := 0 |
| + for job := range q.jobs { |
| + q.reportDrops() |
| + if q.ctx.Err() != nil { |
| + abandoned++ |
| + } else { |
| + q.deliver(job) |
| + } |
| + q.pending.Done() |
| + } |
| + q.reportDrops() |
| + if abandoned > 0 { |
| + q.logf("slides: sign-in links abandoned at shutdown", "count", abandoned) |
| + } |
| +} |
| + |
| +// reportDrops logs the links Send had to drop since the last report. |
| +// Here, on the worker, so the request path never writes a log line. |
| +func (q *mailQueue) reportDrops() { |
| + if n := q.dropped.Swap(0); n > 0 { |
| + q.logf("slides: sign-in mail queue full; links dropped", "count", n) |
| + } |
| +} |
| + |
| +// deliver sends one link if it may go. A panic in the admission check |
| +// or the mailer is this message's failure, not the process's. |
| +func (q *mailQueue) deliver(job mailJob) { |
| + defer func() { |
| + if p := recover(); p != nil { |
| + q.logf("slides: sign-in mail worker recovered from a panic", "panic", fmt.Sprint(p)) |
| + } |
| + }() |
| + if !job.gated || !q.admits(job.to) { |
| + // Not admitted: the link the auth plugin stored is never |
| + // delivered, so it is never redeemed. No address in the log. |
| + if q.refusedLog != "" { |
| + q.logf(q.refusedLog, "stage", "signin.gate", "refusals_since_boot", q.refusals.Add(1)) |
| + } |
| + return |
| + } |
| + ctx, cancel := context.WithTimeout(q.ctx, q.sendTimeout) |
| + defer cancel() |
| + if err := q.next.Send(ctx, job.to, job.subject, job.body); err != nil { |
| + q.logf("slides: send sign-in link", "err", err) |
| + } |
| +} |
| + |
| +// wait blocks until every link queued so far has been sent or refused, |
| +// and every line logged about them has been written. Tests use it to |
| +// read the mailer, and the log, afterwards. |
| +func (q *mailQueue) wait() { |
| + q.pending.Wait() |
| + for q.unlogged.Load() > 0 { |
| + time.Sleep(time.Millisecond) |
| + } |
| +} |
| + |
| +// close stops taking links and drains what is queued, within |
| +// drainTimeout; then it cancels, abandons the rest, and stops the |
| +// worker (see the type's comment). It never waits on the log pump: |
| +// shutdown deliberately does not wait for a write blocked inside the |
| +// logger, which can only finish or die with the process (see pump). |
| +func (q *mailQueue) close() { |
| + q.mu.Lock() |
| + if q.closed { |
| + q.mu.Unlock() |
| + return |
| + } |
| + q.closed = true |
| + close(q.jobs) |
| + q.mu.Unlock() |
| + defer q.cancel() |
| + select { |
| + case <-q.done: |
| + return |
| + case <-time.After(q.drainTimeout): |
| + } |
| + q.cancel() |
| + select { |
| + case <-q.done: |
| + case <-time.After(mailAbandonGrace): |
| + // Counted, not written: logging here could hang close on the |
| + // very sink that may be the trouble. The worker is still running, |
| + // so logf is still safe to call, and never blocks. |
| + q.stuck.Store(true) |
| + q.logf("slides: sign-in mail worker ignored cancellation; left running") |
| + } |
| +} |
diff --git a/internal/slides/mailqueue_test.go b/internal/slides/mailqueue_test.go| new file mode 100644 |
| index 0000000..3740fdb |
| --- /dev/null |
| +++ b/internal/slides/mailqueue_test.go |
| @@ -0,0 +1,283 @@ |
| +package slides |
| + |
| +import ( |
| + "context" |
| + "fmt" |
| + "log/slog" |
| + "runtime" |
| + "strings" |
| + "sync" |
| + "testing" |
| + "time" |
| +) |
| + |
| +// stuckHandler is a slog handler whose every write blocks until release |
| +// is closed: a log sink that has stalled, as a full pipe does. It |
| +// remembers what it was finally allowed to write. |
| +type stuckHandler struct { |
| + release chan struct{} |
| + mu *sync.Mutex |
| + msgs *[]string |
| +} |
| + |
| +func (h stuckHandler) Enabled(context.Context, slog.Level) bool { return true } |
| +func (h stuckHandler) Handle(_ context.Context, r slog.Record) error { |
| + <-h.release |
| + h.mu.Lock() |
| + *h.msgs = append(*h.msgs, r.Message) |
| + h.mu.Unlock() |
| + return nil |
| +} |
| +func (h stuckHandler) WithAttrs([]slog.Attr) slog.Handler { return h } |
| +func (h stuckHandler) WithGroup(string) slog.Handler { return h } |
| + |
| +// funcSender adapts a function to mail.Sender. |
| +type funcSender func(ctx context.Context, to string) error |
| + |
| +func (f funcSender) Send(ctx context.Context, to, _, _ string) error { return f(ctx, to) } |
| + |
| +func everyone(string) bool { return true } |
| + |
| +// TestMailQueueSendNeverBlocksWhenFull: Send is the request path, so it |
| +// may not block or do I/O even when the queue is full — not even to log |
| +// the drop (Astra on oficina/slides #20, round 2, P1: a "queue full" log |
| +// line on a stalled sink held the request, and saturation is |
| +// admission-dependent). The worker is stuck on its first link and the |
| +// log sink is stuck too; twice the queue's depth of Sends must still |
| +// return at once, and the drops are logged by the worker afterwards. |
| +func TestMailQueueSendNeverBlocksWhenFull(t *testing.T) { |
| + release := make(chan struct{}) |
| + var once sync.Once |
| + free := func() { once.Do(func() { close(release) }) } |
| + stuck := funcSender(func(ctx context.Context, _ string) error { |
| + select { |
| + case <-release: |
| + case <-ctx.Done(): |
| + } |
| + return nil |
| + }) |
| + var mu sync.Mutex |
| + var msgs []string |
| + q := newMailQueue(stuck, everyone, nil, slog.New(stuckHandler{release, &mu, &msgs})) |
| + t.Cleanup(func() { free(); q.close() }) |
| + |
| + done := make(chan struct{}) |
| + go func() { |
| + for i := 0; i < 2*mailQueueDepth; i++ { |
| + _ = q.Send(context.Background(), fmt.Sprintf("p%d@example.test", i), "s", "b") |
| + } |
| + close(done) |
| + }() |
| + select { |
| + case <-done: |
| + case <-time.After(2 * time.Second): |
| + free() |
| + <-done |
| + t.Fatal("Send blocked with the queue full: the request path waited on the queue or the log") |
| + } |
| + // The drops are the worker's to log, through the pump, once the |
| + // sink is free. |
| + free() |
| + q.close() |
| + deadline := time.Now().Add(2 * time.Second) |
| + for time.Now().Before(deadline) { |
| + mu.Lock() |
| + for _, m := range msgs { |
| + if strings.Contains(m, "links dropped") { |
| + mu.Unlock() |
| + return |
| + } |
| + } |
| + mu.Unlock() |
| + time.Sleep(5 * time.Millisecond) |
| + } |
| + t.Fatalf("no drop was logged with twice the queue's depth sent; the log has %q", msgs) |
| +} |
| + |
| +// TestMailQueueQueuesEveryAddress: the queue's fill level must not |
| +// depend on admission, so Send queues a refused address's link exactly |
| +// as it queues a permitted one; only the worker asks. |
| +func TestMailQueueQueuesEveryAddress(t *testing.T) { |
| + release := make(chan struct{}) |
| + stuck := funcSender(func(ctx context.Context, _ string) error { |
| + select { |
| + case <-release: |
| + case <-ctx.Done(): |
| + } |
| + return nil |
| + }) |
| + var asked sync.Map |
| + admits := func(to string) bool { asked.Store(to, true); return to == "first@example.test" } |
| + q := newMailQueue(stuck, admits, nil, slog.New(slog.DiscardHandler)) |
| + t.Cleanup(func() { close(release); q.close() }) |
| + _ = q.Send(context.Background(), "first@example.test", "s", "b") |
| + deadline := time.Now().Add(2 * time.Second) |
| + for _, ok := asked.Load("first@example.test"); !ok && time.Now().Before(deadline); _, ok = asked.Load("first@example.test") { |
| + time.Sleep(5 * time.Millisecond) |
| + } |
| + for i := 0; i < 5; i++ { |
| + _ = q.Send(context.Background(), fmt.Sprintf("refused%d@example.test", i), "s", "b") |
| + } |
| + if got := len(q.jobs); got != 5 { |
| + t.Fatalf("queued %d refused links, want all 5: fill level must not depend on admission", got) |
| + } |
| +} |
| + |
| +// TestMailQueueSurvivesAPanic: a panic in the mailer is that message's |
| +// failure; the worker recovers, logs, and sends the next one (Astra, |
| +// round 2, P2). |
| +func TestMailQueueSurvivesAPanic(t *testing.T) { |
| + var mu sync.Mutex |
| + var sent []string |
| + calls := 0 |
| + s := funcSender(func(_ context.Context, to string) error { |
| + mu.Lock() |
| + defer mu.Unlock() |
| + calls++ |
| + if calls == 1 { |
| + panic("mailer exploded") |
| + } |
| + sent = append(sent, to) |
| + return nil |
| + }) |
| + q := newMailQueue(s, everyone, nil, slog.New(slog.DiscardHandler)) |
| + t.Cleanup(q.close) |
| + _ = q.Send(context.Background(), "first@example.test", "s", "b") |
| + _ = q.Send(context.Background(), "second@example.test", "s", "b") |
| + q.wait() |
| + mu.Lock() |
| + defer mu.Unlock() |
| + if len(sent) != 1 || sent[0] != "second@example.test" { |
| + t.Fatalf("sent = %v, want the second link after the first panicked", sent) |
| + } |
| +} |
| + |
| +// TestMailQueueCloseIsBoundedAndStopsTheWorker: close drains within its |
| +// deadline, then cancels the send in flight, abandons the rest, and the |
| +// worker is gone — no goroutine outlives close (Astra, round 2, P2). |
| +func TestMailQueueCloseIsBoundedAndStopsTheWorker(t *testing.T) { |
| + baseline := runtime.NumGoroutine() |
| + honours := funcSender(func(ctx context.Context, _ string) error { |
| + <-ctx.Done() // a relay that never answers, but obeys cancellation |
| + return ctx.Err() |
| + }) |
| + q := newMailQueue(honours, everyone, nil, slog.New(slog.DiscardHandler)) |
| + q.drainTimeout = 50 * time.Millisecond |
| + for i := 0; i < 3; i++ { |
| + _ = q.Send(context.Background(), fmt.Sprintf("p%d@example.test", i), "s", "b") |
| + } |
| + start := time.Now() |
| + q.close() |
| + if took := time.Since(start); took > 2*time.Second { |
| + t.Fatalf("close took %v, want about its 50ms deadline", took) |
| + } |
| + select { |
| + case <-q.done: |
| + default: |
| + t.Fatal("the worker was still running after close") |
| + } |
| + deadline := time.Now().Add(time.Second) |
| + for runtime.NumGoroutine() > baseline && time.Now().Before(deadline) { |
| + time.Sleep(5 * time.Millisecond) |
| + } |
| + if n := runtime.NumGoroutine(); n > baseline { |
| + t.Fatalf("%d goroutines after close, %d before: the queue leaked", n, baseline) |
| + } |
| +} |
| + |
| +// TestAppCloseStopsTheMailWorker: the app owns its queue, and Close |
| +// stops its worker — no test that builds an app and closes it leaves |
| +// one running behind it. |
| +func TestAppCloseStopsTheMailWorker(t *testing.T) { |
| + _, a, err := New(testDB(t), Config{Env: EnvDevelopment, Origin: "http://localhost:8091", Logger: quietLogger()}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + q := a.mail |
| + if err := a.Close(); err != nil { |
| + t.Fatal(err) |
| + } |
| + select { |
| + case <-q.done: |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("the sign-in mail worker was still running after app.Close") |
| + } |
| +} |
| + |
| +// mailWorkers counts goroutines running a sign-in mail queue's worker |
| +// or log pump — the targeted leak check (goleak's idea, without the |
| +// dependency) TestMain runs after the package's tests. |
| +func mailWorkers() int { |
| + buf := make([]byte, 1<<22) |
| + stacks := string(buf[:runtime.Stack(buf, true)]) |
| + return strings.Count(stacks, "(*mailQueue).run(") + strings.Count(stacks, "(*mailQueue).pump(") |
| +} |
| + |
| +// TestMailQueueCloseIsBoundedWithAStalledLogger: close reports through |
| +// nothing that can block, and never waits on the log pump. With the log |
| +// sink stalled — every write hangs — and a relay that never answers but |
| +// honours cancellation, and WHILE THE SINK IS STILL STALLED: |
| +// |
| +// - close returns within its bound; |
| +// - the mail worker has exited; |
| +// - at most the one log pump remains, blocked in its write. |
| +// |
| +// A write blocked inside the logger cannot be interrupted from Go; it |
| +// can only finish or die with the process, and shutdown deliberately |
| +// does not wait for it. So only once the sink is released must the pump |
| +// exit and the goroutine count return to its baseline (Astra on #20, |
| +// rounds 3 and 4). The test fails if close waited on the pump, or if |
| +// the pump could hold the worker. |
| +func TestMailQueueCloseIsBoundedWithAStalledLogger(t *testing.T) { |
| + waitFor := func(cond func() bool) bool { |
| + deadline := time.Now().Add(2 * time.Second) |
| + for !cond() && time.Now().Before(deadline) { |
| + time.Sleep(5 * time.Millisecond) |
| + } |
| + return cond() |
| + } |
| + baseline := runtime.NumGoroutine() |
| + release := make(chan struct{}) |
| + var once sync.Once |
| + free := func() { once.Do(func() { close(release) }) } |
| + t.Cleanup(free) |
| + var mu sync.Mutex |
| + var msgs []string |
| + honours := funcSender(func(ctx context.Context, _ string) error { |
| + <-ctx.Done() |
| + return ctx.Err() |
| + }) |
| + q := newMailQueue(honours, everyone, nil, slog.New(stuckHandler{release, &mu, &msgs})) |
| + q.drainTimeout = 50 * time.Millisecond |
| + for i := 0; i < 2*mailQueueDepth; i++ { // drops too, so there is plenty to log |
| + _ = q.Send(context.Background(), fmt.Sprintf("p%d@example.test", i), "s", "b") |
| + } |
| + closed := make(chan struct{}) |
| + go func() { q.close(); close(closed) }() |
| + select { |
| + case <-closed: |
| + case <-time.After(2 * time.Second): |
| + free() |
| + <-closed |
| + t.Fatal("close hung on a stalled log sink") |
| + } |
| + |
| + // The sink is still stalled. |
| + select { |
| + case <-q.done: |
| + default: |
| + t.Fatal("the worker was still running after close, held by the stalled log sink") |
| + } |
| + if !waitFor(func() bool { return runtime.NumGoroutine() <= baseline+1 }) { |
| + t.Fatalf("%d goroutines with the sink stalled, %d before: more than the one blocked log pump remains", runtime.NumGoroutine(), baseline) |
| + } |
| + if n := mailWorkers(); n > 1 { |
| + t.Fatalf("%d mail queue goroutines with the sink stalled, want at most the log pump", n) |
| + } |
| + |
| + // Released, the pump finishes its write and exits. |
| + free() |
| + if !waitFor(func() bool { return mailWorkers() == 0 && runtime.NumGoroutine() <= baseline }) { |
| + t.Fatalf("after release: %d mail queue goroutines, %d goroutines against %d before", mailWorkers(), runtime.NumGoroutine(), baseline) |
| + } |
| +} |
diff --git a/internal/slides/render.go b/internal/slides/render.go| index d20fb12..58a9acc 100644 |
| --- a/internal/slides/render.go |
| +++ b/internal/slides/render.go |
| @@ -146,6 +146,9 @@ func init() { |
| switch name { |
| case "credits": |
| t = template.Must(t.ParseFS(credits.Templates(), "templates/*.html")) |
| + case "signin": |
| + // The Keymail continuation's meta refresh (home-oauth-head). |
| + t = template.Must(t.ParseFS(connect.Templates(), "templates/oauth.html")) |
| case "home_link": |
| t = template.Must(t.ParseFS(connect.Templates(), "templates/home_link.html")) |
| } |
diff --git a/internal/slides/templates/signin.html b/internal/slides/templates/signin.html| index 0fb9491..c5f5f0e 100644 |
| --- a/internal/slides/templates/signin.html |
| +++ b/internal/slides/templates/signin.html |
| @@ -1,15 +1,18 @@ |
| {{/* signin — GET /signin (auth.go's signin handler): estilo's sign-in |
| form, or once a link is on its way, the check-your-email state that |
| - replaces it. rastrillo/auth's own handlers never render a page |
| - (magic-links.md: "the sign-in page stays yours"): Begin, Callback and |
| - Verify report every outcome by redirecting back here with a query the |
| - handler reads. |
| + replaces it, or the card between the form and Keymail. rastrillo/auth's |
| + own handlers never render a page (magic-links.md: "the sign-in page |
| + stays yours"): Begin, Callback and Verify report every outcome by |
| + redirecting back here with a query the handler reads. |
| - The form POSTs straight to rastrillo/auth's own Begin behind |
| - gateBegin (app.go). The hidden force=1 says "plain magic link": this |
| - app never offers a keymail or passkey ceremony, and gateBegin sets it |
| - again server-side, so the guarantee does not depend on this field. |
| + The form POSTs to rastrillo/auth's own Begin through gateBegin |
| + (app.go, auth.go). An address whose domain runs Keymail is answered |
| + with this page again in its Keymail state (.Continue): the door's |
| + card, and a meta refresh that goes on to Keymail — a navigation from a |
| + page, which form-action does not govern. The form carries force=1 only |
| + on ?force=1 (a failed Keymail exchange), to ask for an emailed link. |
| The bar shows the brand only: nobody is signed in here to have an |
| account, and everything the brand could link to is behind the |
| session gate. Data: signinView. */}} |
| +{{define "page-head"}}{{with .Continue}}{{template "home-oauth-head" .}}{{end}}{{end}} |
| {{define "content"}}{{with .Sent}}{{template "estilo-signin-sent" .}}{{else}}{{template "estilo-signin" .SignIn}}{{end}}{{end}} |
diff --git a/internal/slidestest/browser_test.go b/internal/slidestest/browser_test.go| index cec28e9..f5dad94 100644 |
| --- a/internal/slidestest/browser_test.go |
| +++ b/internal/slidestest/browser_test.go |
| @@ -82,6 +82,7 @@ func newRigWith(t *testing.T, store blobs.Store) (*harness.Rig, *fakeMailer) { |
| t.Fatalf("slides.New: %v", err) |
| } |
| t.Cleanup(func() { a.Close() }) |
| + mailer.flush = a.FlushMail |
| return localised(t, mux) |
| }) |
| return rig, mailer |
diff --git a/internal/slidestest/capabilities_test.go b/internal/slidestest/capabilities_test.go| index 76dd319..840b817 100644 |
| --- a/internal/slidestest/capabilities_test.go |
| +++ b/internal/slidestest/capabilities_test.go |
| @@ -395,5 +395,6 @@ func newAppOver(t *testing.T, d *db.DB, cfg slides.Config, mailer *fakeMailer) * |
| t.Fatalf("slides.New: %v", err) |
| } |
| t.Cleanup(func() { a.Close() }) |
| + mailer.flush = a.FlushMail |
| return &testApp{Handler: localised(t, mux), mailer: mailer, db: d, app: a} |
| } |
diff --git a/internal/slidestest/harness_test.go b/internal/slidestest/harness_test.go| index 01841e4..dde46af 100644 |
| --- a/internal/slidestest/harness_test.go |
| +++ b/internal/slidestest/harness_test.go |
| @@ -55,6 +55,9 @@ type sentMail struct{ To, Subject, Body string } |
| type fakeMailer struct { |
| mu sync.Mutex |
| sent []sentMail |
| + // flush waits for the app's mail queue: links are sent after the |
| + // response (mailqueue.go), so messages reads only after it. |
| + flush func() |
| } |
| func (m *fakeMailer) Send(_ context.Context, to, subject, body string) error { |
| @@ -66,6 +69,9 @@ func (m *fakeMailer) Send(_ context.Context, to, subject, body string) error { |
| // messages returns a copy of everything sent so far. |
| func (m *fakeMailer) messages() []sentMail { |
| + if m.flush != nil { |
| + m.flush() |
| + } |
| m.mu.Lock() |
| defer m.mu.Unlock() |
| return append([]sentMail(nil), m.sent...) |
| @@ -94,6 +100,7 @@ type testApp struct { |
| Close() error |
| SetExportCaps(perDeck, perInstance int64) |
| ForgetExportJobs() error |
| + FlushMail() |
| } |
| } |
| @@ -133,6 +140,9 @@ func newAppWith(t *testing.T, cfg slides.Config, mailer *fakeMailer) *testApp { |
| // Releases the compiled deck core's wazero runtime; every test app |
| // built over slides.New leaks one without this (task 3 fix round 1). |
| t.Cleanup(func() { a.Close() }) |
| + if mailer != nil { |
| + mailer.flush = a.FlushMail |
| + } |
| return &testApp{Handler: localised(t, mux), mailer: mailer, db: d, app: a} |
| } |
diff --git a/internal/slidestest/keymail_signin_test.go b/internal/slidestest/keymail_signin_test.go| new file mode 100644 |
| index 0000000..c3e2d6d |
| --- /dev/null |
| +++ b/internal/slidestest/keymail_signin_test.go |
| @@ -0,0 +1,246 @@ |
| +package slidestest |
| + |
| +import ( |
| + "fmt" |
| + "html" |
| + "io" |
| + "net/http" |
| + "net/url" |
| + "os" |
| + "regexp" |
| + "runtime" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/oficina/slides/internal/slides/keymailtest" |
| +) |
| + |
| +// TestMain keeps every sign-in in this package inside the process: no |
| +// DNS query or HTTPS probe made by Keymail classification leaves it |
| +// (keymailtest's package comment says how). An address at a domain no |
| +// test registered classifies as plain email. |
| +func TestMain(m *testing.M) { |
| + restore, err := keymailtest.Install() |
| + if err != nil { |
| + fmt.Fprintln(os.Stderr, "keymailtest.Install:", err) |
| + os.Exit(1) |
| + } |
| + code := m.Run() |
| + // No test may leave a sign-in mail worker running: every app and |
| + // every queue a test builds is closed in its cleanup. |
| + if code == 0 { |
| + deadline := time.Now().Add(5 * time.Second) |
| + for mailWorkers() > 0 && time.Now().Before(deadline) { |
| + time.Sleep(10 * time.Millisecond) |
| + } |
| + if n := mailWorkers(); n > 0 { |
| + fmt.Fprintf(os.Stderr, "FAIL: %d sign-in mail queue goroutines still running after the tests\n", n) |
| + code = 1 |
| + } |
| + } |
| + restore() |
| + os.Exit(code) |
| +} |
| + |
| +// The Keymail deployment these tests sign in through: keymail.dev's |
| +// shape, a domain that is its own Keymail (no `_keymail` record). |
| +const ( |
| + keymailHost = "keymail.slides.test" |
| + keymailMember = "paul@" + keymailHost |
| + keymailStranger = "stranger@" + keymailHost |
| +) |
| + |
| +// newKeymailApp is newApp with keymailMember on the allowlist beside |
| +// allowedAddress, and a Keymail deployment holding both keymail |
| +// addresses — one permitted here, one not. |
| +func newKeymailApp(t *testing.T) (*testApp, *keymailtest.Server) { |
| + t.Helper() |
| + km := keymailtest.Serve(t, keymailHost, keymailMember, keymailStranger) |
| + cfg, mailer := newConfig(t) |
| + cfg.AllowedAddresses = append(cfg.AllowedAddresses, keymailMember) |
| + return newAppWith(t, cfg, mailer), km |
| +} |
| + |
| +var authorizeHref = regexp.MustCompile(`href="(https://[^"]+/oauth/authorize\?[^"]+)"`) |
| + |
| +// continuation asserts rec is the page between the form and Keymail — |
| +// a 200 document, never a redirect — and returns the authorize URL it |
| +// carries. |
| +func continuation(t *testing.T, label string, rec interface { |
| + Result() *http.Response |
| +}, body string) *url.URL { |
| + t.Helper() |
| + res := rec.Result() |
| + if res.StatusCode != http.StatusOK { |
| + t.Fatalf("%s: status %d, want 200 — the hop to Keymail must be a page, not a redirect from the form POST (Safari applies form-action to it)", label, res.StatusCode) |
| + } |
| + if loc := res.Header.Get("Location"); loc != "" { |
| + t.Fatalf("%s: Location %q, want none", label, loc) |
| + } |
| + m := authorizeHref.FindStringSubmatch(body) |
| + if m == nil { |
| + t.Fatalf("%s: no Keymail authorize link on the page:\n%s", label, body) |
| + } |
| + u, err := url.Parse(html.UnescapeString(m[1])) |
| + if err != nil { |
| + t.Fatalf("%s: parse authorize link: %v", label, err) |
| + } |
| + if u.Host != keymailHost { |
| + t.Errorf("%s: authorize link goes to %q, want %q", label, u.Host, keymailHost) |
| + } |
| + if !strings.Contains(body, `http-equiv="refresh"`) { |
| + t.Errorf("%s: the continuation does not navigate on by itself (no meta refresh)", label) |
| + } |
| + return u |
| +} |
| + |
| +// TestKeymailAddressGoesToKeymailWhetherPermittedOrNot is the oracle |
| +// test. Whether an address goes to Keymail depends on its domain alone: |
| +// a permitted and a non-permitted Keymail address get the same first |
| +// answer, a continuation page to Keymail, so the answer says nothing |
| +// about the allowlist. Neither is mailed anything. |
| +func TestKeymailAddressGoesToKeymailWhetherPermittedOrNot(t *testing.T) { |
| + app, km := newKeymailApp(t) |
| + |
| + member := post(t, app, "/signin", url.Values{"address": {keymailMember}}) |
| + stranger := post(t, app, "/signin", url.Values{"address": {keymailStranger}}) |
| + |
| + mu := continuation(t, "permitted Keymail address", member, member.Body.String()) |
| + su := continuation(t, "non-permitted Keymail address", stranger, stranger.Body.String()) |
| + |
| + // The same page but for the per-attempt state and PKCE challenge. |
| + mask := func(body string, u *url.URL) string { |
| + return strings.ReplaceAll(body, html.EscapeString(u.String()), "AUTHORIZE") |
| + } |
| + if a, b := mask(member.Body.String(), mu), mask(stranger.Body.String(), su); a != b { |
| + t.Errorf("the two Keymail answers differ beyond the authorize URL:\npermitted:\n%s\nnot permitted:\n%s", a, b) |
| + } |
| + cookieNames := func(res *http.Response) string { |
| + var n []string |
| + for _, c := range res.Cookies() { |
| + n = append(n, c.Name) |
| + } |
| + return strings.Join(n, ",") |
| + } |
| + if a, b := cookieNames(member.Result()), cookieNames(stranger.Result()); a != b { |
| + t.Errorf("cookies differ: permitted %q, not permitted %q", a, b) |
| + } |
| + if n := len(app.mailer.messages()); n != 0 { |
| + t.Errorf("mailer captured %d messages, want 0: a Keymail address is never mailed", n) |
| + } |
| + if n := km.Lookups(); n != 2 { |
| + t.Errorf("Keymail saw %d federation lookups, want 2 (both addresses classified)", n) |
| + } |
| +} |
| + |
| +// TestNonPermittedPlainAddressIsAnsweredSentAndNotMailed: an address at |
| +// a domain with no Keymail still gets the ordinary ?sent=1, permitted |
| +// or not, and only the permitted one is mailed. |
| +func TestNonPermittedPlainAddressIsAnsweredSentAndNotMailed(t *testing.T) { |
| + app, _ := newKeymailApp(t) |
| + for _, address := range []string{deniedAddress, allowedAddress} { |
| + rec := post(t, app, "/signin", url.Values{"address": {address}}) |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Fatalf("POST /signin %q: %d %q, want 303 /signin?sent=1", address, rec.Code, rec.Header().Get("Location")) |
| + } |
| + } |
| + msgs := app.mailer.messages() |
| + if len(msgs) != 1 || msgs[0].To != allowedAddress { |
| + t.Fatalf("mail sent = %+v, want exactly one, to %q", msgs, allowedAddress) |
| + } |
| +} |
| + |
| +// TestKeymailFailureFallsBackToAnEmailLink: rastrillo answers a failed |
| +// Keymail exchange with ?force=1&err=keymail. That page must carry |
| +// force back so the next POST mails a link instead of returning to the |
| +// Keymail that just failed — Paul is never stuck. The ordinary form |
| +// carries no force at all. |
| +func TestKeymailFailureFallsBackToAnEmailLink(t *testing.T) { |
| + app, km := newKeymailApp(t) |
| + forced := `name="force" value="1"` |
| + |
| + if body := get(t, app, "/signin").Body.String(); strings.Contains(body, forced) { |
| + t.Fatalf("the ordinary sign-in form forces an email link; Keymail addresses would never reach Keymail") |
| + } |
| + page := get(t, app, "/signin?force=1&err=keymail") |
| + if page.Code != http.StatusOK || !strings.Contains(page.Body.String(), forced) { |
| + t.Fatalf("GET /signin?force=1&err=keymail: %d, carries force=%v", page.Code, strings.Contains(page.Body.String(), forced)) |
| + } |
| + |
| + rec := post(t, app, "/signin", url.Values{"address": {keymailMember}, "force": {"1"}}) |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Fatalf("forced POST: %d %q, want 303 /signin?sent=1", rec.Code, rec.Header().Get("Location")) |
| + } |
| + msgs := app.mailer.messages() |
| + if len(msgs) != 1 || msgs[0].To != keymailMember || magicLinkPattern.FindString(msgs[0].Body) == "" { |
| + t.Fatalf("mail sent = %+v, want one magic link to %q", msgs, keymailMember) |
| + } |
| + if n := km.Lookups(); n != 0 { |
| + t.Errorf("a forced sign-in classified the address (%d lookups)", n) |
| + } |
| +} |
| + |
| +// keymailRoundTrip runs POST /signin → continuation → Keymail approves |
| +// → GET /auth/callback for address and returns the callback's answer |
| +// and the cookies collected along the way. |
| +func keymailRoundTrip(t *testing.T, app *testApp, km *keymailtest.Server, address string) (*http.Response, []*http.Cookie) { |
| + t.Helper() |
| + begin := post(t, app, "/signin", url.Values{"address": {address}}) |
| + u := continuation(t, address, begin, begin.Body.String()) |
| + cookies := begin.Result().Cookies() |
| + cb := get(t, app, "/auth/callback?"+url.Values{"state": {u.Query().Get("state")}, "code": {km.Approve(address)}}.Encode(), cookies...) |
| + return cb.Result(), append(cookies, cb.Result().Cookies()...) |
| +} |
| + |
| +// TestKeymailMemberIsSignedIn: the whole Keymail round trip for a |
| +// permitted address ends signed in. |
| +func TestKeymailMemberIsSignedIn(t *testing.T) { |
| + app, km := newKeymailApp(t) |
| + res, cookies := keymailRoundTrip(t, app, km, keymailMember) |
| + if res.StatusCode != http.StatusSeeOther { |
| + t.Fatalf("callback: %d, want 303 into the app", res.StatusCode) |
| + } |
| + if rec := get(t, app, "/", cookies...); rec.Code != http.StatusOK { |
| + t.Fatalf("GET / after Keymail sign-in: %d, want 200", rec.Code) |
| + } |
| +} |
| + |
| +// TestKeymailRefusalIsTheDoor: a non-permitted address that Keymail |
| +// has verified is refused AFTER verification, and the refusal is the |
| +// sign-in door, rendered right there with the error and a form for a |
| +// different address — not rastrillo's bare-text 403. No session is |
| +// minted. And the sentence cannot be summoned by anybody else: visiting |
| +// /signin?err=refused shows no such thing (Astra, round 3), because the |
| +// refusal is never a query parameter. |
| +func TestKeymailRefusalIsTheDoor(t *testing.T) { |
| + app, km := newKeymailApp(t) |
| + res, cookies := keymailRoundTrip(t, app, km, keymailStranger) |
| + body, _ := io.ReadAll(res.Body) |
| + if res.StatusCode != http.StatusForbidden || res.Header.Get("Location") != "" { |
| + t.Fatalf("callback for a non-permitted address: %d %q, want 403 and the door itself", res.StatusCode, res.Header.Get("Location")) |
| + } |
| + for _, want := range []string{"estilo-door", `name="address"`, "can’t sign in to Slides"} { |
| + if !strings.Contains(string(body), want) { |
| + t.Errorf("refusal page lacks %q:\n%s", want, body) |
| + } |
| + } |
| + if strings.Contains(string(body), "not admitted here") { |
| + t.Error("the refusal is rastrillo's bare text") |
| + } |
| + if rec := get(t, app, "/", cookies...); rec.Code == http.StatusOK { |
| + t.Fatal("a refused Keymail sign-in left a working session") |
| + } |
| + if spoof := get(t, app, "/signin?err=refused").Body.String(); strings.Contains(spoof, "can’t sign in to Slides") { |
| + t.Fatal("anybody can show the refusal by visiting /signin?err=refused") |
| + } |
| +} |
| + |
| +// mailWorkers counts goroutines running a sign-in mail queue's worker |
| +// or log pump (internal/slides/mailqueue.go) — the targeted leak check |
| +// TestMain runs after the package's tests. |
| +func mailWorkers() int { |
| + buf := make([]byte, 1<<22) |
| + stacks := string(buf[:runtime.Stack(buf, true)]) |
| + return strings.Count(stacks, "(*mailQueue).run(") + strings.Count(stacks, "(*mailQueue).pump(") |
| +} |
diff --git a/internal/slidestest/signin_test.go b/internal/slidestest/signin_test.go| index 5ec4677..02c5011 100644 |
| --- a/internal/slidestest/signin_test.go |
| +++ b/internal/slidestest/signin_test.go |
| @@ -91,13 +91,13 @@ func TestSigninMailsAllowedAddressOnly(t *testing.T) { |
| // the no-oracle guarantee, and the case the first version of gateBegin |
| // got wrong. |
| // |
| -// A denied address is short-circuited before rastrillo/auth's Begin, so |
| -// it can answer ?sent=1 forever. A permitted address reaches Begin, |
| -// which enforces a per-address budget of 5 in a rolling window and |
| -// redirects to ?err=rate once it is spent. So without the fix, six |
| -// POSTs of one candidate address separate "on the list" from "not on |
| -// it" — the whole allowlist, enumerable by anyone, one address at a |
| -// time. |
| +// That version short-circuited a denied address before rastrillo/auth's |
| +// Begin, so it answered ?sent=1 forever, while a permitted address |
| +// reached Begin's per-address budget of 5 and got ?err=rate: six POSTs |
| +// of one candidate address separated "on the list" from "not on it". |
| +// Every address reaches Begin now (gateBegin), so the budgets are the |
| +// same for both; sameAnswer still answers ?err=rate as sent, and this |
| +// pins that. |
| // |
| // Seven POSTs, every one byte-identical to what a denied address gets. |
| // The mailer stops at 5, which is the other half of the proof: the |
diff --git a/internal/slidestest/signin_timing_test.go b/internal/slidestest/signin_timing_test.go| new file mode 100644 |
| index 0000000..461f884 |
| --- /dev/null |
| +++ b/internal/slidestest/signin_timing_test.go |
| @@ -0,0 +1,125 @@ |
| +package slidestest |
| + |
| +import ( |
| + "context" |
| + "fmt" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "strings" |
| + "sync" |
| + "testing" |
| + "time" |
| +) |
| + |
| +// blockingMailer holds every Send until release is closed — an SMTP |
| +// relay that takes as long as the test likes. |
| +type blockingMailer struct { |
| + release chan struct{} |
| + mu sync.Mutex |
| + to []string |
| +} |
| + |
| +func (m *blockingMailer) Send(_ context.Context, to, _, _ string) error { |
| + <-m.release |
| + m.mu.Lock() |
| + m.to = append(m.to, to) |
| + m.mu.Unlock() |
| + return nil |
| +} |
| + |
| +func (m *blockingMailer) sent() []string { |
| + m.mu.Lock() |
| + defer m.mu.Unlock() |
| + return append([]string(nil), m.to...) |
| +} |
| + |
| +// TestSigninNeverWaitsForTheMailer is the timing half of the no-oracle |
| +// guarantee (Astra on #20, P1). When the send ran inside the request, a |
| +// permitted address waited for SMTP before its ?sent=1 and a denied one |
| +// did not — 252 ms against 0.35 ms, the allowlist readable off a |
| +// stopwatch, and force=1 made Keymail addresses readable the same way. |
| +// |
| +// Here the mailer never answers until the test says so. Every POST — |
| +// permitted, denied, and both again with force=1 — must come back while |
| +// it is still blocked, and the permitted links must then go out. |
| +func TestSigninNeverWaitsForTheMailer(t *testing.T) { |
| + bm := &blockingMailer{release: make(chan struct{})} |
| + cfg, _ := newConfig(t) |
| + cfg.Mailer = bm |
| + app := newAppWith(t, cfg, nil) |
| + var once sync.Once |
| + free := func() { once.Do(func() { close(bm.release) }) } |
| + t.Cleanup(free) // before the app's own Close, which drains the queue |
| + |
| + posts := []url.Values{ |
| + {"address": {allowedAddress}}, |
| + {"address": {deniedAddress}}, |
| + {"address": {allowedAddress}, "force": {"1"}}, |
| + {"address": {deniedAddress}, "force": {"1"}}, |
| + } |
| + for _, form := range posts { |
| + done := make(chan *httptest.ResponseRecorder, 1) |
| + go func() { done <- post(t, app, "/signin", form) }() |
| + select { |
| + case rec := <-done: |
| + if rec.Code != 303 || rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Errorf("POST %v: %d %q, want 303 /signin?sent=1", form, rec.Code, rec.Header().Get("Location")) |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Errorf("POST %v waited on the mailer: the response time says the address is permitted", form) |
| + free() |
| + <-done |
| + } |
| + } |
| + |
| + free() |
| + deadline := time.Now().Add(5 * time.Second) |
| + for len(bm.sent()) < 2 && time.Now().Before(deadline) { |
| + time.Sleep(10 * time.Millisecond) |
| + } |
| + time.Sleep(50 * time.Millisecond) // and nothing more arrives |
| + got := bm.sent() |
| + if len(got) != 2 || got[0] != allowedAddress || got[1] != allowedAddress { |
| + t.Fatalf("mail sent = %v, want two links, both to %s", got, allowedAddress) |
| + } |
| +} |
| + |
| +// TestAFullMailQueueAnswersLikeAnEmptyOne: the queue holds 64 links. |
| +// With the mailer stuck on the first (permitted) link, enough POSTs to |
| +// overflow it — each from its own address and IP, so no budget stands |
| +// in — must every one be answered exactly as the first was, and at |
| +// once. Every address's link is queued, permitted or not, so a refused |
| +// address fills the queue as surely as a permitted one (Astra on #20, |
| +// round 2). |
| +func TestAFullMailQueueAnswersLikeAnEmptyOne(t *testing.T) { |
| + bm := &blockingMailer{release: make(chan struct{})} |
| + cfg, _ := newConfig(t) |
| + cfg.Mailer = bm |
| + app := newAppWith(t, cfg, nil) |
| + var once sync.Once |
| + free := func() { once.Do(func() { close(bm.release) }) } |
| + t.Cleanup(free) |
| + |
| + postFrom := func(i int, address string) *httptest.ResponseRecorder { |
| + req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader(url.Values{"address": {address}}.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + req.Header.Set("Origin", testOrigin) |
| + req.Header.Set("Sec-Fetch-Site", "same-origin") |
| + req.RemoteAddr = fmt.Sprintf("198.51.100.%d:1234", i%250+1) |
| + rec := httptest.NewRecorder() |
| + app.ServeHTTP(rec, req) |
| + return rec |
| + } |
| + first := postFrom(0, allowedAddress) |
| + for i := 1; i <= 80; i++ { |
| + start := time.Now() |
| + rec := postFrom(i, fmt.Sprintf("stranger%d@else.test", i)) |
| + if took := time.Since(start); took > time.Second { |
| + t.Fatalf("POST %d took %v with the queue full", i, took) |
| + } |
| + if rec.Code != first.Code || rec.Header().Get("Location") != first.Header().Get("Location") || rec.Body.String() != first.Body.String() { |
| + t.Fatalf("POST %d answered %d %q, want exactly the first answer %d %q", i, rec.Code, rec.Header().Get("Location"), first.Code, first.Header().Get("Location")) |
| + } |
| + } |
| +} |