Calendar signs in through Keymail, without revealing who is admitted
Every Keymail address gets the same continuation page to Keymail; other addresses get ?sent=1, and links are mailed by a bounded, non-blocking queue only to permitted addresses. A refused address sees the door inside the 403. Tests are hermetic. Reviewed by GPT-6-Astra via Codex (#28). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Squashed from fix/keymail-signin into main.
19 files changed,
+1483
−92
AGENTS.md+1 −0internal/calendar/app.go+16 −1internal/calendar/auth.go+9 −71internal/calendar/auth_keymail.go+139 −0internal/calendar/home_router_test.go+1 −0internal/calendar/keymail_signin_test.go+273 −0internal/calendar/keymailtest/keymailtest.go+190 −0internal/calendar/mail_bounded.go+9 −0internal/calendar/mail_bounded_panic_test.go+61 −0internal/calendar/render.go+3 −1internal/calendar/routes.go+7 −15internal/calendar/signin.go+33 −1internal/calendar/signin_mail.go+283 −0internal/calendar/signin_mail_test.go+282 −0internal/calendar/signin_timing_test.go+94 −0internal/calendar/templates/signin.html+5 −3internal/calendar/testing.go+10 −0internal/calendartest/delivery_test.go+1 −0internal/calendartest/main_test.go+66 −0
CI passed — run details
CI log
=== vet === go vet ./... go vet -tags browser ./internal/calendartest/ === fmt === === test === go test ./... ? amadan.net/oficina/calendar/cmd/calendar [no test files] ok amadan.net/oficina/calendar/internal/calendar 5.094s ? amadan.net/oficina/calendar/internal/calendar/icons [no test files] ? amadan.net/oficina/calendar/internal/calendar/keymailtest [no test files] ? amadan.net/oficina/calendar/internal/calendar/seal [no test files] ok amadan.net/oficina/calendar/internal/calendartest 2.882s ok amadan.net/oficina/calendar/internal/guardstest 0.048s ? amadan.net/oficina/calendar/scripts/writeestilo [no test files] === migration-check === go tool rastrillo migration check rastrillo migration check: models and migrations agree
diff --git a/AGENTS.md b/AGENTS.md| index 37c2eeb..733b7b2 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -61,6 +61,7 @@ pass by reading a file the data has not reached. |
| | 5 | Reminders are per PERSON, not per event | Per-event reminders — two people on one meeting want different warnings, and the fix needs a person column anyway | This session, 2026-09-02 | |
| | 6 | Delivery is at-least-once; every ambiguous case resolves towards sending | At-most-once — a reminder that never arrives is worse than one that arrives twice | This session, 2026-09-02 | |
| | 7 | Local sign-in is a magic link, with no passkey second factor | Passkeys — `passkey.Config` derives its relying party from this instance's host, so one person in three team instances enrols three credentials. Same ruling docs made | This session, 2026-09-02 | |
| +| 7a | Local sign-in uses Keymail for an address whose domain runs Keymail, and the magic link for every other address — chosen by the domain alone, never by the allowlist (an oracle otherwise). The Keymail hop is a continuation page, never a 303 out of the form POST (Safari applies form-action to it) | Forcing every address to a magic link | Paul, 2026-10-06 ("I do want it to use keymail") | |
| | 8 | A new event defaults to the person's PERSONAL calendar | Defaulting to the team calendar — the cost of forgetting to change a dropdown should fall on the side of privacy | This session, 2026-09-02 | |
| | H1 | **Optional Home sign-in connects to an existing person after fresh local proof and explicit confirmation; local sign-in remains available** | Treating the Home subject as an email or allocating a new person | User-approved suite connection, 2026-09-10 | |
| | B1 | A booking is confirmed by a BUTTON on the page the mailed link opens — a same-origin POST — never on submit and never on GET | Instant confirmation; confirming on the GET, which every link-scanning mail gateway would press | Booking spec §1, 2026-09-07 | |
diff --git a/internal/calendar/app.go b/internal/calendar/app.go| index 728b30b..0b44f6d 100644 |
| --- a/internal/calendar/app.go |
| +++ b/internal/calendar/app.go |
| @@ -40,6 +40,8 @@ type App struct { |
| // mailRecorder is non-nil only in development — see buildMailer. |
| mailRecorder *recordingMailer |
| + // signinMail is the queue sign-in links go through (signin_mail.go). |
| + signinMail *mailQueue |
| // auth is held so the harness can mint a session the same way a |
| // real sign-in does, rather than by writing a session row by hand. |
| @@ -140,8 +142,14 @@ func New(cfg Config) (*App, error) { |
| // reminders, booking mail. The recorder stays inside it, so tests |
| // still read what was sent (booking spec §6). |
| mailer = newBoundedSender(mailer) |
| - authPlugin, err := newAuthPlugin(origin, writer, instanceKey, mailer, list, logger, homeSettings != nil) |
| + // Sign-in links alone go through a queue: sent after the response, |
| + // to permitted addresses only (signin_mail.go). Closed by Close. |
| + // Refusals are counted in the log without the address (AGENTS.md |
| + // §Logs) — by the queue's worker, through its non-blocking logf. |
| + signinMail := newMailQueueLogging(mailer, list.allows, nil, logger, "calendar: signin refused by allowlist") |
| + authPlugin, err := newAuthPlugin(origin, writer, instanceKey, signinMail, list, logger, homeSettings != nil) |
| if err != nil { |
| + signinMail.close() |
| d.Close() |
| return nil, fmt.Errorf("calendar: wire sign-in: %w", err) |
| } |
| @@ -153,6 +161,7 @@ func New(cfg Config) (*App, error) { |
| logger: logger, |
| store: store, |
| mailRecorder: recorder, |
| + signinMail: signinMail, |
| auth: authPlugin, |
| } |
| h := &handlers{store: store, logger: logger, origin: origin, mailer: mailer, gate: newGate(), viewerFrom: authPlugin.SessionFrom} |
| @@ -160,6 +169,7 @@ func New(cfg Config) (*App, error) { |
| if homeSettings != nil { |
| a.home, err = a.newHome(homeSettings, list) |
| if err != nil { |
| + signinMail.close() |
| d.Close() |
| return nil, err |
| } |
| @@ -185,6 +195,7 @@ func New(cfg Config) (*App, error) { |
| // Native wiring comes before the scheduler starts, so a native |
| // failure leaves no timer goroutine behind to leak. |
| if err := a.wireNative(); err != nil { |
| + signinMail.close() |
| d.Close() |
| return nil, err |
| } |
| @@ -192,6 +203,7 @@ func New(cfg Config) (*App, error) { |
| logger.Error("calendar: scheduled work registration failed", "stage", "schedule.register", "err", scrubErr(err)) |
| }) |
| if err := a.scheduled.start(); err != nil { |
| + signinMail.close() |
| d.Close() |
| return nil, err |
| } |
| @@ -215,6 +227,9 @@ func (a *App) Handler() *http.ServeMux { return a.mux } |
| // Close releases the database. Safe to call on a partially built app. |
| func (a *App) Close() error { |
| a.closeOnce.Do(func() { |
| + if a.signinMail != nil { |
| + a.signinMail.close() |
| + } |
| if a.scheduled != nil { |
| a.scheduled.close() |
| } |
diff --git a/internal/calendar/auth.go b/internal/calendar/auth.go| index baf95fd..3b73235 100644 |
| --- a/internal/calendar/auth.go |
| +++ b/internal/calendar/auth.go |
| @@ -1,5 +1,7 @@ |
| -// Local verification uses an emailed single-use link. Optional Home sign-in |
| -// resolves an explicitly connected existing account; it never creates a person. |
| +// Local sign-in is Keymail for an address whose domain runs Keymail and an |
| +// emailed single-use link for every other address, decided by the domain |
| +// alone (beginSignin). Optional Home sign-in resolves an explicitly |
| +// connected existing account; it never creates a person. |
| package calendar |
| import ( |
| @@ -9,12 +11,10 @@ import ( |
| "database/sql" |
| "fmt" |
| "log/slog" |
| - "net/http" |
| "net/url" |
| "regexp" |
| "strings" |
| "sync" |
| - "sync/atomic" |
| rauth "amadan.net/rastrillo/rastrillo/auth" |
| "amadan.net/rastrillo/rastrillo/mail" |
| @@ -182,78 +182,16 @@ func resolveAllowlist(cfg Config) (allowlist, error) { |
| return allowlist{}, fmt.Errorf("calendar: $CALENDAR_ALLOWED_ADDRESSES is empty and Env is %q, not %q — refusing to start with no allowlist configured; set it to this instance's address(es)", cfg.Env, "development") |
| } |
| -// gateBegin wraps rauth.Auth.Begin so a non-permitted address never |
| -// reaches it. Begin is what sends the mail, while rauth.Config.Authorize |
| -// is checked afterwards, at the callback, once a link has already been |
| -// mailed and clicked — so gating only Authorize would leave this app's |
| -// mail-sending identity an open relay for any address a stranger types |
| -// into the form. |
| -// |
| -// A non-permitted address gets exactly the response Begin's own success |
| -// path sends: the same 303, to the same signinPath+"?sent=1", built the |
| -// same way. That is deliberate. Any observable difference — a different |
| -// status, a different body, even a differently-cased query string — |
| -// would turn this endpoint into a membership oracle, letting an |
| -// attacker learn which addresses this instance admits by watching how |
| -// its response to each one differs. |
| -// |
| -// force=1 is set unconditionally on every permitted request. Begin |
| -// otherwise auto-upgrades an address that resolves to a claimed keymail |
| -// inbox to the keymail ceremony, which redirects somewhere else |
| -// entirely — reopening the same oracle one level narrower, since an |
| -// attacker who suspects an address is keymail-hosted could tell "on the |
| -// allowlist" from "not" by which way the response went. This app never |
| -// wants that ceremony anyway (see the package comment), so forcing the |
| -// plain-link path here makes it a guarantee for every request this |
| -// handler forwards rather than only for the ones built by our own form. |
| -// The refusal is COUNTED IN THE LOG, without the address. |
| -// |
| -// The response has to stay silent; the log does not, and the two rules |
| -// that made this route undiagnosable were pulling in the same |
| -// direction only by accident. The oracle defence forbids telling the |
| -// CLIENT anything, and the logs rule (AGENTS.md §Logs) forbids writing |
| -// an ADDRESS anywhere. Neither forbids recording that a refusal |
| -// happened. Without this line a correctly-configured instance and one |
| -// whose allowlist is missing the only address anybody types are |
| -// indistinguishable from outside AND from `carlos logs`: the operator's |
| -// single remaining move is to read the environment variable and compare |
| -// it by eye, which is how this was actually found after somebody sat |
| -// waiting for a link that was never going to arrive. |
| -// |
| -// The count is the whole payload. "signin refused" with a running total |
| -// is enough to answer "is this instance refusing everything?", which is |
| -// the only question the log needs to answer, and it names nobody. |
| -func gateBegin(next http.HandlerFunc, list allowlist, signinPath string, logger *slog.Logger, refusals *atomic.Int64) http.HandlerFunc { |
| - return func(w http.ResponseWriter, r *http.Request) { |
| - // A malformed body reads back as address == "" below, which is |
| - // never on a non-empty allowlist — the same answer Begin would |
| - // itself reach, just reached before any mail could go out. |
| - _ = r.ParseForm() |
| - if list.allows(r.FormValue("address")) { |
| - if r.Form == nil { |
| - r.Form = url.Values{} |
| - } |
| - r.Form.Set("force", "1") |
| - next(w, r) |
| - return |
| - } |
| - if logger != nil { |
| - logger.Warn("calendar: signin refused by allowlist", |
| - "stage", "signin.gate", "refusals_since_boot", refusals.Add(1)) |
| - } |
| - http.Redirect(w, r, signinPath+"?sent=1", http.StatusSeeOther) |
| - } |
| -} |
| - |
| // newAuthPlugin builds the one *rauth.Auth this process shares. One per |
| // process, never one per request: the underlying flow carries its |
| // rate-limiter budget on the value, so a plugin per request would get a |
| // fresh empty budget every time — no rate limiting at all, silently. |
| // |
| -// Authorize is list.allows as well as gateBegin's gate. The two defend |
| -// different things: gateBegin stops the mail going out, Authorize stops |
| -// a link that was somehow minted for a non-permitted address (mailed |
| -// before the allowlist changed, say) from minting a session. |
| +// Authorize is list.allows as well as mailQueue. The two defend |
| +// different things: mailQueue stops the mail going out, Authorize |
| +// stops a session being minted for a non-permitted address, whether |
| +// Keymail verified it or a link was somehow minted for it (mailed |
| +// before the allowlist changed, say). |
| func newAuthPlugin(origin string, writer *sql.DB, instanceKey string, mailer mail.Sender, list allowlist, logger *slog.Logger, homeEnabled bool) (*rauth.Auth, error) { |
| signin, signedIn := "/signin", "/" |
| if homeEnabled { |
diff --git a/internal/calendar/auth_keymail.go b/internal/calendar/auth_keymail.go| new file mode 100644 |
| index 0000000..334261a |
| --- /dev/null |
| +++ b/internal/calendar/auth_keymail.go |
| @@ -0,0 +1,139 @@ |
| +package calendar |
| + |
| +import ( |
| + "bytes" |
| + "context" |
| + "net/http" |
| + "strings" |
| + |
| + "amadan.net/oficina/home/connect" |
| +) |
| + |
| +// beginSignin is POST /signin, in both modes: rauth's Begin for EVERY |
| +// address, under connect.OAuthContinuation. What the form answers |
| +// depends on the address's domain alone, never on the allowlist — any |
| +// difference between a permitted and a non-permitted address would |
| +// turn this endpoint into a membership oracle. |
| +// |
| +// - A Keymail address (keymail.dev itself, or a domain delegating to |
| +// Keymail with a `_keymail` TXT record), permitted or not, goes to |
| +// Keymail. Begin answers with a 303 to Keymail's authorize URL, |
| +// which OAuthContinuation turns into a page (renderKeymail): CSP's |
| +// form-action 'self' covers a form submission's whole redirect |
| +// chain in Safari, so a 303 out of the POST would be refused. |
| +// Admission is asked only after Keymail has verified the address, |
| +// by rauth's Authorize, and its refusal is the door saying so |
| +// (doorOnRefusal) — told only to someone who has just proved they |
| +// hold the address. |
| +// - Any other address is answered ?sent=1, and mailQueue mails the |
| +// link only if the allowlist holds it — after the response, in a |
| +// worker, so not even the response time differs (signin_mail.go) — |
| +// and this app's sending identity is no open relay for whatever a |
| +// stranger types. |
| +// |
| +// Both kinds of address spend the same rate budgets and parse the same |
| +// way, and a failed send is logged after the response, not answered. |
| +// |
| +// It used to gate the allowlist in front of Begin and set force=1 on |
| +// every permitted request, which kept Keymail out of Calendar |
| +// altogether. Paul ruled on 2026-10-06 that Calendar uses Keymail. |
| +// force now arrives only from the form rastrillo lands a failed |
| +// Keymail exchange on (?force=1&err=keymail), and asks for an emailed |
| +// link instead, so nobody is stuck. |
| +func beginSignin(origin string, begin http.HandlerFunc, render func(http.ResponseWriter, *http.Request, string)) http.HandlerFunc { |
| + return connect.OAuthContinuation(origin, begin, render) |
| +} |
| + |
| +// authRefusal is the bare-text body rastrillo/auth's admit() writes, |
| +// with a 403, when a verified address is not admitted. rastrillo at the |
| +// version pinned here has no hook to render anything else, so |
| +// doorOnRefusal recognises this exact answer; TestKeymailRefusalIsTheDoor |
| +// pins it, so a bump that changes the sentence fails loudly. |
| +const authRefusal = "This address is verified but not admitted here." |
| + |
| +// doorOnRefusal wraps rauth's Callback and Verify, the handlers that end |
| +// in admission, and turns its bare-text refusal into the door, rendered |
| +// in the refusal's own 403 by door (the sign-in page) with the refusal |
| +// marked on the request context (refusedHere). Every other answer, |
| +// cookies included, passes through untouched. |
| +// |
| +// Rendered, not redirected to ?err=refused: a query parameter is |
| +// something anybody can type, and the sentence means "you proved this |
| +// address and it is not admitted" (Astra, round 3 on #28). Remove it |
| +// when rastrillo has 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) { |
| + rec := &bufferedResponse{header: make(http.Header)} |
| + next(rec, r) |
| + if rec.status == http.StatusForbidden && strings.TrimSpace(rec.body.String()) == authRefusal { |
| + for _, c := range rec.header.Values("Set-Cookie") { |
| + w.Header().Add("Set-Cookie", c) |
| + } |
| + w.Header().Set("Cache-Control", "no-store") |
| + door(&forcedStatus{ResponseWriter: w, status: http.StatusForbidden}, |
| + r.WithContext(context.WithValue(r.Context(), refusedKey{}, true))) |
| + return |
| + } |
| + for name, values := range rec.header { |
| + for _, v := range values { |
| + w.Header().Add(name, v) |
| + } |
| + } |
| + if rec.status == 0 { |
| + rec.status = http.StatusOK |
| + } |
| + w.WriteHeader(rec.status) |
| + _, _ = w.Write(rec.body.Bytes()) |
| + } |
| +} |
| + |
| +// 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) |
| +} |
diff --git a/internal/calendar/home_router_test.go b/internal/calendar/home_router_test.go| index acf56b9..6ba2bd4 100644 |
| --- a/internal/calendar/home_router_test.go |
| +++ b/internal/calendar/home_router_test.go |
| @@ -76,6 +76,7 @@ func homeRedirect(t *testing.T, w *httptest.ResponseRecorder) string { |
| func (f *homeRouterFixture) local(t *testing.T) { |
| t.Helper() |
| homeRedirect(t, f.do("POST", "/signin", "address=alice%40example.test&force=1")) |
| + f.app.signinMail.wait() |
| link := regexp.MustCompile(`https://[^\s<>\"]+/auth/verify\?[^\s<>\"]+`).FindString(f.app.mailRecorder.last()) |
| u, err := url.Parse(link) |
| if err != nil || link == "" { |
diff --git a/internal/calendar/keymail_signin_test.go b/internal/calendar/keymail_signin_test.go| new file mode 100644 |
| index 0000000..64241e0 |
| --- /dev/null |
| +++ b/internal/calendar/keymail_signin_test.go |
| @@ -0,0 +1,273 @@ |
| +package calendar |
| + |
| +import ( |
| + "bytes" |
| + "context" |
| + "encoding/base64" |
| + "encoding/json" |
| + "fmt" |
| + "html" |
| + "io" |
| + "log/slog" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "os" |
| + "path/filepath" |
| + "regexp" |
| + "strings" |
| + "sync" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/oficina/calendar/internal/calendar/keymailtest" |
| + "amadan.net/oficina/home/connect" |
| + assertioncrypto "amadan.net/rastrillo/rastrillo/crypto" |
| +) |
| + |
| +// TestMain keeps Keymail classification inside the process: 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 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) |
| +} |
| + |
| +const ( |
| + keymailHost = "keymail.calendar.test" |
| + keymailMember = "paul@" + keymailHost |
| + keymailStranger = "stranger@" + keymailHost |
| + plainMember = "paul@example.test" |
| +) |
| + |
| +// countingMailer remembers every address it was asked to write to. |
| +type countingMailer struct { |
| + mu sync.Mutex |
| + to []string |
| + flush func() // waits for the sign-in mail queue (signin_mail.go) |
| +} |
| + |
| +func (m *countingMailer) Send(_ context.Context, to, _, _ string) error { |
| + m.mu.Lock() |
| + defer m.mu.Unlock() |
| + m.to = append(m.to, to) |
| + return nil |
| +} |
| + |
| +func (m *countingMailer) sent() []string { |
| + if m.flush != nil { |
| + m.flush() |
| + } |
| + m.mu.Lock() |
| + defer m.mu.Unlock() |
| + return append([]string(nil), m.to...) |
| +} |
| + |
| +type keymailFixture struct { |
| + app *App |
| + origin string |
| + mail *countingMailer |
| + km *keymailtest.Server |
| +} |
| + |
| +// newKeymailFixture builds an app admitting plainMember and |
| +// keymailMember, beside a Keymail deployment holding keymailMember and |
| +// keymailStranger. home configures Home, which moves the local form to |
| +// /signin/local. |
| +func newKeymailFixture(t *testing.T, home bool) *keymailFixture { |
| + t.Helper() |
| + km := keymailtest.Serve(t, keymailHost, keymailMember, keymailStranger) |
| + mail := &countingMailer{} |
| + cfg := Config{DBPath: filepath.Join(t.TempDir(), "calendar.db"), Env: "development", Origin: TestOrigin, |
| + AllowedAddresses: plainMember + "," + keymailMember, Mailer: mail, |
| + Logger: slog.New(slog.NewTextHandler(io.Discard, nil))} |
| + if home { |
| + key, err := assertioncrypto.Generate() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + manifest, _ := json.Marshal(map[string]string{"key": base64.StdEncoding.EncodeToString(key.SignPub())}) |
| + cfg.Origin = homeTestOrigin |
| + cfg.Home = &connect.Credentials{Issuer: homeTestIssuer, ConsumerID: "calendar-test", KeysBase64: base64.StdEncoding.EncodeToString(manifest), DEKBase64: base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{7}, 32))} |
| + } |
| + a, err := New(cfg) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + mail.flush = a.signinMail.wait |
| + return &keymailFixture{app: a, origin: cfg.Origin, mail: mail, km: km} |
| +} |
| + |
| +func (f *keymailFixture) post(form url.Values, cookies ...*http.Cookie) *httptest.ResponseRecorder { |
| + r := httptest.NewRequest(http.MethodPost, f.origin+"/signin", strings.NewReader(form.Encode())) |
| + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + r.Header.Set("Origin", f.origin) |
| + for _, c := range cookies { |
| + r.AddCookie(c) |
| + } |
| + w := httptest.NewRecorder() |
| + f.app.mux.ServeHTTP(w, r) |
| + return w |
| +} |
| + |
| +func (f *keymailFixture) get(path string, cookies ...*http.Cookie) *httptest.ResponseRecorder { |
| + r := httptest.NewRequest(http.MethodGet, f.origin+path, nil) |
| + for _, c := range cookies { |
| + r.AddCookie(c) |
| + } |
| + w := httptest.NewRecorder() |
| + f.app.mux.ServeHTTP(w, r) |
| + return w |
| +} |
| + |
| +var authorizeHref = regexp.MustCompile(`href="(https://` + regexp.QuoteMeta(keymailHost) + `/oauth/authorize\?[^"]+)"`) |
| + |
| +// continuation asserts rec is the page between the form and Keymail — |
| +// a 200 document with a meta refresh, never a 303 out of the form POST |
| +// (Safari applies form-action to that redirect) — and returns the |
| +// authorize URL it carries. |
| +func continuation(t *testing.T, label string, rec *httptest.ResponseRecorder) *url.URL { |
| + t.Helper() |
| + if rec.Code != http.StatusOK || rec.Header().Get("Location") != "" { |
| + t.Fatalf("%s: %d %q, want 200 and the continuation page to Keymail", label, rec.Code, rec.Header().Get("Location")) |
| + } |
| + body := rec.Body.String() |
| + m := authorizeHref.FindStringSubmatch(body) |
| + if m == nil || !strings.Contains(body, `http-equiv="refresh"`) { |
| + t.Fatalf("%s: no Keymail continuation on the page:\n%s", label, body) |
| + } |
| + u, err := url.Parse(html.UnescapeString(m[1])) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return u |
| +} |
| + |
| +// TestKeymailAddressGoesToKeymailWhetherPermittedOrNot: in both modes, |
| +// whether an address goes to Keymail depends on its domain alone. A |
| +// permitted and a non-permitted Keymail address get the same first |
| +// answer, the continuation page, and neither is mailed. Before, every |
| +// permitted address was forced to an emailed link and a non-permitted |
| +// one was answered ?sent=1. |
| +func TestKeymailAddressGoesToKeymailWhetherPermittedOrNot(t *testing.T) { |
| + for _, home := range []bool{false, true} { |
| + t.Run(fmt.Sprintf("home=%v", home), func(t *testing.T) { |
| + f := newKeymailFixture(t, home) |
| + member := f.post(url.Values{"address": {keymailMember}}) |
| + stranger := f.post(url.Values{"address": {keymailStranger}}) |
| + mu := continuation(t, "permitted Keymail address", member) |
| + su := continuation(t, "non-permitted Keymail address", stranger) |
| + mask := func(rec *httptest.ResponseRecorder, u *url.URL) string { |
| + return strings.ReplaceAll(rec.Body.String(), html.EscapeString(u.String()), "AUTHORIZE") |
| + } |
| + if a, b := mask(member, mu), mask(stranger, su); a != b { |
| + t.Errorf("the two Keymail answers differ beyond the authorize URL:\n%s\n----\n%s", a, b) |
| + } |
| + if got := f.mail.sent(); len(got) != 0 { |
| + t.Errorf("a Keymail address was mailed: %v", got) |
| + } |
| + if n := f.km.Lookups(); n != 2 { |
| + t.Errorf("Keymail saw %d lookups, want 2", n) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestNonPermittedPlainAddressIsAnsweredSentAndNotMailed: an address |
| +// with no Keymail gets ?sent=1, permitted or not; only the permitted |
| +// one is mailed. |
| +func TestNonPermittedPlainAddressIsAnsweredSentAndNotMailed(t *testing.T) { |
| + f := newKeymailFixture(t, false) |
| + for _, address := range []string{"someone@else.test", plainMember} { |
| + rec := f.post(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")) |
| + } |
| + } |
| + if got := f.mail.sent(); len(got) != 1 || got[0] != plainMember { |
| + t.Fatalf("mail = %v, want one, to %s", got, plainMember) |
| + } |
| +} |
| + |
| +// TestKeymailFailureFallsBackToAnEmailLink: the page rastrillo lands a |
| +// failed Keymail exchange on (?force=1&err=keymail) carries force, so |
| +// the retry mails a link instead of returning to Keymail; the ordinary |
| +// form carries none. |
| +func TestKeymailFailureFallsBackToAnEmailLink(t *testing.T) { |
| + f := newKeymailFixture(t, false) |
| + forced := `name="force" value="1"` |
| + if strings.Contains(f.get("/signin").Body.String(), forced) { |
| + t.Fatal("the ordinary form forces an email link") |
| + } |
| + if !strings.Contains(f.get("/signin?force=1&err=keymail").Body.String(), forced) { |
| + t.Fatal("the err=keymail form does not carry force; a failed Keymail sign-in would loop back to Keymail") |
| + } |
| + rec := f.post(url.Values{"address": {keymailMember}, "force": {"1"}}) |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Fatalf("forced POST: %d %q", rec.Code, rec.Header().Get("Location")) |
| + } |
| + if got := f.mail.sent(); len(got) != 1 || got[0] != keymailMember { |
| + t.Fatalf("mail = %v, want one link to %s", got, keymailMember) |
| + } |
| + if f.km.Lookups() != 0 { |
| + t.Error("a forced sign-in classified the address") |
| + } |
| +} |
| + |
| +// TestKeymailRefusalIsTheDoor: a non-permitted address Keymail verified |
| +// is refused after verification with the door, rendered in the |
| +// refusal's own 403 with the form for a different address — not |
| +// rastrillo's bare text, and not a redirect to a query anybody could |
| +// type. An admitted one is signed in. Both modes. |
| +func TestKeymailRefusalIsTheDoor(t *testing.T) { |
| + for _, home := range []bool{false, true} { |
| + t.Run(fmt.Sprintf("home=%v", home), func(t *testing.T) { |
| + f := newKeymailFixture(t, home) |
| + form := "/signin" |
| + if home { |
| + form = "/signin/local" |
| + } |
| + roundTrip := func(address string) *httptest.ResponseRecorder { |
| + begin := f.post(url.Values{"address": {address}}) |
| + u := continuation(t, address, begin) |
| + return f.get("/auth/callback?"+url.Values{"state": {u.Query().Get("state")}, "code": {f.km.Approve(address)}}.Encode(), begin.Result().Cookies()...) |
| + } |
| + if cb := roundTrip(keymailMember); cb.Code != http.StatusSeeOther || strings.Contains(cb.Header().Get("Location"), "err=") { |
| + t.Fatalf("permitted callback: %d %q", cb.Code, cb.Header().Get("Location")) |
| + } |
| + cb := roundTrip(keymailStranger) |
| + if cb.Code != http.StatusForbidden || cb.Header().Get("Location") != "" { |
| + t.Fatalf("non-permitted callback: %d %q, want 403 and the door itself (bare text is the bug)\n%s", cb.Code, cb.Header().Get("Location"), cb.Body.String()) |
| + } |
| + door := cb.Body.String() |
| + for _, want := range []string{"estilo-door", `name="address"`, "can’t sign in to Calendar"} { |
| + if !strings.Contains(door, want) { |
| + t.Errorf("refusal door lacks %q:\n%s", want, door) |
| + } |
| + } |
| + // Nobody may summon the sentence by typing a URL (Astra, round 3). |
| + if spoof := f.get(form + "?err=refused").Body.String(); strings.Contains(spoof, "can’t sign in to Calendar") { |
| + t.Fatalf("anybody can show the refusal by visiting %s?err=refused", form) |
| + } |
| + }) |
| + } |
| +} |
diff --git a/internal/calendar/keymailtest/keymailtest.go b/internal/calendar/keymailtest/keymailtest.go| new file mode 100644 |
| index 0000000..92ad056 |
| --- /dev/null |
| +++ b/internal/calendar/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/calendar/mail_bounded.go b/internal/calendar/mail_bounded.go| index b2930eb..817e226 100644 |
| --- a/internal/calendar/mail_bounded.go |
| +++ b/internal/calendar/mail_bounded.go |
| @@ -82,6 +82,15 @@ func (b *boundedSender) Send(ctx context.Context, to, subject, body string) erro |
| // however long after this call gave up on it. That is the |
| // bound on abandoned sends: at most maxSendsInFlight of them. |
| defer func() { <-b.slots }() |
| + // A panicking sender is this send's failure, not the process's: |
| + // this goroutine is outside every caller's recover (Astra's |
| + // production-composition probe on #28 took the process down |
| + // through it). done is buffered, so this never blocks. |
| + defer func() { |
| + if p := recover(); p != nil { |
| + done <- ErrMailRefused |
| + } |
| + }() |
| done <- b.next.Send(ctx, to, subject, body) |
| }() |
diff --git a/internal/calendar/mail_bounded_panic_test.go b/internal/calendar/mail_bounded_panic_test.go| new file mode 100644 |
| index 0000000..a50f83d |
| --- /dev/null |
| +++ b/internal/calendar/mail_bounded_panic_test.go |
| @@ -0,0 +1,61 @@ |
| +package calendar |
| + |
| +import ( |
| + "context" |
| + "io" |
| + "log/slog" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "path/filepath" |
| + "strings" |
| + "sync" |
| + "testing" |
| +) |
| + |
| +// panicOnceMailer panics on its first send and records the rest. |
| +type panicOnceMailer struct { |
| + mu sync.Mutex |
| + calls int |
| + to []string |
| +} |
| + |
| +func (m *panicOnceMailer) Send(_ context.Context, to, _, _ string) error { |
| + m.mu.Lock() |
| + defer m.mu.Unlock() |
| + m.calls++ |
| + if m.calls == 1 { |
| + panic("relay exploded") |
| + } |
| + m.to = append(m.to, to) |
| + return nil |
| +} |
| + |
| +// TestAPanickingMailerDoesNotStopSigninMail is Astra's production- |
| +// composition probe (round 3 on #28): the sign-in queue's worker hands |
| +// each link to the bounded sender, which runs the real send on a |
| +// goroutine of its own — outside the worker's recover. A mailer that |
| +// panicked there took the whole process down. Built exactly as New |
| +// builds it, the first link's panic must be contained and the next |
| +// sign-in's link delivered. |
| +func TestAPanickingMailerDoesNotStopSigninMail(t *testing.T) { |
| + m := &panicOnceMailer{} |
| + a, err := New(Config{DBPath: filepath.Join(t.TempDir(), "calendar.db"), Env: "development", Origin: TestOrigin, |
| + AllowedAddresses: "one@example.test,two@example.test", Mailer: m, Logger: slog.New(slog.NewTextHandler(io.Discard, nil))}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + for _, address := range []string{"one@example.test", "two@example.test"} { |
| + r := httptest.NewRequest(http.MethodPost, TestOrigin+"/signin", strings.NewReader(url.Values{"address": {address}}.Encode())) |
| + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + r.Header.Set("Origin", TestOrigin) |
| + a.mux.ServeHTTP(httptest.NewRecorder(), r) |
| + a.FlushSigninMailForTest() |
| + } |
| + m.mu.Lock() |
| + defer m.mu.Unlock() |
| + if len(m.to) != 1 || m.to[0] != "two@example.test" { |
| + t.Fatalf("delivered %v, want the second link after the first send panicked", m.to) |
| + } |
| +} |
diff --git a/internal/calendar/render.go b/internal/calendar/render.go| index 5af6b8a..f01e82a 100644 |
| --- a/internal/calendar/render.go |
| +++ b/internal/calendar/render.go |
| @@ -68,7 +68,9 @@ func init() { |
| t = template.Must(t.ParseFS(appFS, "templates/layout.html", "templates/_chip.html", |
| "templates/_quickadd.html", "templates/_quickpane.html", "templates/_bookingpane.html", |
| "templates/"+name+".html")) |
| - t = template.Must(t.ParseFS(connect.Templates(), "templates/home_link.html")) |
| + // home_link for the account pages; oauth for the Keymail |
| + // continuation's meta refresh (home-oauth-head) on signin. |
| + t = template.Must(t.ParseFS(connect.Templates(), "templates/home_link.html", "templates/oauth.html")) |
| pages[name] = template.Must(t.ParseFS(credits.Templates(), "templates/*.html")) |
| } |
| } |
diff --git a/internal/calendar/routes.go b/internal/calendar/routes.go| index 421decf..8058d10 100644 |
| --- a/internal/calendar/routes.go |
| +++ b/internal/calendar/routes.go |
| @@ -6,7 +6,6 @@ import ( |
| "amadan.net/oficina/home/connect" |
| "amadan.net/oficina/home/handoff" |
| "net/http" |
| - "sync/atomic" |
| "github.com/go-chi/chi/v5" |
| @@ -58,25 +57,18 @@ func (a *App) routes(h *handlers, authPlugin *rauth.Auth, list allowlist, origin |
| r.Get("/signin", h.signin) |
| r.Get("/signed-out", h.signedOut) |
| } |
| - // One counter for the life of the router, so the log line can say |
| - // how many refusals this boot has seen rather than only that one |
| - // happened. routes runs once per App, so the closure below is the |
| - // only holder and no reset can race it. |
| - var refusals atomic.Int64 |
| - signinPath := "/signin" |
| - if a.home != nil { |
| - signinPath = "/signin/local" |
| - } |
| - begin := gateBegin(authPlugin.Begin, list, signinPath, a.logger, &refusals) |
| + begin := beginSignin(origin, authPlugin.Begin, h.renderKeymail) |
| + callback := doorOnRefusal(authPlugin.Callback, h.signin) |
| + verify := doorOnRefusal(authPlugin.Verify, h.signin) |
| if a.home != nil { |
| r.Post("/signin", a.home.routes.LocalGuard(begin)) |
| - r.Get("/auth/callback", a.home.routes.LocalGuard(authPlugin.Callback)) |
| - r.Get("/auth/verify", a.home.routes.LocalGuard(authPlugin.Verify)) |
| + r.Get("/auth/callback", a.home.routes.LocalGuard(callback)) |
| + r.Get("/auth/verify", a.home.routes.LocalGuard(verify)) |
| r.Post("/signout", a.home.routes.Signout) |
| } else { |
| r.Post("/signin", begin) |
| - r.Get("/auth/callback", authPlugin.Callback) |
| - r.Get("/auth/verify", authPlugin.Verify) |
| + r.Get("/auth/callback", callback) |
| + r.Get("/auth/verify", verify) |
| r.Post("/signout", signOutToSignedOut(authPlugin.Signout)) |
| } |
diff --git a/internal/calendar/signin.go b/internal/calendar/signin.go| index 5bee915..5665274 100644 |
| --- a/internal/calendar/signin.go |
| +++ b/internal/calendar/signin.go |
| @@ -5,6 +5,7 @@ import ( |
| "amadan.net/oficina/estilo.git/chrome" |
| "amadan.net/oficina/estilo.git/sky" |
| + "amadan.net/oficina/home/connect" |
| ) |
| // The sign-in pages are the Oficina door (estilo chrome): one card on the |
| @@ -37,6 +38,12 @@ func (h *handlers) signin(w http.ResponseWriter, r *http.Request) { |
| Action: "/signin", |
| OpenSource: &chrome.Link{Href: "/help/open-source", Label: "Open source"}, |
| } |
| + 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 |
| + // retry does not return to the Keymail that just failed. |
| + form.Hidden = []chrome.Hidden{{Name: "force", Value: "1"}} |
| + } |
| switch q.Get("err") { |
| case "": |
| case "rate": |
| @@ -45,9 +52,17 @@ func (h *handlers) signin(w http.ResponseWriter, r *http.Request) { |
| form.AddressError = "That does not look like an email address." |
| case "expired": |
| form.Error = "That link has expired. Ask for a new one." |
| + case "keymail": |
| + form.Error = "Your Keymail sign-in couldn’t be completed. Enter your email to get a link instead." |
| default: |
| form.Error = "That link did not work. Ask for a new one." |
| } |
| + if refusedHere(r) { |
| + // Keymail or a link verified the address and the allowlist does |
| + // not hold it, in this very response (doorOnRefusal). Never from |
| + // the query. The form is the way on. |
| + form.Error = "That address can’t sign in to Calendar. Use a different address, or ask whoever runs this team to add it." |
| + } |
| sent := q.Get("sent") == "1" |
| failed := form.Error != "" || form.AddressError != "" |
| switch { |
| @@ -67,11 +82,28 @@ func (h *handlers) signin(w http.ResponseWriter, r *http.Request) { |
| } |
| // signinData is the sign-in page's own view data: exactly one of the |
| -// three door cards. |
| +// door cards. Continue is set beside Sent on the page between the form |
| +// and Keymail (renderKeymail): its meta refresh goes on to Keymail. |
| type signinData struct { |
| Form *chrome.SignIn |
| Sent *chrome.SignInNotice |
| SignedOut *chrome.SignInNotice |
| + Continue *connect.OAuthData |
| +} |
| + |
| +// renderKeymail is the page between the form and Keymail, in both modes |
| +// (beginSignin): the door's own card saying where the person is going, |
| +// 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 (h *handlers) renderKeymail(w http.ResponseWriter, r *http.Request, destination string) { |
| + p := connect.OAuthPage(destination) |
| + notice := chrome.SignInNotice{ |
| + Icon: assets.Path("static/brand/icon.svg"), App: "Calendar", Sky: sky.For(r.Host), |
| + Heading: p.Title, Body: "Taking you to Keymail to finish signing in.", |
| + Link: &chrome.Link{Href: p.URL, Label: p.ActionLabel}, |
| + } |
| + render(w, "signin", h.page(r, p.Title, signinData{Sent: ¬ice, Continue: &p})) |
| } |
| // signedOutDoor is where Home sign-out lands, drawn as the door: the |
diff --git a/internal/calendar/signin_mail.go b/internal/calendar/signin_mail.go| new file mode 100644 |
| index 0000000..e53fbf4 |
| --- /dev/null |
| +++ b/internal/calendar/signin_mail.go |
| @@ -0,0 +1,283 @@ |
| +package calendar |
| + |
| +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("calendar: 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("calendar: 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("calendar: 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("calendar: 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("calendar: 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("calendar: sign-in mail worker ignored cancellation; left running") |
| + } |
| +} |
diff --git a/internal/calendar/signin_mail_test.go b/internal/calendar/signin_mail_test.go| new file mode 100644 |
| index 0000000..6b78591 |
| --- /dev/null |
| +++ b/internal/calendar/signin_mail_test.go |
| @@ -0,0 +1,282 @@ |
| +package calendar |
| + |
| +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) |
| + } |
| +} |
| + |
| +// 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() |
| + } |
| + // Baselines, not zero: another test's queue may still be shutting |
| + // down beside this one. |
| + baseline, workers := runtime.NumGoroutine(), mailWorkers() |
| + 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 > workers+1 { |
| + t.Fatalf("%d mail queue goroutines with the sink stalled, %d before: want at most the log pump more", n, workers) |
| + } |
| + |
| + // Released, the pump finishes its write and exits. |
| + free() |
| + if !waitFor(func() bool { return mailWorkers() <= workers && runtime.NumGoroutine() <= baseline }) { |
| + t.Fatalf("after release: %d mail queue goroutines, %d goroutines against %d before", mailWorkers(), runtime.NumGoroutine(), baseline) |
| + } |
| +} |
| + |
| +// TestAppCloseStopsTheMailWorker: the app owns its queue, and Close |
| +// stops its worker, so no test that builds and closes an app leaves |
| +// one running. |
| +func TestAppCloseStopsTheMailWorker(t *testing.T) { |
| + a := NewForTest(t, t.TempDir()) |
| + q := a.signinMail |
| + 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") |
| + } |
| +} |
diff --git a/internal/calendar/signin_timing_test.go b/internal/calendar/signin_timing_test.go| new file mode 100644 |
| index 0000000..b7c4966 |
| --- /dev/null |
| +++ b/internal/calendar/signin_timing_test.go |
| @@ -0,0 +1,94 @@ |
| +package calendar |
| + |
| +import ( |
| + "context" |
| + "io" |
| + "log/slog" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "path/filepath" |
| + "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: when the send ran inside the |
| +// request, a permitted address waited for SMTP before its ?sent=1 and a |
| +// stranger did not (Astra measured 252 ms against 0.35 ms on Slides, |
| +// which had the same shape), so the allowlist was readable off a |
| +// stopwatch. With the mailer blocked, every POST — permitted, stranger, |
| +// and both with force=1 — must answer, and the permitted links must |
| +// then go out. |
| +func TestSigninNeverWaitsForTheMailer(t *testing.T) { |
| + bm := &blockingMailer{release: make(chan struct{})} |
| + a, err := New(Config{DBPath: filepath.Join(t.TempDir(), "calendar.db"), Env: "development", Origin: TestOrigin, |
| + AllowedAddresses: plainMember, Mailer: bm, Logger: slog.New(slog.NewTextHandler(io.Discard, nil))}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + var once sync.Once |
| + free := func() { once.Do(func() { close(bm.release) }) } |
| + t.Cleanup(free) // runs before Close, which drains the queue |
| + |
| + for _, form := range []url.Values{ |
| + {"address": {plainMember}}, |
| + {"address": {"someone@else.test"}}, |
| + {"address": {plainMember}, "force": {"1"}}, |
| + {"address": {"someone@else.test"}, "force": {"1"}}, |
| + } { |
| + done := make(chan *httptest.ResponseRecorder, 1) |
| + go func() { |
| + r := httptest.NewRequest(http.MethodPost, TestOrigin+"/signin", strings.NewReader(form.Encode())) |
| + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + r.Header.Set("Origin", TestOrigin) |
| + w := httptest.NewRecorder() |
| + a.mux.ServeHTTP(w, r) |
| + done <- w |
| + }() |
| + 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) |
| + if got := bm.sent(); len(got) != 2 || got[0] != plainMember || got[1] != plainMember { |
| + t.Fatalf("mail sent = %v, want two links, both to %s", got, plainMember) |
| + } |
| +} |
diff --git a/internal/calendar/templates/signin.html b/internal/calendar/templates/signin.html| index 0eac71b..6668e24 100644 |
| --- a/internal/calendar/templates/signin.html |
| +++ b/internal/calendar/templates/signin.html |
| @@ -1,8 +1,10 @@ |
| {{define "title"}}{{.Title}}{{end}} |
| {{/* The Oficina door (estilo chrome). Exactly one card: the form, the |
| - check-your-email notice, or where Home sign-out lands. The form posts |
| - its address as "address", the name gateBegin and the auth plugin |
| - read. */}} |
| + check-your-email notice, the card between the form and Keymail (with |
| + .Data.Continue's meta refresh in head), or where Home sign-out lands. |
| + The form posts its address as "address", the name the auth plugin |
| + reads. */}} |
| +{{define "head"}}{{with .Data.Continue}}{{template "home-oauth-head" .}}{{end}}{{end}} |
| {{define "content"}} |
| {{- with .Data.Form}}{{template "estilo-signin" .}}{{end}} |
| {{- with .Data.Sent}}{{template "estilo-signin-sent" .}}{{end}} |
diff --git a/internal/calendar/testing.go b/internal/calendar/testing.go| index 02f48ef..28bfd08 100644 |
| --- a/internal/calendar/testing.go |
| +++ b/internal/calendar/testing.go |
| @@ -75,6 +75,8 @@ func (a *App) SignInForTest(t TB, address string) *http.Cookie { |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| req.Header.Set("Origin", TestOrigin) |
| a.mux.ServeHTTP(httptest.NewRecorder(), req) |
| + // The link is sent after the response (signin_mail.go). |
| + a.signinMail.wait() |
| link := magicLinkPattern.FindString(a.mailRecorder.last()) |
| if link == "" { |
| @@ -162,6 +164,7 @@ func (a *App) LastMailForTest() (string, error) { |
| if a.mailRecorder == nil { |
| return "", fmt.Errorf("calendar: no mail recorder (Env is %q, not \"development\")", a.cfg.Env) |
| } |
| + a.signinMail.wait() |
| return a.mailRecorder.last(), nil |
| } |
| @@ -213,3 +216,10 @@ func (a *App) SetAfterSlotCheckForTest(f func()) { a.store.afterSlotCheckForTest |
| func (a *App) TrySendNowForTest(bookingID int64) bool { |
| return a.h.trySendNow(context.Background(), bookingID, time.Now()) |
| } |
| + |
| +// FlushSigninMailForTest waits until every sign-in link queued so far |
| +// has been sent or refused. Links go out after the response |
| +// (signin_mail.go), and so does the refusal's log line. |
| +func (a *App) FlushSigninMailForTest() { |
| + a.signinMail.wait() |
| +} |
diff --git a/internal/calendartest/delivery_test.go b/internal/calendartest/delivery_test.go| index 6053699..a98a5d1 100644 |
| --- a/internal/calendartest/delivery_test.go |
| +++ b/internal/calendartest/delivery_test.go |
| @@ -257,6 +257,7 @@ func TestARefusedSignInIsCountedInTheLog(t *testing.T) { |
| t.Cleanup(func() { app.Close() }) |
| post(t, app.Handler(), "/signin", url.Values{"address": {"stranger@example.com"}}) |
| + app.FlushSigninMailForTest() // refusals are logged after the response |
| out := log.String() |
| if !strings.Contains(out, "signin refused by allowlist") { |
diff --git a/internal/calendartest/main_test.go b/internal/calendartest/main_test.go| new file mode 100644 |
| index 0000000..97f7ee6 |
| --- /dev/null |
| +++ b/internal/calendartest/main_test.go |
| @@ -0,0 +1,66 @@ |
| +package calendartest |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "fmt" |
| + "net" |
| + "os" |
| + "runtime" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "amadan.net/oficina/calendar/internal/calendar/keymailtest" |
| +) |
| + |
| +// TestMain keeps Keymail classification inside the process: every POST |
| +// /signin now classifies its address, and without this each one would |
| +// ask the real network whether example.com runs Keymail. Every DNS |
| +// query is answered NXDOMAIN; no fixture is registered here, so every |
| +// address is 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 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) |
| +} |
| + |
| +// mailWorkers counts goroutines running a sign-in mail queue's worker |
| +// or log pump (internal/calendar/signin_mail.go). |
| +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(") |
| +} |
| + |
| +// TestKeymailDiscoveryStaysInProcess: every unforced POST /signin |
| +// classifies its address (rastrillo/auth's Keymail discovery), so this |
| +// package's tests must not ask the real network whether example.com runs |
| +// Keymail. With keymailtest installed every query is answered NXDOMAIN |
| +// in-process, so even example.com, which public DNS resolves, must not. |
| +func TestKeymailDiscoveryStaysInProcess(t *testing.T) { |
| + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) |
| + defer cancel() |
| + addrs, err := net.DefaultResolver.LookupHost(ctx, "example.com") |
| + var dnsErr *net.DNSError |
| + if !errors.As(err, &dnsErr) || !dnsErr.IsNotFound { |
| + t.Fatalf("LookupHost(example.com) = %v, %v; want NXDOMAIN from keymailtest's in-process resolver", addrs, err) |
| + } |
| +} |