amadan
app: fix round 1(b) on Task 3 -- sign-in-specific fallback copy
GET /signin's switch on ?err= fell back to docs.error.generic
("Something went wrong.") for anything it did not have a specific
case for -- reachable in real use via ?err=keymail (a failed keymail
callback) and ?err=1 (a raw Begin failure), and confirmed directly
against this app's own router for the case a person hits constantly:
clicking a magic link a second time. rauth.Auth's Verify handler
gives one error for "unknown, used and expired alike" by design, so
that specific case was already landing on docs.signin.error.expired,
which does offer recovery -- but the switch's actual default branch,
for the other two paths, was not.
Every outcome on this one page means the same operational thing --
sign-in did not complete, ask for a new link -- so its fallback
should never borrow the app-wide generic string, which is written for
a render failure with nowhere styled to land, not for this page's own
ordinary, recoverable outcomes. Added docs.signin.error.unknown
alongside the existing docs.signin.error.* entries and used it in
that default: branch; docs.error.generic is untouched everywhere
else.
Added two tests: one exercises a real magic link clicked twice
end-to-end and confirms the second click resolves the specific
expired-link copy, not the generic one; the other hits the default
branch directly via ?err=keymail/1/unknown-future-value.
3 files changed,
+118
−8
internal/docs/auth.go+13 −7internal/docs/auth_test.go+92 −0internal/docs/render.go+13 −1
diff --git a/internal/docs/auth.go b/internal/docs/auth.go| index 8aa9a37..2b7d0c1 100644 |
| --- a/internal/docs/auth.go |
| +++ b/internal/docs/auth.go |
| @@ -161,12 +161,18 @@ type signinPage struct { |
| // signin serves GET /signin — the form itself, and the landing spot |
| // for every outcome rauth.Auth's handlers redirect back to |
| -// (?sent=1, ?err=rate|address|expired|keymail|1, ?force=1). One |
| -// error message reads all the "something is wrong, try again" shapes: |
| -// rauth.Auth already refuses to be more specific than that between |
| -// "wrong" and "unknown" (Verify's own doc comment — telling those |
| -// apart would be an oracle), so there is nothing this page could add |
| -// by drawing finer distinctions of its own on Begin's outcomes either. |
| +// (?sent=1, ?err=rate|address|expired|keymail|1, ?force=1). rate, |
| +// address and expired each get their own copy below; keymail and 1 |
| +// (a raw Begin failure logged server-side, and Verify's own "unknown, |
| +// used and expired alike" — its own doc comment says rauth.Auth |
| +// refuses to be more specific than that, so there is nothing this |
| +// page could add by drawing a finer distinction of its own) share the |
| +// switch's default branch, docs.signin.error.unknown — never |
| +// docs.error.generic (fix round 1 on Task 3): every outcome on this |
| +// one page means the same operational thing, "sign-in did not |
| +// complete, ask for a new link," and docs.error.generic is authored |
| +// for a render failure with nowhere styled to land, not for this |
| +// page's own ordinary, recoverable outcomes. |
| func (h *handlers) signin(w http.ResponseWriter, r *http.Request) { |
| q := r.URL.Query() |
| data := signinPage{ |
| @@ -188,7 +194,7 @@ func (h *handlers) signin(w http.ResponseWriter, r *http.Request) { |
| case "expired": |
| data.ErrorMessage = rastrillo.T(r, "docs.signin.error.expired") |
| default: |
| - data.ErrorMessage = rastrillo.T(r, "docs.error.generic") |
| + data.ErrorMessage = rastrillo.T(r, "docs.signin.error.unknown") |
| } |
| if err := h.render.page(w, r, "signin", data); err != nil { |
| h.logger.Error("docs: render signin", "err", err) |
diff --git a/internal/docs/auth_test.go b/internal/docs/auth_test.go| index c4a84d6..35bc797 100644 |
| --- a/internal/docs/auth_test.go |
| +++ b/internal/docs/auth_test.go |
| @@ -188,3 +188,95 @@ func TestNewRefusesMissingMailOutsideDevelopment(t *testing.T) { |
| t.Fatal("New must refuse to start outside development with no mail configured") |
| } |
| } |
| + |
| +// TestSecondClickOfTheSameMagicLinkOffersRecovery is fix round 1 on |
| +// Task 3's own regression test: rauth.Auth's Verify handler |
| +// deliberately gives one error for "unknown, used and expired alike" |
| +// (its own doc comment), which means a magic link clicked a second |
| +// time — the ordinary case of someone opening an old sign-in email — |
| +// lands on the exact same ?err=expired outcome as a link that was |
| +// never valid at all. Confirmed directly against this app's own |
| +// router, not assumed: the first click consumes the link and sets a |
| +// session; the second click, with no cookie attached (a fresh tab, or |
| +// the same old email opened again after the first session already |
| +// expired), must redirect back to /signin with the specific "expired |
| +// or already used... enter your email for a new one" copy — never a |
| +// bare, uninformative dead end. |
| +func TestSecondClickOfTheSameMagicLinkOffersRecovery(t *testing.T) { |
| + app, err := docs.NewForTest(t) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + link, err := app.TestSignIn(t, "clicktwice@example.com") |
| + if err != nil { |
| + t.Fatalf("TestSignIn: %v", err) |
| + } |
| + |
| + firstReq := httptest.NewRequest(http.MethodGet, link, nil) |
| + firstRec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(firstRec, firstReq) |
| + if len(firstRec.Result().Cookies()) == 0 { |
| + t.Fatal("first click set no session cookie") |
| + } |
| + |
| + secondReq := httptest.NewRequest(http.MethodGet, link, nil) |
| + secondRec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(secondRec, secondReq) |
| + loc := secondRec.Header().Get("Location") |
| + if !strings.Contains(loc, "err=expired") { |
| + t.Fatalf("second click redirected to %q, want an err=expired outcome", loc) |
| + } |
| + |
| + signinReq := httptest.NewRequest(http.MethodGet, loc, nil) |
| + signinRec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(signinRec, signinReq) |
| + // NewForTest's app.Handler() never wires rastrillo.Options.BaseCatalog/ |
| + // Locales — only cmd/docs/main.go does, for real serving — so |
| + // rastrillo.T falls back to returning its key verbatim here |
| + // (render.go's own doc comment on Locales: "documented, harmless"). |
| + // That makes the raw catalog key the correct, precise thing to |
| + // assert on in this handler-level test: it pins exactly which |
| + // branch of signin's switch ran, without depending on the English |
| + // copy's exact wording. |
| + body := signinRec.Body.String() |
| + if !strings.Contains(body, "docs.signin.error.expired") { |
| + t.Errorf("signin page after a second click did not resolve docs.signin.error.expired; body: %s", body) |
| + } |
| + if strings.Contains(body, "docs.error.generic") { |
| + t.Errorf("signin page after a second click fell back to the app-wide docs.error.generic instead of the sign-in-specific expired copy") |
| + } |
| +} |
| + |
| +// TestSigninUnrecognizedErrorUsesSigninSpecificFallback is fix round |
| +// 1 on Task 3's own finding: GET /signin's switch on ?err= had a |
| +// default: branch falling back to docs.error.generic ("Something went |
| +// wrong.") — the app-wide string written for a render failure with |
| +// nowhere styled to land, not for this page's own ordinary outcomes. |
| +// ?err=keymail (a failed keymail callback, rastrillo/auth's own |
| +// Callback handler) and ?err=1 (a raw Begin failure, logged |
| +// server-side) both reach that default branch in real use — this |
| +// pins both directly against the handler, without depending on |
| +// rastrillo internals staying exactly as they are today, and checks |
| +// the fallback is sign-in-specific rather than the app-wide string. |
| +func TestSigninUnrecognizedErrorUsesSigninSpecificFallback(t *testing.T) { |
| + app, err := docs.NewForTest(t) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // See TestSecondClickOfTheSameMagicLinkOffersRecovery's own comment |
| + // on why the raw catalog key, not the resolved English copy, is |
| + // what this handler-level test can observe and correctly should |
| + // assert on. |
| + for _, errParam := range []string{"keymail", "1", "some-future-value-nobody-has-written-a-case-for-yet"} { |
| + req := httptest.NewRequest(http.MethodGet, "/signin?err="+errParam, nil) |
| + rec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(rec, req) |
| + body := rec.Body.String() |
| + if !strings.Contains(body, "docs.signin.error.unknown") { |
| + t.Errorf("err=%q: signin page did not resolve docs.signin.error.unknown; body: %s", errParam, body) |
| + } |
| + if strings.Contains(body, "docs.error.generic") { |
| + t.Errorf("err=%q: signin page fell back to the app-wide docs.error.generic instead of the sign-in-specific fallback", errParam) |
| + } |
| + } |
| +} |
diff --git a/internal/docs/render.go b/internal/docs/render.go| index 67c481d..2be369e 100644 |
| --- a/internal/docs/render.go |
| +++ b/internal/docs/render.go |
| @@ -71,7 +71,18 @@ var Catalog = rastrillo.Catalog{ |
| // docs.signin.* — GET /signin (auth.go's signin handler): the |
| // form itself, and every outcome rastrillo/auth's own handlers |
| - // redirect back here with (?sent=1, ?err=rate|address|expired|1). |
| + // redirect back here with (?sent=1, |
| + // ?err=rate|address|expired|keymail|1). docs.signin.error.unknown |
| + // is that switch's own default: branch (fix round 1 on Task 3): |
| + // every one of these outcomes means the same operational thing on |
| + // this one page — sign-in did not complete, ask for a new link — |
| + // so its fallback must not borrow docs.error.generic ("Something |
| + // went wrong."), an app-wide string written for a render failure |
| + // with nowhere else to land, not for this page's own normal, |
| + // recoverable outcomes. See auth.go's signin handler for why no |
| + // finer distinction between ?err=keymail and ?err=1 is drawn |
| + // either (rastrillo/auth's own handlers already refuse to be an |
| + // oracle between "wrong" and "unknown"). |
| "docs.signin.title": "Sign in", |
| "docs.signin.body": "Enter your email address and we'll send you a link to sign in.", |
| "docs.signin.address.label": "Email address", |
| @@ -80,6 +91,7 @@ var Catalog = rastrillo.Catalog{ |
| "docs.signin.error.rate": "Too many attempts. Wait a few minutes and try again.", |
| "docs.signin.error.address": "That doesn't look like a valid email address.", |
| "docs.signin.error.expired": "That link has expired or was already used. Enter your email below for a new one.", |
| + "docs.signin.error.unknown": "We couldn't sign you in. Enter your email below for a new link.", |
| // docs.signout.label is the sign-out control's text (list.html's |
| // and show.html's page headers) — both its button label and its |