amadan

oficina / sheets Public

Sheets signs in through Keymail, without revealing who is admitted

A Keymail address, permitted or not, gets the same continuation page to
Keymail instead of a 303 from the form POST, which Safari's form-action
blocked. Other addresses get ?sent=1. Links are mailed by a bounded,
non-blocking queue, and only to permitted addresses, so neither the
answer nor its timing reveals the allowlist. A refused address sees the
door inside the 403. Reviewed by GPT-6-Astra via Codex (#45).

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

Squashed from fix/keymail-signin into main.
Paul Campbell pushed by claude 0dbb0858a2d944008dab2ec68df36a4df8f207f1 parent 686355f
16 files changed, +1339 −63
  • cmd/sheets/main.go +5 −1
  • internal/sheets/app.go +3 −3
  • internal/sheets/auth.go +18 −4
  • internal/sheets/begin.go +135 −39
  • internal/sheets/begin_test.go +28 −1
  • internal/sheets/home.go +3 −6
  • internal/sheets/home_routes.go +6 −6
  • internal/sheets/home_test.go +1 −0
  • internal/sheets/keymail_signin_test.go +226 −0
  • internal/sheets/keymailtest/keymailtest.go +190 −0
  • internal/sheets/mailqueue.go +283 −0
  • internal/sheets/mailqueue_registry.go +43 −0
  • internal/sheets/mailqueue_test.go +286 −0
  • internal/sheets/signin.go +10 −3
  • internal/sheets/signin_test.go +1 −0
  • internal/sheets/signin_timing_test.go +101 −0

CI passed — run details

CI log 
amadan: .amadan/timeout: this job's budget is 15m0s (this runner allows up to 30m0s)
=== vet ===
CGO_ENABLED=0 go vet ./...
CGO_ENABLED=0 go vet -tags browser ./internal/sheets/
=== fmt ===
=== build ===
CGO_ENABLED=0 go build ./...
=== migrations ===
go tool rastrillo migration check
rastrillo migration check: models and migrations agree
=== go-test ===
CGO_ENABLED=0 go test -count=1 ./...
?   	amadan.net/oficina/sheets/cmd/sheets	[no test files]
ok  	amadan.net/oficina/sheets/internal/guardstest	0.119s
ok  	amadan.net/oficina/sheets/internal/sheets	6.181s
ok  	amadan.net/oficina/sheets/internal/sheets/calc	0.010s
ok  	amadan.net/oficina/sheets/internal/sheets/format	0.003s
?   	amadan.net/oficina/sheets/internal/sheets/keymailtest	[no test files]
ok  	amadan.net/oficina/sheets/internal/sheets/live	0.053s
ok  	amadan.net/oficina/sheets/internal/sheets/seal	0.003s
ok  	amadan.net/oficina/sheets/internal/sheets/sealtest	0.003s
ok  	amadan.net/oficina/sheets/internal/sheets/store	5.492s
ok  	amadan.net/oficina/sheets/internal/sheets/xlsx	0.510s
?   	amadan.net/oficina/sheets/tools/gencorpus	[no test files]
=== race ===
go test -race -count=1 ./internal/sheets/store/... ./internal/sheets/live/...
ok  	amadan.net/oficina/sheets/internal/sheets/store	52.406s
ok  	amadan.net/oficina/sheets/internal/sheets/live	1.066s
diff --git a/cmd/sheets/main.go b/cmd/sheets/main.go
index 68e781d..7fa5f19 100644
--- a/cmd/sheets/main.go
+++ b/cmd/sheets/main.go
@@ -128,7 +128,11 @@ func main() {
// otherwise Serve would open a second connection to the same file.
opts.DBPath = ""
- if err := rastrillo.Serve(opts); err != nil {
+ err = rastrillo.Serve(opts)
+ // Sign-in links are sent after the response (mailqueue.go); send
+ // what is still queued before the process goes.
+ sheets.DrainSigninMail()
+ if err != nil {
logger.Error("serve", "err", err)
os.Exit(1)
}
diff --git a/internal/sheets/app.go b/internal/sheets/app.go
index 0dc93c3..86eb225 100644
--- a/internal/sheets/app.go
+++ b/internal/sheets/app.go
@@ -46,9 +46,9 @@ type Config struct {
// the CSRF same-origin check's reference value.
Origin string
- // Admit is the allowlist, consulted before a link is SENT.
- // auth.Config.Authorize covers the same list at redemption; this
- // one exists so we do not email strangers. See handleBegin.
+ // Admit is the allowlist, the static half of App.Admits. Admits is
+ // consulted before a link is SENT (admissionMailer) and at
+ // redemption (auth.Config.Authorize). See handleBegin.
Admit func(string) bool
}
diff --git a/internal/sheets/auth.go b/internal/sheets/auth.go
index a0f7a2a..681460c 100644
--- a/internal/sheets/auth.go
+++ b/internal/sheets/auth.go
@@ -151,17 +151,23 @@ func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger, authorize func(string
// is the version that says so.
return nil, errors.New("sign-in: refusing to wire auth with no admission gate")
}
+ queue := newMailQueue(env.Mailer, authorize, nil, logger)
+ if a, ok := env.Mailer.(queueAttacher); ok {
+ a.attachQueue(queue.wait)
+ }
signedInPath := "/sheets"
if env.Home != nil {
signedInPath = connect.LocalCompletePath
}
- return auth.New(auth.Config{
+ plugin, err := auth.New(auth.Config{
DB: db,
Origin: env.Origin,
InstanceKey: env.InstanceKey,
- Mailer: env.Mailer,
- Authorize: authorize,
- SigninPath: "/signin",
+ // Begin runs for every address (handleBegin); the mail goes
+ // out after the response, and only to one authorize admits.
+ Mailer: queue,
+ Authorize: authorize,
+ SigninPath: "/signin",
// Where a fresh session lands. auth defaults this to "/",
// which is the anonymous entry page — so without this line
// every magic-link click arrives on a page that exists to
@@ -170,6 +176,14 @@ func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger, authorize func(string
Subject: chrome.EmailSubject("Sheets"),
Logger: logger,
})
+ if err != nil {
+ queue.close()
+ return nil, err
+ }
+ signinQueuesMu.Lock()
+ signinQueues[plugin] = queue
+ signinQueuesMu.Unlock()
+ return plugin, nil
}
// admits builds the admission gate.
diff --git a/internal/sheets/begin.go b/internal/sheets/begin.go
index a66d910..a0ded16 100644
--- a/internal/sheets/begin.go
+++ b/internal/sheets/begin.go
@@ -1,49 +1,145 @@
package sheets
-import "net/http"
+import (
+ "bytes"
+ "context"
+ "net/http"
+ "strings"
-// handleBegin gates POST /signin on admission BEFORE auth mints and
-// emails a link.
+ "amadan.net/oficina/home/connect"
+)
+
+// handleBegin is POST /signin, in both modes: auth's Begin for EVERY
+// address, under connect.OAuthContinuation.
+//
+// What the form answers depends on the address's domain, never on
+// admission. Answering "that address cannot sign in" — or answering an
+// admitted address differently from a refused one in any way — would
+// turn this endpoint into a membership oracle: anyone could enumerate
+// who is on the team by watching which addresses get a different
+// answer. So:
//
-// rastrillo/auth checks Config.Authorize at admission — the moment a
-// verified address would receive a session — not when the link is sent.
-// That is the right place for the security decision, and the security
-// property holds without this: an address that is not admitted cannot
-// get a session no matter how many links it redeems.
+// - A Keymail address (a domain that runs Keymail, or delegates to it
+// with a `_keymail` TXT record), admitted or not, goes to Keymail.
+// Begin answers with a 303 to Keymail's authorize URL, and
+// OAuthContinuation turns that into a page (renderHomeOAuth): CSP's
+// form-action 'self' covers a form submission's whole redirect
+// chain in Safari, so a 303 out of the POST was refused there
+// ("Refused to load https://keymail.dev/oauth/authorize… form-action",
+// Paul, 2026-10-06). Admission is asked after Keymail has verified
+// the address, by auth's Authorize (App.Admits), and a 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 App.Admits says yes — after the response, in a
+// worker, so not even the response time differs (mailqueue.go). Without that, anyone who
+// can reach the instance could make it email a live sign-in link to
+// any address they name — abuse of our sending identity, found by
+// POSTing a stranger's address at the deployed instance. The cost
+// is real and accepted: a member who mistypes their own address is
+// told to check their email and nothing arrives.
//
-// What it leaves open is abuse, not access. Without this gate, anyone
-// who can reach the instance can make it email a live sign-in link to
-// any address they name, at whatever rate the IP limiter allows. The
-// recipient gets unsolicited mail from us and our sending identity wears
-// it. Found by POSTing a stranger's address at the deployed instance and
-// getting ?sent=1 back — the unit tests all passed, because they tested
-// admission rather than delivery.
+// Both kinds of address spend the same rate budgets and are parsed the
+// same way, so ?err=rate and ?err=address say nothing about admission
+// either; and since the send happens after the response, a failed one
+// is logged rather than answered.
//
-// The refusal is deliberately INVISIBLE. Answering "that address cannot
-// sign in" would turn this endpoint into a membership oracle: anyone
-// could enumerate who is on the team by watching which addresses get a
-// different answer. So a non-admitted address gets exactly the response
-// an admitted one gets, and no email. The cost is real and accepted: a
-// member who mistypes their own address is told to check their email and
-// nothing arrives. Every mainstream provider makes the same trade.
+// It used to gate admission in front of Begin instead, which answered a
+// non-admitted Keymail address ?sent=1 and sent an admitted one to
+// Keymail: the oracle, one level narrower.
func (a *App) handleBegin(w http.ResponseWriter, r *http.Request) {
- // Parse before reading the field: Begin would do this itself, and
- // this handler has to read the address before deciding to call it.
- if err := r.ParseForm(); err != nil {
- a.badRequest(w, r, http.StatusBadRequest, "Malformed request.")
- return
+ connect.OAuthContinuation(a.origin, a.auth.Begin, a.renderHomeOAuth)(w, r)
+}
+
+// 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 auth's Callback and Verify, the handlers that end
+// in admission, and turns its bare-text refusal into the sign-in door,
+// rendered in the refusal's own 403 by door (handleSignin) 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 on oficina/slides #20, round
+// 3). 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)
+ }
+ 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)
}
- // App.Admits, NOT a.admit. The two must ask the same question as
- // auth's own Authorize, or the answers disagree: a.admit is the
- // static SHEETS_OWNERS list, so a guest somebody shared a sheet
- // with would be admitted at redemption and never sent a link to
- // redeem — a share that silently does nothing. Caught by the
- // two-user isolation suite the moment a guest tried to sign in.
- if !a.Admits(r.FormValue("address")) {
- // Identical to auth's own success redirect, so the two cases are
- // indistinguishable from outside.
- http.Redirect(w, r, "/signin?sent=1", http.StatusSeeOther)
- return
+}
+
+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
}
- a.auth.Begin(w, r)
+ return b.body.Write(p)
}
diff --git a/internal/sheets/begin_test.go b/internal/sheets/begin_test.go
index f5c4bad..4169d8a 100644
--- a/internal/sheets/begin_test.go
+++ b/internal/sheets/begin_test.go
@@ -22,6 +22,25 @@ type recorder struct {
mu sync.Mutex
sent []string
bodies []string
+ // waits are the sign-in mail queues in front of this recorder:
+ // links go out after the response (mailqueue.go), so every read
+ // waits for them first.
+ waits []func()
+}
+
+func (m *recorder) attachQueue(wait func()) {
+ m.mu.Lock()
+ m.waits = append(m.waits, wait)
+ m.mu.Unlock()
+}
+
+func (m *recorder) flush() {
+ m.mu.Lock()
+ waits := append([]func(){}, m.waits...)
+ m.mu.Unlock()
+ for _, w := range waits {
+ w()
+ }
}
func (m *recorder) Send(_ context.Context, to, _, body string) error {
@@ -35,6 +54,7 @@ func (m *recorder) Send(_ context.Context, to, _, body string) error {
// lastLink pulls the verify URL out of the most recent email.
func (m *recorder) lastLink(t *testing.T) string {
t.Helper()
+ m.flush()
m.mu.Lock()
defer m.mu.Unlock()
if len(m.bodies) == 0 {
@@ -49,6 +69,7 @@ func (m *recorder) lastLink(t *testing.T) string {
}
func (m *recorder) addresses() []string {
+ m.flush()
m.mu.Lock()
defer m.mu.Unlock()
return append([]string(nil), m.sent...)
@@ -67,6 +88,12 @@ func appWithRecorder(t *testing.T) (*App, *recorder) {
}
func appWithOrigin(t *testing.T, origin string) (*App, *recorder) {
+ t.Helper()
+ return appWithOwners(t, origin, signedInAddress)
+}
+
+// appWithOwners is appWithOrigin with the SHEETS_OWNERS list spelled out.
+func appWithOwners(t *testing.T, origin string, owners ...string) (*App, *recorder) {
t.Helper()
d, err := rdb.Open(filepath.Join(t.TempDir(), "test.db"), quietLogger())
if err != nil {
@@ -77,7 +104,6 @@ func appWithOrigin(t *testing.T, origin string) (*App, *recorder) {
t.Fatalf("migrate: %v", err)
}
rec := &recorder{}
- owners := []string{signedInAddress}
sealer, err := seal.New(make([]byte, dekBytes))
if err != nil {
t.Fatalf("seal.New: %v", err)
@@ -97,6 +123,7 @@ func appWithOrigin(t *testing.T, origin string) (*App, *recorder) {
if err != nil {
t.Fatalf("NewAuth: %v", err)
}
+ t.Cleanup(func() { CloseSigninMail(a) })
app.UseAuth(a)
rememberMailer(app, rec)
return app, rec
diff --git a/internal/sheets/home.go b/internal/sheets/home.go
index a1261f2..e744ad8 100644
--- a/internal/sheets/home.go
+++ b/internal/sheets/home.go
@@ -159,13 +159,10 @@ func (a *App) homeLocalComplete(w http.ResponseWriter, r *http.Request) {
}
a.homeRoutes.LocalComplete(w, r)
}
-func (a *App) homeLocalBegin(w http.ResponseWriter, r *http.Request) {
- connect.OAuthContinuation(a.origin, a.handleBegin, a.renderHomeOAuth)(w, r)
-}
-// renderHomeOAuth is the page between the door and Keymail, and it is
-// the same card: the refresh stays Home's (head), the body is the
-// door's notice.
+// renderHomeOAuth is the page between the door and Keymail, in both
+// modes (handleBegin), and it is the same card: the refresh stays
+// Home's (head), the body is the door's notice.
func (a *App) renderHomeOAuth(w http.ResponseWriter, r *http.Request, destination string) {
p := connect.OAuthPage(destination)
a.render(w, r, "home_oauth", frame{parts: []string{p.Title}, signinPage: true}, http.StatusOK, homeOAuthView{
diff --git a/internal/sheets/home_routes.go b/internal/sheets/home_routes.go
index 6aecf88..057cd83 100644
--- a/internal/sheets/home_routes.go
+++ b/internal/sheets/home_routes.go
@@ -28,15 +28,15 @@ func (a *App) mountAuth(r chi.Router) {
// baked into every emailed link, so moving it later invalidates
// links already in people's inboxes.
if a.homeRoutes != nil {
- r.Post("/signin", a.homeRoutes.LocalGuard(a.homeLocalBegin))
- r.Get("/auth/callback", a.homeRoutes.LocalGuard(a.auth.Callback))
- r.Get("/auth/verify", a.homeRoutes.LocalGuard(a.auth.Verify))
- r.Post("/auth/verify", a.homeRoutes.LocalGuard(a.auth.Verify))
+ r.Post("/signin", a.homeRoutes.LocalGuard(a.handleBegin))
+ r.Get("/auth/callback", a.homeRoutes.LocalGuard(doorOnRefusal(a.auth.Callback, a.handleSignin)))
+ r.Get("/auth/verify", a.homeRoutes.LocalGuard(doorOnRefusal(a.auth.Verify, a.handleSignin)))
+ r.Post("/auth/verify", a.homeRoutes.LocalGuard(doorOnRefusal(a.auth.Verify, a.handleSignin)))
r.Post("/signout", a.homeRoutes.Signout)
} else {
r.Post("/signin", a.handleBegin)
- r.Get("/auth/callback", a.auth.Callback)
- r.Get("/auth/verify", a.auth.Verify)
+ r.Get("/auth/callback", doorOnRefusal(a.auth.Callback, a.handleSignin))
+ r.Get("/auth/verify", doorOnRefusal(a.auth.Verify, a.handleSignin))
r.Post("/signout", a.signOutToPage)
}
}
diff --git a/internal/sheets/home_test.go b/internal/sheets/home_test.go
index aa3dff7..16254ce 100644
--- a/internal/sheets/home_test.go
+++ b/internal/sheets/home_test.go
@@ -49,6 +49,7 @@ func newHomeFixtureWith(t *testing.T, directory string) *homeFixture {
if err != nil {
t.Fatal(err)
}
+ t.Cleanup(func() { CloseSigninMail(localAuth) })
app.UseAuth(localAuth)
if err := app.ConfigureHome(&cfg); err != nil {
t.Fatal(err)
diff --git a/internal/sheets/keymail_signin_test.go b/internal/sheets/keymail_signin_test.go
new file mode 100644
index 0000000..edeb63e
--- /dev/null
+++ b/internal/sheets/keymail_signin_test.go
@@ -0,0 +1,226 @@
+package sheets
+
+import (
+ "fmt"
+ "html"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "os"
+ "regexp"
+ "strings"
+ "testing"
+ "time"
+
+ "amadan.net/oficina/sheets/internal/sheets/keymailtest"
+)
+
+// TestMain keeps Keymail classification inside the process: every DNS
+// query is answered NXDOMAIN and only the Keymail fixtures a test
+// registers answer HTTPS (keymailtest). Before it, every POST /signin
+// in this suite classified its address against the real network.
+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 plugin a
+ // test builds is closed in its cleanup (CloseSigninMail).
+ 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.sheets.test"
+ keymailMember = "paul@" + keymailHost
+ keymailStranger = "stranger@" + keymailHost
+)
+
+// keymailApp is a standalone app whose owners include keymailMember,
+// beside a Keymail deployment holding keymailMember and keymailStranger.
+func keymailApp(t *testing.T) (*App, *recorder, *keymailtest.Server) {
+ t.Helper()
+ km := keymailtest.Serve(t, keymailHost, keymailMember, keymailStranger)
+ app, rec := appWithOwners(t, testOrigin, signedInAddress, keymailMember)
+ return app, rec, km
+}
+
+func keymailPost(t *testing.T, h http.Handler, form url.Values, cookies ...*http.Cookie) *httptest.ResponseRecorder {
+ t.Helper()
+ 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)
+ r.Header.Set("Sec-Fetch-Site", "same-origin")
+ for _, c := range cookies {
+ r.AddCookie(c)
+ }
+ w := httptest.NewRecorder()
+ h.ServeHTTP(w, r)
+ return w
+}
+
+func keymailGet(t *testing.T, h http.Handler, path string, cookies ...*http.Cookie) *httptest.ResponseRecorder {
+ t.Helper()
+ r := httptest.NewRequest(http.MethodGet, testOrigin+path, nil)
+ for _, c := range cookies {
+ r.AddCookie(c)
+ }
+ w := httptest.NewRecorder()
+ h.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 redirect from 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 — the hop to Keymail must not be a redirect from the form POST", 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
+}
+
+// TestKeymailAddressGoesToKeymailWhetherAdmittedOrNot: whether an
+// address goes to Keymail depends on its domain alone. An admitted and
+// a non-admitted Keymail address get the same first answer — the
+// continuation page — so the answer says nothing about who is on the
+// team. Before, the admission gate answered the stranger ?sent=1 and
+// sent the member to Keymail with a 303 out of the form POST, which
+// Safari refused under form-action 'self'.
+func TestKeymailAddressGoesToKeymailWhetherAdmittedOrNot(t *testing.T) {
+ app, mail, km := keymailApp(t)
+ h := app.Router()
+ member := keymailPost(t, h, url.Values{"address": {keymailMember}})
+ stranger := keymailPost(t, h, url.Values{"address": {keymailStranger}})
+ mu := continuation(t, "admitted Keymail address", member)
+ su := continuation(t, "non-admitted 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:\nadmitted:\n%s\nnot admitted:\n%s", a, b)
+ }
+ if got := mail.addresses(); len(got) != 0 {
+ t.Errorf("a Keymail address was mailed: %v", got)
+ }
+ if n := km.Lookups(); n != 2 {
+ t.Errorf("Keymail saw %d lookups, want 2: both addresses classified", n)
+ }
+}
+
+// TestHomeLocalKeymailIsTheSameAnswer is the same in Home mode, where the
+// local form posts to /signin under LocalGuard.
+func TestHomeLocalKeymailIsTheSameAnswer(t *testing.T) {
+ km := keymailtest.Serve(t, keymailHost, keymailMember, keymailStranger)
+ f := newHomeFixture(t)
+ member := f.request(t, http.MethodPost, "/signin", url.Values{"address": {signedInAddress}}, false)
+ if member.Code != http.StatusSeeOther || member.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("Home-local plain member: %d %q", member.Code, member.Header().Get("Location"))
+ }
+ stranger := f.request(t, http.MethodPost, "/signin", url.Values{"address": {keymailStranger}}, false)
+ continuation(t, "Home-local non-admitted Keymail address", stranger)
+ if n := km.Lookups(); n != 1 {
+ t.Errorf("Keymail saw %d lookups, want 1", n)
+ }
+}
+
+// TestNonAdmittedPlainAddressIsAnsweredSentAndNotMailed: an address
+// with no Keymail still gets ?sent=1, admitted or not, and only the
+// admitted one is mailed.
+func TestNonAdmittedPlainAddressIsAnsweredSentAndNotMailed(t *testing.T) {
+ app, mail, _ := keymailApp(t)
+ h := app.Router()
+ for _, address := range []string{"stranger@example.com", signedInAddress} {
+ rec := keymailPost(t, h, 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 := mail.addresses(); len(got) != 1 || got[0] != signedInAddress {
+ t.Fatalf("mail = %v, want one, to %s", got, signedInAddress)
+ }
+}
+
+// TestKeymailFailureFallsBackToAnEmailLink: the form rastrillo lands a
+// failed Keymail exchange on carries force, and a forced POST mails a
+// link without classifying.
+func TestKeymailFailureFallsBackToAnEmailLink(t *testing.T) {
+ app, mail, km := keymailApp(t)
+ h := app.Router()
+ if body := keymailGet(t, h, "/signin?force=1&err=keymail").Body.String(); !strings.Contains(body, `name="force"`) {
+ t.Fatal("the err=keymail page does not carry force")
+ }
+ rec := keymailPost(t, h, 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 := mail.addresses(); len(got) != 1 || got[0] != keymailMember {
+ t.Fatalf("mail = %v, want one link to %s", got, keymailMember)
+ }
+ if km.Lookups() != 0 {
+ t.Error("a forced sign-in classified the address")
+ }
+}
+
+// TestKeymailRefusalIsTheDoor: a non-admitted address Keymail verified
+// is refused after verification with the sign-in door, rendered in the
+// refusal's own 403 — not rastrillo's bare text, and not a redirect to a
+// query anybody could type. No session is minted. An admitted one is
+// signed in.
+func TestKeymailRefusalIsTheDoor(t *testing.T) {
+ app, _, km := keymailApp(t)
+ h := app.Router()
+ roundTrip := func(address string) *httptest.ResponseRecorder {
+ begin := keymailPost(t, h, url.Values{"address": {address}})
+ u := continuation(t, address, begin)
+ return keymailGet(t, h, "/auth/callback?"+url.Values{"state": {u.Query().Get("state")}, "code": {km.Approve(address)}}.Encode(), begin.Result().Cookies()...)
+ }
+ if cb := roundTrip(keymailMember); cb.Code != http.StatusSeeOther || cb.Header().Get("Location") != "/sheets" {
+ t.Fatalf("admitted callback: %d %q, want 303 /sheets", cb.Code, cb.Header().Get("Location"))
+ }
+ cb := roundTrip(keymailStranger)
+ if cb.Code != http.StatusForbidden || cb.Header().Get("Location") != "" {
+ t.Fatalf("non-admitted 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())
+ }
+ for _, c := range cb.Result().Cookies() {
+ if strings.Contains(c.Name, "session") && c.MaxAge >= 0 && c.Value != "" {
+ t.Fatalf("a refused sign-in set a session cookie %q", c.Name)
+ }
+ }
+ door := cb.Body.String()
+ for _, want := range []string{"estilo-door", `name="address"`, "can’t sign in to Sheets"} {
+ if !strings.Contains(door, want) {
+ t.Errorf("refusal door lacks %q:\n%s", want, door)
+ }
+ }
+ // The sentence means "you proved this address and it is not
+ // admitted"; nobody may summon it by typing a URL (Astra, round 3).
+ if spoof := keymailGet(t, h, "/signin?err=refused").Body.String(); strings.Contains(spoof, "can’t sign in to Sheets") {
+ t.Fatal("anybody can show the refusal by visiting /signin?err=refused")
+ }
+}
diff --git a/internal/sheets/keymailtest/keymailtest.go b/internal/sheets/keymailtest/keymailtest.go
new file mode 100644
index 0000000..92ad056
--- /dev/null
+++ b/internal/sheets/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/sheets/mailqueue.go b/internal/sheets/mailqueue.go
new file mode 100644
index 0000000..e807aac
--- /dev/null
+++ b/internal/sheets/mailqueue.go
@@ -0,0 +1,283 @@
+package sheets
+
+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("sheets: 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("sheets: 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("sheets: 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("sheets: 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("sheets: 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("sheets: sign-in mail worker ignored cancellation; left running")
+ }
+}
diff --git a/internal/sheets/mailqueue_registry.go b/internal/sheets/mailqueue_registry.go
new file mode 100644
index 0000000..cb63a65
--- /dev/null
+++ b/internal/sheets/mailqueue_registry.go
@@ -0,0 +1,43 @@
+package sheets
+
+import (
+ "sync"
+
+ "amadan.net/rastrillo/rastrillo/auth"
+)
+
+// signinQueues is every queue NewAuth built, by the plugin it feeds, so
+// CloseSigninMail can stop one and DrainSigninMail all of them.
+var (
+ signinQueuesMu sync.Mutex
+ signinQueues = map[*auth.Auth]*mailQueue{}
+)
+
+// CloseSigninMail stops the sign-in mail queue behind x, sending what
+// is still in it within the queue's deadline. Tests close every plugin
+// they build with it.
+func CloseSigninMail(x *auth.Auth) {
+ signinQueuesMu.Lock()
+ q := signinQueues[x]
+ delete(signinQueues, x)
+ signinQueuesMu.Unlock()
+ if q != nil {
+ q.close()
+ }
+}
+
+// DrainSigninMail stops every sign-in mail queue and sends what is
+// still in them. main calls it once Serve returns.
+func DrainSigninMail() {
+ signinQueuesMu.Lock()
+ qs := signinQueues
+ signinQueues = map[*auth.Auth]*mailQueue{}
+ signinQueuesMu.Unlock()
+ for _, q := range qs {
+ q.close()
+ }
+}
+
+// queueAttacher is a test mailer that wants to wait for the queue in
+// front of it before it is read (begin_test.go's recorder).
+type queueAttacher interface{ attachQueue(wait func()) }
diff --git a/internal/sheets/mailqueue_test.go b/internal/sheets/mailqueue_test.go
new file mode 100644
index 0000000..d506fbe
--- /dev/null
+++ b/internal/sheets/mailqueue_test.go
@@ -0,0 +1,286 @@
+package sheets
+
+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)
+ }
+}
+
+// TestCloseSigninMailStopsTheWorker: every plugin NewAuth builds owns a
+// queue, and CloseSigninMail stops its worker — which is what every
+// test that builds one calls in its cleanup, so none leaves a worker
+// behind.
+func TestCloseSigninMailStopsTheWorker(t *testing.T) {
+ app, _ := appWithOrigin(t, testOrigin)
+ signinQueuesMu.Lock()
+ q := signinQueues[app.auth]
+ signinQueuesMu.Unlock()
+ if q == nil {
+ t.Fatal("NewAuth registered no mail queue for its plugin")
+ }
+ CloseSigninMail(app.auth)
+ select {
+ case <-q.done:
+ case <-time.After(2 * time.Second):
+ t.Fatal("the sign-in mail worker was still running after CloseSigninMail")
+ }
+}
diff --git a/internal/sheets/signin.go b/internal/sheets/signin.go
index 199fe1c..df02aad 100644
--- a/internal/sheets/signin.go
+++ b/internal/sheets/signin.go
@@ -42,8 +42,8 @@ func signinError(code string) (text string, field bool) {
return "Too many attempts. Wait a few minutes, then try again.", false
case "address":
// rastrillo's ?err=address is an address it could not parse.
- // A refused (unadmitted) address is never told so: handleBegin
- // answers it exactly as it answers an admitted one.
+ // A refused (unadmitted) address is never told so at the form:
+ // handleBegin answers it exactly as it answers an admitted one.
return "That doesn’t look like an email address.", true
case "expired":
return "That link has expired or was already used. Enter your email to get a new one.", false
@@ -81,7 +81,7 @@ func (a *App) signinFormPath() string {
// the skip in that mode.
func (a *App) handleSignin(w http.ResponseWriter, r *http.Request) {
q := r.URL.Query()
- if a.home == nil && q.Get("sent") == "" && q.Get("err") == "" && q.Get("force") == "" {
+ if a.home == nil && !refusedHere(r) && q.Get("sent") == "" && q.Get("err") == "" && q.Get("force") == "" {
if _, ok := a.identity(r); ok {
http.Redirect(w, r, signedInLanding(r), http.StatusSeeOther)
return
@@ -96,6 +96,13 @@ func (a *App) handleSignin(w http.ResponseWriter, r *http.Request) {
return
}
text, field := signinError(q.Get("err"))
+ if refusedHere(r) {
+ // Keymail or a link verified the address and Admits said no
+ // (doorOnRefusal), in this response. Never from the query: told
+ // only to someone who just proved they hold the address; the
+ // form below is the way on.
+ text, field = "That address can’t sign in to Sheets. Use a different address, or ask someone to share a sheet with it.", false
+ }
form := chrome.SignIn{
Icon: brand.Path("sheets"),
App: "Sheets",
diff --git a/internal/sheets/signin_test.go b/internal/sheets/signin_test.go
index 77a0993..3acb42e 100644
--- a/internal/sheets/signin_test.go
+++ b/internal/sheets/signin_test.go
@@ -90,6 +90,7 @@ func testAuth(t *testing.T) *App {
if err != nil {
t.Fatalf("NewAuth: %v", err)
}
+ t.Cleanup(func() { CloseSigninMail(a) })
return New(Config{DB: d.G, Auth: a, Admit: Admits([]string{"paul@example.com"})})
}
diff --git a/internal/sheets/signin_timing_test.go b/internal/sheets/signin_timing_test.go
new file mode 100644
index 0000000..3e2a104
--- /dev/null
+++ b/internal/sheets/signin_timing_test.go
@@ -0,0 +1,101 @@
+package sheets
+
+import (
+ "context"
+ "net/http/httptest"
+ "net/url"
+ "path/filepath"
+ "sync"
+ "testing"
+ "time"
+
+ rdb "amadan.net/rastrillo/rastrillo/db"
+ "amadan.net/rastrillo/rastrillo/migrate"
+
+ "amadan.net/oficina/sheets/internal/sheets/seal"
+)
+
+// 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, an admitted 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 team was readable off a stopwatch.
+// With the mailer blocked, every POST — admitted, stranger, and both
+// with force=1 — must answer, and the admitted links must then go out.
+func TestSigninNeverWaitsForTheMailer(t *testing.T) {
+ d, err := rdb.Open(filepath.Join(t.TempDir(), "test.db"), quietLogger())
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { d.Close() })
+ if _, err := migrate.Apply(context.Background(), d, BootSchema); err != nil {
+ t.Fatal(err)
+ }
+ sealer, err := seal.New(make([]byte, dekBytes))
+ if err != nil {
+ t.Fatal(err)
+ }
+ app := New(Config{DB: d.G, Sealer: sealer, Origin: testOrigin, Admit: Admits([]string{signedInAddress})})
+ bm := &blockingMailer{release: make(chan struct{})}
+ a, err := NewAuth(d.Writer(), AuthEnv{Origin: testOrigin, InstanceKey: "test-instance-key", Owners: []string{signedInAddress}, Mailer: bm}, quietLogger(), app.Admits)
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { CloseSigninMail(a) })
+ app.UseAuth(a)
+ var once sync.Once
+ free := func() { once.Do(func() { close(bm.release) }) }
+ t.Cleanup(free)
+ h := app.Router()
+
+ for _, form := range []url.Values{
+ {"address": {signedInAddress}},
+ {"address": {"stranger@example.com"}},
+ {"address": {signedInAddress}, "force": {"1"}},
+ {"address": {"stranger@example.com"}, "force": {"1"}},
+ } {
+ done := make(chan *httptest.ResponseRecorder, 1)
+ go func() { done <- keymailPost(t, h, 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 admitted", 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] != signedInAddress || got[1] != signedInAddress {
+ t.Fatalf("mail sent = %v, want two links, both to %s", got, signedInAddress)
+ }
+}