amadan

oficina / memoria Public

fix/r1-signin-outcome

Squashed from fix/r1-signin-outcome into main.
Paul Campbell pushed by claude 6a9acda2cf807d3e87837ba05449689ebf852417 parent 29247af
3 files changed, +198 −7
  • internal/memoria/handlers.go +37 −7
  • internal/memoria/signinoutcome_test.go +33 −0
  • internal/memoriatest/signinoutcome_test.go +128 −0

CI passed — run details

CI log 
=== vet ===
go vet ./...
=== fmt ===
=== test ===
go test ./...
?   	amadan.net/oficina/memoria/cmd/memoria	[no test files]
ok  	amadan.net/oficina/memoria/internal/memoria	0.652s
ok  	amadan.net/oficina/memoria/internal/memoria/chunk	0.004s
?   	amadan.net/oficina/memoria/internal/memoria/icons	[no test files]
ok  	amadan.net/oficina/memoria/internal/memoria/index	0.030s
ok  	amadan.net/oficina/memoria/internal/memoria/notion	0.017s
ok  	amadan.net/oficina/memoria/internal/memoria/query	0.003s
ok  	amadan.net/oficina/memoria/internal/memoria/seal	0.003s
ok  	amadan.net/oficina/memoria/internal/memoria/sibling	3.019s
ok  	amadan.net/oficina/memoria/internal/memoriatest	1.311s
?   	amadan.net/oficina/memoria/tools/bench	[no test files]
?   	amadan.net/oficina/memoria/tools/drive	[no test files]
?   	amadan.net/oficina/memoria/tools/seed	[no test files]
?   	amadan.net/oficina/memoria/tools/vendorassets	[no test files]
=== migration-check ===
go run github.com/carlosframework/rastrillo/cmd/rastrillo migration check
rastrillo migration check: models and migrations agree
diff --git a/internal/memoria/handlers.go b/internal/memoria/handlers.go
index 21a8c94..8096f4f 100644
--- a/internal/memoria/handlers.go
+++ b/internal/memoria/handlers.go
@@ -8,6 +8,7 @@ import (
"log/slog"
"mime/multipart"
"net/http"
+ "net/url"
"strconv"
"strings"
@@ -1094,15 +1095,44 @@ func (a *app) deleteDocument(w http.ResponseWriter, r *http.Request) {
func (a *app) localSignin(w http.ResponseWriter, r *http.Request) {
p := page{Chunks: a.chunkCount()}
p.ForceEmail = a.home != nil && r.URL.Query().Get("force") == "1"
- switch r.URL.Query().Get("outcome") {
- case "sent":
- p.Flash, p.FlashKind = "Check your email — the link signs you in.", "good"
- case "denied":
- p.Flash, p.FlashKind = "That address is not on this instance.", "danger"
+ p.Flash, p.FlashKind = signinOutcome(r.URL.Query())
+ render(w, "signin", p)
+}
+
+// signinOutcome turns the query a sign-in redirect lands with into the
+// sentence the page says. The vocabulary is not ours: rastrillo's auth
+// handlers and beginGuard write it (?sent=1, ?err=rate|address|expired|
+// keymail|1, ?reauth=1), and this page must read exactly those words.
+// It read ?outcome= for months — a spelling nothing ever wrote — so every
+// sign-in, sent or failed, came back to a blank form. The regression test
+// follows real redirects rather than building the query by hand, because
+// a hand-built query is written from the same model as this switch.
+//
+// There is no "not on this instance" sentence, deliberately: beginGuard
+// answers a stranger with ?sent=1 so the page is not an oracle for who
+// the owners are.
+func signinOutcome(q url.Values) (msg, kind string) {
+ if q.Get("sent") != "" {
+ return "Check your email. We've sent you a link to sign in.", "good"
+ }
+ switch q.Get("err") {
+ case "":
+ case "rate":
+ return "Too many sign-in attempts. Wait a few minutes, then try again.", "danger"
+ case "address":
+ return "That doesn't look like an email address. Check it and try again.", "danger"
case "expired":
- p.Flash, p.FlashKind = "That link had expired. Here is a fresh start.", "danger"
+ // rastrillo says one thing for unknown, used and expired alike.
+ return "That sign-in link has expired or was already used. Enter your email for a new one.", "danger"
+ case "keymail":
+ return "Keymail couldn't finish signing you in. Enter your email and we'll send you a link instead.", "danger"
+ default:
+ return "Something went wrong and no link was sent. Try again.", "danger"
}
- render(w, "signin", p)
+ if q.Get("reauth") != "" {
+ return "Sign in again to continue.", "warn"
+ }
+ return "", ""
}
// sniffBytes is how much of an upload looksLikeText inspects. Named
diff --git a/internal/memoria/signinoutcome_test.go b/internal/memoria/signinoutcome_test.go
new file mode 100644
index 0000000..bc02ecf
--- /dev/null
+++ b/internal/memoria/signinoutcome_test.go
@@ -0,0 +1,33 @@
+package memoria
+
+import (
+ "net/url"
+ "testing"
+)
+
+// The route-driven test (memoriatest.TestSigninPageSaysWhatHappened)
+// follows every ?err= a test can reach through the real handlers. Two it
+// cannot: ?err=keymail needs a keymail round trip to fail after the
+// pending cookie verified, and ?err=1 needs signin.Begin to fail for a
+// reason other than rate or address. Both are rastrillo's words
+// (auth/handlers.go, v0.23.0), read off the source, not guessed. A code
+// the page does not know still says something: silence is the bug.
+func TestSigninOutcomeCoversTheCodesNoRouteTestReaches(t *testing.T) {
+ for _, c := range []struct {
+ query, kind string
+ }{
+ {"force=1&err=keymail", "danger"},
+ {"err=1", "danger"},
+ {"err=some-future-code", "danger"},
+ {"reauth=1", "warn"},
+ } {
+ q, _ := url.ParseQuery(c.query)
+ msg, kind := signinOutcome(q)
+ if msg == "" || kind != c.kind {
+ t.Errorf("?%s: got (%q, %q), want a %s sentence", c.query, msg, kind, c.kind)
+ }
+ }
+ if msg, _ := signinOutcome(url.Values{}); msg != "" {
+ t.Errorf("a plain visit says %q", msg)
+ }
+}
diff --git a/internal/memoriatest/signinoutcome_test.go b/internal/memoriatest/signinoutcome_test.go
new file mode 100644
index 0000000..7d7977c
--- /dev/null
+++ b/internal/memoriatest/signinoutcome_test.go
@@ -0,0 +1,128 @@
+package memoriatest
+
+import (
+ "io"
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+)
+
+// B5 (polish audit, 2026-09-23): the sign-in page said nothing after any
+// sign-in attempt. It switched on ?outcome=, a word nothing writes;
+// rastrillo's auth handlers and memoria's own beginGuard redirect with
+// ?sent=1 and ?err=rate|address|expired|keymail|1. So "Send me a link"
+// came back to the same blank form, and a rate limit, an unparseable
+// address and a dead link were all silent.
+//
+// Every case here drives the REAL flow and follows the Location it was
+// given — never a hand-built query. A hand-built query is written from
+// the same model as the switch that reads it, which is exactly how the
+// old ?outcome= test-free code agreed with itself for months. Each case
+// also asserts the Location's query, so if rastrillo renames a code the
+// failure names the new spelling rather than a missing sentence.
+//
+// Verified red 2026-09-23 against the old localSignin (switch on
+// "outcome"): every subtest failed with the blank form.
+
+// followToSignin performs the redirect a browser would and returns the
+// page body. It refuses anything that is not a redirect to /signin, so a
+// changed status cannot pass as "the page rendered without the flash".
+func followToSignin(t *testing.T, h http.Handler, rr *httptest.ResponseRecorder, wantQuery string) string {
+ t.Helper()
+ if rr.Code != http.StatusSeeOther && rr.Code != http.StatusFound {
+ t.Fatalf("status = %d, want a redirect to the sign-in page; body %q", rr.Code, rr.Body.String())
+ }
+ loc := rr.Header().Get("Location")
+ if !strings.HasPrefix(loc, "/signin?") {
+ t.Fatalf("Location = %q, want /signin?…", loc)
+ }
+ if got := strings.TrimPrefix(loc, "/signin?"); got != wantQuery {
+ t.Fatalf("Location query = %q, want %q — rastrillo's vocabulary moved; "+
+ "signinOutcome must read the new word", got, wantQuery)
+ }
+ req := httptest.NewRequest(http.MethodGet, loc, nil)
+ page := httptest.NewRecorder()
+ h.ServeHTTP(page, req)
+ if page.Code != http.StatusOK {
+ t.Fatalf("GET %s = %d", loc, page.Code)
+ }
+ b, _ := io.ReadAll(page.Body)
+ return string(b)
+}
+
+func assertFlash(t *testing.T, body, tone, sentence string) {
+ t.Helper()
+ if !strings.Contains(body, `rst-tone="`+tone+`"`) {
+ t.Errorf("no %s-toned callout on the page the redirect landed on", tone)
+ }
+ if !strings.Contains(body, sentence) {
+ t.Errorf("page does not say %q", sentence)
+ }
+}
+
+func TestSigninPageSaysWhatHappened(t *testing.T) {
+ t.Run("a sent link says check your email", func(t *testing.T) {
+ // Both writers of ?sent=1: rastrillo after mailing an owner's
+ // link, and beginGuard answering a stranger with the owner's
+ // answer by design.
+ for _, address := range []string{dialTestOwner, "stranger@attacker.example"} {
+ h, _ := signinApp(t)
+ rr := beginSignin(t, h, address)
+ body := followToSignin(t, h, rr, "sent=1")
+ assertFlash(t, body, "positive", "Check your email")
+ }
+ })
+
+ t.Run("an address rastrillo cannot parse says so", func(t *testing.T) {
+ // Admitted by the owners list, refused by signin's parser: a
+ // dotless domain. Only an owner reaches rastrillo at all.
+ const owner = "paul@localhost"
+ h, _ := signinAppWithOwners(t, []string{owner})
+ rr := beginSignin(t, h, owner)
+ body := followToSignin(t, h, rr, "err=address")
+ assertFlash(t, body, "negative", "look like an email address")
+ })
+
+ t.Run("a rate-limited address says wait", func(t *testing.T) {
+ // signin's per-address budget, spent on an owner whose links
+ // are being sent (the stub mailer takes them).
+ h, _ := signinApp(t)
+ var rr *httptest.ResponseRecorder
+ for i := 0; i < 20; i++ {
+ rr = beginSignin(t, h, dialTestOwner)
+ if strings.Contains(rr.Header().Get("Location"), "err=rate") {
+ break
+ }
+ }
+ body := followToSignin(t, h, rr, "err=rate")
+ assertFlash(t, body, "negative", "Too many sign-in attempts")
+ })
+
+ t.Run("a dead magic link says it expired", func(t *testing.T) {
+ h, _ := signinApp(t)
+ req := httptest.NewRequest(http.MethodGet, "/auth/verify?token=not-a-real-token", nil)
+ rr := httptest.NewRecorder()
+ h.ServeHTTP(rr, req)
+ body := followToSignin(t, h, rr, "err=expired")
+ assertFlash(t, body, "negative", "expired or was already used")
+ })
+
+ t.Run("a keymail return with no pending cookie says it expired", func(t *testing.T) {
+ h, _ := signinApp(t)
+ req := httptest.NewRequest(http.MethodGet, "/auth/callback?code=x&state=y", nil)
+ rr := httptest.NewRecorder()
+ h.ServeHTTP(rr, req)
+ body := followToSignin(t, h, rr, "err=expired")
+ assertFlash(t, body, "negative", "expired or was already used")
+ })
+
+ t.Run("a plain visit says nothing", func(t *testing.T) {
+ h, _ := signinApp(t)
+ rr := httptest.NewRecorder()
+ h.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/signin", nil))
+ if strings.Contains(rr.Body.String(), "rst-callout") {
+ t.Error("a sign-in page nobody submitted carries a callout")
+ }
+ })
+}